Update: allow autofixing when using processors (fixes #7510) #9090

not-an-aardvark · 2017-08-08T06:20:51Z

What is the purpose of this pull request? (put an "X" next to item)

[x] Add something to the core

What changes did you make? (Give an overview)

This implements the proposal from #7510 (comment) to allow autofixes to be applied when using processors.

The processor API basically remains the same. The only changes are:

Processors can now export a supportsAutofix: true property, which opts a processor into autofixing.
If a processor supports autofixing, the postprocess method is expected to also transform autofix ranges as necessary when it transforms the locations of reported problems. Afterwards, the transformed fixes are applied to the original, unprocessed text.

Multipass autofixing works by calling preprocess and postprocess for each pass.

(One alternative approach would be to run multipass autofixing on the processed text, and only return the fixed text to the postprocessor afterwards. As discussed in #7510, though, that would make it difficult for postprocessors because they would need to figure out how to split the fixed text into code blocks again, which would require some sort of imprecise diffing against the original text.)

Summary of changes:

linter.verify and linter.verifyAndFixnow accept optional preprocess and postprocess arguments for splitting up the text into code blocks and combining the messages from each code block. By default, the text is "preprocessed" into a single code block containing the code, and the problems are "postprocessed" by flattening the array. (This is the same as not applying a processor at all.)
Applying processors is now is the responsibility of Linter rather than CLIEngine. (However, determining which processors need to be applied for a given file is still the responsibility of CLIEngine.)

TODO:

Add more tests
Add documentation
Get feedback on this API from people who have implemented processors (cc @BenoitZugmeyer, @btmills, others?)
Approve this API in a TSC meeting

Is there anything you'd like reviewers to focus on?

Does the API seem reasonable?
Does the API provide enough information to processors to allow them map autofix ranges correctly?
Is this a breaking change? (It seems like if a processor currently doesn't apply a transformation to fix ranges, then the original fix ranges will get used, which might be wildly incorrect and result in broken code.) One way to avoid this could be to have processors opt-in to allowing autofixes.
Does it make sense to allow postprocess without preprocess? The default postprocess (flattening the array of problems) will almost always do the wrong thing if multiple code blocks are being linted, since some of the locations will probably be invalid. It might be better to just throw a validation error.
This effectively allows plugins to remove or add any message to the user's reported problems list, with no opt-in. Are we sure we're okay with that? (Actually, that was already possible with processors before this change, but processors haven't gotten a lot of use because they prevent autofixing. Now that they don't have downsides, I can imagine that plugins could use them just for the purpose of modifying the problems list.)

mention-bot · 2017-08-08T06:20:57Z

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gyandeeps, @mysticatea and @nzakas to be potential reviewers.

eslintbot · 2017-08-14T04:00:41Z

LGTM

eslintbot · 2017-08-14T08:35:42Z

LGTM

BenoitZugmeyer · 2017-08-14T13:57:43Z

That's great! I didn't look at the implementation in details yet, but I'll try to adapt my plugin to work with this branch, to give you my feedbacks if needed.

eslintbot · 2017-08-20T04:39:47Z

LGTM

not-an-aardvark · 2017-08-20T04:52:45Z

Thoughts after writing a simple implementation for this as part of the tests:

Does linter.verify actually need processor options, or should the options just be added to linter.verifyAndFix? It seems like the only reason Linter needs processors at all is to apply them between autofix passes, since everything else could be done by the consumer of Linter. On the other hand, it would be nice to keep the API consistent.
Does it make sense to allow postprocess without preprocess? The default postprocess (flattening the array of problems) will almost always do the wrong thing if multiple code blocks are being linted, since some of the locations will probably be invalid. It might be better to just throw a validation error.
Is it worth adding preprocess and postprocess options to RuleTester? It would be possible to simulate those options outside of RuleTester, but it might be nice to have them as a convenience feature.
This effectively allows plugins to remove or add any message to the user's reported problems list, with no opt-in. Are we sure we're okay with that? (Actually, that was already possible with processors before this change, but processors haven't gotten a lot of use because they prevent autofixing. Now that they don't have downsides, I can imagine that plugins could use them just for the purpose of modifying the problems list.)

eslintbot · 2017-08-29T05:56:49Z

LGTM

eslintbot · 2017-08-30T08:20:26Z

LGTM

eslintbot · 2017-08-30T22:11:14Z

LGTM

not-an-aardvark · 2017-08-31T05:05:43Z

TSC Summary: This issue proposes allowing autofixing to occur when using processors, by having processors modify autofix locations as necessary as part of the postprocess method.

TSC Question: Should we accept this proposal? If so, should we make this available to processors by default, or add an opt-in for a processor to declare that it supports autofixing?

ilyavolodin

In general, I'm fine with those changes, however, I don't think they will cover all use-cases. And in some cases, they will make it impossible to restore code back into original format, I think. Like when you had to substitute some variables/names in preprocessor. You might not be able to find them again in postprocessor after fixes.

ilyavolodin · 2017-08-31T20:00:23Z

lib/cli-engine.js

+    const fixedResult = linter.verifyAndFix(text, config, {
+        filename,
+        allowInlineConfig,
+        fix: typeof fix !== "undefined" && fix,


This would result in performance impact, I would guess. Now we are trying to fix every error, even if we don't need to.

I don't think so -- SourceCodeFixer has a fast path to just skip all fixes when fix: false is used.

I mean in the case of processor not supporting autofixing (and not wanting to support it in the future). Autofixing will still happen, but it's results will be ignored by postprocess function.

Oh, I see. This could be addressed by making autofixing opt-in for processors (#9090 (comment)). Currently, fixes would still get applied by ESLint in this case, but the processor would usually generate broken code.

Opt-in sounds fine to me.

ilyavolodin · 2017-08-31T20:02:06Z

lib/linter.js

+     *      Mostly useful for testing purposes.
+     * @param {boolean} [filenameOrOptions.allowInlineConfig] Allow/disallow inline comments' ability to change config once it is set. Defaults to true if not supplied.
+     *      Useful if you want to validate JS without comments overriding rules.
+     * @param {function(string): string[]} [filenameOrOptions.preprocess] preprocessor for source text. If provided, this should


Since this is a public API, this would make it a breaking change.

Could you elaborate on why it would be a breaking change? It seems like this is just adding additional functionality, not breaking any existing functionality.

Ah, sorry, for some reason I though it's a new argument, not a new property on the object. I'm not really sure how to treat those. On one hand it's a change of a signature of publicly available API, on the other - JS works without it just fine, so it's not a breaking change, I guess.

I don't think there's any ambiguity here. Code that would previously work when using the API will still work, so it's not a breaking change. (This would also be the case if a new argument was added.)

ilyavolodin · 2017-08-31T20:03:07Z

lib/linter.js

+        const preprocess = filenameOrOptions && filenameOrOptions.preprocess || (rawText => [rawText]);
+        const postprocess = filenameOrOptions && filenameOrOptions.postprocess || lodash.flatten;
+
+        return postprocess(


I liked it a lot more when processors were limited to cli-engine, honestly, but I think that's a personal preference.

I share @ilyavolodin's preference here - organizationally, it feels like Linter shouldn't know about processors. I don't have a specific rule of thumb or design pattern or anything like that to support that feeling, unfortunately - maybe separation of concerns, but one could argue that's too broad.

However, #9090 (comment) clarified for me that postprocess needs to be run in between getting errors in verify and applying fixes. I don't see an easy way around that, so thankfully preprocess and postprocesscan be treated as black boxes from Linter's perspective.

btmills

I think this is on the right track. Other than making autofixing opt-in, I don't see any hangups. I'll give this another pass once that change is made. Nice work @not-an-aardvark!

btmills · 2017-09-04T21:42:07Z

docs/developer-guide/working-with-plugins.md

@@ -80,7 +80,7 @@ processors: {
 The `preprocess` method takes the file contents and filename as arguments, and returns an array of strings to lint. The strings will be linted separately but still be registered to the filename. It's up to the plugin to decide if it needs to return just one part, or multiple pieces. For example in the case of processing `.html` files, you might want to return just one item in the array by combining all scripts, but for `.md` file where each JavaScript block might be independent, you can return multiple items.

 The `postprocess` method takes a two-dimensional array of arrays of lint messages and the filename. Each item in the input
-array corresponds to the part that was returned from the `preprocess` method. The `postprocess` method must adjust the location of all errors and aggregate them into a single flat array and return it.
+array corresponds to the part that was returned from the `preprocess` method. The `postprocess` method must adjust the location of all errors, (including the range in the `fix` property, if present), and aggregate them into a single flat array and return it.


Noting that the text about adjusting the fix property, inserted after "must adjust the location of all errors" will need to change once processor autofixing is made opt-in - perhaps by moving it into its own sentence.

btmills · 2017-09-04T21:43:12Z

docs/user-guide/command-line-interface.md

-
-1. This option throws an error when code is piped to ESLint.
-1. This option has no effect on code that uses processors.
+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. Not all problems are fixable using this option, and this option throws an error when code is piped to ESLint.


Noting that this will also need to change slightly once autofixing in processors becomes opt-in to mention that a processor might not support it.

btmills · 2017-09-04T21:46:18Z

lib/linter.js

@@ -1181,6 +1210,11 @@ class Linter {
     * @param {boolean} options.allowInlineConfig Flag indicating if inline comments
     *      should be allowed.
     * @param {boolean|Function} options.fix Determines whether fixes should be applied
+     * @param {Function} options.preprocess preprocessor for source text. If provided, this should
+     *      this should accept a string of source text, and return an array of code blocks to lint.


"this should" is duplicated at the end of the previous line and the beginning of this line.

btmills · 2017-09-04T21:51:22Z

lib/linter.js

+        const preprocess = filenameOrOptions && filenameOrOptions.preprocess || (rawText => [rawText]);
+        const postprocess = filenameOrOptions && filenameOrOptions.postprocess || lodash.flatten;
+
+        return postprocess(


I share @ilyavolodin's preference here - organizationally, it feels like Linter shouldn't know about processors. I don't have a specific rule of thumb or design pattern or anything like that to support that feeling, unfortunately - maybe separation of concerns, but one could argue that's too broad.

However, #9090 (comment) clarified for me that postprocess needs to be run in between getting errors in verify and applying fixes. I don't see an easy way around that, so thankfully preprocess and postprocesscan be treated as black boxes from Linter's perspective.

not-an-aardvark · 2017-09-04T22:32:44Z

Thanks for the reviews!

Bikeshed: It looks like there's consensus that processors should have to opt-in to autofixing. What should the API for that look like? Right now, I'm thinking that processors could have a boolean supportsAutofix: true property in addition to preprocess and postprocess, and CLIEngine would verify that property is true before enabling autofix mode for Linter.

ilyavolodin · 2017-09-06T16:53:09Z

A boolean in the exported properties sounds good to me. One other question I have based on your list of things happening in this PR. How does this affect multi-pass autofixing? If we don't modify the code directly, we can't do multi-pass, however, if we do modify the code, postprocessor might not be able to figure out how to change it back to the original format (as I outlined in my comment).

not-an-aardvark · 2017-09-06T16:56:33Z

I described how multi-pass autofixing works in the original PR and this comment. Basically, preprocess and postprocess are run for on each pass, and then ESLint applies the postprocessed fixes to the original code. Is there a particular part of the explanation that you're confused about?

eslintbot · 2017-09-06T18:09:15Z

LGTM

eslintbot · 2017-09-06T18:22:30Z

LGTM

ilyavolodin · 2017-09-06T18:28:51Z

@not-an-aardvark Sorry, I forgot that original comments contained the explanation. That answers my question, and I think it should work just fine.

btmills

Nice work on this, @not-an-aardvark. Makes sense to me 👌

kaicataldo

Nice work @not-an-aardvark!

kaicataldo · 2017-09-11T16:12:18Z

lib/linter.js

+     *      Mostly useful for testing purposes.
+     * @param {boolean} [filenameOrOptions.allowInlineConfig] Allow/disallow inline comments' ability to change config once it is set. Defaults to true if not supplied.
+     *      Useful if you want to validate JS without comments overriding rules.
+     * @param {function(string): string[]} [filenameOrOptions.preprocess] preprocessor for source text. If provided, this should


Typo: this should is repeated in this comment

eslintbot · 2017-09-11T16:33:01Z

LGTM

eslintbot · 2017-09-12T03:07:25Z

LGTM

not-an-aardvark · 2017-09-14T23:40:42Z

This was accepted in today's TSC meeting.

not-an-aardvark requested a review from a team August 10, 2017 05:37

This was referenced Aug 11, 2017

Fix: don't autofix with linter.verifyAndFix when fix: false is used #9098

Merged

Plugin does not work with autofix eslint/eslint-plugin-markdown#58

Closed

not-an-aardvark force-pushed the processor-autofixes branch from c9cdaa9 to 6475a80 Compare August 14, 2017 04:00

not-an-aardvark force-pushed the processor-autofixes branch from c780ebc to e4a2622 Compare August 20, 2017 04:39

not-an-aardvark removed the do not merge This pull request should not be merged yet label Aug 20, 2017

not-an-aardvark changed the title ~~[WIP] Update: allow autofixing when using processors (fixes #7510)~~ Update: allow autofixing when using processors (fixes #7510) Aug 20, 2017

not-an-aardvark mentioned this pull request Aug 23, 2017

--fix mode not fixing anything with eslint-plugin-script-tags. #9142

Closed

This was referenced Aug 23, 2017

"--fix" mode not working? mapbox/eslint-plugin-script-tags#6

Closed

How to format code in <script> tags? prettier/prettier#2648

Closed

not-an-aardvark mentioned this pull request Aug 29, 2017

Chore: Make SourceCodeFixer accept text instead of a SourceCode instance #9178

Merged

not-an-aardvark force-pushed the processor-autofixes branch from e4a2622 to 01b23b8 Compare August 29, 2017 05:56

not-an-aardvark force-pushed the processor-autofixes branch from 01b23b8 to c6d7567 Compare August 30, 2017 08:20

not-an-aardvark force-pushed the processor-autofixes branch from c6d7567 to fdfd775 Compare August 30, 2017 22:11

not-an-aardvark added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Aug 31, 2017

ilyavolodin reviewed Aug 31, 2017

View reviewed changes

btmills reviewed Sep 4, 2017

View reviewed changes

not-an-aardvark force-pushed the processor-autofixes branch from f7a215d to 6ddc6c7 Compare September 6, 2017 18:22

btmills approved these changes Sep 11, 2017

View reviewed changes

kaicataldo approved these changes Sep 11, 2017

View reviewed changes

not-an-aardvark added 8 commits September 11, 2017 23:05

Update: allow autofixing when using processors (fixes #7510)

120beda

Don't pass a filename argument to processors when using Linter

350e282

Update documentation

493bb57

Add processor tests to Linter

bb2cae1

Fix repeated words in jsdoc comment

aa1779d

Add supportsAutofix opt-in for processors

baf4bb5

Fix linter errors

fb247a8

Remove duplicate text in comment

0acd744

not-an-aardvark force-pushed the processor-autofixes branch from 24d10db to 0acd744 Compare September 12, 2017 03:07

not-an-aardvark merged commit 0c720a3 into master Sep 15, 2017

not-an-aardvark deleted the processor-autofixes branch September 15, 2017 01:15

Arcanemagus mentioned this pull request Sep 19, 2017

fixOnSav does not work on source.js.embedded.html scope AtomLinter/linter-eslint#992

Closed

eslint-deprecated bot locked and limited conversation to collaborators Mar 15, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 15, 2018

Update: allow autofixing when using processors (fixes #7510) #9090

Update: allow autofixing when using processors (fixes #7510) #9090

Conversation

not-an-aardvark commented Aug 8, 2017 • edited

mention-bot commented Aug 8, 2017

eslintbot commented Aug 14, 2017

eslintbot commented Aug 14, 2017

BenoitZugmeyer commented Aug 14, 2017

eslintbot commented Aug 20, 2017

not-an-aardvark commented Aug 20, 2017

eslintbot commented Aug 29, 2017

eslintbot commented Aug 30, 2017

eslintbot commented Aug 30, 2017

not-an-aardvark commented Aug 31, 2017

ilyavolodin left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btmills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark commented Sep 4, 2017 • edited

ilyavolodin commented Sep 6, 2017

not-an-aardvark commented Sep 6, 2017 • edited

eslintbot commented Sep 6, 2017

eslintbot commented Sep 6, 2017

ilyavolodin commented Sep 6, 2017

btmills left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Sep 11, 2017

eslintbot commented Sep 12, 2017

not-an-aardvark commented Sep 14, 2017

not-an-aardvark commented Aug 8, 2017 •

edited

ilyavolodin left a comment •

edited

not-an-aardvark commented Sep 4, 2017 •

edited

not-an-aardvark commented Sep 6, 2017 •

edited