New: Suggestions

[ilyavolodin]

Summary

This RTC proposes introducing a new mechanism for providing users with suggestions. It's very similar to fixes mechanism that already exists in ESLint, but will provide ability to create multiple suggestions per rule, suggestions will not be automatically applied by using --fix flag, and will require user interaction. This feature will only be exposed through the ESLint API, and will not be available on the CLI.

Related Issues

eslint/eslint#7873
eslint/eslint#6733 (comment)

[mysticatea]

Thank you for your proposal.

I failed to read how this RFC changes public APIs. Would you elaborate about it?

[platinumazure]

Thanks for the RFC!

I guess my main question is... Why do suggestions need to be separate from autofixes? Could we just add an attribute to autofixes which says whether it's a suggestion or meant to be a safe autofix? What value do suggestions add that autofix does not or cannot provide via an enhancement to autofix?

[ilyavolodin]

There may be many suggestions per rule error, none of them should be applied automatically at any point. That behavior feels different enough to me to make it into it's own thing, and not try to piggy back on autofixes.

[ilyavolodin]

@mysticatea I updated PR with your suggestions and some additional changes. Please take a look when you have a chance. CC: @gaearon

[not-an-aardvark]

Could you give an example of a case where there would be many suggestions for a rule error?

[mysticatea]

Thank you for the update.

I have more some suggestions.

• About how editor integrators use the suggestions properties.
• About processors which don't support autofix.

[mysticatea]

I have one example, no-useless-escape rule. We have given up fixing the errors of no-useless-escape rule because it can be two cases. We can use suggestions for that.

• Remove this \.
• Use \\ instead.

[platinumazure]

Why not just allow autofixes to include a new Boolean property to say if it's safe to apply as an autofix or not?

It seems the mechanism for describing what needs to change, at least, is exactly the same as autofix, so I feel we should reuse that if we can. I think the difference is we would be able to have more suggestions/fixes (such as no-useless-escape, sort-keys, etc.) but only specific fixes are applied automatically. Or we could have multiple levels of fixes based on how "safe" we think they are, and autofix only applies the safest fixes.

[mysticatea]

@platinumazure How do you think to handle multiple suggestions on fix?

[platinumazure]

@platinumazure How do you think to handle multiple suggestions on fix?

The same way as currently: Treat them as a combined fix for collision purposes, don't apply the fix automatically if it does collide. In the case of suggestions (which I am interpreting as fixes not applied automatically in my proposal), then the suggestion isn't applied anyway. It would be up to the integration to decide how to apply the fixes.

I could see us considering some quality-of-life options, such as not showing suggestions if autofixes conflict, not retrieving suggestions until all fixes are applied, etc., but I don't have anything specific in mind.

[mysticatea]

@platinumazure No, we cannot combine two suggestions because the two are different solutions for the same error. For example,

new RegExp("*\.js", "u")
//           ^^
//           ├ Remove the '\'. (no-useless-escape)
//           └ Replace the '\' by '\\'. (no-useless-escape)

no-useless-escape provides two solutions for a \, then people will choose the correct solution on their editor integration.

[platinumazure]

Oh, that's a key insight. Okay, I agree, that changes things.

Maybe we could require that every report have either: Suggestions (array), fix (single fixable object), or neither. And suggestions shouldn't be automatically applied. I think now I'm on the same page as @ilyavolodin (more or less).

[ilyavolodin]

@platinumazure Sorry about not providing concrete example. I was struggling to come up with simple enough example of suggestion that would be visually easy to understand. I think one problem that I see with suggestions is that they might be taken too far, as in suggest refactoring of a large chunk of code. I think that's outside of the responsibilities of ESLint.
But another example might be a custom rule that somebody might write that would suggest replacing callback pattern with either promises or async/await. Neither way is safe to do automatically, and neither is a "correct" solution, but a user can choose what he prefers. One more example might be a custom rule that sees _.clone() and suggests to add import of either lodash or underscore, or maybe even suggest replacing it completely with Object.assign in some cases. Again, user has to decide which one he wants to apply.
Fixes are basically yes/no question. And suggestions are multiple choice (in most cases).

[ilyavolodin]

@mysticatea Updated again to incorporate your suggestions. Thank you very much for those, they are very helpful!

[mysticatea]

LGTM with a minor suggestion. Thank you!

[ilyavolodin]

Moving this into final commenting stage. @eslint/eslint-tsc please take a look.

[gaearon]

@ilyavolodin Thank you so much for driving this!

If there’s anything I can clarify about our use case for this (for React Hooks) I’d be happy to provide details. (I linked to a few issues which you can see above.)

[platinumazure]

I'm hoping to review this in the next day or two. Any chance we could wait to merge until, say, Wednesday? (If I don't review by then, just go ahead and merge.)

Sorry, I've been traveling and forgot about this RFC.

[ilyavolodin]

Going to merge this in, as it doesn't seem like there are any objections.

[wdoug]

I see that the process for getting an RCF approved here is entirely independent of the actual implementation. @ilyavolodin, are you planning on working on the implementation of this feature, or, if not, do you know if anyone is planning on working on it?

[gaearon]

I don't think anyone is working on it. Would you like to? It would be an amazing contribution.

[ilyavolodin]

@wdoug Unfortunately, I don't have time to work on this right now. So if you would like to give it a try, that would be great!

[wdoug]

I would like to help if I can. I'll see if I can get familiar enough with the codebase to get a pull request up in my free time. If you end up having time to work on it before I get something up @ilyavolodin just let me know.

[gaearon]

@wdoug Thanks for looking into it!

[wdoug]

@ilyavolodin, if you get a chance to check it out, I opened a PR for this here that could use some feedback and verification that I understood the RFC correctly.