From 69f688048e880a5f01d11cf78b1eaeb13258ae9e Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 11 Apr 2021 23:11:44 -0400 Subject: [PATCH 1/6] New: Fixable Disable Directives --- .../2021-fixable-disable-directives/design.md | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 designs/2021-fixable-disable-directives/design.md diff --git a/designs/2021-fixable-disable-directives/design.md b/designs/2021-fixable-disable-directives/design.md new file mode 100644 index 00000000..77a766ae --- /dev/null +++ b/designs/2021-fixable-disable-directives/design.md @@ -0,0 +1,104 @@ +- Repo: eslint/eslint +- Start Date: 2021-04-11 +- RFC PR: (leave this empty, to be filled in later) +- 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): _**`meta`**_. + +## 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 `"meta"` 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` to compute: + - For comments that 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 +- 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 `"meta"` a good name for the fix type? + - I'd considered `"directive"`, but that felt like it could be too specific to be extended for other meta/configuration in the future. + +## Help Needed + +I'm looking forward to implementing this if approved! 🙌 + +## Frequently Asked Questions + +You tell me! + +## 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 From 91317ee283ee10f2a71ed63cb19988ebf02ac024 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 12 Apr 2021 17:35:04 -0400 Subject: [PATCH 2/6] First two FAQs: autofix by default, opt out --- designs/2021-fixable-disable-directives/design.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/designs/2021-fixable-disable-directives/design.md b/designs/2021-fixable-disable-directives/design.md index 77a766ae..27dd6806 100644 --- a/designs/2021-fixable-disable-directives/design.md +++ b/designs/2021-fixable-disable-directives/design.md @@ -95,7 +95,15 @@ I'm looking forward to implementing this if approved! 🙌 ## Frequently Asked Questions -You tell me! +> Will these be autofixed by default? + +Yes: 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. + +> 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 `meta`. ## Related Discussions From c87e3b96b76d55006607ebd82ae6d9c7f19b5a3a Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 12 Apr 2021 21:14:49 -0400 Subject: [PATCH 3/6] Clarified default CLI behavior --- .../2021-fixable-disable-directives/design.md | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/designs/2021-fixable-disable-directives/design.md b/designs/2021-fixable-disable-directives/design.md index 27dd6806..4c159b23 100644 --- a/designs/2021-fixable-disable-directives/design.md +++ b/designs/2021-fixable-disable-directives/design.md @@ -8,7 +8,7 @@ ## 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): _**`meta`**_. +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 @@ -26,7 +26,7 @@ Recapping the existing flow of code: This RFC proposes making three changes: -- In the patched `options.fix`, consider a problem's meta type to be `"meta"` if its `ruleId` is `null` +- 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` to compute: - For comments that are the only non-whitespace on their line, delete that line @@ -86,8 +86,9 @@ Otherwise, this change is additive in behavior. ## Open Questions -- Is `"meta"` a good name for the fix type? - - I'd considered `"directive"`, but that felt like it could be too specific to be extended for other meta/configuration in the future. +- 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 @@ -97,13 +98,24 @@ I'm looking forward to implementing this if approved! 🙌 > Will these be autofixed by default? -Yes: 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. +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 `meta`. +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 From 2027aab23d2daeca3344dbee03559cc7adb1b2e0 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 13 Apr 2021 00:14:44 -0400 Subject: [PATCH 4/6] Elaborated on fixes to account for remaining used rules --- .../2021-fixable-disable-directives/design.md | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/designs/2021-fixable-disable-directives/design.md b/designs/2021-fixable-disable-directives/design.md index 4c159b23..077422ed 100644 --- a/designs/2021-fixable-disable-directives/design.md +++ b/designs/2021-fixable-disable-directives/design.md @@ -28,11 +28,18 @@ 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` to compute: - - For comments that are the only non-whitespace on their line, delete that line - - Otherwise, delete just the comment and any now-unnecessary surrounding whitespace +- Add a `fix` method to the unused directive problems returned by `applyDirectives` that uses the `SourceCode` -### Fix Behavior Examples +### 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 */ @@ -42,6 +49,11 @@ This RFC proposes making three changes: - // 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 From 87ff30dccec835263ba523e0ce0707518153afa8 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 13 Apr 2021 00:16:30 -0400 Subject: [PATCH 5/6] Remaining --> --- designs/2021-fixable-disable-directives/design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2021-fixable-disable-directives/design.md b/designs/2021-fixable-disable-directives/design.md index 077422ed..9730c41a 100644 --- a/designs/2021-fixable-disable-directives/design.md +++ b/designs/2021-fixable-disable-directives/design.md @@ -37,7 +37,7 @@ Directives where at least one rule is still used will have only the unused rule 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 --> +- Otherwise, delete just the comment and any now-unnecessary surrounding whitespace #### Fix Behavior Examples From 6e89415466f870c562922fa1aa6fdcaa59fa371d Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Wed, 28 Apr 2021 10:19:00 -0700 Subject: [PATCH 6/6] Update designs/2021-fixable-disable-directives/design.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: 薛定谔的猫 --- designs/2021-fixable-disable-directives/design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2021-fixable-disable-directives/design.md b/designs/2021-fixable-disable-directives/design.md index 9730c41a..c04ea08c 100644 --- a/designs/2021-fixable-disable-directives/design.md +++ b/designs/2021-fixable-disable-directives/design.md @@ -1,6 +1,6 @@ - Repo: eslint/eslint - Start Date: 2021-04-11 -- RFC PR: (leave this empty, to be filled in later) +- RFC PR: https://github.com/eslint/rfcs/pull/78 - Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg) # Fixable Disable Directives