NNBD. Type promotion doesn't work in some cases

[sgrekhov]

Run the following code in dartanalyzer with --enable-experiment=non-nullable

class A {
  foo() {}
}

main() {
  A? a = null;
  a = new A();
  if (a != null) {
    a.foo();        // ok
    {a.foo();}      // ok
    () {a.foo();};  //  error - The expression is nullable and must be null-checked before it can be used. - null_check_operator_A01_t01.dart:52:9 - unchecked_use_of_nullable_value
  }
}

dartanalyzer version 2.7.0-dev.2.1

What is interesting here, that if to remove a = new A(); issue disappears. For example there are NO ISSUE for the following cases

  A? a = new A();
  if (a != null) {...

  A? a = null;
  if (a != null) {...

And THERE IS the issue for the code

  A? a = new A();
  a = new A();
  if (a != null) {...

Expected result: there must be no error in any case

[eernstg]

I think this is working as intended (but it's about promotion so we haven't got the specification pinned down exactly yet). The point is that the existence of a function object (based on the function literal () {a.foo();}) changes the rules about promotion, because it's in principle possible that this function gets executed at any point in time (no attempt is made to track where the value of that function literal expression goes, it is simply considered to be available to all unknown code, as if it had been assigned to a global variable). That again means that a cannot be promoted in the body of the if (at least not in that function body) because the function could be called at a time where the promotion of a is not valid, in particular because there is an assignment to a (a = new A()).

So there's a non-trivial amount of caution being applied here, and in each case it might be possible to prove that the promotion could soundly be performed, but the general rule has to be quite pessimistic.

@leafpetersen, do you expect promotion to be applied to a in the example?

[leafpetersen]

Yes, any promoted variable which is assigned anywhere in the enclosing method is demoted to its declared type on entry to a function literal.

[sgrekhov]

@leafpetersen, @eernstg, thank you for the clarification. I'll have to revisit all the tests which uses Expect.throws(() {a.foo();})....

Yes, any promoted variable which is assigned anywhere in the enclosing method is demoted to its declared type on entry to a function literal.

I think, it makes sense to not to take into account assignments in a dead code. For example

main() {
  A? a = new A();
  if (a != null) {
    () {a.foo();}; // error
  }
  if (false) {
    a = new A(); // assignment in a dead code
  }
}

[eernstg]

dead code

That's an interesting point: We do have some usages where if (bool.fromEnvironment(...)) is used to get something like an #ifdef directive in Dart. This means that we can't expect dead code to be eliminated, and in the case where we compile for deployment and all those #ifdefs have been disabled, we should get all the optimizations we can.

But it could be a source of confusion if the #ifdef'd code contains a throw expression or similar, and that enables promotions elsewhere. This could suddenly mean that the program compiles in debug mode, but when we switch to a deployment compilation we suddenly have a bunch of compile-time errors, because of the missing (or just different) promotions.

[lrhn]

Detecting dead code is an option, but I'm not sure it's one which pays for its own complexity. Most production code will not contain any dead code. We can make the analyzer warn about dead code, and most people will just delete it then.

There is the situation where code is dead because it depends on a constant from the environment, but it would not be dead when running the same program in a different configuration. I'd probably still not want to require dead-code detection.

However, since we do have some flow analysis, it might make sense to not demote variables in functions if all assignments to the variable occurs before the function is even created. That would apply to the example here. It's all a question of how clever we can (efficiently) be compared to how understandable it is to users.

[leafpetersen]

However, since we do have some flow analysis, it might make sense to not demote variables in functions if all assignments to the variable occurs before the function is even created.

I don't think the style of analysis we're proposing to do is compatible with this. There was an issue filed many moons ago about using a fixed point analysis (or maybe it was discussion in a doc). The decision was not to do this - we collect assigned variables syntactically, and then do a one pass flow analysis.