Skip to content

Analyzer: Consider noting failed null promotion in error messages #42388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
leafpetersen opened this issue Jun 18, 2020 · 4 comments
Closed

Analyzer: Consider noting failed null promotion in error messages #42388

leafpetersen opened this issue Jun 18, 2020 · 4 comments
Labels
legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Milestone

Comments

@leafpetersen
Copy link
Member

Based on a real example encountered by @yjbanov , consider this example:

void foo(int y) {
  int? x;
  for(int i = 0; i < y; i++) {
    switch(i) {
      case 0:
      x ??= 3;
      x.isEven;
      break;
      default:
      (() { x ??= 4;})();
    }
  }
}

The call to x.isEven is an error because the x ??= 3 can't promote, because there's an assignment to x in a closure which could be live on the back edge of the loop. This is easy to miss though, and so it is surprising that x.isEven is an error. The error message that the analyzer gives is correct, but doesn't help the user understand why promotion is not kicking in:

 error • An expression whose value can be 'null' must be null-checked before it can be dereferenced. • /Users/leafp/tmp/test.dart:9:7 • unchecked_use_of_nullable_value

It seems like at least for error messages around nullability, it might sometimes be feasible to notice that the variable in question was not able to be promoted, and give notice of that in some way, e.g something like:

 error • A variable which was declared nullable could not be determined to be non-null because it is written to in a closure, and must be null-checked before it can be dereferenced. • /Users/leafp/tmp/test.dart:9:7 • unchecked_use_of_nullable_value

Not sure if we have enough information lying around to give this kind of feedback, but could be nice if so.

@leafpetersen leafpetersen added legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release labels Jun 18, 2020
@stereotype441
Copy link
Member

This is similar to an idea from some months ago: #38773

@dnfield
Copy link
Contributor

dnfield commented Jul 1, 2020

Another simplified example:

void main() {
  int? a = 10;
  () {
    if (a != null && a > 1) {
      a = 2;
    }
  }();
}

Gives you

dart --enable-experiment=non-nullable main.dart
main.dart:6:24: Error: Operator '>' cannot be called on 'int?' because it is potentially null.
    if (a != null && a > 1) {

Which is very confusing, since in my real code I got to this by what I thought was some minor refactoring (the real example involved moving code that had been in the main block of a function into the then closure of a future), and as far as I can tell from the error message I did in fact determine that a is not null (even though I later assign it in the closure).

@dnfield
Copy link
Contributor

dnfield commented Jul 8, 2020

One thing that didn't occur to me that is possible without creating an explicit temporary variable is to rewrite my code as

void main() {
  int? a = 10;
  () {
    if (a != null && a! > 1) {
      a = 2;
    }
  }();
}

which passes, and feels slightly less strange than creating another variable to check inside the closure.

@devoncarew devoncarew added this to the Future milestone Oct 29, 2020
@devoncarew devoncarew added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug labels Oct 29, 2020
@srawlins
Copy link
Member

I'm closing this as complete; @stereotype441 I think mostly completed the work of "not promoted why" features. And when there are cases where we still don't provide context messages, I think he's opened individual issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants