[NNBD] Type promotion fails for variable uses in anonymous functions

[nex3]

The following function:

int foo({int? arg}) {
  arg = 0;
  return (() => arg)();
}

Produces this error:

  error • A value of type 'int?' can't be returned from the function 'foo' because it has a return type of 'int'. • test.dart:3:10 • return_of_invalid_type

Despite the fact that arg will clearly always be non-null when it's returned. There's not even a hypothetical risk that arg will be reassigned to null between the function being created and being invoked, since the non-nullable assignment appears before the function is created and no nullable assignments exist anywhere.

[stereotype441]

Yeah, this is a known limitation of type promotion. When it sees an anonymous function, it makes a pretty conservative assumption about what might happen to any variables that anonymous function reads: it reasons that if the variable is ever written to, then it's possible that the variable's promotion will be invalidated by a write sometime between the time it's created and the time it's called. So to be on the safe side, it invalidates the type promotion.

Obviously in this example, that's overly conservative, for three reasons: (a) because the anonymous function is called exactly once, immediately after it's created, (b) because the only assignment to arg happens before the anonymous function is created, and (c) because that assignment always assigns a non-null value.

I believe we have some other bugs in the language repo discussing similar issues. I'll try to dig them up and link them here, so that we remember to think about this case when we think about future improvements to type promotion.

[stereotype441]

FWIW, you can work around this limitation by creating a temporary variable that's never written to:

int foo({int? arg}) {
  arg = 0;
  var tmp = arg;
  return (() => tmp)();
}

[nex3]

This is a pretty rough thing to have to work around—Dart heavily encourages the use of anonymous functions for things like collection manipulation, so added friction passing values in produces a lot of pain.

Obviously in this example, that's overly conservative, for three reasons: (a) because the anonymous function is called exactly once, immediately after it's created, (b) because the only assignment to arg happens before the anonymous function is created, and (c) because that assignment always assigns a non-null value.

For what it's worth, (a) here is basically a coincidence—in the actual code in which I'm seeing this issue, there's no way for the analyzer to know that the callback is only called once. But (b) and (c) are both usually true for these situations.

[renatoathaydes]

FYI the concept used in Java and Kotlin of effectively final might be useful.

For example, in Kotlin, this works fine:

fun main() {
    println(foo(true)())
}
fun foo(b: Boolean): () -> Int {
    var n: Number
    n = if (b) 1 else 0.3
    if (n is Int) {
        return { n }
    }
    return { 0 }
}

The closure can only accept n because, even though it's not final, it never changes and that can be safely determined by the compiler.

If you re-assign n at any point in the function AFTER the closure capturing it is created, it won't compile anymore:

fun foo(b: Boolean): () -> Int {
    var n: Number
    n = if (b) 1 else 0.3
    if (n is Int) {
        return { n } // Smart cast to 'Int' is impossible, because 'n' is a local variable that is captured by a changing closure
    }
    n = 4
    return { 0 }
}

Dart should use the same behaviour IMHO.

[renatoathaydes]

BTW In Kotlin, unlike Java, the variable can even be re-assigned as long as it is never re-assigned AFTER the closure captures it (and I didn't use an argument as in the Dart example because in Kotlin, arguments are always final).

[gazialankus]

I think implementing this one must be the easiest among the three reasons:

(c) because that assignment always assigns a non-null value

It's easier to explain away the two other cases with technical limitations but this one is a bit surprising.

FWIW I recently came across this in this example: https://dartpad.dev/?id=fc440d205d8855f5ebbc919a12eec8ea

[stereotype441]

Note that this issue is regularly reported as a bug; it seems that a lot of folks intuitively expect flow analysis to be smart enough to handle this correctly. Here is the latest report: dart-lang/sdk#50573.

[stereotype441]

I added some ideas to dart-lang/sdk#50573 about a possible implementation strategy. Copying them here so that all the discussion is in one place:

FWIW, it's not a trivial fix to do, because type analysis happens in a two fixed passes: one pass where we resolve each bare identifier to its declaration, and a second pass where we assign a type to everything. (There are some assumptions pretty deeply baked in to the analyzer and front end that would make it difficult to expand this to more than two passes). In the front end the first pass happens during parsing, so we have some additional limitations on what data we can collect during that pass. At the moment, the data we collect is approximately: for every block, what variables does it assign to and/or write capture? Returning to your first example:

void foo([int? x]) {
  x ??= 42;
  print(x + 1); // <- not an error; x is non-null because of the above line.
  void y() {
    print(x + 1); // <-- error because I don't know.
  }
  y();
}

During the first pass, the only information we collect about x is that there is an assignment to it somewhere inside the outermost block. Then, during the second pass, when we reach the declaration of void y(), all we know is that there's a write to x somewhere, but we don't know that that write has definitely occurred in the past. So we don't know whether it's safe to preserve the promotion or not.

The way I would approach improving this is to change the first pass so that instead of keeping track of information on a block-by-block basis, it builds up a simplified graph of the control flow in the function, and then I'd add a step in between the two passes that analyzes that simplified graph to determine which variables need to be demoted at the beginning of each block. So for your example above, the first pass would essentially model the code as:

declare x
write to x
create closure y

And it would see that at the time y is created, all assignments to x are in the past (and there are no loops allowing those assignments to be reachable again), so there is no need to demote x.

[lrhn]

Another pattern I keep seeing is what I'd call "monotonic" variables. It might be assigned in several places, including inside closures, but it's never assigned something that would demote it. It's usually around nullability

Example: https://www.reddit.com/r/dartlang/comments/13slo75/why_is_there_no_control_flow_type_analysis_for/

Would it be possible do detect that a nullable variable is never assigned a nullable value, even if it might be assigned inside closures, and if so, allow promotion to work.

void foo(int? x) {
  bar() {
     print(x ??= 42);
  }
  baz() {
    if (x != null) {
      print(x.toRadixString(16);
    }
 }
 baz();
 bar();
 baz();
}

Here we'd normally give up on promoting x because it's assigned to in a closure.
However, we can see that it's never assigned null anywhere, so if the if (x != null) is true, then it must stay true.

It's probably not trivial, because of self references. Will x = test ? 42 : x; count as assigning a nullable value?
In this case, we can see that it's still monotonic. It does not demote, because it only assigns null if x is already null.
(But now it gets complicated, we'd have to have some notion of "this expression has type int? & current type of x".)

[stereotype441]

@lrhn

Would it be possible do detect that a nullable variable is never assigned a nullable value, even if it might be assigned inside closures, and if so, allow promotion to work.

Unfortunately doing this would require a pretty substantial overhaul of how type inference works in the analyzer and the CFE. The key issue is that the design of type inference (and hence flow analysis) in both codebases assumes that the code is visited exactly once, in source order*. In order to detect that a nullable variable is never assigned a nullable value, we would need to visit all writes to the variable before any possibly-promoted reads, and the chances of that order being compatible with source order are pretty low (especially where closures are involved!)

(*Minor exception: for (A; B; C) D is visited in order A, B, D, C.)

[FMorschel]

Could the analyser detect if the closure creation happens on return or is the last thing on the body? Then in some cases, this would be solved.

Something else that got me thinking was, could we, in these cases where the variable is from outside the scope of the closure, add a better error message? At least for now, so people can understand this limitation.

---

Edit

Coming from https://dart-review.googlesource.com/c/sdk/+/387862 on the review for
pkg/analysis_server/lib/src/services/correction/dart/flutter_swap_with_child.dart. Not an example of my suggestion, but a case where this happened and a better error message would have helped.