diff --git a/designs/2021-fixable-disable-directives/design.md b/designs/2021-fixable-disable-directives/design.md new file mode 100644 index 00000000..c04ea08c --- /dev/null +++ b/designs/2021-fixable-disable-directives/design.md @@ -0,0 +1,136 @@ +- Repo: eslint/eslint +- Start Date: 2021-04-11 +- RFC PR: https://github.com/eslint/rfcs/pull/78 +- Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg) + +# Fixable Disable Directives + +## Summary + +Inline disable directives such as `/* eslint-disable */` that do not suppress any violations may be detected with the `--report-unused-disable-directives` CLI option, but there is no way to automatically remove those comments. +This RFC proposes adding removal of those directives to the `--fix` CLI option as a new [`--fix-type`](https://eslint.org/docs/user-guide/command-line-interface#-fix-type): _**`directive`**_. + +## Motivation + +Manually deleting comments from `--report-unused-disable-directives` is cumbersome, especially in large repositories with many directives. +Users would benefit from a quick way to fix these without having to manually map between CLI output lines and files. + +## Detailed Design + +Recapping the existing flow of code: + +- When the `--fix-type` CLI option is specified, `options.fix` is patched to filter on the `rule.meta` of each fix's corresponding `ruleId` +- Unused disable directives are calculated by an `applyDirectives` function within the `applyDisableDirectives` function called in the linter's `_verifyWithoutProcessors` + - These problems have a `ruleId` of `null` +- `_verifyWithoutProcessors` is called within the call stack of each of the 1-10 passes in the linter's `verifyAndFix` + +This RFC proposes making three changes: + +- In the patched `options.fix`, consider a problem's meta type to be `"directive"` if its `ruleId` is `null` +- Pass the `SourceCode` being linted as a parameter to `applyDisableDirectives` +- Add a `fix` method to the unused directive problems returned by `applyDirectives` that uses the `SourceCode` + +### Fix Behavior + +Directives where at least one rule is still used will have only the unused rule names removed from their source text. + +Directives where all >=1 rules are unused will use the `SourceCode` to compute: + +- If they are the only non-whitespace on their line, delete that line +- Otherwise, delete just the comment and any now-unnecessary surrounding whitespace + +#### Fix Behavior Examples + +```diff +- /* eslint-disable */ +``` + +```diff +- // eslint-disable-next-line -- related explanation +``` + +```diff +- // eslint-disable-next-line unused, used ++ // eslint-disable-next-line used +``` + +```diff +- before /* eslint-disable-next-line -- related explanation */ after ++ before after +``` + +```diff +- before // eslint-disable-next-line -- related explanation ++ before +``` + +```diff + // before +- // eslint-disable-next-line + // after +``` + +Multiline block comments are already not allowed to be disable directives. [eslint/eslint#10334](https://github.com/eslint/eslint/issues/10334) + +## Documentation + +- [Command Line Interface > Fixing Problems](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems)'s `--fix` documentation should mention the added directives fixing and the new `--fix-type` +- [Rules > Report Unused ESLint Disable Comments](https://eslint.org/docs/user-guide/configuring/rules#report-unused-eslint-disable-comments) should link to that documentation + +## Drawbacks + +Like any new feature, this flag will slightly increase the complexity and maintenance costs of ESLint core. + +This RFC's implementation would lock in the name for a new `--fix-type` even though we only have one concrete use case for it. + +## Backwards Compatibility Analysis + +It is unlikely but not impossible that some `--fix` dependant users would rely on unused disable directives remaining in code. + +Otherwise, this change is additive in behavior. + +## Alternatives + +- Applying directive fixes after the `verifyAndFix` passes + - Not ideal: deleting a comment could introduce new rule violations that would warrant running >=1 other pass +- Writing an external tool to apply these options + - Not ideal: the internal implementation is simple; external would have to deal with variant formatters, etc and suffer from the same introduced rule violations + +## Open Questions + +- Is `"directive"` a good name for the fix type? + - It feels like it could be too specific to be extended for other meta/configuration in the future. + - This RFC originally proposed `"meta"` but that goes too far in being overly vague. + +## Help Needed + +I'm looking forward to implementing this if approved! 🙌 + +## Frequently Asked Questions + +> Will these be autofixed by default? + +If `--fix` and `--report-unused-disable-directives` are both true, then yes. +There is no additional configuration that would need to be provided. + +If `--fix` is true but `--report-unused-disable-directives` is not, there will be no behavior change from this RFC. +Unused directives would not add to reported problems and thus no new fixers would be added to them. + +If `--fix` is not true but `--report-unused-disable-directives` is, there will be no observable change from this RFC for CLI clients. +Problems for unused directives will have a `fix` property attached but it will not be used without `--fix`. + +> Can I opt out? + +Yes. +If you are in the peculiar situation of needing to enable `--fix` and `--report-unused-disable-directives` _without_ fixing those directives _(why?)_, you can use `--fix-type` with all types except `directive`. + +> How do the generated fixes compare to fixes from rules? + +Problems reported by directive usage checking are joined with remaining rule violation problems in a single array. +This should allow directive fixing to seamlessly act similarly to rule fixing. + +## Related Discussions + +- [eslint/eslint#10334](https://github.com/eslint/eslint/issues/10334) - Report an error for eslint-disable-line comments that span multiple lines #1033) +- [eslint/eslint#9249](https://github.com/eslint/eslint/issues/9249) - Add CLI option to report unused eslint-disable directives +- [eslint/eslint#11815](https://github.com/eslint/eslint/issues/11815) - --report-unused-disable-directives should be autofixable