New: Fixable Disable Directives #78

JoshuaKGoldberg · 2021-04-12T03:15:48Z

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: directive.

Related Issues

eslint/eslint#10334 - Report an error for eslint-disable-line comments that span multiple lines #1033)
eslint/eslint#9249 - Add CLI option to report unused eslint-disable directives
eslint/eslint#11815 - --report-unused-disable-directives should be autofixable

ljharb · 2021-04-12T04:16:33Z

If this won’t be autofixed by default (it should be) I would like a way to configure it in eslintrc (and a shared config) so that it is.

designs/2021-fixable-disable-directives/design.md

nzakas · 2021-04-13T00:34:26Z

@ljharb fixing can’t be toggled in a config file. It’s a separate mode that ESLint runs in and you don’t want someone else’s config that you extend determining whether or not fixes are applied.

nzakas

Thanks for putting this together. As mentioned elsewhere, I think there is some additional complexity around expectations with certain CLI flag usage that we need to dig deeper into.

designs/2021-fixable-disable-directives/design.md

nzakas · 2021-04-13T00:43:36Z

designs/2021-fixable-disable-directives/design.md

+
+> Will these be autofixed by default?
+
+Yes: problems reported by directive usage checking are joined with remaining rule violation problems in a single array.


I’m not sure having this fixable just when using —fix is the right approach. Right now we don’t fix rules that wouldn’t otherwise output warnings. Since unused directives are not warned by default, I don’t think they should be auto fixed by default. Maybe if both —fix and —report-unused-disable-directives are used, then it would make sense to remove them, or else a separate flag might be a better way to go.

wait, are these not reported by default? i don’t think I’ve ever provided the flag, but I’m pretty sure I’ve seen the output by default

I phrased this poorly, and this part of the RFC is inaccurate. This was intended to convey that the fixers for unused directives will be added to reported problems "by default" -- as in, if --report-unused-disable-directives is true, there's nothing else you need to do to have the fixers be created. They still won't be applied if --fix is not true.

I.e. you must provide both --fix and --report-unused-disable-directives for them to be fixed.

I'll clarify this wording.

Oh, and no, --report-unused-disable-directives is IME off by default.

Added FAQs in c87e3b9.

is it that the flag is required to make them errors, but they're warnings by default?

It's off by default on the CLI.

Default CLI value: https://github.com/eslint/eslint/blob/08ae31e539e381cd0eabf6393fa5c20f1d59125f/conf/default-cli-options.js#L30

Options processing in the CLI: https://github.com/eslint/eslint/blob/49d16977d969070e5240074e76036f56631a90d3/lib/cli.js#L120

Defaulting the value to "off" in applying disable directives: https://github.com/eslint/eslint/blob/52e2cf50b35d57fb8466e0bcd0581eff1590fb4c/lib/linter/apply-disable-directives.js#L121

Defaulting the value to "off" in verify options: https://github.com/eslint/eslint/blob/9d6063add931f0803cae1676d5df307baf114360/lib/linter/linter.js#L502

Interestingly, CLI-related places in code allow it to be boolean | undefined although others allow it to be "error" | "warn" | "off" | null.

I set up a quick little playground for it: https://github.com/JoshuaKGoldberg/eslint-disable-directives-testing

There is also reportUnusedDisableDirectives configuration option.

reportUnusedDisableDirectives: true config option produces warnings, --report-unused-disable-directives CLI option produces errors.

aha! i'm surely using reportUnusedDisableDirectives: true in my shared config, and that resolves my confusion :-)

in that case, when that is true in my shared config, they should automatically be autofixed, yes?

ljharb · 2021-04-13T00:49:43Z

@nzakas i don’t want someone else to control whether any fixes are applied or not, indeed - but which fixed are applied when any are being applied? That’s already what shared rule config dictates, and i definitely would want to be able to have that delegated to a shared config.

mdjermanovic · 2021-04-13T01:53:28Z

but which fixed are applied when any are being applied? That’s already what shared rule config dictates, and i definitely would want to be able to have that delegated to a shared config.

I don't think we already have a config option to specify which fixes should be applied. There is only a CLI option --fix-type.

ljharb · 2021-04-13T02:02:27Z

@mdjermanovic what i mean is, "enabling a rule" and "disabling a rule" and "configuring a rule" is already exerting control over what fixes are applied.

designs/2021-fixable-disable-directives/design.md

nzakas · 2021-04-21T00:41:07Z

@ljharb i think we are getting a bit too esoteric on this topic. Feel free to open a discussion if you want to talk more, but let’s keep the discussion here focused on the RFC itself.

nzakas · 2021-04-28T00:31:33Z

@eslint/eslint-tsc can we get some more eyes on this?

designs/2021-fixable-disable-directives/design.md

Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>

nzakas

I think the behavior as described now makes sense. This would end up being a breaking change, but I think it nicely balances what users might expect with the additional functionality.

btmills

LGTM. I think including this as a new --fix-type makes a lot of sense from a user's perspective. Thanks for putting this together and being ready to implement it!

btmills · 2021-05-01T02:54:05Z

designs/2021-fixable-disable-directives/design.md

+
+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


Right now all messages with fixes have a non-null rule ID. I could imagine e.g. an IDE integration assuming the rule ID is always a string when displaying or applying fixes. Our LintMessage docs never promise that typeof message.fix !== "undefined" implies typeof message.ruleId === "string", so this change is compatible with our stated API. While that means it's not a breaking change to the Node.js API, I wouldn't be surprised if it requires fixes in some integrations.

btmills · 2021-05-01T03:01:10Z

designs/2021-fixable-disable-directives/design.md

+
+## Open Questions
+
+- Is `"directive"` a good name for the fix type?


The only other name I'm thinking of is "eslint-disable", which is even more specific to this particular use if that's desirable.

nzakas · 2021-05-03T18:10:52Z

Because we have two approvals and it's been 22 days, I'm moving this to the Final Commenting phase.

nzakas · 2021-05-20T20:44:38Z

With apologies for the delay, this RFC has been approved.

JoshuaKGoldberg · 2021-05-20T21:40:17Z

Awesome, thanks so much for the RFC review @nzakas & co.! I'll endeavor to send a PR as soon as I can - likely within a week.

bmish · 2021-06-05T14:05:05Z

designs/2021-fixable-disable-directives/design.md

+
+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.


What if there other potential problems regarding directives we want to detect/fix in the future? --fix-type: directive doesn't indicate the specific directive problem, so would it be intended to encompass a variety of potential directive problems (perhaps incorporating new ones on major version bumps?)? Unused disable directives is just one potential problem, eslint-plugin-eslint-comments enumerates a host of other potential directive problems.

bmish · 2021-06-05T14:06:44Z

designs/2021-fixable-disable-directives/design.md

+
+Otherwise, this change is additive in behavior.
+
+## Alternatives


It would be good to include other potential API designs in this section, even if we don't choose to use them:

Using a more specific fix-type of unused-disable-directives

Providing a new option --fix-unused-disable-directives

Using a new ESLint core rule no-unused-disable-directives for detecting and fixing this problem, similar to the rule eslint-comments/no-unused-disable

bmish · 2021-06-05T14:42:54Z

designs/2021-fixable-disable-directives/design.md

+
+## Frequently Asked Questions
+
+> Will these be autofixed by default?


What's the behavior when using both --fix and --fix-type directive but not --report-unused-disable-directives?

then no unused-disable-directives problems will be reported (and no fixed). :)
it's as the same as --fix-type=layout(or else) - if no layout rules enabled, no layout errors reported.

* New: Fixable Disable Directives * First two FAQs: autofix by default, opt out * Clarified default CLI behavior * Elaborated on fixes to account for remaining used rules * Remaining --> * Update designs/2021-fixable-disable-directives/design.md Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com> Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>

New: Fixable Disable Directives

69f6880

eslint-github-bot bot added the triage label Apr 12, 2021

JoshuaKGoldberg mentioned this pull request Apr 12, 2021

--report-unused-disable-directives should be autofixable eslint/eslint#11815

Closed

First two FAQs: autofix by default, opt out

91317ee

JoshuaKGoldberg commented Apr 12, 2021

View reviewed changes

designs/2021-fixable-disable-directives/design.md Outdated Show resolved Hide resolved

nzakas requested changes Apr 13, 2021

View reviewed changes

nzakas added Initial Commenting This RFC is in the initial feedback stage breaking change This RFC contains breaking changes and removed triage labels Apr 13, 2021

Clarified default CLI behavior

c87e3b9

JoshuaKGoldberg requested a review from nzakas April 13, 2021 01:18

mdjermanovic reviewed Apr 13, 2021

View reviewed changes

designs/2021-fixable-disable-directives/design.md Outdated Show resolved Hide resolved

Josh Goldberg added 2 commits April 13, 2021 00:14

Elaborated on fixes to account for remaining used rules

2027aab

Remaining -->

87ff30d

aladdin-add reviewed Apr 28, 2021

View reviewed changes

designs/2021-fixable-disable-directives/design.md Outdated Show resolved Hide resolved

aladdin-add approved these changes Apr 28, 2021

View reviewed changes

Update designs/2021-fixable-disable-directives/design.md

6e89415

Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>

nzakas approved these changes Apr 28, 2021

View reviewed changes

btmills approved these changes May 1, 2021

View reviewed changes

nzakas added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels May 3, 2021

nzakas merged commit 101d00f into eslint:main May 20, 2021

JoshuaKGoldberg deleted the fixable-disable-directives branch May 20, 2021 21:39

JoshuaKGoldberg mentioned this pull request May 23, 2021

Breaking: Fixable disable directives (fixes #11815) eslint/eslint#14617

Merged

1 task

bmish reviewed Jun 5, 2021

View reviewed changes

JoshuaKGoldberg mentioned this pull request Dec 16, 2022

Change Request: Rename internal CLIEngine to ESLintEngine or similar? eslint/eslint#16670

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Fixable Disable Directives #78

New: Fixable Disable Directives #78

JoshuaKGoldberg commented Apr 12, 2021 •

edited

ljharb commented Apr 12, 2021

nzakas commented Apr 13, 2021

nzakas left a comment

nzakas Apr 13, 2021

ljharb Apr 13, 2021

JoshuaKGoldberg Apr 13, 2021

JoshuaKGoldberg Apr 13, 2021

JoshuaKGoldberg Apr 13, 2021

ljharb Apr 13, 2021

JoshuaKGoldberg Apr 13, 2021

mdjermanovic Apr 13, 2021

ljharb Apr 13, 2021

ljharb commented Apr 13, 2021 •

edited

mdjermanovic commented Apr 13, 2021

ljharb commented Apr 13, 2021

nzakas commented Apr 21, 2021

nzakas commented Apr 28, 2021

nzakas left a comment

btmills left a comment

btmills May 1, 2021

btmills May 1, 2021

nzakas commented May 3, 2021

nzakas commented May 20, 2021

JoshuaKGoldberg commented May 20, 2021

bmish Jun 5, 2021

bmish Jun 5, 2021

bmish Jun 5, 2021

aladdin-add Jun 5, 2021 •

edited


		> 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 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


		## Open Questions

		- Is `"directive"` a good name for the fix type?


		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.


		Otherwise, this change is additive in behavior.

		## Alternatives


		## Frequently Asked Questions

		> Will these be autofixed by default?

New: Fixable Disable Directives #78

New: Fixable Disable Directives #78

Conversation

JoshuaKGoldberg commented Apr 12, 2021 • edited

Summary

Related Issues

ljharb commented Apr 12, 2021

nzakas commented Apr 13, 2021

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Apr 13, 2021 • edited

mdjermanovic commented Apr 13, 2021

ljharb commented Apr 13, 2021

nzakas commented Apr 21, 2021

nzakas commented Apr 28, 2021

nzakas left a comment

Choose a reason for hiding this comment

btmills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented May 3, 2021

nzakas commented May 20, 2021

JoshuaKGoldberg commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aladdin-add Jun 5, 2021 • edited

Choose a reason for hiding this comment

JoshuaKGoldberg commented Apr 12, 2021 •

edited

ljharb commented Apr 13, 2021 •

edited

aladdin-add Jun 5, 2021 •

edited