Allow "field promotion" to apply to abstract getters.

A quirk of analyzer and CFE implementations is that they resolve
property gets such as `foo.bar` to specific field or getter
declarations that may be not be directly defined on the target type;
for example if `foo` has type `B`, and `B` extends `A`, and `A`
contains a field called `bar`, then `foo.bar` is considered to refer
to `A.bar`, for example:

    class A {
      int? bar;
    }
    class B extends A {}
    f(B foo) {
      print(foo.bar); // Resolved to `A.bar`.
    }

This is in contrast with the language specification, which makes a
clean distinction between class _declarations_ and the _interfaces_
implied by those declarations. While a class declaration can contain
(among other things) both getters and fields, which might be concrete
or abstract, an interface doesn't distinguish between getters and
fields, and is inherently abstract.

The advantage of the analyzer/CFE approach is that it allows more
intuitive error messages and "go to definition" behavior, which
benefits users. But it has some ill-defined corner cases involving
complex class hierarchies, because not every property access can be
resolved to a unique declaration (sometimes a getter is multiply
inherited from multiple interfaces, for example). The language spec
approach has the advantage of being well-defined and consistent even
in situations involving complex class hierarchies.

When I initially implemented field promotion, I took advantage of this
quirk of the analyzer and CFE implementations, so that I could make
property gets that refer to field declarations promotable, while
keeping property gets that refer to abstract getter declarations
non-promotable. This caused unpredictable behaviors in the ill-defined
corner cases. It also meant that in certain rare cases, a property
access might not be promoted even when it would be sound to do so
(e.g. a property access might refer to a private abstract getter
declaration, but the only concrete _implementation_ of that abstract
getter was a final field).

This CL changes the rule for promotability so that any get of a
private property is considered promotable, provided that the
containing library doesn't contain any concrete getters, non-final
fields, or external fields with the same name. It no longer matters
whether the private property refers to a field or a getter. This rule
is simpler than the old rule, restores the spec's clean distinction
between class declarations and interfaces, and allows more promotions
without sacrificing soundness.

For additional details please see the breaking change announcement at
#54056, as well as the original
change proposal at dart-lang/language#3328.

Fixes #54056.
Fixes dart-lang/language#3328.

Change-Id: I38ffcb9ecce8bccb93b1b2586a1222a0fb1005a7
Bug: #54056
Bug: dart-lang/language#3328
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/337609
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Lasse Nielsen <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>

Author: stereotype441

CHANGELOG.md

@@ -1,5 +1,49 @@
 ## 3.3.0
 
+### Language
+
+- **Breaking Change** [#54056][]: The rules for private field promotion have
+  been changed so that an abstract getter is considered promotable if there are
+  no conflicting declarations (i.e., there are no non-final fields, external
+  fields, concrete getters, or `noSuchMethod` forwarding getters with the same
+  name in the same library). This makes the implementation more consistent and
+  allows type promotion in a few rare scenarios where it wasn't prevoiusly
+  allowed. It is unlikely, but this change could in principle cause a breakage
+  by changing an inferred type in a way that breaks later code. For example:
+
+  ```dart
+  class A {
+    int? get _field;
+  }
+  class B extends A {
+    final int? _field;
+    B(this._field);
+  }
+  test(A a) {
+    if (a._field != null) {
+      var x = a._field; // Previously had type `int?`; now has type `int`
+      ...
+      x = null; // Previously allowed; now causes a compile-time error.
+    }
+  }
+  ```
+
+  Affected code can be fixed by adding an explicit type annotation (e.g., in the
+  above example `var x` can be changed to `int? x`).
+
+  It's also possible that some continuous integration configurations might fail
+  if they have been configured to treat warnings as errors, because the expanded
+  type promotion could lead to one of the following warnings:
+
+  - unnecessary_non_null_assertion
+  - unnecessary_cast
+  - invalid_null_aware_operator
+
+  These warnings can be addressed in the usual way, by removing the unnecessary
+  operation in the first two cases, or changing `?.` to `.` in the second case.
+
+[#54056]: https://github.com/dart-lang/sdk/issues/54056
+
 ### Libraries
 
 #### `dart:core`

pkg/_fe_analyzer_shared/lib/src/field_promotability.dart

@@ -222,16 +222,16 @@ abstract class FieldPromotability<Class extends Object, Field, Getter> {
   ///
   /// [isAbstract] indicates whether the getter is abstract.
   ///
-  /// Note that unlike [addField], this method does not return a
-  /// [PropertyNonPromotabilityReason]. The caller may safely assume that the
-  /// reason that getters are not promotable is
-  /// [PropertyNonPromotabilityReason.isNotField].
-  void addGetter(ClassInfo<Class> classInfo, Getter getter, String name,
+  /// A return value of `null` indicates that this getter *might* wind up being
+  /// promotable; any other return value indicates the reason why it
+  /// *definitely* isn't promotable.
+  PropertyNonPromotabilityReason? addGetter(
+      ClassInfo<Class> classInfo, Getter getter, String name,
       {required bool isAbstract}) {
     // Public fields are never promotable, so we may safely ignore getters with
     // public names.
     if (!name.startsWith('_')) {
-      return;
+      return PropertyNonPromotabilityReason.isNotPrivate;
     }
 
     // Record the getter name for later use in computation of `noSuchMethod`
@@ -242,6 +242,10 @@ abstract class FieldPromotability<Class extends Object, Field, Getter> {
 
       // The getter is concrete, so no fields with the same name are promotable.
       _fieldNonPromoInfo(name).conflictingGetters.add(getter);
+
+      return PropertyNonPromotabilityReason.isNotField;
+    } else {
+      return null;
     }
   }
 

pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart

@@ -5704,14 +5704,17 @@ Message _withArgumentsFieldNotPromotedBecauseNotFinal(
 }
 
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
-const Template<Message Function(String name, String string)>
-    templateFieldNotPromotedBecauseNotPrivate =
-    const Template<Message Function(String name, String string)>(
-        "FieldNotPromotedBecauseNotPrivate",
-        problemMessageTemplate:
-            r"""'#name' refers to a public field so it couldn't be promoted.""",
-        correctionMessageTemplate: r"""See #string""",
-        withArguments: _withArgumentsFieldNotPromotedBecauseNotPrivate);
+const Template<
+    Message Function(
+        String name,
+        String
+            string)> templateFieldNotPromotedBecauseNotPrivate = const Template<
+        Message Function(String name, String string)>(
+    "FieldNotPromotedBecauseNotPrivate",
+    problemMessageTemplate:
+        r"""'#name' refers to a public property so it couldn't be promoted.""",
+    correctionMessageTemplate: r"""See #string""",
+    withArguments: _withArgumentsFieldNotPromotedBecauseNotPrivate);
 
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const Code<Message Function(String name, String string)>
@@ -5728,7 +5731,7 @@ Message _withArgumentsFieldNotPromotedBecauseNotPrivate(
   if (string.isEmpty) throw 'No string provided';
   return new Message(codeFieldNotPromotedBecauseNotPrivate,
       problemMessage:
-          """'${name}' refers to a public field so it couldn't be promoted.""",
+          """'${name}' refers to a public property so it couldn't be promoted.""",
       correctionMessage: """See ${string}""",
       arguments: {'name': name, 'string': string});
 }

pkg/_fe_analyzer_shared/test/field_promotability_test.dart

@@ -88,6 +88,8 @@ main() {
       check(nonPromotabilityInfo.keys).unorderedEquals({'_f'});
       check(nonPromotabilityInfo['_f']!.conflictingGetters)
           .unorderedEquals([getter]);
+      check(getter.nonPromotabilityReason)
+          .equals(PropertyNonPromotabilityReason.isNotField);
     });
 
     test('in an abstract class', () {
@@ -98,42 +100,50 @@ main() {
       check(nonPromotabilityInfo.keys).unorderedEquals({'_f'});
       check(nonPromotabilityInfo['_f']!.conflictingGetters)
           .unorderedEquals([getter]);
+      check(getter.nonPromotabilityReason)
+          .equals(PropertyNonPromotabilityReason.isNotField);
     });
   });
 
   test('abstract getter does not render a private field non-promotable', () {
     var f = Field('_f', isFinal: true);
     var c = Class(fields: [f]);
-    var d = Class(isAbstract: true, getters: [Getter('_f', isAbstract: true)]);
+    var getter = Getter('_f', isAbstract: true);
+    var d = Class(isAbstract: true, getters: [getter]);
     var nonPromotabilityInfo = _TestFieldPromotability().run([c, d]);
     check(nonPromotabilityInfo).isEmpty();
     check(f.nonPromotabilityReason).equals(null);
+    check(getter.nonPromotabilityReason).equals(null);
   });
 
   test('public concrete getter is ignored', () {
     // Since public fields are never promotable, there's no need for the
     // algorithm to keep track of public concrete getters.
     var f = Field('f', isFinal: true);
     var c = Class(fields: [f]);
-    var d = Class(getters: [Getter('f')]);
+    var getter = Getter('f');
+    var d = Class(getters: [getter]);
     // Therefore the map returned by `_TestFieldPromotability.run` is empty.
     var nonPromotabilityInfo = _TestFieldPromotability().run([c, d]);
     check(nonPromotabilityInfo).isEmpty();
     check(f.nonPromotabilityReason)
         .equals(PropertyNonPromotabilityReason.isNotPrivate);
+    check(getter.nonPromotabilityReason)
+        .equals(PropertyNonPromotabilityReason.isNotPrivate);
   });
 
   group('unimplemented getter renders a field non-promotable:', () {
     test('induced by getter', () {
       var f = Field('_f', isFinal: true);
       var c = Class(fields: [f]);
-      var d =
-          Class(isAbstract: true, getters: [Getter('_f', isAbstract: true)]);
+      var getter = Getter('_f', isAbstract: true);
+      var d = Class(isAbstract: true, getters: [getter]);
       var e = Class(implements: [d]);
       var nonPromotabilityInfo = _TestFieldPromotability().run([c, d, e]);
       check(nonPromotabilityInfo.keys).unorderedEquals({'_f'});
       check(nonPromotabilityInfo['_f']!.conflictingNsmClasses)
           .unorderedEquals([e]);
+      check(getter.nonPromotabilityReason).equals(null);
     });
 
     test('induced by field', () {
@@ -151,11 +161,13 @@ main() {
   test('unimplemented getter in an abstract class is ok', () {
     var f = Field('_f', isFinal: true);
     var c = Class(fields: [f]);
-    var d = Class(isAbstract: true, getters: [Getter('_f', isAbstract: true)]);
+    var getter = Getter('_f', isAbstract: true);
+    var d = Class(isAbstract: true, getters: [getter]);
     var e = Class(isAbstract: true, implements: [d]);
     var nonPromotabilityInfo = _TestFieldPromotability().run([c, d, e]);
     check(nonPromotabilityInfo).isEmpty();
     check(f.nonPromotabilityReason).equals(null);
+    check(getter.nonPromotabilityReason).equals(null);
   });
 
   test('unimplemented abstract field renders a field non-promotable:', () {
@@ -237,6 +249,7 @@ class Field {
 class Getter {
   final String name;
   final bool isAbstract;
+  late final PropertyNonPromotabilityReason? nonPromotabilityReason;
 
   Getter(this.name, {this.isAbstract = false});
 }
@@ -265,7 +278,8 @@ class _TestFieldPromotability extends FieldPromotability<Class, Field, Getter> {
             isExternal: field.isExternal);
       }
       for (var getter in class_.getters) {
-        addGetter(classInfo, getter, getter.name,
+        getter.nonPromotabilityReason = addGetter(
+            classInfo, getter, getter.name,
             isAbstract: getter.isAbstract);
       }
     }

pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart

@@ -510,6 +510,7 @@ class TypeSystemOperations
   @override
   PropertyNonPromotabilityReason? whyPropertyIsNotPromotable(
       covariant ExecutableElement property) {
+    if (property.isPublic) return PropertyNonPromotabilityReason.isNotPrivate;
     if (property is! PropertyAccessorElement) {
       return PropertyNonPromotabilityReason.isNotField;
     }
@@ -523,7 +524,6 @@ class TypeSystemOperations
       return PropertyNonPromotabilityReason.isNotField;
     }
     if (field.isPromotable) return null;
-    if (field.isPublic) return PropertyNonPromotabilityReason.isNotPrivate;
     if (field.isExternal) return PropertyNonPromotabilityReason.isExternal;
     if (!field.isFinal) return PropertyNonPromotabilityReason.isNotFinal;
     // Non-promotion reason must be due to a conflict with some other

pkg/analyzer/lib/src/generated/resolver.dart

@@ -5279,7 +5279,7 @@ class _WhyNotPromotedVisitor
         PropertyNonPromotabilityReason.isNotField =>
           "'$propertyName' refers to a getter so it couldn't be promoted.",
         PropertyNonPromotabilityReason.isNotPrivate =>
-          "'$propertyName' refers to a public field so it couldn't be "
+          "'$propertyName' refers to a public property so it couldn't be "
               "promoted.",
         PropertyNonPromotabilityReason.isExternal =>
           "'$propertyName' refers to an external field so it couldn't be "

pkg/analyzer/lib/src/summary2/library_builder.dart

@@ -1301,8 +1301,11 @@ class _FieldPromotability extends FieldPromotability<InterfaceElement,
         continue;
       }
 
-      addGetter(classInfo, accessor, accessor.name,
+      var nonPromotabilityReason = addGetter(classInfo, accessor, accessor.name,
           isAbstract: accessor.isAbstract);
+      if (enabled && nonPromotabilityReason == null) {
+        _potentiallyPromotableFields.add(accessor.variable as FieldElementImpl);
+      }
     }
   }
 }

pkg/analyzer/test/src/dart/resolution/field_promotion_test.dart

@@ -654,9 +654,9 @@ PrefixedIdentifier
   identifier: SimpleIdentifier
     token: _foo
     staticElement: self::@class::C::@getter::_foo
-    staticType: int?
+    staticType: int
   staticElement: self::@class::C::@getter::_foo
-  staticType: int?
+  staticType: int
 ''');
   }
 

pkg/front_end/lib/src/fasta/source/source_library_builder.dart

@@ -1654,10 +1654,12 @@ class SourceLibraryBuilder extends LibraryBuilderImpl {
           }
         } else if (member is SourceProcedureBuilder && member.isGetter) {
           if (member.isSynthetic) continue;
-          fieldPromotability.addGetter(classInfo, member, member.name,
+          PropertyNonPromotabilityReason? reason = fieldPromotability.addGetter(
+              classInfo, member, member.name,
               isAbstract: member.isAbstract);
-          individualPropertyReasons[member.procedure] =
-              PropertyNonPromotabilityReason.isNotField;
+          if (reason != null) {
+            individualPropertyReasons[member.procedure] = reason;
+          }
         }
       }
     }
@@ -1673,7 +1675,9 @@ class SourceLibraryBuilder extends LibraryBuilderImpl {
             !member.isStatic &&
             member.isGetter) {
           individualPropertyReasons[member.procedure] =
-              PropertyNonPromotabilityReason.isNotField;
+              member.memberName.isPrivate
+                  ? PropertyNonPromotabilityReason.isNotField
+                  : PropertyNonPromotabilityReason.isNotPrivate;
         }
       }
     }
@@ -1694,7 +1698,9 @@ class SourceLibraryBuilder extends LibraryBuilderImpl {
             !member.isStatic &&
             member.isGetter) {
           individualPropertyReasons[member.procedure] =
-              PropertyNonPromotabilityReason.isNotField;
+              member.memberName.isPrivate
+                  ? PropertyNonPromotabilityReason.isNotField
+                  : PropertyNonPromotabilityReason.isNotPrivate;
         }
       }
     }

pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart

@@ -533,10 +533,6 @@ class OperationsCfe
         // We don't promote methods.
         return false;
       }
-      if (property.isAbstract && !property.isAbstractFieldAccessor) {
-        // We don't promote direct references to abstract getter declarations.
-        return false;
-      }
     }
     String name = property.name.text;
     if (!name.startsWith('_')) return false;

pkg/front_end/messages.yaml

@@ -5547,7 +5547,7 @@ FieldNotPromotedBecauseNotField:
     }
 
 FieldNotPromotedBecauseNotPrivate:
-  problemMessage: "'#name' refers to a public field so it couldn't be promoted."
+  problemMessage: "'#name' refers to a public property so it couldn't be promoted."
   correctionMessage: "See #string"
   script: >
     class C {

pkg/front_end/testcases/inference_update_2/basic_field_promotion.dart

@@ -54,8 +54,8 @@ void testPublicField(C c) {
 void testPrivateAbstractGetter(C c) {
   if (c._privateAbstractGetter != null) {
     var x = c._privateAbstractGetter;
-    // `x` has type `int?` so this is ok
-    x = null;
+    // `x` has type `int` so this is ok
+    acceptsInt(x);
   }
 }
 

pkg/front_end/testcases/inference_update_2/basic_field_promotion.dart.strong.expect

@@ -44,8 +44,8 @@ static method testPublicField(self::C c) → void {
 }
 static method testPrivateAbstractGetter(self::C c) → void {
   if(!(c.{self::C::_privateAbstractGetter}{core::int?} == null)) {
-    core::int? x = c.{self::C::_privateAbstractGetter}{core::int?};
-    x = null;
+    core::int x = c.{self::C::_privateAbstractGetter}{core::int?} as{Unchecked} core::int;
+    self::acceptsInt(x);
   }
 }
 static method testPublicAbstractGetter(self::C c) → void {

pkg/front_end/testcases/inference_update_2/basic_field_promotion.dart.strong.transformed.expect

@@ -44,8 +44,8 @@ static method testPublicField(self::C c) → void {
 }
 static method testPrivateAbstractGetter(self::C c) → void {
   if(!(c.{self::C::_privateAbstractGetter}{core::int?} == null)) {
-    core::int? x = c.{self::C::_privateAbstractGetter}{core::int?};
-    x = null;
+    core::int x = let core::int? #t2 = c.{self::C::_privateAbstractGetter}{core::int?} in #t2 == null ?{core::int} #t2 as{Unchecked} core::int : #t2{core::int};
+    self::acceptsInt(x);
   }
 }
 static method testPublicAbstractGetter(self::C c) → void {

pkg/front_end/testcases/inference_update_2/basic_field_promotion.dart.weak.expect

@@ -44,8 +44,8 @@ static method testPublicField(self::C c) → void {
 }
 static method testPrivateAbstractGetter(self::C c) → void {
   if(!(c.{self::C::_privateAbstractGetter}{core::int?} == null)) {
-    core::int? x = c.{self::C::_privateAbstractGetter}{core::int?};
-    x = null;
+    core::int x = c.{self::C::_privateAbstractGetter}{core::int?} as{Unchecked} core::int;
+    self::acceptsInt(x);
   }
 }
 static method testPublicAbstractGetter(self::C c) → void {

pkg/front_end/testcases/inference_update_2/basic_field_promotion.dart.weak.modular.expect

@@ -44,8 +44,8 @@ static method testPublicField(self::C c) → void {
 }
 static method testPrivateAbstractGetter(self::C c) → void {
   if(!(c.{self::C::_privateAbstractGetter}{core::int?} == null)) {
-    core::int? x = c.{self::C::_privateAbstractGetter}{core::int?};
-    x = null;
+    core::int x = c.{self::C::_privateAbstractGetter}{core::int?} as{Unchecked} core::int;
+    self::acceptsInt(x);
   }
 }
 static method testPublicAbstractGetter(self::C c) → void {

pkg/front_end/testcases/inference_update_2/basic_field_promotion.dart.weak.transformed.expect

@@ -44,8 +44,8 @@ static method testPublicField(self::C c) → void {
 }
 static method testPrivateAbstractGetter(self::C c) → void {
   if(!(c.{self::C::_privateAbstractGetter}{core::int?} == null)) {
-    core::int? x = c.{self::C::_privateAbstractGetter}{core::int?};
-    x = null;
+    core::int x = c.{self::C::_privateAbstractGetter}{core::int?};
+    self::acceptsInt(x);
   }
 }
 static method testPublicAbstractGetter(self::C c) → void {