Remove prefer-const as it breaks code if standard --fix is used for autoformatting

[fenomas]

What version of this package are you using?
standard@16.0.4

What operating system, Node.js, and npm version?
the online repl

What happened?
When using standard --fix as an autoformatter, the prefer-const rule autofix breaks code if the developer autoformats frequently.

This happens whenever a variable needs to be reassignable, but the code where it gets reassigned isn't present (e.g. because it's not written yet, temporarily commented out, etc.). In such cases, autoformatting with standard --fix will change the declaration to a const, which will break the code later when the variable reassignments are restored.

Note: this issue only really applies if the developer uses standard --fix like an autoformatter, running it frequently as code is written. In the comments it became that several maintainers don't use --fix this way, and see it as something to be done frequently, e.g. during code tests. However the readme and docs refer to --fix as an auto formatter, so this issue is written with that assumption.

What did you expect to happen?
The ideal case would be to show a linter warning for let variables that aren't reassigned, but not convert them to const during autoformat. From the comments it sounds like this isn't possible, so removing prefer-const sounds like the only fix.

Alternately, if standard --fix is not meant to be used frequently as an autoformatter, then there's no need to remove the rule (but in that case the docs should be changed to refer to the command some other way, and standard might want to choose an official preferred autoformatter).

Are you willing to submit a pull request to fix this bug?
No.

Note: this issue is edited thanks to discussion w/ maintainers - please note that the top ~25 comments are about the pre-edit version.

[voxpelli]

In my personal opinion:

This same argument can be applied to lots of in-progress code.

Comment out an if-statement and you'll get complaints about the indentation of its content.

A linter as I see it is supposed to protect and improve the final product, not the in-progress code.

If one eg. have an auto-fix on save, then the easiest fix is to remove that auto-fix and instead do manual fixing.

I personally never run any automated fixing and often opt to fix things manually, by eg using the hover boxes that appear on lint errors in VS Code.

[fenomas]

Hi @voxpelli,

Comment out an if-statement and you'll get complaints about the indentation of its content

Sure, and as noted I think a complaint (warning) would be correct here. The current implementation silently changes the code, potentially into a broken state.

A linter as I see it is supposed to protect and improve the final product, not the in-progress code.
If one eg. have an auto-fix on save, then the easiest fix is to remove that auto-fix and instead do manual fixing.

Um. That's reasonable as a preference, but what you're saying here implies that standard's autoformat is a potentially destructive operation, and the developer needs to understand when it is and isn't safe to do. That's not how autoformat works in any other tool I've used, surely it's not the intent here?

[fenomas]

@voxpelli Sorry, so the answer here is that you agree autoformat is breaking code in the listed cases, and it's not a bug because your preference is to not autoformat in those cases?

[voxpelli]

@fenomas I can see the issue with this rule when it comes to autofixing on save, but I'm not convinced that is necessarily an issue with standard.

If we see it as a design goal that standard should work painlessly with autofix on save, then yes, I agree, this is a bug.

Designing standard to work well with autofix on save would make it less powerful in other scenarios though.

How have you set up your autofix on save? Using the standard vscode extension? (I'm not terribly familiar with that extension, but I think eg. @divlo knows it quite well)

You could set up your own eslint setup with eslint-config-standard and override the rule for let/const there if you want to get around it for now.

[voxpelli]

Rule in question: prefer-const

To note is also that there's not really any way of having a rule active in ESLint, but disabling the autofix for it. So you would have to make it on the call to the fix:

eslint somedir --fix --rule "somerule: 0"

Whether something like that would be possible to add to eg. the standard extension for VSCode, that I don't know unfortunately.

[fenomas]

If we see it as a design goal that standard should work painlessly with autofix on save, then yes, I agree, this is a bug.
Designing standard to work well with autofix on save would make it less powerful in other scenarios though.

@voxpelli This is a confusing answer, as autofix-on-save appears to be a configurable option in all of the official editor plugins. All of their docs have a note about how to enable it, with no warnings - even the readme of this repo has a note about it. It looks like a regular bread-and-butter feature, not an unsupported (much less unsupportable) edge case.

With that said though, personally I don't use fix-on-save, and I don't think it's directly related to the issue. The high-order bit is, autoformat is normally a "safe", non-destructive operation. Nobody assumes that it will always fix all errors, but people do assume that it won't change working code into a non-working state.

So personally, I use it more frequently than I save - whenever something needs to be formatted, I hit autoformat. And I can't recall ever having seen an autoformatter that wasn't safe by default - not with standard (outside of this issue), prettier, eslint, vscode's default formatter, etc.

[theoludwig]

Just to be clear: We use the formatting behavior of ESLint.
And the same thing for the auto-fix thing in the VSCode extension.

Keep in mind that standard is only a wrapper around ESLint to get linting quickly but under the hood, it uses the APIs ESLint provides.

If you really want this behavior, you could open an issue in the ESLint repo.
But as said:

A linter as I see it is supposed to protect and improve the final product, not the in-progress code.

[LinusU]

[...] but people do assume that it won't change working code into a non-working state.

I have yet to se it demonstrated that it turns any code into a non-working state though?

[fenomas]

@divlo

If you really want this behavior, you could open an issue in the ESLint repo.

As far as I know eslint's formatter is safe by default. The rule we're talking about is not enabled by eslint:recommended.

A linter as I see it is supposed to protect and improve the final product, not the in-progress code.

I don't know what that means. When is code ever "final"? If you put a note in the readme saying "warning: do not run standard --fix on code that is still in-progress", and someone asked how to tell whether their code was "in-progress" or not, what would the answer be?

@LinusU

As in the original issue, the code doesn't break immediately but will break when the reassignable variable gets reassigned. E.g. when code that was temporarily commented out gets uncommented, etc.

[voxpelli]

A linter as I see it is supposed to protect and improve the final product, not the in-progress code.

I don't know what that means. When is code ever "final"?

A linter looks at the piece of code as is. It doesn't assume that any code has been commented out or should be expected to be added. It looks at the code as is and sees if it fulfils the requirements that has been set up.

standard has a requirement that const should be used wherever it can be used.

That "can" is determined by looking at the state of the project as its linted.

If you run standard without --fix, then you'll get errors and warnings. If you run standard --fix, then it will fix your current state of the project to adhere to as many of those requirements as possible.

Edit: Also, bear in mind that we are many co-maintainers on this project and not all of us are working on all of the tools, I myself have worked on none of the autofix stuff, hence why my response there may have sounded confusing.

[fenomas]

A linter looks at the piece of code as is. It doesn't assume that any code has been commented out or should be expected to be added.

I fully agree, but that's the opposite of what's been said so far. If the linter requires code to be "final" (still no idea what that means), then it's obviously making assumptions about what future edits will (not) happen. If it takes code as-is, then it shouldn't assume anything about the state of the code.

I mean, consider the error where a variable is declared but never used. If the code is "final", then it's safe to fix that error by removing the variable entirely. But you wouldn't want your formatter to remove all unused variables silently by default, right? I'm saying the same logic should apply here.

---

PS: is this "linters are only supposed to work with final code" idea discussed anywhere I could look at? I've used eslint and other linters pretty extensively and I've never heard anything at all similar.

[dougwilson]

Hi @fenomas the standard is based on eslint directly. For your above comparison, the eslint no-unused-vars is not a fixable rule, so that is why --fix does not remove them, but eslint has the prefer-const rule as fixable, so --fix will change your lets into consts when able. It is actually eslint, not standard that is making the distinction upon which one of those are changed when you run --fix.

[fenomas]

@dougwilson Of course I'm aware that unused-vars isn't fixable in eslint - I was just making an illustrative comparison for why the prefer-const rule shouldn't be applied by default.

It is actually eslint, not standard that is making the distinction upon which one of those are changed

It's standard that's choosing which fixable rules to apply. ESlint has a number of rules that are fixable, but which standard does not autofix by default - I'm suggesting that prefer-const should be one of them.

[dougwilson]

So just to clarify: are you saying that standard should never make suggestions for if vars should be const or not at all, or that it should, just that it shouldn't auto fix them?

[fenomas]

@dougwilson

What did you expect to happen?
A warning about let variables that are not reassigned would be the right approach, without auto-converting them.

But if that's not possible, and the only options are both warning+autofix, or neither, then neither would seem preferable. At the end of the day, there are situations where an author declares a let intentionally, to convey that a variable may be reassigned even though it isn't reassigned currently. In those cases, autoconverting to const throws away the developer's semantic intent.

[dougwilson]

Right, that is what I'm trying to say: that a warning without autofix is not possible, at least not working you opening a feature request for that in eslint, as eslint doesn't offer such a configuration for this. That is why the above was pointing you to eslint, if that makes sense.

[fenomas]

Thanks for clarifying that @dougwilson. In that case, per my last comment I think the right thing to do here is for standard to not enable prefer-const by default.

PS: it would be helpful if you could shed any light on the "linters are only for the final product, not the in-progress code" policy quoted extensively above. I've looked around various eslint docs, and the policy doesn't appear to exist outside this thread.

[dougwilson]

I am not familiar with that policy, and unfortunately cannot shed any light on it.

[fenomas]

I only ask because (a) the policy appears to not exist, and (b) it's the only reason given so far for why this issue is closed.

Honestly, I'm not trying to be a weird hardass here - if the answer to this issue is "we agree there are potential risks but standard is opinionated and our opinion is to keep the current behavior", then that's 100% a reasonable answer. But currently the only answer (from @voxpelli) is: "fixing on save isn't supported because linters are only meant for final code". And it appears that neither standard nor eslint actually has or follows that policy/design/intent.

[dougwilson]

Hi @fenomas apologies on not being able to elaborate; I am actually just a user of standard (and so somewhate vested in what the rules are) but not technically part of the project as a maintainer or anything. Apologies if I caused any confusion on that. You can typically see what role different commenters have based on the badge github places on the comments, though.

[fenomas]

@dougwilson Understood, and thanks for clarifying.

(questions for maintainers about the "only final code" policy remain open.)

[voxpelli]

it would be helpful if you could shed any light on the "linters are only for the final product, not the in-progress code" policy quoted extensively above

There is no such "policy" and it's also not something where standard differs from other linters.

(As eg. pointed out, the decision to make prefer-const fixable lies with ESLint)

So, no policy, just me trying to formulate my thoughts.

If you want an explicit reason for the closing this then I would eventually close this due to:

• The decision to make prefer-const fixable lies with ESLint
• standard should not remove prefer-const

Hence making this an issue with ESLint, not standard.

[fenomas]

The decision to make prefer-const fixable lies with ESLint
Hence making this an issue with ESLint, not standard.

@voxpelli I'm not suggesting that prefer-const should not be fixable. I'm saying it should not be applied by default.

ESLint does not apply the rule by default, so there is no issue to report to ESLint. Standard does apply it by default, so I'm reporting here. Thanks for understanding.

[voxpelli]

Right, here's the discussions why it was added:

• Add rule "prefer-const" eslint-config-standard#133
• Rule suggestion: prefer-const #523

[fenomas]

Thank you for the links - I searched issues, but I didn't think to search pull requests.

[fenomas]

I've just realized, maybe the disconnect here is whether standard --fix is an autoformat command, or whether it's a fix conformance errors command.

The readme and other docs here describe it as an autoformatter, so that's what I've been assuming. And if you use standard --fix like an autoformatter (meaning you run it frequently, whenever something isn't formatted correctly) then sooner or later prefer-const inevitably breaks your code. The more frequently you autoformat, the more it will happen.

But it sounds like maintainers here are using --fix more as an occasional conformance tool, which is only run at specific times (e.g. as a hook before tests or commits). If used that way, I can certainly see how prefer-const wouldn't cause any problems.

@voxpelli, is that latter usage what you meant about using --fix on final code? If so then I think we were talking past each other, as I've been assuming the former kind of usage.

[voxpelli]

@fenomas Very good formulation of the situation, thanks! 🙏

You are entirely correct. To me it's a fix conformance errors command.

@LinusU @divlo What are your perceptions? Should we improve / clarify the docs on this?

[fenomas]

Thanks for confirming @voxpelli - apologies if I sounded snippy before, but I was really confused why it would be necessary to finalize code before autoformatting. 😅 I'll edit the original issue to make things clearer.

As for the issue itself, to be honest if prefer-const was removed then I think standard --fix can be pretty functionally used as a regular autoformatter. I haven't deeply investigated, but I was previously using it that way and I don't remember it breaking anything (outside of this issue).

OTOH if --fix isn't meant to be used frequently, documenting that also works. But that means standard users realistically need to use something else an autoformatter, which may lead to confusion if their formatter does anything that contradicts --fix.