Change Request: Enable reportUnusedDisableDirectives config option by default

[bmish]

2022-12 UPDATE

I have opened an RFC for this:

• feat!: flexible config + default reporting of unused disable directives rfcs#100

ESLint version

8.5.0

What problem do you want to solve?

Outdated/unused ESLint disable directive comments (e.g. notConsole(); // eslint-disable-line no-console) are currently a blind spot for most apps/repositories. These leftover comments cause clutter and confusion, and detecting them requires going out of one's way by using one of the following existing solutions:

1. Run linting with eslint --report-unused-disable-directives (errors, but only applies during CLI)
2. Add reportUnusedDisableDirectives: true to .eslintrc.js config file (warns)
3. Enabling third-party rule eslint-comments/no-unused-disable (errors, but requires third-party plugin)

These existing solutions are fine but they lack discoverability and require manual work to adopt in every single repository so are thus not commonly used.

What do you think is the correct solution?

As a breaking change, I propose changing the default value for the config option reportUnusedDisableDirectives from false to true. This will cause users to see additional warnings if they have unused disable directives.

Displaying these warnings should help raise awareness about the problem of unused disable directives, and encourage many users to remove unused disable directives caused by their changes.

Note that since these are just warnings, users could also choose to ignore them and let them pile up, as a potential downside. But luckily, the new functionality from running eslint --fix --report-unused-disable-directives could be used to fix/remove all of them at any time.

Another note is that we need to limit any default behavior to warnings and not errors for this due to our semver policy. More context in:

• Chore: enable reportUnusedDisableDirectives in eslint-config-eslint #14699 (comment)

  ...because of our semver policy, which says that bug fixes that produce fewer errors should not break builds.

• Allow "reportUnusedDisableDirectives" option to "error" #12703 (comment)

  If we allowed for errors to be reported with reportUnusedDisableDirectives, we would limit what kind of changes we could publish in a semver-patch release (since fixing a bug that would result in fewer errors could create additional unused disable directives).

Participation

• I am willing to submit a pull request for this change.

Additional comments

No response

[nzakas]

This is an interesting idea. I think there’s a larger question of whether or not we should make the functionality on by default and then not allow it to be turned off? As in, should we remove the command line flag completely?

Worth noting from the docs:

Warning: When using this option, it is possible that new errors will start being reported whenever ESLint or custom rules are upgraded. For example, suppose a rule has a bug that causes it to report a false positive, and an eslint-disable comment is added to suppress the incorrect report. If the bug is then fixed in a patch release of ESLint, the eslint-disable comment will become unused since ESLint is no longer generating an incorrect report. This will result in a new reported error for the unused directive if the report-unused-disable-directives option is used.

[bmish]

@nzakas my proposal is currently only referring to changing the default of the reportUnusedDisableDirectives config option, and not the --report-unused-disable-directives CLI option. I limited it to the config option which warns instead of errors to avoid affecting the semver policy / issue you quoted.

However, I am open to being more aggressive here. Personally, the behavior I would prefer is to always error on unused directives (regardless of where ESLint is run from).

• If some directives become unused due to bug fixes in ESLint/plugin minor version updates and cause linting to fail, I'm fine with that, especially since they can be easily fixed with eslint --fix --report-unused-disable-directives
• "Always erroring on unused directives" is a much simpler story to tell to users than the confusing mix of opt-in warning/error config/CLI options we have today

So if we can consider this more aggressive behavior, let me know, and I'll re-focus the proposal around this. Otherwise, I'll keep the proposal limited to the more conservative change.

In addition, removing the one-off CLI/config options for reporting disable directives would be a nice simplification too. Personally, I'd likely be fine removing the options, but I'd be curious if others have a strong use case for disabling/silencing unused disable directive violations.

[nzakas]

@ljharb thats pretty tangential to the topic at hand. Feel free to open a separate issue if you’d like to discuss that.

@bmish Lets see what other team members think. I’m happy to consider both subtle and aggressive changes. What I don’t want to do is a bunch of incremental changes if we ultimately want to rethink this functionality as a whole.

[mdjermanovic]

I'd likely be fine removing the options, but I'd be curious if others have a strong use case for disabling/silencing unused disable directive violations.

A use case where reporting unused disable directives is undesirable is when different ESLint configurations are used at different stages of a workflow. In that case, a directive can be unused if the rule wasn't enabled in one config, but may become used with another config at a later stage. We already don't support this use case well, but enforcing the removal of unused disable directives without an opt-out could add more problems for users with such workflows.

[nzakas]

Fair enough. What do you think of the proposal as @bmish originally proposed?

my proposal is currently only referring to changing the default of the reportUnusedDisableDirectives config option, and not the --report-unused-disable-directives CLI option. I limited it to the config option which warns instead of errors to avoid affecting the semver policy / issue you quoted.

I’m not sure I understand how we can change this in one spot but not the other. Can you explain more?

[mdjermanovic]

I support changing the default value of the reportUnusedDisableDirectives config option to true because it would raise awareness of the feature. If for any reason it isn't desirable to report unused directives, user can restore the current behavior by setting reportUnusedDisableDirectives: false.

[mdjermanovic]

how we can change this in one spot but not the other

API option reportUnusedDisableDirectives, if set, always overrides config option reportUnusedDisableDirectives:

eslint/lib/linter/linter.js

Lines 596 to 605 in 648fe1a

	let reportUnusedDisableDirectives = providedOptions.reportUnusedDisableDirectives;
	if (typeof reportUnusedDisableDirectives === "boolean") {
	reportUnusedDisableDirectives = reportUnusedDisableDirectives ? "error" : "off";
	}
	if (typeof reportUnusedDisableDirectives !== "string") {
	reportUnusedDisableDirectives =
	linterOptions.reportUnusedDisableDirectives
	? "warn" : "off";
	}

The CLI flag --report-unused-disable-directives maps to "error" on API:

eslint/lib/cli.js

Line 120 in 648fe1a

reportUnusedDisableDirectives: reportUnusedDisableDirectives ? "error" : void 0,

All of this is a bit confusing, but in the end, for CLI users it normalizes to one of these three behaviors:

1. Don't report (neither reportUnusedDisableDirectives: true in config nor --report-unused-disable-directives on CLI were set)
2. Report as warnings (reportUnusedDisableDirectives: true is set in the config, but --report-unused-disable-directives wasn't provided on CLI).
3. Report as errors (--report-unused-disable-directives was provided on CLI; in this case, the config option is ignored even if it is reportUnusedDisableDirectives: false).

The current default behavior is 1. After this change, the default behavior will be 2.

This change wouldn't affect:

• Users who already have reportUnusedDisableDirectives config option (either true or false).
• CLI users with --report-unused-disable-directives flag.
• API users with a non-nullish value passed as reportUnusedDisableDirectives ("off", "warn", or "error").

[bmish]

@mdjermanovic thanks for the context, that's helpful.

It sounds like there is support for my original proposal (i.e. warning on unused directives).

But I haven't yet heard opinions on the more aggressive version which is erroring by default on unused directives. Is erroring still something we should consider now? Is anyone explicitly against erroring? Should we defer this question and just start with warning in ESLint v9 but consider erroring in a subsequent major version? Although as @nzakas said, it would be nice to figure out the endgame now.

[ljharb]

Since I can't make it error via my shared config, and I don't want to update 300+ projects to pass the command line flag, I'd love the default to be changed.

[bmish]

@ljharb that's a great argument for erroring by default, which I support.

I think the main question is just whether we're okay with the potential semver implications of erroring by default. I'm bringing this up because it sounded like in the past we were not okay with them, so I'm wondering if we're open to reconsidering this:

• Chore: enable reportUnusedDisableDirectives in eslint-config-eslint #14699 (comment)

  ...because of our semver policy, which says that bug fixes that produce fewer errors should not break builds.

• Allow "reportUnusedDisableDirectives" option to "error" #12703 (comment)

  If we allowed for errors to be reported with reportUnusedDisableDirectives, we would limit what kind of changes we could publish in a semver-patch release (since fixing a bug that would result in fewer errors could create additional unused disable directives).

[nzakas]

Just to make sure I’m understanding correctly, what would the CLI —report-unused-disable-directives flag do now? It would just bump up the severity from warning to error?

And that means you can’t opt-out of warning on disable directives from the CLI, only in config? (If true, not a fan.)

@bmish regarding erroring, can you explain how that would interplay with the command line flag?

[bmish]

@nzakas your questions have got me thinking that the current boolean design of these options is particularly confusing and unintuitive, making it hard to predict the interplay.

What if I described a new "explicit severity" design:

• The config and CLI options are both changed to accept standard severity values: off, warn, error
• If both options are present, the CLI option takes precedence over (overrides) the config option
• If only one of the options is present, the value of the present option is used
• If neither option is present, the default value is used
• Note: There's only one underlying setting, and the config and CLI options are just two different ways of controlling it

Benefits of this design:

• The user should always be able to achieve the desired effect (off / warn / error), regardless of where they set the option from
• No confusing inconsistency where one of the options triggers errors but the other triggers warnings
• reportUnusedDisableDirectives: 'error' can now be part of a shareable config

On top of this design, we can still debate what the default value should be:

• Today: Default is off
• Conservative proposal: Default changed to warn
• Aggressive proposal: Default changed to error (potential semver implication, but this is my preference if acceptable)

[nzakas]

That sounds like a good direction to head in.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[nzakas]

Assigning to @bmish to continue work

[kachkaev]

I’d love to be able to set reportUnusedDisableDirectives: "error" instead of true. At the moment, it is necessary to add --report-unused-disable-directives to every monorepo workspace and this is easy to forget. Examples:

• Add --report-unused-disable-directives to lint:eslint hashintel/hash#1608
• Add --report-unused-disable-directives to lint:eslint blockprotocol/blockprotocol#812.

I can try creating a PR that supports reportUnusedDisableDirectives: "error", "warn" and "off" in addition to true / false / undefined. Any reasons why this would be a bad idea?

[bmish]

This will require an RFC. I can work on putting together an RFC based on what I proposed in my previous comment.

[bmish]

I have opened an RFC for this:

• feat!: flexible config + default reporting of unused disable directives rfcs#100

[sam3k]

2023-11-30 tsc-meeting note: waiting on @bmish to finish PR (draft).

[bmish]

Part 1 is done but I'm going to reopen this until part 2 of the RFC is done:

Part 1: add support for the new severity levels

• feat: Support custom severity when reporting unused disable directives #17212

Part 2: warn by default

• feat: warn by default for unused disable directives #17879

[mdjermanovic]

@bmish would you like to submit a PR for part 2 as well? This feature is planned for the first v9 alpha release, which will be next week.

[bmish]

Yes I started part 2 in:

• feat: warn by default for unused disable directives #17879.