Hint to catch redundant //ignores

[Hixie]

It'd be great if there was a lint that would trigger whenever there was an // ignore that wasn't actively silencing an analyzer message. This would let us catch old ignores that work around bugs that have since been fixed, or ignores that we don't need because we disabled the relevant lint, etc.

cc @a14n

[bwilkerson]

I completely agree that we need this. It might even be reasonable to make it be opt-in.

However, we can't implement it as a normal lint because there's no way to guarantee that all of the other lints will have already been run. This would need to run after all other diagnostics have been generated, otherwise we won't be able to determine whether a given ignore is valid.

[zoechi]

dup of #57404

[bwilkerson]

Because it can't be implemented as a lint, the linter issue has been closed.

[srawlins]

I propose the following static analysis rule:

A new error code (a HintCode) called something like unused_ignore_comment.

This error code cannot be ignored with an ignore: or ignore_for_file: comment. It can be ignored via analysis_options.yaml.

Only unused ignore comments for error codes which exist are reported. For example, a typo like // ignore: daed_code does not ignore any real error, but no unused_ignore_comment Hint will be reported, since there exists no error code, daed_code.

This new error code is not intended to catch typos. It is intended to catch ignore comments which once were used, and are no longer.

If we did report comments like // ignore: to disregard and // Some things I wish I could\n// ignore: dead code for example, I think users would be fighting a lot of unintentional "ignore comments." Which, as stated above, themselves cannot be ignored from within the source file.

[bwilkerson]

This error code cannot be ignored with an ignore: or ignore_for_file: comment.

I don't see any value in adding this special case handling. I think the number of people who will choose to ignore this hint will be very small and I don't see why we'd disallow it. Besides, if we want a way to make certain diagnostics impossible to ignore then that mechanism would cover this case without special casing.

... I think users would be fighting a lot of unintentional "ignore comments."

I doubt that's true, but we could gather evidence easily enough.

As a counter argument, though, I'll note that we recently removed a diagnostic (because the error was removed from the spec). If there are any ignore comments for that diagnostic, your proposed limitation would prevent this lint from helping users clean up their code. I think we could safely not generate diagnostics for comments that are not of the right form, but I'd prefer for the hint to work if what follows the ignore: is a comma separated list of identifiers. That should cover the examples you gave while still allowing it to catch errors that have been removed.

[srawlins]

I don't see any value in adding this special case handling. I think the number of people who will choose to ignore this hint will be very small and I don't see why we'd disallow it.

I thought it would be a lot of effort to allow ignoring these, but I think I was wrong. These should be ignorable.

[srawlins]

I see the flutter repo using the ignore syntax like this in a dozen places:

https://github.com/flutter/flutter/blob/b9a56b9f48462d897101284f5f57b8568ef683c6/packages/flutter/lib/src/cupertino/text_theme.dart#L122

// ignore: avoid_unused_constructor_parameters, the parameter is deprecated.

I guess at a minimum I would ignore "ignored codes" with whitespace (don't call them out as unused). Also I've seen this:

// ignore: prefer_void_to_null, https://github.com/dart-lang/sdk/issues/34416

So... ignore "ignored codes" that... are not valid Dart identifiers?

[bwilkerson]

The way analyzer processes ignore comments is to take everything following ignore:, split it on commas, trim off white space at the beginning and end, and convert to lower case. That gives it a list of codes to ignore. This hint should process them the same way, and should share as much code as possible with IgnoreInfo in order to ensure consistency should that we change the way the comments are processed.

Every valid code name is a valid identifier. In both of the examples you gave we'd end up with items in the list that are not valid identifiers. I suspect we'll get the best results by discarding / ignoring anything that isn't a valid identifier, and then validating that each remaining identifier is a known code and that a diagnostic with that code is actually ignored at that location.

That hint processing will need to happen after all of the diagnostics have been computed, including lints, in order to know whether a diagnostic is reported at that location. Note that that means that it might be harder than I'd first assumed to make this hint be ignorable. If that's the case, then I don't think we should do the extra work until we know of a valid use case for allowing them to be ignored.

A new error code (a HintCode) called something like unused_ignore_comment.

I'm not sold on the name because of the fact that a single comment can contain both valid (used) and invalid (unused) codes. In that case the comment isn't unused, just one of the codes. Maybe "code" instead of "comment"?

[srawlins]

I've got a WIP if anyone is interested. https://dart-review.googlesource.com/c/sdk/+/126620