Null safety promotion breaks when using local functions

[Kavantix]

Example code that is incorrectly analyzed:

void main() {
  String? test;
  void t() {
    test = null;
  }
  if (test != null) {
    test += ''; // error test can be null
  }
}

This is the same on all dart versions that support nnbd

I already read here that this might be by design but it is quite annoying and at least the error could be better (took me a while to figure out it was the local function that was breaking it).

[bwilkerson]

We are actively working on improving the diagnostics to make this easier to understand. Progress is being tracked on #44897.

[mit-mit]

Closing as duplicate

[Kavantix]

@mit-mit do you mean it's a duplicate of #44897?
Because if so I would have to disagree.
Even though there are some cases where the promotion cannot be correct, here I have an if not null right around where I use it so it should definitely work in this case IMHO

[mit-mit]

I may have misunderstood (I'm on triage duty, and triaging many issues). cc @lrhn can you comment?

[Kavantix]

No worries, thanks for taking the time!

[lrhn]

It doesn't work because there is an assignment to the variable in a function somewhere. The analysis assumes that that function can be called at any time (because ruling it out is hard in general), and therefore it can't assume that the test result still applies at the later use.
We might be able to detect in some situations that the function cannot possibly be called between test and use, but ... it's going to be hard to define those cases in such a way that people can reliably rely on them, and not have their code become invalid because of a small change to the code that moves it just outside the safe area. An unpredictable boundary for a feature can be worse than just plain not having the feature. At least it's easier to understand.

Getting a better error message is a good goal, which is what #44897 is about, so if that's the goal, then this issue is a duplicate.

If you want the language to actually promote in this situation, then we can keep this issue open as a feature request, but I don't think it's something you should expect to happen. It's not a low hanging fruit for promotion enhancement, so even if we want to make some things better, this particular case is not going to high on the list.

[Kavantix]

Thanks for explaining why it doesn't work in this case.

and not have their code become invalid because of a small change to the code that moves it just outside the safe area.

That's exactly what seems to happen now to the user, you simply use the variable in a local function and suddenly you code no longer compiles.

An unpredictable boundary for a feature can be worse than just plain not having the feature. At least it's easier to understand.

I agree that it should be predictable, perhaps simply only checking the previous statement would do then one could at least use a null check instead of always needing to do a null check and an !

So for instance:

if (test != null) test += '';

Works but:

if (test != null) {
 
 otherStuff();
  test += ''; // can be null
}

does not work.

But if you then do:

if (test != null) {
 
 otherStuff();
  if (test != null) test += '';
}

it can work again

The benefit being here that one never needs to rewrite it to this:

test = test! + '';

[mit-mit]

@Kavantix I'm having trouble following your example here with otherStuff. Can you share a DartPad link that illustrates it?

[Kavantix]

@mit-mit the example is my idea, not how it currently works.

I was simply trying to pose a solution to be able to do a null check instead of having to resort to using a ! for this case.
So the idea being that if there is no code between the null check and the statement where you use the variable that was checked then we can now for sure that it cannot have changed.
And by only doing it at that spot it is still predictable to the user.

So in my example above the difference between the two with otherStuff is that one is doing the null check more than 1 statement before using the test variable whereas the latter is checking for null right before the statement that uses the test variable.

[lrhn]

"No code between" is a very tricky thing to define.
It seems simple with

if (test != null) test += '';

but only if test is a field, not a getter/setter pair. If we make one of those promote, and not the other, then we are breaking the getter/field symmetry and makes it a breaking change to go from a field to a getter. So, we don't want that ... and if don't want to break getter/field symmetry, that basically blows away any attempt to promote fields based on reading their value twice.

Also, it looks like

if (test != null) someFunction(test);

should work, but it only works if someFunction is a function, not if it's a function-typed getter (because then the getter is called between the two test evaluations). The same syntax works or is rejected depending on how things are declared. Might not be a problem, readers usually know whether something is a function or a getter, but still slightly worrying.

[mit-mit]

Closing as by-design.

[Kavantix]

@lrhn I get your point so then this will just have to stay open as a feature request until somewhere in the future this promotion will be possible.

[lrhn]

We definitely want to make it easier to work with nullable fields. I'm not convinced that type promotion is the way to do it, we might need a separate feature, like check-and-bind.

An either case, I'm closing this issue as a duplicate of the issues related to field promotion in the language repository.

[Kavantix]

@lrhn This one is not about a field though it’s a local variable
Or does using it in a local function basically ‘promote’ it to field under the hood?

[lrhn]

Right, this is a local variable, so some of my answer was unrelated. The issue is still that the variable might change value between check and use, which it's assumed to be able to because there is an assignment in a closure, and we don't analyze where that closure might go.
It's easier to see that the use is immediately after the check, and here it is, but it's still a very small window where that promotion is valid. The if (text != null) text += something; could work, but only because text is read before something is evaluated. If it had been text = text.concat(something), and text wasn't String, then we'd have to check whether concat could be a getter. If anything happens between the two, like if (text != null && text == constant) text += something; would not be safe (because constant might change to a getter which could call the closure changing the value of text).

These caveats still makes the promotion too unpredictable for users, very small changes can invalidate the promotion, and I don't think the effort needed to make it work for the cases that are actually safe is worth it.

If anything, I'd rather work on detecting whether the closure containing the assignment can escape or not, and if not, then only invalidated the promotion when the function is actually called. I think that has a much higher chance of providing promotion in general, than trying to detect whether the use is "right after" the check. It's non-trivial, and not something we currently do.

[Kavantix]

Makes sense, thanks for taking the time to explain it :)