'Fix all auto-fixable problems' doesn't fix as many issues as possible

[darkred]

I have ESLint extension 1.0.7 with VSCode 1.6.0 in win10 x64
and ESLint 3.7.1 globally installed.

First of all, please note that since ESLint 2.9.0:

when you use the --fix option, ESLint will make multiple passes over your code in an attempt to fix as many issues as possible.

Let's say I have the following .eslintrc.js in C:\Users\Kostas:

module.exports = {
    "env": {
        "browser": true,
        "es6": true,
        "greasemonkey": true,
        "jquery": true
    },
    "extends": "eslint:recommended",
    "rules": {
        "indent": ["error","tab"],       
    }
};

Now, I have this Javascript file:

var localTimezone;

    function convertDates(f) {
        if (f === 'f1') {
            format = 'DD.MM.YYYY HH:mm';
        } else {
            format = 'MMMM DD, YYYY';
        }
    }

If I do eslint --fix myfile.js via CLI (a single time) it will be fixed into:

var localTimezone;

function convertDates(f) {
    if (f === 'f1') {
        format = 'DD.MM.YYYY HH:mm';
    } else {
        format = 'MMMM DD, YYYY';
    }
}

But, in VSCode the ``Eslint: Fix all auto-fixable problems` command doesn't fix all indentation at once:
executing it the first time will only fix indentation on line 3,
a second time fixes lines 4 + 9,
and finally a 3rd time fixes lines 5-8,
i.e. it requires multiple (3) executions.

For reference, in Atom with linter 1.11.18 and linter-eslint 8.0.0 using my global ESLint installation as well,
with the relevant command (Linter eslint: Fix File) all indentation is fixed with a single execution.

[ivancuric]

Can confirm. It only does a single pass.

[egamma]

The above example is handled fine by the JS formatter and we now support format on save in 1.6.

Are there other examples than formatting of fixes that require multiple passes?

[carlin-q-scott]

Maybe the extension could run the formatter instead of autofix from the context menu if it handles this better. I haven't had this issue outside of dealing with white-space.

[darkred]

@egamma

The above example is handled fine by the JS formatter and we now support format on save in 1.6.

Format on save requires that you have installed formatter extensions, which will run on each save.
First of all, I don't want running a formatter every time I save a file - I prefer to do that manually.
Also, JSCS and ESlint have become a single team since April.
ESLint provides lots of stylistic rules with auto-fix already,
and I'm sure the "gap" between JSCS and ESLInt will soon be bridged. (see issue 5856 of ESLint for more).
Plus, a formatter will beautify the code, but it doesn't fix all stylistic problems that ESLint (incl. JSCS) detects.

Are there other examples than formatting of fixes that require multiple passes?

Yes. when the errors are both stylistic and non-stylistic.

Example:

/*eslint quotes: ["error", "single"], yoda: "error"*/

var color;

if ("red" === color) {
    // ...
}

Here line 5 has 2 errors: a yoda and a quote one:
the first pass fixes the quote error,
and an extra pass is needed for the yoda error.

And, in general, I can't rule out that there aren't other cases other than formatting that require multiple passes.

The Fix all auto-fixable problems command currently only fixes those errors that it displays,
i.e. it only does a single pass.

For reference, what the --fix option of ESLint offers (1, 2) is that:

This option instructs ESLint to try to fix as many issues as possible. The fixes are made to the actual files themselves and only the remaining unfixed issues are output.

So, my suggestion is to offer a user setting with which to implement ESLint '--fix' functionality as it is in CLI:
i.e. to replace the current file with one having as many issues as possible fixed ,
and then report only which unfixed issues remain.

[egamma]

@darkred Frankly, I cannot get warm for implementing formatting using a multi-pass linting approach. Formatters are great at formatting and I don't want to maintain separate settings for formatting and linting. See also https://medium.com/@Jakeherringbone/why-linting-makes-me-yawn-cadbd9a51ca9#.9tw56pje0 from @alexeagle by https://github.com/Microsoft/vscode-tslint/issues/84#issuecomment-246158200.

For VS Code we have a commit hook that doesn't allow a check-in if there are formatting violations. This implemented using a formatter and not a linter.

[darkred]

Frankly, I cannot get warm for implementing formatting using a multi-pass linting approach.

I'm afraid that might have been a misunderstanding here. To clarify:
the current command is [ESLint] Fix all auto-fixable problems.
My suggestion is to rename it to [ESLint] Fix file
and make it so that it runs eslint --fix myfile.js in the background
and replace the current file with the output it.

[egamma]

@darkred sorry, my comment was less about implementation. I've just reacted to the 'identity crisis' of linters vs formatters. I'll calm down now 😄

My suggestion is to rename it to [ESLint] Fix file and make it so that it runs eslint --fix myfile.js in the background and replace the current file with the output it.

This would destroy markers like break points. The right way to do this is to diff the output with the current version and generate separate edits to create a minimal update, but @dbaeumer knows all about this...

[dsbert]

This behavior causes problems when performing a very common set of actions. If I have a code block nested inside another code block and delete the outer code block, then the inner block is now improperly indented. When I save the document, only the first level of the code block is moved to the correct indentation. Now any code within that block is incorrectly indented.

[dbaeumer]

As @egamma pointed out the fix should only provide a minimal edit set to keep as many markers in the file as possible. In addition the time a save participant has in VS Code is very limited. All participant have one second and if you are the guy who causes other to not run 3 times you will be disabled. So everything we do here in save participation must keep performance in mind.

[alexeagle]

Per the discussion of preferring a formatter to do formatting:
What is the ultimate reason these users don't use a formatter extension for VSCode instead? Maybe this problem would be less acute if we had the right advertising somewhere, to say "it looks like you're trying to format your file, here are two extensions that do that for you". Or do they have some objection to using a formatter (eg. not sufficiently configurable, which IMO is not a big deal, any consistent formatting is better than forcing your personal tastes on others)

[dsbert]

@alexeagle The following is just my opinion

1. The built in VS Code formatter leaves a lot to be desired. It is not very configurable and you end up in a situation where the formatter actually creates linter errors.
2. Using a separate formatter extension creates an additional dependency where you now have to duplicate your style in both the formatter configuration file and the .eslintrc file
3. The auto-fix can detect and fix problems that a formatter would typically not involve itself with, such as replacing var with const. A formatter could be made to do this, but then the feature creep is going in the other direction and that defeats the purpose of separating formatting and linting anyway.
4. You will never get people to agree on one objectively correct way to format code, so I don't think it's fair to assume the formatter author knows best.

[joshfttb]

I have to agree with @dsbert on this. I just deleted an outmost code block on a mocha file and had to run fix five times to fix my indenting down the chain.

I could install and configure a separate formatter... but on the other hand eslint --fix does the job just fine.

[dbaeumer]

Does anyone of you know why eslint needs n rounds to fix up these things. Doing n rounds in the extension needs quite some coding since it is a pre save hook and all the modifications need to be described on the version of the document. The way how I understand the round concept in eslint is that fixes are applied like this fix(fix(fix(fix(n) which moves the document to version n + 4. So I can't simply take the fixes compute the rounds 2, 3, 4 and apply them to the document version n. Moreover I suspect that the edits might be overlapping and therefore a merge might not be possible. So the only save answer I see here is to run the fix rounds until I reach a fix point and then use a compare algorithm to compute a minimal edit set :-(

[darkred]

@dbaeumer check eslint/eslint#5329 (comment),
eslint/eslint#5329 (comment) and
eslint/eslint#5329 (comment)
(issue 5329 is the one referenced in the 2.9.0 changelog of ESLint in which this behavior was implemented).

[dbaeumer]

@darkred thanks for the pointers. Reading through it makes it clear that the n round approach is here to stay. So I will look into how we can best adopt this. One thing that is already clear is that I need to time budget this, given that a save participant can only run for so many ms. So it still might be the case that not all auto fixes are applied in one save.

[joshfttb]

Personally I'm less worried about on save than when I run the command manually. 

Josh F

Sent from
https://polymail.io/

On Wed, Nov 02, 2016 at 08:58 "Dirk Bäumer"

<
mailto:

wrote:

a, pre, code, a:link, body { word-wrap: break-word !important; }

https://github.com/darkred
thanks for the pointers. Reading through it makes it clear that the n round approach is here to stay. So I will look into how we can best adopt this. One thing that is already clear is that I need to time budget this, given that a save participant can only run for so many ms. So it still might be the case that not all auto fixes are applied in one save.

—

You are receiving this because you are subscribed to this thread.

Reply to this email directly,
#154 (comment)
, or
https://github.com/notifications/unsubscribe-auth/ACeafmXRMBGTHbUJuArE7mR7UDb4JIVaks5q6IjngaJpZM4KT2vS
.

[gcazaciuc]

Is anyone still looking into this ? if not i might take a stab at it in the next few days

[dbaeumer]

A pull request is highly appreciated here.

[leandono]

Any update about this issue?

[dbaeumer]

No update. PR is still welcome.

[gcazaciuc]

@dbaeumer I've began taking a look at it. However, it seems we are doing loads of processing with the ranges reported by Eslint and it's really hard to figure out what's going on.
My question is why we can't use the outputFixes() CLI method on Eslint and remove all that range translation logic which is error prone ?
Similar to how Atom does it( https://github.com/AtomLinter/linter-eslint/blob/master/src/worker.js):

function fixJob({ cliEngineOptions, contents, eslint, filePath }) {
  const report = lintJob({ cliEngineOptions, contents, eslint, filePath })

  eslint.CLIEngine.outputFixes(report)

  if (!report.results.length || !report.results[0].messages.length) {
    return 'Linter-ESLint: Fix complete.'
  }
  return 'Linter-ESLint: Fix attempt complete, but linting errors remain.'
}

Basically just run validate on the file, get the report and pass it to outputFixes() cli method.
I know there might be some issues with unsaved files but I believe any conflicts , if they occur , should be harmless and recoverable from.

Thoughts ?

[thernstig]

@arcanis and @nunogois it is already possible to fix this with VS Code ESLint + Prettier extension as I explained here in this post #154 (comment) It is a workaround but it works for now. Ideally one should not use the Prettier extension just to use the ESLint extension properly. Which is what this issue is about to fix.

[kuceb]

@arcanis also, potential performance is not the blocker here, it's the merging fixes logic mentioned above. I just mentioned performance since that should also be considered

[zfeher]

I've experienced this multiple save or exec of ESLint: Fix all auto-fixable Problems command to fix/format a file completely recently.

Can be reproduced on this repo: https://github.com/octref/veturpack Clone it, install deps, and open Counter.vue and try issuing the ESLint: Fix all auto-fixable Problems a couple of times it needs 3-4 round.

Hope it can be fixed one day, really annoying and makes manual or auto eslint fix unusable :(

[raix]

@bkucera are you still working on a pr? and is it solving the actual issue or just running fix a couple of times behind the scenes? (I would imagine that basing fixes on a list of rules might not be enough - so eslint should be able to provide the fix?)

[kuceb]

@raix running fix multiple times behind the scenes (until there are no more fixable problems left) which is what ESLint does via the cli.

[dtinth]

This issue can be solved by registering ESLint as a formatter rather than a command.

I tried playing around with vscode-eslint’s source code and modified it so that it takes the final output from eslint --fix and send the whole formatted code back to VS Code (rather than sending individual fixes as separate edit).

As you can see here, when I run the autofix command, the cursor, which used to be on line 2 on the word arguments, immediately jumps down to the bottom, since the whole editor’s content is replaced.

For comparison, here’s the original behavior — I had to run it several times, but here the cursor should stay at the word arguments and not jump down to the bottom of the file.

However, by registering ESLint as a document formatter (activated via “Format document” command instead), we can simply send the final code to VS Code, and it will diff the editor’s contents with the formatted code to compute the minimal changes to apply. As far as I know, this function is not exposed via the extension API anywhere except through the “Format document” command. I mentioned this about a year ago in this issue.

Here, I updated the extension to register itself as a code formatter, and by calling the “Format document” command, the cursor stays at the correct position:

[SpencerCarstens]

Brilliant!

On Fri, Sep 20, 2019 at 9:38 AM Thai Pangsakulyanont < ***@***.***> wrote: This issue can be solved by registering ESLint as a formatter rather than a command. I tried playing around with vscode-eslint’s source code and modified it so that it takes the final output from eslint --fix and send the whole formatted code back to VS Code (rather than sending individual fixes as separate edit). As you can see here, when I run the autofix command, the cursor, which used to be on line 2 on the word arguments, immediately jumps down to the bottom, since the whole editor’s content is replaced. [image: after1] <https://user-images.githubusercontent.com/193136/65342933-ff6c3080-dbfd-11e9-80ea-dcaf8f5eb3f8.gif> For comparison, here’s the original behavior — I had to run it several times, but here the cursor should stay at the word arguments and not jump down to the bottom of the file. [image: before] <https://user-images.githubusercontent.com/193136/65343552-71914500-dbff-11e9-98ed-5512036c1c45.gif> However, by registering ESLint as a document formatter (activated via “Format document” command instead), we can simply send the final code to VS Code, and it will diff the editor’s contents with the formatted code to compute the minimal changes to apply <https://github.com/microsoft/vscode/blob/66dc7a11b709c9353d6bb2fd144c7e088ad35ecf/src/vs/editor/common/services/editorSimpleWorker.ts#L445>. As far as I know, this function is not exposed via the extension API anywhere except through the “Format document” command. I mentioned this about a year ago in this issue <#154 (comment)> . Here, I updated the extension to register itself as a code formatter, and by calling the “Format document” command, the cursor stays at the correct position: [image: after] <https://user-images.githubusercontent.com/193136/65343294-d8fac500-dbfe-11e9-8a7f-20aa7fe4187c.gif> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#154?email_source=notifications&email_token=AAMT4BCJH2XHGF7LWV3BTWTQKT36RA5CNFSM4CSPNPJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7HH2CQ#issuecomment-533626122>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMT4BGSL5PRZJMPGE7NJ4DQKT36RANCNFSM4CSPNPJA> .

-- Best, Spencer Carstens

[dtinth]

PR to register vscode-eslint as document formatter submitted — #766

[tcg]

I don't want to pile on, just wondering: Is this related to the "toggling" behavior, where multiple saves will result in content switching among 2 or more formatted states?

(I'm researching solutions for issue shown in the gifs here: https://github.com/tcg/vscode-eslint-prettier-autoformat-glitch)

[moltar]

@tcg afaik this issue is unrelated to what you are experiencing.

Your issue is a conflict of different formatters fighting for the right format. Just use ESLint with prettier config, and disable prettier itself. I.e. get ESLint CLI config to work properly with prettier and disable prettier ext.

[tcg]

Thanks @moltar. Unfortunately I already “just” did that. I think. I’m reading your last two sentences as recommending opposite things.

So , ESLint w/ eslint-prettier plugin and no Prettier extension (my current setup) ??

Or ESLint and Prettier extensions, somehow configured not to interfere w/ each other’s rules?

[ThomasKientz]

@tcg no prettier extension, but running eslint --fix at save by your IDE or webpack. https://prettier.io/docs/en/integrating-with-linters.html

[tcg]

@ThomasKientz Thanks for that clarification.

Sorry to hijack!

I started everything from scratch, and followed those directions (yesterday, and again this morning) and still seeing some weird behavior, but I now think it's specific to rules/code I'm looking at, and not across the board.

I really appreciate the extra info! 👋

[matthewwithanm]

@tcg I think you're still going to see weird behavior. With the suggested "ESLint with prettier config", you'll need to save multiple times to get all your fixes run. (That's what this issue is about.)

[SikoSoft]

The issue still persists. :(

Also, there are no other formatters installed, and my settings file looks like this:

"eslint.autoFixOnSave": true,
"editor.formatOnSave": false

[dtinth]

A new version with the fix is not released yet. You can run download and run this extension from source code in the meantime.

[dbaeumer]

I released 2.0.4 today which addresses this. Please note that for large file you need to increase the time budget to compute the fixes using editor.codeActionsOnSaveTimeout

[kg-currenxie]

Works so well! My life is completely changed! No more Cmd+S, Cmd+S, Cmd+S, Cmd+S 😂 ❤️

[Tenemo]

The fix is amazing, thank you!