Add CLI option to report unused eslint-disable directives

[not-an-aardvark]

(Repost of #6190)

/* eslint-disable */ comments are useful to document when code is an exception to a given rule. If a piece of code was previously an exception, but now the rule is no longer being violated, it's useful to remove the comment so that it doesn't hide unintended problems in the future. Unfortunately, ESLint doesn't provide any way to detect unused comments, and it's not possible to do this with a custom rule using the public rule API because rules can't view the configurations of other rules to determine whether a directive is used.

When this was discussed in #6190, there was consensus that if a behavior like this was added to core, it would have to be implemented as a CLI flag. The TSC decided to wait for a prototype before determining whether to accept the feature. A few months later, the issue was closed because eslint-plugin-eslint-comments was released, which achieves the same behavior in a plugin by monkeypatching ESLint's APIs.

I'd like to reopen discussion on this because I think this would still be a useful feature, and I don't think using a plugin that monkeypatches core APIs is an acceptable solution in the long term, given that it breaks down the boundaries that we enforce between rules.

(Case in point: We recently refactored the context.report internals, causing eslint-plugin-eslint-comments to break because it was trying to monkeypatch an API that no longer existed (#9193). We ended up adding this and this to prevent the monkeypatches in eslint-plugin-eslint-comments from breaking, but those are hacks and I'd like to get rid of them.)

The proposal is to add a --report-unused-disable-directives CLI flag (along with corresponding reportUnusedDisableDirectives options in CLIEngine and Linter). When the flag is enabled, ESLint adds a new reported problem with error severity when it encounters an unused eslint-disable comment (or variants like eslint-disable-line).

---

I've created a prototype implementation (#9250). The implementation is actually fairly simple -- most of the diff in the PR comes from additional tests, docs, and passthrough options.

There are a few edge cases to consider -- I've written a bit about them below.

Edge cases

Definitions:

• "unused directive": A directive that would prevent errors from being reported, but no errors were actually reported, so it didn't suppress anything.
• "redundant directive": A directive that has no effect because the list of disabled rules is the same before and after applying it.

Unused directives would need to be detected in core, since they depend on whether other rules report errors. Redundant directives could be detected by external rules (although they would sometimes have to use strange report locations to prevent their own reports from being suppressed). Because of this, I decided that the flag should only cause unused directives to be reported. Redundant directives are not reported unless they are also unused.

Examples:

1. First comment is unused, second comment is unused and redundant

   /* eslint-disable */
   /* eslint-disable */

2. First comment is unused, second comment is redundant

   /* eslint-disable */
   /* eslint-disable */
   (reported problem)

3. First comment is unused

   /* eslint-disable foo */
   /* eslint-disable */
   (reported problem from foo)

4. First comment is unused

   /* eslint-disable foo */
   /* eslint-disable */
   (reported problem from an unrelated rule)

5. First comment is unused, second comment is redundant

   /* eslint-disable */
   /* eslint-disable foo */
   (reported problem from foo)

6. Second comment is redundant and unused

   /* eslint-disable */
   /* eslint-disable foo */
   (reported problem from an unrelated rule)

7. First comment is unused, second comment is redundant and unused

   /* eslint-disable */
   /* eslint-disable foo */
   /* eslint-enable foo */
   (reported problem from foo)

8. Comment is redundant

   /* eslint-enable */

[not-an-aardvark]

TSC Summary: This issue proposes adding a CLI flag to report unused eslint-disable directives.

TSC Question: Should we accept this proposal?

[not-an-aardvark]

This was accepted in today's TSC meeting.

[klimashkin]

Thanks for great work @not-an-aardvark.
From description I didn't quit get, is it possible to use reportUnusedDisableDirectives option right in .eslintrc or we still need to use eslint-plugin-eslint-comments which will use that option internally?

[not-an-aardvark]

This is only available as a command-line flag at the moment, and can't be enabled in a configuration file. (Since this is not a rule, it was decided in #6190 that it shouldn't be configured in the same way as a rule.)

One potential solution would be a reportUnusedDisableDirectives top-level option in a config. I would be slightly concerned about allowing this option to be specified in shareable configs (given the potential footgun described in the documentation, it seems like a good idea to make users explicitly opt-in).