Boolean variables sometimes fail to promote when implicitly typed

[stereotype441]

(Note: technically this is a bug in the implementations, not in the language spec. However, since both the analyzer and CFE exhibit the same bug, fixing it would technically constitute a breaking change, so I think it's better to track it as a "language bug")

Prior to shipping null safety support, I implemented a feature in flow analysis wherein local boolean variables could contribute to type promotion. For example:

test(Object o) {
  bool isInt = o is int;
  if (isInt) {
    o.isEven; // Ok, `o` is known to be an `int`
  }
}

This works with variables that are assigned after their declaration as well, e.g.:

test(Object o) {
  bool isInt;
  isInt = o is int;
  if (isInt) {
    o.isEven; // Ok, `o` is known to be an `int`
  }
}

I've discovered a flaw in my implementation: if the boolean variable in question has an implicit type, then flow analysis fails to consider the RHS of its declaration as a possible source of promotion information. Which means that this works:

test(Object o) {
  var isInt;
  isInt = o is int;
  if (isInt) {
    o.isEven; // Ok, `o` is known to be an `int`
  }
}

But this does not:

test(Object o) {
  var isInt = o is int;
  if (isInt) {
    o.isEven; // ERROR: `isEven` not defined for type `Object`
  }
}

I feel pretty strongly that we should fix this bug in a future release of Dart. Technically it is a breaking change to the language, since any change in the behavior of inference can potentially break a sufficiently contrived program. But it is probably a really minor breakage. So the question is, is it a minor enough breakage that we can fix it for all language versions that support null safety? Or do we need to wait until we're making a minor version number bump to the language, and condition the bug fix on the language version?

I will try to gather some information about how breaking the change would be, and update this bug when I have more information.

[munificent]

I feel pretty strongly that we should fix this bug in a future release of Dart.

Me too.

[stereotype441]

I prototyped a fix (https://dart-review.googlesource.com/c/sdk/+/210160) and ran it through google3 to see how wide ranging the consequences would be. Here's what I found:

• 17 null checks (!) will become unnecessary (13 in open source, 4 in closed source)
• 1 null aware operator (?.) will become unnecessary (closed source)
• 4 casts will become unnecessary (3 in open source, 1 in closed source)

That's a big enough effect that it seems worth making this fix. Unfortunately, the amount of open source code that would be affected is large enough that I'm not sure we could safely slip this change in without associating it with a specific language version. (If we did try to slip it in, then the maintainers of the affected open source packages might choose to remove the unnecessary null checks / casts, and then when their customers downloaded the updated package version and tried to run it using an older, stable version of dart, they would have compile errors).

I'll consult with the language team about how to roll the fix out safely.

[leafpetersen]

SGTM. Per discussion, I think putting this behind a language version is probably the right thing to do on balance. We can piggy-back this on constructor tearoffs. There will need to be a breaking change announcement and a CHANGELOG entry.

[stereotype441]

As of dart-lang/sdk@1b18e8f I believe this is fully implemented. The fix is tied to the constructor-tearoffs language feature so it will take effect when that feature rolls out.

[bernaferrari]

Does that fix this? if/else works, but assert doesn't.

void test1(int? value) {
    if (value == null) return;
    value.bitLength; // this works without a '?'
}

void test2(int? value) {
    assert(value != null, 'aaa');
    value.bitLength; // won't compile without a '?'
}

[mateusfccp]

@bernaferrari Assertions won't promote variables, and this is the desired behavior, as assertions are debug-only and evaluated at runtime.

[jodinathan]

is this in the beta or dev sdk?
we are refactoring a big project and this would improve a lot our new code

[stereotype441]

@jodinathan it's in beta right now (which is currently at 2.15.0-178.1.beta) but not stable (which is currently at 2.14.4). Note that if you want to take advantage of the feature you'll have to upgrade your tools to beta and ensure that your code uses language version 2.15 (either by setting the sdk requirement in your pubspec to something like ^2.15.0, or by putting // @dart=2.15 at the top of your source files).

[jodinathan]

@stereotype441 made it work, thanks.

However, it seems it doesn't work with null check on objects, right?
ie:

    List<Object>? ll;
    var ifn = ll?.isNotEmpty == true;
    var ifn2 = ll != null;
    
    if (ifn) {
      print(ll.length); // illegal, ll may be null
    }
    if (ifn2) {
      print(ll.length); // ok
    }

[stereotype441]

@jodinathan Yes, that's true, the pattern ll?.isNotEmpty == true doesn't promote ll. The feature request for that is being tracked here: #1224