Should null-check/assert patterns produce a warning on a constant with a nullable type?

[sgrekhov]

Analyzer produces a warning in case of use of null-check and null-asser patterns on a constant of nullable type.

const int? one = 1;

main() {
  int x = 1;
  if(x case one?) { // unnecessary_null_check_pattern warning
    print("One");
  }
  if(x case one!) { // unnecessary_null_assert_pattern warning
    print("One");
  }
}

Please confirm if it is really intended.

cc @eernstg

[eernstg]

As mentioned here, these warnings are not part of the language specification, but they are recommended.

Just to state an opinion, it seems reasonable for the warning to be emitted based on the static type of the expression, not on the actual value (just like non-constant expressions where we must rely on the static type).

On the other hand, it might also be useful to emit a diagnostic message (a lint, probably) in the case where a constant variable declaration has an explicit type annotation, and the type is a proper supertype of the run-time type of the value. Similarly for a final variable where the lint could flag declarations where the type annotation is a proper supertype of the static type of the initializing expression.

This means that we'd lint det declaration const int? one = 1;.

Then again, this might not be a good idea at all: If the type annotation is a widely used public type T and the other type S (the run-time type of the constant variable and the static type of the initializing expression for the final variable) is a private type (or just a less-widely used public type) then T might be a much better choice of "official" type for this value than S. We probably don't want to force developers into a style where many top-level constant/final variable declarations have an // ignore: ... comment just because they give access to an instance of a private class.

So I'd tend to prefer that the warning relies on the type annotation (not the actual value), and the fact that the type annotation is a proper supertype should not be linted, because it isn't necessarily a code smell.

@bwilkerson, WDYT?

[lrhn]

I agree that the warnings seem correct and intended.

The ? and ! of a pattern are applied to the matched value, before the result of that is maybe passed to the inner pattern.

The matched value type is the type of x which is int. Doing a null check on that is unnecessary. It shouldn't matter what the pattern below the ? or ! is.

[sgrekhov]

No warning if the type of x (matched value) is int?.

[lrhn]

If the matched value type is nullable, then a pattern of nonNullConstant? is redundant because it's completely equivalent to just nonNullConstant.`

I don't think it's a warning we have recommended (don't remember), but it's OK to warm about it as unnecessary code.

The pattern nonNullConstant!, on the other hand, behaves differently than just nonNullConstant, in that it throws on null. It's a weird pattern. If you consider ! as an assertion that should never trigger, what is what I'd recommend - you should never deliberately throw an Error - then it is actually equivalent to nonNullConstant, and a warning could be reasonable.

But this should be answered by the analyzer people who decided which warnings to give.

[bwilkerson]

We currently aren't producing a warning for the case of a nullable value and a null-check pattern:

const int one = 1;
void f(int? x) {
  if (x case one?) {}
}

We should consider adding that case to the test even though it would be a breaking change.

All of the other cases are intentional.