Field promotion erroneously carried from one cascade to another when the cascade target is unstable

[stereotype441]

The following program is currently accepted by the analyzer and CFE when the "inference_update_2" flag (field promotion) is enabled:

class C {
  D _d;
  C(this._d);
}

class D {
  final E? _e;
  D(this._e);
}

class E {
  late C _c;
  void f() {
    _c._d = D(null);
  }
  void g() {}
}

void test(C c) {
  (c._d).._e!.f();
  (c._d).._e.g(); // (1)
}

main() {
  var e = E();
  var d = D(e);
  var c = C(d);
  e._c = c;
  test(c);
}

But running the program yields a null-reference exception:

Unhandled exception:
NoSuchMethodError: The method 'g' was called on null.
Receiver: null
Tried calling: g()
#0      Object.noSuchMethod (dart:core-patch/object_patch.dart:38:5)
#1      test (file:///usr/local/google/home/paulberry/dart-new/sdk/tests/language/inference_update_2/foo_test.dart:21:14)
#2      main (file:///usr/local/google/home/paulberry/dart-new/sdk/tests/language/inference_update_2/foo_test.dart:29:3)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:296:19)
#4      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:189:12)

This is a soundness bug--the type system should have produced an error indicating that the target of .g() (on the line marked (1)) requires a null check.

[stereotype441]

I know how I want to fix this, but I'm about to go on a 3 week vacation, so to make sure I don't forget, I'm writing up my plans here.

The cause of this issue is that flow analysis internally tracks promotions for all property gets, regardless of whether field promotion is enabled, and regardless of whether the property is stable (i.e. associated with a private final field); but at the last minute, when the type of a field is needed, it suppresses promotion if necessary. The mechanism for suppressing promotion is the _Reference.isPromotable flag, which is set to false if field promotion is disabled, the property is unstable, or if the target of the property get is unstable. This is convenient for producing the "why not promoted" context messages (which customers have found very helpful thus far), because flow analysis can easily see what type the property get would have been if it had been promotable. However, it's a bit of a hack, because it means that flow analysis is internally re-using the same SSA node for distinct accesses to unstable properties, even though those distinct accesses don't actually produce the same value. With field promotion disabled, everything works out because all property references have isPromotable set to false, therefore we have the invariant that any time we re-use an SSA node where we shouldn't, promotion is suppressed. With field promotion enabled, things still mostly work out, because references to unstable properties have isPromotable set to false, and this is inherited by sub-properties, maintaining the invariant.

However, the implicit variable tracking the target of a cascade has its isPromotable flag set to true, so that a ! applied to a stable field in one cascade section will cause promotion of the same field in subsequent cascade sections. This breaks the invariant. That's what's going on in the above example--the two references to .._e in test are using the same SSA node (due to the hack) and are marked as promotable (due to being inside cascade sections), so flow analysis thinks that it's sound to promote the second one based on the fact that the first one has a ! after it. But that's not sound because f() modifies the value of c.d in between the two references.

In principle, we could fix this with a band-aid that allocates a fresh SSA node for any cascade target that's unstable (i.e. has its .isPromotable flag set to false). But we would be layering a hack on top of a hack, and I would worry that this would be a long term source of technical debt in an area of the codebase that is already quite complex. I would much rather rework the computation of "why not promoted" messages so that gathers together information from multiple releated SSA nodes, and then remove the hack that's causing us to re-use the SSA node for unstable property gets. This would have the side benefit of making "why not promoted" analysis able to detect other circumstances where a field promotion fails due to the SSA node changing (e.g. in a._b!.f(); a = ...; a._b.f(), the second .f() is invalid because a was written to).