Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional option for --fix #10855

Closed
nzakas opened this issue Sep 12, 2018 · 19 comments

Comments

@nzakas
Copy link
Member

commented Sep 12, 2018

The problem you want to solve.

Currently, the --fix command forces ESLint to apply fixes for all rules that are currently enabled. While this is helpful some of the time, there is a distinct difference between stylistic rules and non-stylistic rules that this command doesn't take into account right now.

Developers tend to like autofixing style rules so they don't have to think about these little details. They tend to get annoyed when ESLint says to insert a space here or there. Non-stylistic rules, on the other hand, might be used as an educational tool. For example, prefer-const is arguably a good educational tool and something is lost when prefer-const is autofixed. Similarly, rules that enforce best practices are often more useful when not autofixed so developers can see the warning and better understand what will happen to their code when autofixed.

Your take on the correct solution to problem.

I'd like to add an option to the --fix command, such that you can do:

  • --fix problems - fixes only non-stylistic rules
  • --fix styles - fixes only stylistic rules
  • --fix all - fixes all rules

In each case, every rule is executed but only the specifies rule type will apply fixes, so --fix styles will also run the non-stylistic rules but will not apply fixes for non-stylistic rules.

Backwards Compatibility: The all option should be the default so --fix continues to function as it does right now.

I'd like to add a --fix-type flag that is used to specify which types of fixes to apply for both --fix and --fix-dry-run. The possible values are:

  • --fix-type problem only applies fixes for rules that are specified as problems
  • --fix-type suggestion only applies fixes for rules that are specified as suggestions
  • --fix-type style only applies fixes for rules that are specified as stylistic

You can specify multiple fix types using either commas (--fix-type problem,suggestion) or multiple flags (--fix-type problem --fix-type suggestion).

If you don't use --fix-type then all possible fixes are applied (same as current behavior).

Rough Implementation Details

  • Add a meta.type property to each rule (both core and plugin) that can be one of these values:
    • problem - the rule is flagging a pattern that might cause errors or confusion
    • suggestion - the rule is flagging a potentially better way of doing something
    • style - the rule is flagging a stylistic error that, when changed, does not change the meaning of the code
  • Use the fix option of CLIEngine to specify a function that uses the --fix-type information to filter out fixes that should not be applied.
  • When --fix-type is specified, all configured rules are executed but only rules matching the --fix-type will apply fixes.

Concrete Use Case

The primary use case I'm thinking of here is when people want to autofix styles in their code editor but do not want to autofix other problems in order to use ESLint as an educational tool.

Are you willing to implement this?

Yes! I'd love to dive into this as a way to start getting involved a bit more going forward. :) It may take me a while to implement based on my energy levels, but gotta start somewhere!

@nzakas nzakas self-assigned this Sep 12, 2018

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Sep 12, 2018

👋 Thanks for writing up this proposal.

A few questions/notes came to mind when reading this:

  • It seems like the distinction between a stylistic rule and a non-stylistic rule could be fuzzy in some cases. For example, my intuition would be to consider prefer-const a stylistic rule because the code will still work fine if let is used instead. I don't feel strongly about it and I'd be fine with calling prefer-const a non-stylistic rule, but I'm wondering if we would want a more precise guideline for what counts as stylistic.
  • You mentioned that the primary use case is in editor integrations. I'm not sure if you've seen, but in #8039 we recently added a hook for CLIEngine users to provide a filter function for fixes (although that hook isn't exposed to the CLI). If we added a good way for integrations to determine whether a rule is stylistic, do you think that would fulfill the same use case (since integrations could provide a filter function that only applies fixes from stylistic rules)? Or is it important that we make this functionality first-class for ease of use?
  • I would favor using something other than meta.docs.category to distinguish between stylistic/non-stylistic rules, because meta.docs.category is already used by some plugins for plugin-specific categories.
  • You mentioned that "rules that enforce best practices are often more useful when not autofixed so developers can see the warning and better understand what will happen to their code when autofixed." With some rules, such as no-useless-escape, we currently don't implement an autofixer because the warning might be signaling a bug in the user's code, which seems similar to the type of case you're describing here. If we added a way to only fix stylistic issues, is your expectation that we would add fixers for rules like no-useless-escape since the user would have the option to turn them off? More generally, I'm trying to get a better sense of the ways you envision this being used.
  • eslint --fix problems foo.js is already a valid ESLint invocation that does something else (it runs ESLint with the --fix flag on two files called problems and foo.js). Changing the functionality would technically be backwards-incompatible, although this case seems unlikely enough that it might not be worth addressing.
@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Sep 12, 2018

I'm fine with adding something like that in. I'm not sure about editors as primary use-case, since they can already do this by using meta.docs.category and either having a separate run with just the stylistic/problem rules, or using filtering technique that @not-an-aardvark mentioned above. But I think this might be useful for the command line users. My question is: What about plugins? Do we want to support this behavior for plugins? If not, how should the new commands behave? If we do, how do plugins identify rules to be fixed (I assume through meta property, just like core rules would).

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

@not-an-aardvark thanks for the feedback, let me try to answer your questions:

  1. I've always considered a rule to be stylistic if implementing the fix doesn't change the meaning of the code. So prefer-const I would consider non-stylistic because it changes (albeit slightly) the meaning of the code.
  2. Thanks for pointing out #8039, I did not see that! Very cool. That could definitely be used to implement my suggestion. I'm still of the mind that a command line option is important for this, but it seems like an editor could use the existing functionality plus a categorization mechanism to implement without a command line change.
  3. Yeah, I wasn't too keen on using meta.docs.category either, just wanted to throw it out there as a potential. I'd more like meta.type as "problem" or "style".
  4. I wouldn't change no-useless-escape. I'm not suggesting giving fine-grained control of autofixing, only to provide a very broad categorization in order to give some additional control to end users. To me, there are three categories of rule fixes:
  5. It is completely safe to make the fix because it does not change the meaning of the code (semi)
  6. It is somewhat safe to make the fix because it does not meaningfully change the meaning of the code (prefer-const)
  7. It is not safe to make the fix because the change significantly changes the meaning of the code (no-useless-escape)
  8. It is not safe to make the fix because it is not clear what the fix would be (no-empty)
  9. Nice catch! I'll label this as "breaking."

@ilyavolodin Yes, I'd envision whatever we use to flag rules as a particular type would be made available to plugins.

By the way, one of my biggest regrets is that we never really drew a firm line in the sand between rules that are stylistic and rules that are not, and I think the project has had some growing pains because of that. I'm hoping this can be one step in the direction of making things a bit more obvious.

@platinumazure platinumazure added evaluating and removed triage labels Sep 12, 2018

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

After giving this some thought, I'd like to revise my proposal a bit. There were a couple of things that bothered me about my proposal based on the feedback:

  1. It is a breaking change. I really didn't want to introduce breaking behavior.
  2. The prefer-const example that @not-an-aardvark brought up made me think that just having problems and styles wasn't actually specific enough.

So, I'd like to update my proposal as follows:

  1. Add a meta.type property to all rules (also available to plugins) that allows you to specify the type of rule. The type of rule would be singular, so "problem" not "problems".
  2. Allow three designations of rule types in order to match the three different type of fixes ESLint does:
    • problem - something that is a possible error (no-unexpected-multiline)
    • suggestion - something that isn't a possible error but you might want to change (prefer-const)
    • style - something that doesn't affect what the code means
  3. Instead of changing how --fix works, I propose instead adding a new --fix-type flag that lets you specify the type of fixes to apply. So you would use --fix --fix-type problem, for example. This has the added benefit of also working with --fix-dry-run, which is something my original proposal did not take into account. You can specify more than one fix type using commas (--fix-type problem,style) or multiple flags (--fix-type problem --fix-type style).

@nzakas nzakas removed the breaking label Sep 13, 2018

@platinumazure

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@nzakas I like the new proposal.

I'll note that your proposed types do roughly map to three of our existing categories:

  • problem = Possible Error
  • suggestion = Best Practices
  • style = Stylistic Issues

But I do like the idea of having a separate, singular type field.

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

@platinumazure thanks, and yes you're completely correct. I've always had a love-hate relationship with the meta.docs.category field. :)

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2018

TSC Summary: This proposal seeks to add a --fix-type flag that can limit the fixes that are applied by ESLint based on the type of rule. Rules would be updated with a meta.type property that is one of: problem, suggestion, or style. You could then do, for example, --fix-type problem to just fix problems.

TSC Question: Shall we accept this proposal?

@aladdin-add

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

Is there a possibility to use the meta.fixable? it can be "whitespace" or "code", but seems we are only checking its existence. if not exist, ESLint does not apply fixes even if the rule implements fix functions.
could someone explain a bit its design purpose?

eslint/lib/linter.js

Lines 832 to 834 in a6ebfd3

if (problem.fix && rule.meta && !rule.meta.fixable) {
throw new Error("Fixable rules should export a `meta.fixable` property.");
}

plz let me know if I'm missing something! 😄

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2018

@aladdin-add

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

well, thanks for the insights! Given that, I would slightly favor using the field meta.fixable, but allow it being something like "style" | "problem" | "suggest" , than creating a new field.

  • "whitespace" => "style"
  • "code" => "problem" & "suggest"

thoughts?

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2018

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

My concern with this proposal is that we are the ones who decide the brackets for new fixable feature, not the end user. While in most cases, there's a somewhat of a line between stylistic issue and a problem, there are a bunch of rules that could be considered either or, depending on the perspective. Would love to somehow allow end consumer to configure which rules should be fixed, and maybe provide a set of default to get started with (like eslint:recommended).

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2018

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

As I’ve mentioned previously, a stylistic change is any change that does not substantially change the meaning of the code.

I'm still not entirely clear on what this means based on your examples. I suspect I might be misunderstanding this definition.

To me, it seems like prefer-const does not substantially change the meaning of code; ostensibly, the code represents exactly the same behavior regardless of whether let or const is used.

I think using const instead of let is sometimes encouraged because it communicates addditional information to the reader which might not have been immediately obvious without looking at the surrounding code. But code authors make many choices like this to improve readability, some of which would typically be considered "stylistic" (e.g. deciding where to put blank lines, as a hint to indicate related sections of code). So it's not obvious to me that there's a clear line between prefer-const and "stylistic" rules.

I don't have strong preferences about where we draw the line, but I share @ilyavolodin's concern that the line will be ambiguous. It might be useful to have more examples of rules that are considered "stylistic" versus "non-stylistic".

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2018

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

I'd be fine with deferring the categorization of existing rules to the implementation phase. However, I'm currently unconvinced that a good place to draw the line exists at all. (Personally, I would consider all autofixable rules "stylistic" because they don't change the behavior of the code.) It could be that the distinction is intuitively clear to others but not to me.

My concern is that if we don't have a clear guideline for how to define whether a rule is stylistic, then plugins could be all over the place in how they categorize their rules, and as a result the flag might not end up being very useful to users. So I think having a general guideline is important for deciding whether to accept the feature, even if we make tweaks to the guideline after accepting it. As I mentioned above, the current guideline of whether an autofixer "substantially changing the meaning of the code" is confusing to me, because as I interpret the guideline, it's not clear that any existing autofixers actually do that.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

This issue was accepted in today's TSC meeting.

@not-an-aardvark not-an-aardvark added accepted and removed evaluating labels Sep 28, 2018

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2018

From the TSC discussion, we agreed on a general definition that style rules do not modify the AST as part of a fix. Suggestion and problem rules may modify the AST as part of a fix.

@IanVS

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

I’m not sure there’s a big need for per-rule
configuring of autofix but even if there is, I don’t see that feature as
mutually exclusive to this one.

FWIW, if anyone does need per-rule fixing, I threw together https://github.com/IanVS/eslint-filtered-fix a while back, which does just that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.