`unnecessary_null_check_pattern` false negative

[FMorschel]

Repro:

// ignore_for_file: unused_local_variable
class Foo {
  num? foo;
  num bar = 0;
}

void foo(Foo f) {
  if (f case Foo(:num foo?, :var bar?,)) {}
}

The second ? inside the pattern match triggers unnecessary_null_check_pattern but not the first. I'd say that since we are testing for the explicit num (non-nullable type), it should trigger, too.

[lrhn]

The null check is performed before the num type check, so at the point where it happens, it's not unnecessary (trivially true).

It's the same as if (x != null && x is num)....
Do you get a warning for that?

[FMorschel]

Yes, we do... kinda 😅

I know it is not the same thing but there is some unnecessary verification being done in both cases.

[RohitSaily]

It's the same as if (x != null && x is num)...

The type check aspect should be considered non-necessary in this case then, making it consistent with the example from above (rewritten/paraphrased from image)

void f(num? x)
{	if (x!=null && x is num) // ERROR: unnecessary_type_check "x is num"
		{}
}

so this would happen

// ignore_for_file: unused_local_variable
class Foo {
  num? foo;
  num bar = 0;
}

void foo(Foo f) {
  if (f case Foo(:num foo?, :var bar?,)) {} // ERROR: unnecessary_type_check ":num foo?"
}

and direct corrections would be either :var foo?, :final foo?, or :num foo

[FMorschel]

This is less preferred IMO since the type could be there to align with another instance creation in an OR pattern (below). I'd still vote for us to change the evaluation order here so that it is detected as I suggested.

// ignore_for_file: unused_local_variable
class Foo {
  num? foo;
  num bar = 0;
}

class Bar extends Foo {
  int? get foo => super.foo?.toInt();
}

void foo(Foo f) {
  // now we _need_ num since it is casting Bar.foo here
  if (f case Foo(:num foo) || Bar(:num foo?)) {}
}

WDYT @bwilkerson?

[RohitSaily]

Wouldn't choosing the correction :num foo apply here?

void foo(Foo f) {
  if (f case Foo(:num foo) || Bar(:num foo)) {}
}

And that can only be suggested in the case the specified type is different. When the type is the same the corrections can have the user choose between var foo? or final foo?, although if they are using lint rules that require the type to be specified, the type annotation is still relevant

---

I did intuitively think the evaluation order was the other way around for some reason. I'm personally on board with changing it if it's not breaking in any way

[FMorschel]

Wouldn't choosing the correction :num foo apply here?

It would definitely work. I just feel like these suggestions might also have :num? foo? (which is a bit unintuitive IMO but totally understandable once we get this is the right order to read) in some cases, and it may be what the user wants.

I did intuitively think the evaluation order was the other way around for some reason. I'm personally on board with changing it if it's not breaking in any way

Me too. I think it might be breaking if we have:

void foo(Foo f) {
  if (f case Foo(:var foo) || Bar(:num? foo?)) {}
}

This would mean that I want Bar.foo to be cast as num?, but for this block to only happen when it is non-null. I'm unsure of how common that is, though, if at all.

[bwilkerson]

I'd still vote for us to change the evaluation order ...

The evaluation order is given by the Dart Language Specification (or at least the feature spec that hasn't yet been integrated into the base specification). We can't change that without a change to the specification.

If we wanted to catch this case, and I'm not at all convinced that we do, then it would probably be something along the lines of a lint that would look for variable patterns with an explicit type annotation where the type of the variable is "obvious" (for some definition of "obvious").