Tslint: Discussion: should developers blindly accept --fix'es

[alexeagle]

I have heard of some developers running tslint --fix in their editor, to make the dev workflow the same as running a formatter.

I am concerned about this because I want tslint to have the freedom to:

• make fixes which might not be safe, eg. var -> let
• make fixes which try to restore developer intent rather than preserve semantics, eg. s.trim() -> s = s.trim()
• offer more than one suggested fix (we do this for some checks in http://ErrorProne.info for example)

Another way to say this is: I think a formatter should be blindly run because it has safety guarantees, and a linter's fixes should be inspected by a developer.

I think that conflating a formatter and a linter causes this rough edge, where we'll accidentally be forced into very conservative --fix behavior due to developer expectations. We should decide within the tslint project what the stance on this is and set developer expectations accordingly.

[ajafff]

Given that the developer doesn't see what exactly changed and does not explicitly approve each fix before being applied, I'm strongly against unsafe fixes.

I'm against var -> let, because it might cause crashes at runtime. Even the compiler does not catch every variable access in the temporal dead zone. We could of course check if the fix is safe, but that would increase complexity without much benefit.
Same with triple-equals. Autofixing this could cause changes at runtime. eslint even removed the fixer from their eqeqeq rule for some time because of this.

Looking at eslint there are mostly safe fixes. The only exceptions seem to be var -> let and == -> ===

If the user had the chance to assess every fix before applying, that would be a totally different story.

[rich-newman]

@ajafff has asked me to comment, as the current behavior confused the hell out of me re no-var-keyword/prefer-const.

For a var in local scope (in a function) we allow a fix for var -> let (no-var-keyword). We also allow a fix for let -> const if applied after the first fix (prefer-const). However we currently won't allow the prefer-const rule to directly fix var -> const because we think it's too dangerous.

This leads to rather weird behaviour currently with the recommended rule set: if you fix once a local var goes to let. If you fix again it can go to const.

I tend to agree with @ajafff that if you run with --fix it shouldn't be possible for your code to break. Particularly if you're running with the recommended set of rules.

If we do want potentially unsafe fixes maybe we have a --unsafe_fix option?

At the very least we should be consistent between rules though, so we don't get double fix issues like above. That is, prefer-const and no-var-keyword should behave the same with respect to vars.

[adidahiya]

I like @rich-newman's and @JoshuaKGoldberg's proposal, a --fix-unsafe option to apply more liberal fixes.

[bitjson]

A --fix-unsafe option sounds ideal. Is this issue waiting on a PR, or is there more to discuss?

[JoshuaKGoldberg]

@bitjson it sounds like it's ready to be added 😄

@ajafff for confirmation?

[NaridaL]

It might be sensible to have the unsafe/safe flag on individual Replacements, instead of per-rule. For example, a fixer for object-literal-sort keys might not be safe if the values have side effects:

// sorting the keys alphabetically is fine:
const o = {
    b: 1,
    a: 2, 
}

// sorting the keys will have semantic differences 
// (which might still be ok, but the dev should look over it first):
let i = 1;
const p = {
    b: i++,
    a: i++,
}

[danbeam]

a --fix-unsafe or --fix={safe,unsafe} sounds good to me

[freshp86]

Providing a way to perform fixes even if these fixes can cause runtime behavior changes seems reasonable to me. Otherwise the usefulness of the linter tool is reduced unnecessarily.

Developers can run the tool first, and audit the automated fixes 1-by-1, (for example using git diff), to decide whether to accept them or not.

a --fix-unsafe or --fix={safe,unsafe} sounds good to me

+1

[JoshuaKGoldberg]

@adidahiya is this still something you would consider in scope for TSLint given #4534?

If so, I think allowing --fix to remain the same and --fix unsafe to enable Replacements with a new unsafe: boolean seems reasonable.

If not, should this be closed?

[JoshuaKGoldberg]

Per #4534, closing this very large change request. Ah well!

adidahiya and I talked offline and both expressed a lack of time to work on these radical large changes. If you'd like to see this change, your best bet would be to tackle it in ESLint itself.