Compiler should not print warnings

[Hixie]

When I compile this:

void main() {
  const bool test = true;
  print(test!); // ignore: unnecessary_non_null_assertion                                                                                         
}

...I get:

test.dart:3:9: Warning: Operand of null-aware operation '!' has type 'bool' which excludes null.
  print(test!); // ignore: unnecessary_non_null_assertion
    ^
true

I'm happy for the analyzer to give the warning, but why is the compiler also doing so? And how do I silence it?

The context is that I have code (a Flutter plugin) that needs to compile against two versions of an API. In one version (Flutter stable) a particular field is nullable, in the more recent version (Flutter master), it's non-nullable. I don't mind having lots of // ignores in the code during the transition, but I don't want users to be faced with lots of warnings they can't do anything about when compiling their apps.

[zanderso]

I haven't caught up with the thread, but if the consensus is to pass --verbosity=error to the Dart front-end, then this PR will do it.

[mit-mit]

I'm very sceptical that this change is for the better, and would like to have it reverted pending further analysis and studies actually proving that this is a good change for users.

[Hixie]

@mit-mit https://github.com/dart-lang/sdk/issues/46264#issuecomment-1125713761 shows there's definitely a problem right now, and that silencing the warnings from other packages would definitely improve the situation, but if we can find an even better solution I'm definitely open to it. Some of the ideas above (about showing all the warnings/lints/etc that the analyzer gives, but maybe not for dependencies) seem like they'd be improvements, but difficult to do today efficiently.

[mit-mit]

@Hixie @jacob314 yes, but what says that it doesn't introduce a ton of other problems? I mean, these warnings were all added for a reason so brute force suppressing them seems like a rather odd choice.

We need to be convinced that this is a net improvement across the board, and I don't see enough data here to make me convinced that is the case.

Also, if not technically then in spirit, I consider this a breaking change, so that part of the process should have been followed too.

[natebosch]

what says that it doesn't introduce a ton of other problems?

The only problem we are aware of is that the workflow describe by Leaf is slightly less safe. What other problems are you worried about?

I consider this a breaking change

What do you think is breaking about this change?

[mit-mit]

The only problem we are aware of is that the workflow describe by Leaf is slightly less safe. What other problems are you worried about?

I worry about a developer making a small tweak to their app that they consider safe, doing a flutter build apk, not seeing any warnings or errors, and then concluding that they are ready to prod, where in fact a warning would have told them that they weren't.

For me to not worry about that, I'd need to get convinced that none of our current warnings can point to issues that would be bad for a production app. I've not yet seen such analysis.

What do you think is breaking about this change?

It's a significant change in behaviour to programs that fall within the compatibility guidelines outlined here: https://github.com/dart-lang/sdk/blob/master/docs/process/breaking-changes.md#scope-of-compatibility

[Hixie]

@mit-mit What warnings does the compiler emit that would fall into that category? The warnings we're talking about here are pretty trivial (redundant null-aware operators). I very rarely see warnings from the compiler; I'm not actually aware of any other warnings that the compiler emits, this is why when I originally filed this bug I really did not expect it to be controversial.

Re the breaking-change nature, the only change that's been made so far is to Flutter, whose breaking change policy was followed as far as I can tell (https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes). I agree that if we were to make a change to Dart we should follow those guidelines. (I also think, like others have suggested above, that there's value in making a more serious change here, to emit many more warnings but to make it actually useful, e.g. emitting all the warnings/lints the analyzer emits, and not doing so for dependencies that are out of control of the developer.)

[lrhn]

I tried to read the language spec and null-safety spec, and the only static warnings I could find were:

• Inexhaustive enum/enum? switch - spec, nnbd.
• ClassName?.member - nnbd
• null aware operator on non-nullable expression - nnbd
• null check operator (!) on non-nullable expression - nnbd

Am I missing any? (There were other warnings, that we wanted to either turn into errors or ignore, but I don't know if we did so or not. Can't find them in spec/nnbd-spec. @eernstg?)

None of these seem catastrophic to ignore. That's why they're not errors to begin with. The null-aware/checking operators are simply dead code, and might make types less precise (nonNull?.foo is probably still nullable, even if nonNull.foo isn't.)

The switch case warning may be significant, there is a case you're doing nothing about, but not getting exhaustiveness-benefits again only worsens your types (there's a path through the switch which does nothing, and therefore cannot promote or escape). The code still type-checks and returns properly, even when the switch does nothing.

[leafpetersen]

The switch case warning may be significant, there is a case you're doing nothing about, but not getting exhaustiveness-benefits again only worsens your types (there's a path through the switch which does nothing, and therefore cannot promote or escape). The code still type-checks and returns properly, even when the switch does nothing.

This is definitely not the case.

/// If value is non-null, something is null, otherwise you have something
void foo(SomeEnum? value, int? something) {
  switch (value) {
     case SomeEnum.A: return; break;
     case null: break;
  }
  something!.isEven;
}

Add a case SomeEnum, the warning tells you that you are missing a case. Ignore the warning, your code blows up.

[leafpetersen]

The switch case warning may be significant, there is a case you're doing nothing about, but not getting exhaustiveness-benefits again only worsens your types (there's a path through the switch which does nothing, and therefore cannot promote or escape). The code still type-checks and returns properly, even when the switch does nothing.

This is definitely not the case.

Further to the above, there are some of things that are currently analyzer hints, but which if we unify hints and warnings would likely become warnings. Some of these are also not in this category, for example the missing return warning.

[Hixie]

There are tons of things that the linter points out that are far more likely to be critically problematic than inexhaustive enums in switches, like hash_and_equals. We're currently woefully under-serving people who use the compiler as their sole source of diagnostics. I don't think anyone is arguing that we couldn't make that better.

As discussed earlier, there's a wide spectrum of possible places we could land with respect to the compiler and the analyzer outputting the known set of warnings and lints. Right now we are at what I believe is literally the single worst point in that spectrum.

If we want to better serve people who don't use the analyzer, which sounds like everyone is ok with doing, then we should do that (modulo prioritizing that work in the context of the other work we're doing).

[mit-mit]

I agree that if we were to make a change to Dart we should follow those guidelines.

While technically you are only changing Flutter, effectively you are changing the core experience of writing Dart code, so I stand by my earlier claim that this is effectively a breaking change in experience.

not doing so for dependencies that are out of control of the developer

I'm very interested in investigating that change. From initial chats with the CFE team it shouldn't be too difficult.

There are tons of things that the linter points out that are far more likely to be critically problematic

That something else is more problematic, doesn't make the issue we're discussing here less problematic in itself.

[lrhn]

There are tons of things that the linter points out that are far more likely to be critically problematic

That something else is more problematic, doesn't make the issue we're discussing here less problematic in itself.

I think giving some warnings, but not all, is actually worse than giving none. It is less problematic to remove that one warning from the compiler than to retain it.

If you use only the compiler, we warn about inexhaustive switches. Yey!

You then fix those, you have no warnings, and you might thing your code is fine. The analyzer would have told you lots of ways in which your code is not fine.

Or you can look at the code and decide that the switch being inexhaustive is fine, but you have no way to disable the warning. You'll have to insert a dummy break; default: at the end of the switch anyway, just to silence the warning. If the switch is even in your code, it might be in another library and be silenced by // ignore:, but the compiler doesn't know what that means.

If you never get any warnings from the compiler at all, you may still think everything is fine, or you might start looking for a way to get actual warnings. I think that providing some warnings, but not all, and not the most important ones, increases the risk of people not using the analyzer. If it does anything, and if not, our choice here doesn't matter, and I'd still vote for consistency.

I want, longer term, a compile command which can provide either all warnings or no warnings, depending on what you want to use it for, and which honors // ignore: comments and analysis_options.yaml. I actually want three modes: No warnings, current package warnings only (the ones I can actually fix), and whole program warnings (in case I want to gauge the quality of my dependencies too). The third one should not be the default.

The first step of that is to add a flag which disables/enables warnings in the compiler, and I think we should do that now. Then we can discuss what the default should be.

[natebosch]

The first step of that is to add a flag which disables/enables warnings in the compiler, and I think we should do that now. Then we can discuss what the default should be.

IIUC we have that flag and this issue is largely the discussion about what the default should be.

[eernstg]

@lrhn wrote:

There were other warnings, that we wanted to either turn into errors or ignore, but I don't know if we did so or not. Can't find them in spec/nnbd-spec.

Apart from the ones you mentioned we have "parameter's default value differs from overridden parameter's", and that's going away with null safety.

We discussed turning TypeLiteral?.staticMethod() into an error at the language team meeting August 25, 2021, but no decision was made. See also https://github.com/dart-lang/language/issues/1819 where several comments express the desire to make C?.m() an error when m is a static method declared in C, and when C.m is a constructor.

[mit-mit]

I'm still against wholesale turning off warnings. We're not likely to be able to turn it on again (what would be another breaking change, and those are hard to roll out), so we'd need to be convinced that any warning is irrelevent. I'm not ready to make that conclusion.

@Hixie @jacob314 it seems the main motivating reason for this was avoiding spurious warnings from packages that the developers code depends on. I chatted with the CFE team, and we think it's a minor change to apply filtering to not show warnings from outside the app root directory. If we make that change, are you happy keeping warnings generally?

[Hixie]

I don't object to warnings in general, if they can be silenced (using the same mechanisms as the analyzer, currently // ignore, .analysis_options.yml, and so on). I do think it is very confusing that the compiler and the analyzer disagree about what warnings should be shown.

Personally I don't ever want these warnings; when I say flutter run I just want it to run because I'll be using hot reload and the analyzer to get feedback (do these warnings show up with every hot reload?), and when I use dart run I am usually doing it because I'm using my app and redirecting stderr and stdout and don't want the compiler contributing its own opinions into my outputs.

[jacob314]

@mit-mit If we hide warnings from packages that the developer depends on and respect // ignore and .analysis_options.yaml in the CFE then I'm fine with leaving the warnings on.

[johnniwinther]

Supporting // ignore in the CFE is major task that might also hit performance. Currently the analyzer parses all comments whereas the CFE skips them at the earliest state in the compilation, and the semantics of the ignore comments (the used names) is directly tied to the analyzers messaging system, which is not shared with the CFE.

[jacob314]

Given that supporting // ignore in the CFE is a major task that might hit performance, I think the current solution of hiding warnings by default for flutter run||build is the best solution. We should revisit in the future if there is a performant way to run the linter as part of the CFE.

[mit-mit]

Can you point to any analysis of how common using // ignore for warnings is? My gut tells me that it's pretty uncommon, but I have no actual data behind that.

[Hixie]

There's 614 // ignores in the flutter/flutter codebase alone, FWIW.

[jacob314]

There are about 150 // ignore in flutter/devtools. I don't see anything fundamentally less ignore-able about most of the existing Flutter warnings than many of the lints we enable by default. There is a selection bias today as ignoring warnings doesn't really seem like it works as you can still see the warnings in your output when you flutter run. Here is an example of ignoring invalid_null_aware_operator in a fairly popular dart command line tool: https://github.com/trevorwang/retrofit.dart/blob/master/generator/lib/src/generator.dart#L799

In g3 there is at least one code generator that generates code withignore_for_file: invalid_null_aware_operator That seems like a fairly reasonable thing for a code generator to do as there is no real harm in adding an extra ? if it makes it easier to write the code generator.

[natebosch]

Here is an example of ignoring invalid_null_aware_operator in a fairly popular dart command line tool: https://github.com/trevorwang/retrofit.dart/blob/master/generator/lib/src/generator.dart#L799

This is not a valid ignore. It may have been valid in older SDK versions? It's maybe an artifact of an out of order migration?

[mit-mit]

I'm aware that ignoring lints is pretty common. But given the the topic here is hiding of CFE (common front end) warnings, I think the relevant data point is about hiding those (or analyzer warnings similar to those)?

[jakemac53]

Fwiw, I think you could make a pretty solid argument that the invalid_null_aware_operator, in the situation described, is emitting a false positive. The ? is in fact required, because the code is explicitly trying to handle multiple versions of the dependency, and it can safely do so by including a ?. I don't believe it can avoid this without taking into account package version constraints, which the compiler probably should not do, and for that reason it should be a lint and not a warning, and removed from the compiler.

The same probably goes for most if not all warnings defined in the spec.

Yes, the language tries to pretend version constraints and pub itself do not exist. But, the fact of the matter is they do exist, and this warning is actually actively encouraging you to make potentially unsafe changes, because it doesn't understand those things.

[bwilkerson]

I don't believe it can avoid this without taking into account package version constraints, which the compiler probably should not do ...

I agree that the compiler shouldn't look at package version constraints. But it's less clear to me that we shouldn't do that in the analyzer. The analyzer is intentionally not just a compiler, so if it would be more helpful to our users for it to consider package version constraints, I'm not sure we should take that option off the table without first doing a cost/benefit analysis. (Analyzer already does a fair bit of analysis of the pubspec.yaml file.)

[jakemac53]

But it's less clear to me that we shouldn't do that in the analyzer.

Right, conceptually I think it would be nice if the analyzer could take your package version constraints into account. In practice I think it might not be feasible, but that is a different question. I could imagine a lot of benefits though.

[Hixie]

FWIW in a bunch of Flutter code to avoid the unignorable compiler warning log spam when writing code that deals with APIs that are nullable in old versions and non-nullable in new ones we've explicitly used hacky functions that happen to not cause any warnings today, like this:

/// This allows a value of type T or T? to be treated as a value of type T?.
///
/// We use this so that APIs that have become non-nullable can still be used
/// with `!` and `?` on the stable branch.
T? _ambiguate<T>(T? value) => value;

Interestingly, this pattern is starting to seep into the ecosystem:

https://www.google.com/search?q=flutter+%22ambiguate%22+site%3Apub.dev

[jakemac53]

It's the new unawaited! lol

[leafpetersen]

FWIW in a bunch of Flutter code to avoid the unignorable compiler warning log spam when writing code that deals with APIs that are nullable in old versions and non-nullable in new ones we've explicitly used hacky functions that happen to not cause any warnings today, like this:

Without wading into this again, I would strongly caution against driving long term strategic choices in our tooling off of short term issues like this. Mixed mode code won't be a thing within a year or so. There may (or may not) be good reasons for doing this long term, but short term pain around the migration is not a good motivation for driving the long term UX of our product. I would strongly suggest staying focused on the long term UX of the language that we want to see for our users.

[Hixie]

This is not a mixed mode issue, it's an issue of an API that changed from nullable to non-nullable, which is not going away. I agree that we should focus on long term issues; the above is an example of the kind of thing people are doing because of (unignorable bogus) warnings today, and I see no reason to believe that's a short-term issue.

[lrhn]

That is actually a very interesting situation.

Changing a function from returning Foo? to returning Foo is a safe and non-breaking change (modulo the usual caveats about people implementing interfaces, but say it's a static function).

It's non-breaking because everything you can do on a Foo?, you can also do on a Foo (and more!), that's what object oriented subtyping is all about.

Some of the things you can do on Foo? are unnecessary on Foo. That's because we have operations on Foo? that are not just methods. If ?? was a method on Foo?, then it was also a method on Foo, and nobody would complain about calling it. Language-level operators are not virtual methods, the compiler knows and understands what they do (heck, the language specification knows), which is why we know when using them is unnecessary. (We could also have an annotation @NullableOnly used on extensions on nullable types, which introduces warnings if you use it on a non-nullable type, because it's unnecessary there. That would make sense too.)

That's where the warnings come in. Warnings are not errors, so the change is still non-breaking, even if it makes existing code get warnings for doing unnecessary things, and out tools are able to recognize those null-related unnecessary things. But analysis only ever takes one version of the code into consideration.

This is code that wants to be compatible with multiple distinct versions of the same API, and it does so by doing things that are unnecessary for some of the versions. That gives rise to warnings when analyzed only against the other versions. The warning is technically correct (best correct!) in the context where analysis runs, but its not absolutely correct in the global context.

And therefore it makes sense to be able to silence the warning. There is no workaround, no way to rewrite and not get the warning while still satisfying the original goal.

That's why all warnings must be silenceable. There can be reasons for keeping the warning-triggering code in the program, reasons why the analysis leading to the warning is wrong, because it just doesn't have all the facts. We know that's the case, otherwise it could have been an error.

So, either make compilers respect // ignore (which is a big effort), or make them be able to not emit warnings at all. (Or, heck, let them accept a file of "ignore warnings" declarations which say which files and line/column ranges to not show warnings for, or maybe even specific warnings. Then we can create a tool to generate such a file from // ignore: comments. Or something else, options are plenty.)

(If we want a general helper instead of _ambiguate, maybe:

extension MakeNullable<T extends Object> on T? {
  T? get nullable => this;
}

)

[gmurray81]

Here's my two cents on this. I'm working on transpiling another language to Dart to use in a package for flutter. If the compiler emits non-ignorable warnings for things like redundant null assertion operators then it is literally asking for my transpilation's flow analysis to be at least as complex as the flow analysis that is causing this warning to be emitted. When, alternatively, if I can assert that I'm using the operators appropriately enough (if redundantly) in the transpilation output, being able to just ignore the error is far far preferable to taking on unbounded amounts of extra complexity in tracking with Dart's code flow analysis.

If I could just ignore the error when I build that would be one thing, but because the end user is the one compiling all the packages, I really need to prevent them from seeing a warning if I have asserted that it is ok.

[bwilkerson]

@gmurray81

I can think of a couple of solutions for you situation. The first is to generate an ignore_for_file comment at the top of every file. That will cause the diagnostics to not be reported to your users.

The second, and in my mind better, is to run dart fix over the generated files. It has the ability to fix that particular diagnostic, which means that the code would be better without you needing to re-implement flow analysis (which I agree you want to avoid doing for several reasons).

[leafpetersen]

The first is to generate an ignore_for_file comment at the top of every file. That will cause the diagnostics to not be reported to your users.

I don't think the compilers respect ignore comments (at least not yet). The dart fix suggestion seems like a good workaround. Just in the spirit of suggesting expedient workarounds (not to deny the validity of your use case, thanks for the input!), another option might be to always first upcast to the nullable version of the type before emitting the ?.. That is, emit (e as T?)?.foo instead of e?.foo. That is, assuming you reliably know the type of e.

[gmurray81]

yes, I have been unable to find a way to ignore the warning with ignore_for_file. I'm of the mind that no warning should ever be unignoreable unless its really an error. And if an automated "fixer" can find a warning and absolutely reliably auto correct it, then it should definitely be a warning, and, I believe, handled by the analyzer rather than the compiler.

The dart fix suggestion is interesting to me. However, I'd have a few questions about that approach:

1. is it possible to run this targetting a specific rule in order to reduce the scope of the churn.
2. is running dart fix gauranteed not to impact the behavior of the modified logic? Or it is assumed that a human is going to review the modifications for validity.

If a human is supposed to confirm validity of the mutations then this probably isn't a valid approach to work around the issue.

Isn't an unnecessary shift to a nullable context going to generate its own unignorable warning maybe?

All in all, this should simply be an ignorable warning. Generated/Transpiled code is a valid use case that should be considered in the tooling.

[gmurray81]

as an addendum, a language that seems to be trying to eschew reflection in a lot of cases should make special affordances to not interfere with the ease of generating logic without compulsory warnings.

[leafpetersen]

2. is running dart fix gauranteed not to impact the behavior of the modified logic?

I believe this is the case.

Isn't an unnecessary shift to a nullable context going to generate its own unignorable warning maybe?

This will generate an analyzer hint (which can be ignored, but which will probably not be visible to end users anyway since it is presumably in a different package). It will generate no compiler diagnostic.

All in all, this should simply be an ignorable warning. Generated/Transpiled code is a valid use case that should be considered in the tooling.

Acknowledged.

[lrhn]

The argument against up-casting is that the compiler can then not (or not as easily) choose to just ignore the null-assert/null-aware check. An e?.foo where e is non-nullable should be trivially equivalent to just e.foo, but (e as T?)?.foo needs the compiler to recognize that (e as T?)?.foo is just e.foo as well. Which it is, even for extension methods, but not as trivially. It works, but by doing two unnecessary operations instead of one, to switch one warning to another hint. It'd be preferable to not have to do anything.

[Hixie]

The issue linked above (https://github.com/VeryGoodOpenSource/dart_frog/issues/460) is an interesting example of how this is causing more confusion than help in the wider community, IMHO.

[johnniwinther]

cc @stereotype441

[munificent]

We have changed the language specification so that tools are no longer required to report all specified warnings. More context is here.

This shouldn't be interpreted to mean that the language team prefers that any particular tool does not report warnings. We are just removing a constraint on the discussion. If, say, CFE decides they think it's best to stop reporting warnings, now they can do that without being in violation of the spec. But if CFE wants to continue reporting some or all of the warnings, they are free to do that as well. It's a product/UX decision now and not a specification/correctness issue.