feat: Per rule autofix configuration #125
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,179 @@ | ||
| - Repo: <https://github.com/eslint/eslint> | ||
| - Start Date: 2024-10-22 | ||
| - RFC PR: | ||
| - Authors: [Samuel Therrien](https://github.com/Samuel-Therrien-Beslogic) (aka [@Avasam](https://github.com/Avasam)) | ||
|
|
||
| # Per-rule autofix configuration | ||
|
|
||
| ## Summary | ||
|
|
||
| <!-- One-paragraph explanation of the feature. --> | ||
| This feature aims to make it possible to control autofixes through shareable configuration on a per-rule basis. | ||
|
|
||
| ## Motivation | ||
|
|
||
| <!-- Why are we doing this? What use cases does it support? What is the expected | ||
| outcome? --> | ||
| Some rules provide autofixing, which is great, but can sometimes be broken or otherwise simply unwanted for various reasons. | ||
| Unsafe autofixes should be suggestions, and broken fixes should be reported, *but* ESLint is a large ecosystem where some very useful plugins are not always actively maintained. Even then, wanting to disable an autofix for project-specific or personal reasons could still happen. | ||
|
|
||
| ## Detailed Design | ||
|
|
||
| <!-- | ||
| This is the bulk of the RFC. | ||
|
|
||
| Explain the design with enough detail that someone familiar with ESLint | ||
| can implement it by reading this document. Please get into specifics | ||
| of your approach, corner cases, and examples of how the change will be | ||
| used. Be sure to define any new terms in this section. | ||
| --> | ||
|
|
||
| Similar to how Ruff (<https://docs.astral.sh/ruff/settings/#lint_unfixable>) does it, a top-level key to specify which rules to not autofix would be in my opinion the least disruptive and forward/backwards compatible. It should be overridable in per-file configurations, and picked up when extending a configuration. | ||
|
There was a problem hiding this comment. An alternative could be to expand rule config values to allow objects in which this would be a property. For example: export default [
{
rules: {
"@eslint-community/eslint-comments/no-unused-disable": {
severity: "error",
options: [/* ... */],
disableAutofixes: true
}
}
}
];The advantages of this approach are that all the configurations for the rule can be in one place, and it would be easier to add more "meta" options if needed in the future. There was a problem hiding this comment. We considered this a while back, even without trying to turn off autofixing. I'm not a fan of forcing rules to have to write out "severity" and "options", which is why we stuck with just an array. This would also complicate rule configuration merging. There was a problem hiding this comment. Given a large number of config in the community are already in array format, I don't think we can drop it; instead, we can support both: "@eslint-community/eslint-comments/no-unused-disable": ["error", {...}]
// is as the same as:
"@eslint-community/eslint-comments/no-unused-disable": {
severity: "error",
options: [/* ... */],
disableAutofixes: false, // the default
}There was a problem hiding this comment. I don't think anyone was suggesting dropping the current format. I just think an object format adds additional complexity when merging rule configurations that isn't necessary. Plus, to modify a rule to disable autofix, you'd first need to convert the array into an object vs. adding a new key elsewhere. There was a problem hiding this comment.
Another advantage is that autofixes could be turned off from the CLI (with There was a problem hiding this comment. Changing the rule config format was a considered option (although not too much thoughts was put into what exactly that change would entail). There was a problem hiding this comment. @fasttime this is additional syntax, though. Config comments and the command line don't already support this object notation. There was a problem hiding this comment.
Well, true. I should have written "with the same new syntax". My point is that rules can be configured in multiple ways, but the suggested approach only allows disabling autofixes inside a config. I think while we are here, we should at least ask the question if it's sensible to disable autofixes from the CLI and in config comments. If the answer is no, we can go with the suggested approach and be assured that we won't have to rethink the design later. There was a problem hiding this comment.
Config comments I wanna say no. Wanting to autofix or not is something to decide at the time of use. So when a user action is triggered (autofix on save in editor, which is already configurable in VSCode) or when calling the CLI (either on a CI or locally) by reading configuration files. I find it really hard to imagine a rule that's you'd want to sometimes autofix, and sometimes not. Especially since autofixes don't change whether your linting "failed/passed" as a whole (I mean in the general idea that either something is reported or not, not about the return code 0/1). If a rule is "sometimes too fragile to fix", it should never be autofixed. If it's incidentally supported through inline configs, sure, but I wouldn't put any effort towards supporting it. Whether this should be configurable from the CLI: Prior art says yes. Looking at Ruff https://docs.astral.sh/ruff/configuration/#full-command-line-interface, their CLI accepts But even then with ESLint's way of having any config overridable on a per-file-selector basis (which Ruff doesn't have) and 9.12's Implement alternate config lookup (eslint/eslint#18742). I can hardly see a scenario where the CLI flag needs to be passed to disable specific rules' autofixes. Over simply using file selectors or nested configs in subfolders. and/or passing the paths to lint on the CLI. |
||
|
|
||
| Concretely, it could look like this: | ||
|
|
||
| ```js | ||
| export default [ | ||
| { | ||
| disableAutofixes: { | ||
| // We don't want this to autofix, as a rule suddenly not failing should require human attention | ||
| "@eslint-community/eslint-comments/no-unused-disable": true, | ||
| }, | ||
|
Comment on lines
+38
to
+41
There was a problem hiding this comment. I just learned about https://eslint.org/docs/latest/use/configure/configuration-files#reporting-unused-disable-directives . Which I believe should be possible to disable the autofix just like any other rules as proposed by this RFC. However, it doesn't expose itself as a rule. How should this be handled? I could:
export default [{
linterOptions: {
reportUnusedDisableDirectives: "error",
autofixUnusedDisableDirectives: false
}
}];
export default [{
linterOptions: {
reportUnusedDisableDirectives: ["error", {autofix: false}]
}
}];
export default [{
disableAutofixes: {
// We don't want this to autofix, as a rule suddenly not failing should require human attention
"reportUnusedDisableDirectives": true,
},
}];
Comment on lines
+38
to
+41
There was a problem hiding this comment. Should |
||
| rules: { | ||
| '@eslint-community/eslint-comments/no-unused-disable': 'error', | ||
| } | ||
| }, | ||
| { | ||
| files: ["*.spec.js"], | ||
| disableAutofixes: { | ||
| // Let's pretend we want this to be autofixed in tests, for the sake of the RFC | ||
| "@eslint-community/eslint-comments/no-unused-disable": false, | ||
| }, | ||
| }, | ||
| ] | ||
| ``` | ||
|
|
||
| The fix should still exist as a suggestion. Only autofixing (when running `eslint --fix` or editor action on save) should be disabled. | ||
Samuel-Therrien-Beslogic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| This means removing the `LintMessage.fix` object into the `LintMessage.suggestions` array ([`LintMessage` API](https://eslint.org/docs/latest/integrate/nodejs-api#-lintmessage-type)) | ||
|
|
||
| ```js | ||
| const newSuggestion = { | ||
| desc: 'Apply the disabled autofix', | ||
| fix: lintMessage.fix, | ||
| } | ||
| if (!lintMessage.suggestions) { | ||
| lintMessage.suggestions = [newSuggestion] | ||
| } else { | ||
| lintMessage.suggestions.push(newSuggestion) | ||
| } | ||
| lintMessage.fix = undefined | ||
| ``` | ||
|
|
||
| The chosen key name `disableAutofixes` aims to remove the concern about "turning on" an autofix that doesn't exist. Disabling autofixes for a rule that doesn't have any or doesn't exist should be a no-op. Just like turning `off` a rule that doesn't exist. The reasoning being that this allows much more flexible shareable configurations. | ||
|
|
||
| ## Documentation | ||
|
|
||
| <!-- | ||
| How will this RFC be documented? Does it need a formal announcement | ||
| on the ESLint blog to explain the motivation? | ||
| --> | ||
| I think that "Configuring autofixes" or "Disabling autofixes" could be documented as a subsection of [Configuring Rules](https://eslint.org/docs/latest/use/configure/rules). Or as a section on the same level (between "Configuring Rules" and "Configuring Plugins") | ||
Samuel-Therrien-Beslogic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| As a new top-level property added to configuration objects, `disableAutofixes` should be documented in [Configuration Files > Configuration Objects](https://eslint.org/docs/latest/use/configure/configuration-files#configuration-objects) section. Additionally we may want to add a note to [Custom Rules](https://eslint.org/docs/latest/extend/custom-rulesd) to mention that some autofixes will be converted automatically into suggestions when the new feature is used. | ||
|
|
||
| ## Drawbacks | ||
|
|
||
| <!-- | ||
| Why should we *not* do this? Consider why adding this into ESLint | ||
| might not benefit the project or the community. Attempt to think | ||
| about any opposing viewpoints that reviewers might bring up. | ||
|
|
||
| Any change has potential downsides, including increased maintenance | ||
| burden, incompatibility with other tools, breaking existing user | ||
| experience, etc. Try to identify as many potential problems with | ||
| implementing this RFC as possible. | ||
| --> | ||
| A potential drawback I could see is that the configuration for autofixing a rule is not directly related with the rule itself. As a counter, I'd say this is already the case for plenty of rule-related settings, environment and parser configurations, etc. It's also less of a drawback than [Alternatives - Configure in the rule itself](#configure-in-the-rule-itself). | ||
|
|
||
| ## Backwards Compatibility Analysis | ||
|
|
||
| <!-- | ||
| How does this change affect existing ESLint users? Will any behavior | ||
| change for them? If so, how are you going to minimize the disruption | ||
| to existing users? | ||
| --> | ||
| Given that this proposal adds a new optional configuration section, this feature should be fully backwards compatible. Users that don't want to use this feature should stay completely unaffected. (see [Alternatives - Configure in the rule itself](#configure-in-the-rule-itself)) | ||
|
|
||
| ## Alternatives | ||
|
|
||
| <!-- | ||
| What other designs did you consider? Why did you decide against those? | ||
|
|
||
| This section should also include prior art, such as whether similar | ||
| projects have already implemented a similar feature. | ||
| --> | ||
|
|
||
| ### Configure in the rule itself | ||
|
|
||
| Another approach I can think of is to encode that in the rule config itself. Something like `"my-plugin/my-rule": "[{severity: "error", autofix: False}, {...otherConfigs}]"` but it's harder to commit to such a change, and means that any config extension needs to reconfigure the rule correctly just to disable autofixing (which is already an issue when someone wants to set a pre-configured rule as warning for example) | ||
|
|
||
| ### Use of a 3rd-party plugin | ||
|
|
||
| <https://www.npmjs.com/package/eslint-plugin-no-autofix> is a tool that exists to currently work around this limitation of ESLint, but it is not perfect. | ||
|
|
||
| 1. It is an extra third-party dependency, with its own potential maintenance issues (having to keep up with ESLint, separate dependencies that can fall out of date, obsolete, unsecure, etc.) | ||
| 2. It may not work in all environments. For example, pre-commit.ci: <https://github.com/aladdin-add/eslint-plugin/issues/98> | ||
| 3. It may not work correctly with all third-party rules: <https://github.com/eslint-community/eslint-plugin-eslint-comments/issues/234> | ||
|
|
||
| ## Open Questions | ||
|
|
||
| <!-- | ||
| This section is optional, but is suggested for a first draft. | ||
|
|
||
| What parts of this proposal are you unclear about? What do you | ||
| need to know before you can finalize this RFC? | ||
|
|
||
| List the questions that you'd like reviewers to focus on. When | ||
| you've received the answers and updated the design to reflect them, | ||
| you can remove this section. | ||
| --> | ||
| - Where exactly should the documentation go ? | ||
| - Where this needs to be implemented in code. Those familiar with ESLint's codebase are welcome to provide this information | ||
|
|
||
| ## Help Needed | ||
|
|
||
| <!-- | ||
| This section is optional. | ||
|
|
||
| Are you able to implement this RFC on your own? If not, what kind | ||
| of help would you need from the team? | ||
| --> | ||
| My knowledge of ESLint's internals isn't that great. Whilst I think it's above the average user due to reading and configuring a lot, I haven't yet even learned how to write a plugin, and haven't migrated any project to ESLint 9 yet. | ||
| My free time both at work and personal, is currently also very limited (see how long it too me to just get to writing this RFC). | ||
| So I unfortunately don't think I can implement this feature myself, due to both a lack of time, personal motivation (I won't be able to use it for a while, but will push us towards ESLint 9 once implemented), and experience. | ||
|
|
||
| ## Frequently Asked Questions | ||
|
|
||
| <!-- | ||
| This section is optional but suggested. | ||
|
|
||
| Try to anticipate points of clarification that might be needed by | ||
| the people reviewing this RFC. Include those questions and answers | ||
| in this section. | ||
| --> | ||
|
|
||
| Q: Could `disableAutofixes` be an array of autofixes to disable? | ||
| A: `disableAutofixes` is a record to allow re-enabling autofixes in downstream configurations and on a per-file basis. We could allow a shorthand to `disableAutofixes` to accept an array of rules to disable the autofix for, but that would result in additional complexity on the implementation side with marginal benefits to the user. | ||
|
|
||
| ## Related Discussions | ||
|
|
||
| <!-- | ||
| This section is optional but suggested. | ||
|
|
||
| If there is an issue, pull request, or other URL that provides useful | ||
| context for this proposal, please include those links here. | ||
| --> | ||
| - <https://github.com/eslint/eslint/issues/18696> | ||
| - <https://github.com/aladdin-add/eslint-plugin/issues/98> | ||
| - <https://github.com/eslint-community/eslint-plugin-eslint-comments/issues/234> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing details on how this will be implemented. Please take a look at the code and see how you expect this feature to be implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I really need to learn about how ESLint is implemented I can, but idk when I'll get to it. I'd be more than happy if an existing contributor is willing to fill in this section for me as I am not personally concerned with implementation details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code a bit, I would say this is probably a good place to start exploring: https://github.com/eslint/eslint/blob/main/lib/eslint/eslint.js#L386-L394
It's what's being used to determine if a rule should have a fix applied to it, based on a few different conditions. Based on the response from that, a "fixer" is passed back to the linter to use as part of its fix attempt loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Samuel-Therrien-Beslogic exploring the implementation is part of the RFC process. We can't really evaluate any proposal without it.