Migration: propagate non-null intent when all downstream nodes have non-null intent.

[stereotype441]

Capturing some discussion between me and @MichaelRFairhurst this morning

Consider the following code:

int f1(int x) => x; // (1)
void g(int/*!*/ Function(int) callback) {
  callback(null);
}
void test() {
  var f2 = (int x) => x; // (2)
  g((int x) => x); // (3)
  g(f2); // (4)
  g(f1); // (5)
}

Currently the migration engine transforms (3) into g((int? x) => x!) because if x were not null-checked, the function literal wound have type int? Function(int?), and hence couldn't be passed to g.

Semantically, the call sites at (4) and (5) are equivalent to (3), so it seems reasonable that the tool should perform similar migrations to f1 and f2, changing (1) to int f1(int? x) => x!;, and (2) to var f2 = (int? x) => x!;. But currently it doesn't. The reasoning process is: in (3) the function literal exists solely so that it can be passed to g, so no functionality is lost by adding the null check. Whereas the functions declared in (1) and (2) might be used in some other context where their ability to return a null value is important. As a result, migration adds a cast to (4) and (5) that is guaranteed to fail, and the user has to take manual action to address the problem (see #40471).

But we can do better. Since the migration tool does whole program analysis, it should know whether the functions declared in (1) and (2) are used in any other context; if they aren't it can safely propagate the null check into them, producing a better migration result.

This requires a small change to the way the nullability graph propagates non-null intent. Today the rule is: given a node A, if there is any node B pointed to by A via a hard edge, and B has non-null intent, then non-null intent is propagated to A. We would keep that rule, and add the additional rule: given a node A, if all nodes B pointed to by A have non-null intent (regardless of the type of edge connecting them), then non-null intent is propagated to A.

Note: in addition to fixing cases involving callback functions like the one above, we expect this design to in general reduce the number of null checks the tool has to introduce, because if it detects that all calls to a function use its return value in a way that requires it to be non-null, it will consolidate the null check into the function itself rather than force all call sites to do the null check.

Note: this algorithm affects not just the behavior of the f functions in the example above, but also the g function. Once all the f functions have had non-nullable intent propagated through them, all nodes pointed to by the node for callback's parameter will have non-null intent, so it will be marked as having non-null intent, and the null check will be pushed back to the explicit null, like so:

int f1(int x) => x;
void g(int/*!*/ Function(int) callback) {
  callback(null!); // (1) 
}
void test() {
  var f2 = (int x) => x;
  g((int x) => x);
  g(f2);
  g(f1);
}

Of course, it's trivial to see that (1) will fail at runtime. We think this is good, since the code is almost certainly in error, and pushing the null check back to (1) makes that trivially apparent to the user.

Note: in order for this to work properly, we need to make sure that when the EdgeBuilder encounters an expression statement, it draws an edge from the type of the expression to always. This will ensure that in the following code, the call at (2) prevents us from adding a null check to (1):

int f(int x) => x; // (1)
void test() {
  f(null); // (2)
  print(f(1) + 1);
}

Note: if a function in the package's public API might return null, but all call sites within the package null-check its result, the migration tool will go ahead and mark the function's return type as non-null (and add null checks within the function itself). This may not be what the user wants. We think this is ok--it is consistent with the principle that the migration tool should bias toward non-nullability as much as it can based on the examples it sees in the source code given to it; if the user wants to allow additional nullability, they should either add a test case or a /*?*/ hint.

Note: this should also improve our migration results for the case where a list is created, populated, and then passed to a function expecting a list of non-null, e.g. in the following code a null check will be added to (1) rather than a cast added to (2):

int/*?*/ f() { ... }
void g(List<int/*!*/> x) { ... }
void test() {
  List<int> x = <int>[];
  x.add(f()); // (1)
  g(x); // (2)

[stereotype441]

@MichaelRFairhurst notes that if we make this change, we'll also have to make additional graph edges to track situations where a variable's value might not be used. Consider:

int/*1*/ f(int x) => x;
void g(bool b, int/*?*/ x) {
  int/*2*/ foo = f(x);
  if (b) {
    foo + 1;
  }
}

There's only one edge leading out from node 2, pointing to "never" (caused by the statement foo + 1). And only one edge leading out from node 1, pointing to node 2. So the algorithm above would propagate non-null intent back to the return type of f, and from there back to the call site, resulting in:

int/*1*/ f(int x) => x;
void g(bool b, int/*?*/ x) {
  int/*2*/ foo = f(x!);
  if (b) {
    foo + 1;
  }
}

This is bad because it changes behavior in the case where b is false. He proposes dealing with this by creating an edge from node 2 to somewhere representing the possibility that the variable will go unused.

[stereotype441]

There's a problem in the algorithm proposed above: it does the wrong thing for local variables. Consider:

void f(int x, bool b1, bool b2, bool b3) {
  if (b1) print(x + 1);
  if (b2) print(x + 2);
  if (b3) print(x + 3);
}
main() {
  f(null, false, false, false);
}

Analyzing this code causes three edges to be created pointing from the nullability of x to never.
Since never has non-null intent, and there are no other edges pointing away from x, the algorithm proposed above would cause non-null intent to be propagated back to x, which would force a null check into main (which would be guaranteed to fail).

@MichaelRFairhurst and I believe that we can solve this by classifying nodes into two kinds: "variable-like" nodes and "dispatch-like" nodes. A node that is "dispatch-like" has the property that if a null value flows into it through one of its upstream edges at any point in program execution, that event is guaranteed to be followed by a null value flowing out of it along at least one of its downstream edges; a "variable-like" node does not have this property. We would only do the back propagation of non-null intent proposed above on dispatch-like nodes.

Examples of dispatch-like nodes are the nodes associated with return types (since returned values are immediately propagated to their respective call sites) and the nodes associated with callback parameters (since values passed into callbacks are immediately propagated to the callback's implementation). Examples of variable-like nodes are nodes associated with the nullability of ordinary function parameters and local variables.

Note that one way of implementing the distinction between variable-like and dispatch-like would be to create an artifical edge pointing from any variable-like node to "always". But this might not provide the best user experience, since the edge would show up in the preview tool.

[stereotype441]

@a14n made a similar suggestion. His example looks like this:

class A {
  Platform platform;
  String get _env => platform.environment['key'];
  void m1() {
    List<String> v = _env.split('.');
  }
  void m2() {
    int v = _env.length;
  }
}

The migration tool suggests:

class A {
  late Platform  platform;
  String? get _env => platform.environment['key'];
  void m1() {
    List<String >  v = _env!.split('.');
  }
  void m2() {
    int  v = _env!.length;
  }
}

But this would be better:

class A {
  late Platform  platform;
  String get _env => platform.environment['key']!;
  void m1() {
    List<String >  v = _env.split('.');
  }
  void m2() {
    int  v = _env.length;
  }
}

[stereotype441]

As of 1c7fe71, the null safety migration tool has been removed from active development and retired. No further work on the tool is planned.

If you still need help, or you believe this issue has been closed in error, please feel free to reopen.