reportUnusedDisableDirectives should distinguish between globally disable rules and no violation of globally enabled rule

[inga-lovinde]

ESLint version

v8.56.0

What problem do you want to solve?

ESLint currently does not distinguish between disable directives that are unused because the rule they're disabling is already disabled in eslint config, and disable directives that are unused because the code they're disabling it for does not actually violate the rule.

In scenarios when multiple eslint configs are used, running eslint --fix on smaller config with reportUnusedDisableDirectives enabled will currently remove both kinds of unused disabled directives, causing eslint on larger config to fail.

One example would be this: @typescript-eslint has a "type-checked" ruleset which requires it to basically compile the entire project, which can take a lot of time on larger projects, regardless of the number of files checked (because it needs the type information from the entire project to check these files). So it is natural to not use this ruleset in fast checks (e.g. on precommit) when we're only interested in knowing if some specific files have any obvious errors, but not interested in spending a minute to get an answer for just one file.
Running eslint --fix my-single-file.ts with the main eslint config will take minutes on large codebases. Running it with the supplementary eslint config that disables type-checked rules and TS project loading will only take fractions of a second, but (if reportUnusedDisableDirectives is enabled) will also cause it to remove all directives disabling specific type-checked rules on specific lines.

There is way to work around: it's to disable reportUnusedDisableDirectives in the smaller config. But this has a drawback that running eslint --fix with the smaller config will not remove directives that are actually unused.

What do you think is the correct solution?

I think it would be nice if eslint distinguished between "this directive is unused because I know that the next line does not violate this rule" and "this directive is unused because the rule is globally disabled, but I don't know if the next line violates this rule", and allowed enabling autofixing for the former but not for the latter.

Participation

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

Additional comments

No response

[nzakas]

ESLint currently does not distinguish between disable directives that are unused because the rule they're disabling is already disabled in eslint config, and disable directives that are unused because the code they're disabling it for does not actually violate the rule.

Sorry, it's not quite clear what scenario you're describing here. Can you please provide some code as an example, preferably a StackBlitz that shows the behavior you're describing?

[inga-lovinde]

@nzakas , a very simplified example would be https://stackblitz.com/edit/stackblitz-starters-omyohp?file=index.js

npm run lint there would produce warnings for lines 10 and 22.
npm run lint:fast there would produce warnings for lines 3, 10, and 22, even though these are very different cases.

Now imagine that no-plusplus rule is very complex, and takes ten minutes to check. It might make sense for the project to have a "basic" eslint config (like in .eslintrc.js in that file), and "fast" config for when we don't have ten minutes but still want to check what we can in a reasonable time.
The disable directives on lines 3 and 16 are actually needed, otherwise we'll get an error when running full and slow eslint check by npm run lint.
The disable directives on lines 10 and 22 are redundant (as npm run lint tells us).
However, running npm run lint:fast:fix will remove three directives (on lines 3, 10 and 22), even though removing the directive on line 3 is a mistake.

Of course, lint with "fast" config has no way of distinguishing between cases of line 3 and line 10; the rule is disabled, so it has no way of knowing whether the directive is needed or not.
But even lint with "fast" config could, in theory, distinguish between cases of line 3 and line 22 (for line 3: it does not know whether the rule is violated, because it's disabled in eslintrc; for line 22: it knows that the rule is not violated).
Of course, there is no way to get npm run lint:fast:fix to remove directives on lines 10 and 22 while leaving directives on lines 3 and 16. But in some scenarios, it would be preferable if it only removed directive on line 22, than if it would remove all three lines (3, 10 and 22).

Currently, they only way to prevent lint with "fast" config from producing warnings for line 3 / from removing directive on line 3 in fix mode is to add "reportUnusedDisableDirectives": true. This workaround is perfectly fine, but it would be nice if we could still get it to report unused disabled directives, just not all of them.

[nzakas]

Thanks, that's very helpful. Just to make sure I understand what you're requesting, let me try to summarize:

Current Behavior

A disable directive (/* eslint-disable foo*/) is considered unused if either of the following is true:

1. The rule foo is disabled in the config file
2. The rule foo is enabled but would not produce a violation

In both cases --fix will remove the disable directive.

Desired Behavior

When --fix is used, do not remove the disable directive for case 1. Continue to remove the disable directive for case 2.

Do I have that correct?

[inga-lovinde]

This is not quite my original idea, because I think that in some cases it would be useful to remove disable directives for rules disabled in the config file, too.

So in my original idea, the desired behavior would be to introduce two separate controls for these two cases (e.g. reportUnusedDisableDirectives and reportInapplicableDisableDirectives, but I'm bad at naming things).

But lacking that, changing the current behavior from "warns in both cases, removes directives in both cases" to "warns in both cases, removes only in case 2" would also solve the immediate issue.

[nzakas]

Got it. Splitting the behavior into two properties would be a breaking change, which we'd need to wait until v10 to do (if the team agreed to that proposal).

First, we'd need to see how the team feels about making any change to this behavior. I'm slightly in favor of the change I proposed, so interested to hear what the rest of @eslint/eslint-team thinks.

[JoshuaKGoldberg]

This makes sense. I'm in favor of making the option more granular and/or splitting into multiple options. In typescript-eslint we commonly tell folks that if they want fast linting, they can use strategies like the one mentioned here with configs like tseslint.configs.disableTypeChecked.

Tangentially related: I also filed #18230 to track also reporting on unused/redundant inline config comments. I imagine if that is accepted, we'd want to apply a similar strategy from this issue.

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

[Rec0iL99]

ping @eslint/eslint-team

[fasttime]

If reportUnusedDisableDirectives is split into two new options and the current name is kept as a shorthand for both options, it should be possible to implement this feature as a non-breaking change in v9, assuming that the autofix behavior remains unchanged. We would need an RFC to understand the details.