[Breaking change] Account for field promotion to Null when computing reachability based on equality tests.

Fixes #56893.
Fixes dart-lang/language#4127.

Bug: #56893
Bug: dart-lang/language#4127
Change-Id: If8bb0144ebe7024a7f4f7c1733e24632b4549c7f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/389660
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Kallen Tu <[email protected]>

Author: stereotype441

CHANGELOG.md

@@ -31,6 +31,16 @@ main() {
 }
 ```
 
+#### Other Language Changes
+
+- **Breaking Change** [#56893][]: If a field is promoted to the type `Null`
+  using `is` or `as`, this type promotion is now properly accounted for in
+  reachability analysis. This makes the type system more self-consistent,
+  because it mirrors the behavior of promoted local variables. This change is
+  not expected to make any difference in practice.
+
+[#56893]: https://github.com/dart-lang/sdk/issues/56893
+
 #### Dart to Javascript Compiler (dart2js)
 
 - The dart2js compiler which is invoked when the command

pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart

@@ -5396,7 +5396,7 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
         propertyMember: propertyMember,
         promotionKey: propertySsaNode.promotionKey,
         model: _current,
-        type: unpromotedType,
+        type: promotedType ?? unpromotedType,
         ssaNode: propertySsaNode);
     if (wholeExpression != null) {
       _storeExpressionInfo(wholeExpression, propertyReference);
@@ -5418,7 +5418,7 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
         propertyMember: propertyMember,
         promotionKey: propertySsaNode.promotionKey,
         model: _current,
-        type: unpromotedType,
+        type: promotedType ?? unpromotedType,
         ssaNode: propertySsaNode);
     _stack.add(new _PropertyPatternContext<Type>(
         _makeTemporaryReference(

pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart

@@ -7408,16 +7408,10 @@ main() {
     });
 
     group('and equality:', () {
-      test('promoted type ignored on LHS', () {
-        // Normally flow analysis understands when an `if` test is guaranteed to
-        // succeed (or fail) based on the static types of the LHS and RHS. But
-        // due to https://github.com/dart-lang/language/issues/4127, this
-        // doesn't fully work when the LHS or RHS is a property reference; in
-        // that case, the unpromoted type is used.
-
-        // This test is here to make sure we don't change the existing behavior
-        // by accident; if/when we fix #4127, this test should be changed
-        // accordingly.
+      test('promoted type accounted for on LHS', () {
+        // Flow analysis understands when an `if` test is guaranteed to succeed
+        // (or fail) based on the static types of the LHS and RHS. Make sure
+        // this works when the LHS or RHS is a property reference.
         h.addMember('C', 'f', 'Object?', promotable: true);
         h.thisType = 'C';
         h.run([
@@ -7426,21 +7420,15 @@ main() {
           if_(thisProperty('f').eq(nullLiteral), [
             checkReachable(true),
           ], [
-            checkReachable(true),
+            checkReachable(false),
           ]),
         ]);
       });
 
-      test('promoted type ignored on RHS', () {
-        // Normally flow analysis understands when an `if` test is guaranteed to
-        // succeed (or fail) based on the static types of the LHS and RHS. But
-        // due to https://github.com/dart-lang/language/issues/4127, this
-        // doesn't fully work when the LHS or RHS is a property reference; in
-        // that case, the unpromoted type is used.
-
-        // This test is here to make sure we don't change the existing behavior
-        // by accident; if/when we fix #4127, this test should be changed
-        // accordingly.
+      test('promoted type accounted for on RHS', () {
+        // Flow analysis understands when an `if` test is guaranteed to succeed
+        // (or fail) based on the static types of the LHS and RHS. Make sure
+        // this works when the LHS or RHS is a property reference.
         h.addMember('C', 'f', 'Object?', promotable: true);
         h.thisType = 'C';
         h.run([
@@ -7449,7 +7437,7 @@ main() {
           if_(nullLiteral.eq(thisProperty('f')), [
             checkReachable(true),
           ], [
-            checkReachable(true),
+            checkReachable(false),
           ]),
         ]);
       });

tests/language/inference_update_2/promoted_field_type_in_equality_test.dart

@@ -2,14 +2,13 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
-// Tests the behavior described in
-// https://github.com/dart-lang/language/issues/4127, namely the fact that when
-// deciding whether an `==` or `!=` comparison is guaranteed to evaluate to
-// `true` or `false`, flow analysis considers promoted fields to have their base
-// type rather than their promoted type.
+// Tests that the behavior described in
+// https://github.com/dart-lang/language/issues/4127 has been fixed. That is,
+// when deciding whether an `==` or `!=` comparison is guaranteed to evaluate to
+// `true` or `false`, flow analysis considers promoted fields to have their
+// promoted type.
 
-// This test is here to make sure we don't change the existing behavior by
-// accident; if/when we fix #4127, this test should be changed accordingly.
+// This test acts as a regression test for #4127.
 
 import '../static_type_helper.dart';
 
@@ -30,10 +29,10 @@ class C {
       y = null;
     }
     // In analyzing the `==` check, flow analysis assumes that `_f` has its
-    // unpromoted type (`Object?`), so both branches of the `if` are
-    // reachable. Therefore both `x` and `y` should both be demoted here.
+    // promoted type (`Null`), so only the `x = null` branch is
+    // reachable. Therefore only `x` should be demoted here.
     x.expectStaticType<Exactly<int?>>();
-    y.expectStaticType<Exactly<int?>>();
+    y.expectStaticType<Exactly<int>>();
   }
 
   void testImplicitThisReferenceOnRhsOfEquals() {
@@ -49,10 +48,10 @@ class C {
       y = null;
     }
     // In analyzing the `==` check, flow analysis assumes that `_f` has its
-    // unpromoted type (`Object?`), so both branches of the `if` are
-    // reachable. Therefore both `x` and `y` should both be demoted here.
+    // promoted type (`Null`), so only the `x = null` branch is
+    // reachable. Therefore only `x` should be demoted here.
     x.expectStaticType<Exactly<int?>>();
-    y.expectStaticType<Exactly<int?>>();
+    y.expectStaticType<Exactly<int>>();
   }
 
   void testImplicitThisReferenceOnLhsOfNotEquals() {
@@ -68,9 +67,9 @@ class C {
       y = null;
     }
     // In analyzing the `!=` check, flow analysis assumes that `_f` has its
-    // unpromoted type (`Object?`), so both branches of the `if` are
-    // reachable. Therefore both `x` and `y` should both be demoted here.
-    x.expectStaticType<Exactly<int?>>();
+    // promoted type (`Null`), so only the `y = null` branch is
+    // reachable. Therefore only `y` should be demoted here.
+    x.expectStaticType<Exactly<int>>();
     y.expectStaticType<Exactly<int?>>();
   }
 
@@ -87,9 +86,9 @@ class C {
       y = null;
     }
     // In analyzing the `!=` check, flow analysis assumes that `_f` has its
-    // unpromoted type (`Object?`), so both branches of the `if` are
-    // reachable. Therefore both `x` and `y` should both be demoted here.
-    x.expectStaticType<Exactly<int?>>();
+    // promoted type (`Null`), so only the `y = null` branch is
+    // reachable. Therefore only `y` should be demoted here.
+    x.expectStaticType<Exactly<int>>();
     y.expectStaticType<Exactly<int?>>();
   }
 
@@ -106,10 +105,10 @@ class C {
       y = null;
     }
     // In analyzing the `==` check, flow analysis assumes that `this._f` has its
-    // unpromoted type (`Object?`), so both branches of the `if` are
-    // reachable. Therefore both `x` and `y` should both be demoted here.
+    // promoted type (`Null`), so only the `x = null` branch is
+    // reachable. Therefore only `x` should be demoted here.
     x.expectStaticType<Exactly<int?>>();
-    y.expectStaticType<Exactly<int?>>();
+    y.expectStaticType<Exactly<int>>();
   }
 
   void testExplicitThisReferenceOnRhsOfEquals() {
@@ -125,10 +124,10 @@ class C {
       y = null;
     }
     // In analyzing the `==` check, flow analysis assumes that `this._f` has its
-    // unpromoted type (`Object?`), so both branches of the `if` are
-    // reachable. Therefore both `x` and `y` should both be demoted here.
+    // promoted type (`Null`), so only the `x = null` branch is
+    // reachable. Therefore only `x` should be demoted here.
     x.expectStaticType<Exactly<int?>>();
-    y.expectStaticType<Exactly<int?>>();
+    y.expectStaticType<Exactly<int>>();
   }
 
   void testExplicitThisReferenceOnLhsOfNotEquals() {
@@ -144,9 +143,9 @@ class C {
       y = null;
     }
     // In analyzing the `!=` check, flow analysis assumes that `this._f` has its
-    // unpromoted type (`Object?`), so both branches of the `if` are
-    // reachable. Therefore both `x` and `y` should both be demoted here.
-    x.expectStaticType<Exactly<int?>>();
+    // promoted type (`Null`), so only the `y = null` branch is
+    // reachable. Therefore only `y` should be demoted here.
+    x.expectStaticType<Exactly<int>>();
     y.expectStaticType<Exactly<int?>>();
   }
 
@@ -163,9 +162,9 @@ class C {
       y = null;
     }
     // In analyzing the `!=` check, flow analysis assumes that `this._f` has its
-    // unpromoted type (`Object?`), so both branches of the `if` are
-    // reachable. Therefore both `x` and `y` should both be demoted here.
-    x.expectStaticType<Exactly<int?>>();
+    // promoted type (`Null`), so only the `y = null` branch is
+    // reachable. Therefore only `y` should be demoted here.
+    x.expectStaticType<Exactly<int>>();
     y.expectStaticType<Exactly<int?>>();
   }
 }
@@ -183,10 +182,10 @@ void testExplicitPropertyReferenceOnLhsOfEquals(C c) {
     y = null;
   }
   // In analyzing the `==` check, flow analysis assumes that `c._f` has its
-  // unpromoted type (`Object?`), so both branches of the `if` are
-  // reachable. Therefore both `x` and `y` should both be demoted here.
+  // promoted type (`Null`), so only the `x = null` branch is
+  // reachable. Therefore only `x` should be demoted here.
   x.expectStaticType<Exactly<int?>>();
-  y.expectStaticType<Exactly<int?>>();
+  y.expectStaticType<Exactly<int>>();
 }
 
 void testExplicitPropertyReferenceOnRhsOfEquals(C c) {
@@ -202,10 +201,10 @@ void testExplicitPropertyReferenceOnRhsOfEquals(C c) {
     y = null;
   }
   // In analyzing the `==` check, flow analysis assumes that `c._f` has its
-  // unpromoted type (`Object?`), so both branches of the `if` are
-  // reachable. Therefore both `x` and `y` should both be demoted here.
+  // promoted type (`Null`), so only the `x = null` branch is
+  // reachable. Therefore only `x` should be demoted here.
   x.expectStaticType<Exactly<int?>>();
-  y.expectStaticType<Exactly<int?>>();
+  y.expectStaticType<Exactly<int>>();
 }
 
 void testExplicitPropertyReferenceOnLhsOfNotEquals(C c) {
@@ -221,9 +220,9 @@ void testExplicitPropertyReferenceOnLhsOfNotEquals(C c) {
     y = null;
   }
   // In analyzing the `!=` check, flow analysis assumes that `c._f` has its
-  // unpromoted type (`Object?`), so both branches of the `if` are
-  // reachable. Therefore both `x` and `y` should both be demoted here.
-  x.expectStaticType<Exactly<int?>>();
+  // promoted type (`Null`), so only the `y = null` branch is
+  // reachable. Therefore only `y` should be demoted here.
+  x.expectStaticType<Exactly<int>>();
   y.expectStaticType<Exactly<int?>>();
 }
 
@@ -240,9 +239,9 @@ void testExplicitPropertyReferenceOnRhsOfNotEquals(C c) {
     y = null;
   }
   // In analyzing the `!=` check, flow analysis assumes that `c._f` has its
-  // unpromoted type (`Object?`), so both branches of the `if` are
-  // reachable. Therefore both `x` and `y` should both be demoted here.
-  x.expectStaticType<Exactly<int?>>();
+  // promoted type (`Null`), so only the `y = null` branch is
+  // reachable. Therefore only `y` should be demoted here.
+  x.expectStaticType<Exactly<int>>();
   y.expectStaticType<Exactly<int?>>();
 }