language icon indicating copy to clipboard operation
language copied to clipboard

Flow analysis loses track of promoted field types in equality tests

Open stereotype441 opened this issue 1 year ago • 1 comments

Normally flow analysis understands that comparing two values with static type Null is guaranteed to succeed, e.g.:

int test() {
  if (null == null) {
    return 0;
  }
  // No `return` statement but that's ok because this code path is unreachable
}

This works even if one of the values being compared is Null solely because it got type promoted via an is test:

int test(int? value) {
  if (value is! Null) return 0;
  Null n = value; // ok because `value` is promoted to type `Null` now
  if (value == null) {
    return 0;
  }
  // No `return` statement but that's ok because this code path is unreachable
}

(Note that an is or as test is the only way to promote to the type Null. Flow analysis doesn't promote values to the type Null based on an == check because it isn't really that useful to do so. And we have an analyzer warning that discourages people from using is Null or is! Null, so probably this doesn't arise very often in practice).

Unfortunately, due to a bug in how flow analysis tracks field type promotion, it doesn't work if the value being compared is a promoted field:

class C {
  final int? _f;
  C(this._f);
}
int test(C c) {
  if (c._f is! Null) return 0;
  Null n = c._f; // ok because `c._f` is promoted to type `Null` now
  if (c._f == null) {
    return 0;
  }
  // Error: missing `return` statement
}

This is a weird corner of the language, and one that I suspect pretty much never arises in practice. But it would be nice to be consistent. In fact, the only reason I discovered this issue is because I inadvertently fixed it in the process of working on a change that makes the internals of flow analysis more uniform (and my fix changed the behavior of a few obscure tests). I do want to go through with that change, though, and so I'm filing this issue to document that the current behavior is a mistake, and that fixing it will be deliberate.

Before I actually change the behavior I'll do some experiments with google3 to verify my suspicion that this pretty much never arises in practice.

stereotype441 avatar Oct 10 '24 13:10 stereotype441

I've verified that fixing this issue causes zero failures in Google3.

stereotype441 avatar Oct 13 '24 15:10 stereotype441

Fixed by https://github.com/dart-lang/sdk/commit/c3ce683cca524069c6237a4e60c281ac519513a8.

stereotype441 avatar Oct 23 '24 16:10 stereotype441