dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.06k stars 1.55k forks source link

Linter null safety testing does not respect short circuit evaluation #51699

Open lukehutch opened 1 year ago

lukehutch commented 1 year ago

This code won't compile, for nullable y:

int x = /* ... */;
int? y = /* ... */;
if (y != null && x > y) { /* ... */ }

What is needed is

int x = /* ... */;
int? y = /* ... */;
if (y != null && x > y!) { /* ... */ }

The ! is unnecessary because of short circuit evaluation, but the linter doesn't seem to know that.

lrhn commented 1 year ago

Can you show actual runnable code which fails? How do you compile it?

This runs for me locally (3.0.0-...) and in DartPad (2.19.4).

void main() {
  int x = 2;
  int? y = 1 as dynamic; // Avoid promoting assignment
  if (y != null && x > y) {  print("OK"); }
}

If your y variable is not a local variable, then it can't be promoted, but then it also can't be declared just before an if.

lukehutch commented 1 year ago

My apologies, I have significantly revised the code since I filed this, and I can't reproduce this now, nor can I find the code in the VS Code timeline. I just extracted the part where it was failing and rewrote as x and y without testing it. I'll close this and re-open if I see it again. Thanks for testing.

Dart 2.19.2 / VSCode

lukehutch commented 1 year ago

OK, I can reproduce this now.

Broken (should compile fine though):

class Val {}

class ValSetter {
  void Function(Val, String)? valueSetter;

  void setValue(Val? oldValue, String newValue) {
    if (oldValue != null && valueSetter != null) {
      valueSetter(oldValue, newValue);    // "The function can't be unconditionally invoked because it can be 'null'."
    }
  }
}

Working version:

class Val {}

class ValSetter {
  void Function(Val, String)? valueSetter;

  void setValue(Val? oldValue, String newValue) {
    if (oldValue != null && valueSetter != null) {
      valueSetter!(oldValue, newValue);    // Need "valueSetter!" to fix. Note that "oldValue!" is not needed though.
    }
  }
}
lrhn commented 1 year ago

The valueSetter field is not a local variable, and you can only promote local variables. That's why it doesn't work.

See: https://dart.dev/guides/language/effective-dart/usage#consider-assigning-a-nullable-field-to-a-local-variable-to-enable-type-promotion

I'd rewrite it as:

if (oldValue != null) {
   valueSetter?.call(oldValue, newValue);
}
lukehutch commented 1 year ago

Ah, thanks, and sorry for the false alarm, I had forgotten that rule.

lukehutch commented 1 year ago

@lrhn I have a suggestion about this, since I have run into this a number of times, and I always seem to forget it's the same problem:

In cases where a local variable would be able to be promoted, but a non-local cannot be, and there is a nullability error, can you please add an explanation to the error message explaining why the variable could not be promoted?

lrhn commented 1 year ago

That sounds like a job for the analyzer. Let's reopen it as a request for that. (There probably is one already, but they'll know where to find it.)

That is: Give a special warning when an unpromoted non-local variable fails to be valid, but it would have worked if the previous test had been able to promote.

(It's non-trivial if the expression is more than just a single identifier, but the single identifier is also the one that looks the most like it should work.)

srawlins commented 1 year ago

@stereotype441 did add "why not promoted" messaging in analyzer, but it may not be applied to unchecked_use_of_nullable_value errors.