Allow //ignore comments to suppress static errors

[srawlins]

In closing #27218 (I think for Dart 2.9) we changed the behavior of // ignore: comments such that they could no longer suppress "errors" or "warnings" (defined by the language specification); they could only ignore "hints" (like unused_import) or "lints."

The initial idea was that nothing good can come from // ignore-ing a compile-time error, and a user may be confused about what // ignore does if not suppress the error at runtime as well.

The change was ultimately made so that security-motivated lint rules could be written and could not be ignored. The SecurityLintCode class makes use of un-ignorability. Lint rules like unsafe_html use this. This feature means that developers cannot ignore a security-motivated lint rule inline.

The big problem which has arisen since closing #27218 is that the analyzer is not infallible. If the analyzer reports a false positive error diagnostic, the developer can do nothing inline to suppress that erroneous diagnostic. They can bulk ignore the diagnostic code in an analysis_options.yaml file, but not file-by-file. This problem is big because of CI systems. Generally a CI bot/queue/configuration is red or green. If a code repository has one false positive static analysis diagnostic, then the "analyze" bot/queue/configuration is just red.

I think the analyzer team should revisit un-ignorable errors, and likely reduce the concept to just un-ignorable security-motivated lints.

[srawlins]

CC @pq @stereotype441

CC @Hixie @jonahwilliams who ran into this in flutter/flutter#70506 and flutter/flutter#52899

[srawlins]

This would fix #42977

[Hixie]

I wrote #27218 (comment) and #42977 (comment) in support of allowing errors to be ignored.

I would also make security lints ignorable, FWIW. There's always going to be cases where a lint is wrong. For example, suppose someone wants to write a security tutorial, and wants to analyze their code. They might want to have code that shows the vulnerability, and so they're going to need to ignore the lint.

[pq]

FWIW: I'm very much in favor of a revert. There are other ways we can enforce a policy of unignorability internally and this has clearly turned out to be too big a hammer...

[matanlurey]

There needs to be a way to keep security lints un-ignoreable in google3, but I'm neutral on the other cases presented.

[lrhn]

Allowing language errors to be ignored seems counter-productive. Only the analyzer will actually ignore them, the compilers will still refuse to compile the code. Language errors won't have "false positives". If it's an error, it's a compile-time error.

Ignoring language warnings seems fair to me, it's just a warning, it doesn't mean the code is necessarily wrong. But then I don't think there should be any "language mandated warnings" to begin with.

Anything which can have false negatives with no easy workaround (small, local rewrite) should probably be ignorable, but whether something is ignorable should perhaps itself be configurable instead of hard-coded. (Add !important, so use_single_quotes: error !important, like in CSS 😏 ).

[Hixie]

Only the analyzer will actually ignore them, the compilers will still refuse to compile the code

This isn't true in practice. For example, people may be analyzing code that has known missing dependencies, e.g. the issue that led me to investigate this earlier today was that the analyzer was complaining about a missing import because the dart:ui it was analyzing against wasn't the dart:ui that I was going to run the code with. Or maybe the code will be preprocessed before compilation, but the analyzer is running against source because it's in the IDE. Or maybe the user is using an experimental compiler that implements a feature the analyzer doesn't know about.

[lrhn]

If the program you are analyzing isn't the program you are compiling, or the analyzer and compiler are not using the same language, then obviously any connection between the results is tenuous at best. Those are the assumptions that makes it possible to have an analyzer separate from the compiler.
(And why having libraries with different signatures on different platform makes things hard for users, since the analyzer can only see one version at a time).

[Hixie]

I mean, I get that in theory. In practice, there may not be an analyzer that matches the language/compiler in use. Having to abandon analysis entirely, or having to wrap the analyzer in code that post-processes the messages (something Flutter used to do), seems like a high cost compared to just being able to use an existing feature to silence analyzer output on a per-line basis.

This may be a philosophy vs pragmatism discussion. Pragmatic software tends to get more users than philosophically correct software, so I personally think pragmatism should always win, but I understand the attraction of making a philosophically-correct product as well.

[devoncarew]

the analyzer was complaining about a missing import because the dart:ui it was analyzing against wasn't the dart:ui that I was going to run the code with

For what it's worth, dart:ui was one of the reasons we pushed forward with this solution. Allowing errors to be easily ignored was an easy tool for users to use - it facilitated technical debt building up in the ecosystem. It was too easy to just ignore an error and push forward with a temporary solution, and then for that solution to become permanent. I see having two different public APIs for dart:ui being one of the pieces of technical debt that was allowed to build up.

[matanlurey]

If we want to allow a:

# analysis_options.yaml
analyzer:
  allow_ignoring_errors: true

... for a select few projects, I have no strong opinion there, but I agree with @devoncarew otherwise.

[Hixie]

For what it's worth, dart:ui was one of the reasons we pushed forward with this solution.

I'm 100% behind removing technical debt. Unfortunately, making //ignore not work on errors didn't help at all here because we promptly added an exception for that very code. That's how I ran into this -- I was trying to move where the //ignore was from a package into autogenerated code (which seems like an improvement, if maybe only a minor one), and was unable to because while the package was exempt and could //ignore the problematic code, the autogenerated code was not and could not.

allow_ignoring_errors: true

FWIW this wouldn't really help in the case I reference above, since the //ignore in question is present for every Flutter app (or at least, every Flutter app compiled for web), so we'd have to enable it in our global analysis options, which seems to defeat the point of making it opt-in.