Field type promotion: fix CFE handling of abstract fields.

Bug: dart-lang/language#2020
Change-Id: Ie35f7ff2d3de33c5f94bb7b703c13c9764a85c70
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/269981
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>

Author: stereotype441

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

@@ -1540,14 +1540,17 @@ class SourceClassBuilder extends ClassBuilderImpl
     }
     for (Procedure procedure in cls.procedures) {
       // An instance getter makes fields with the same name unpromotable if it's
-      // concrete.
-      if (procedure.isGetter &&
-          procedure.isInstanceMember &&
-          !procedure.isAbstract &&
+      // concrete.  Also, an abstract instance setter that's desugared from an
+      // abstract non-final field makes fields with the same name unpromotable.
+      if (procedure.isInstanceMember &&
           _isPrivateNameInThisLibrary(procedure.name)) {
-        ProcedureStubKind procedureStubKind = procedure.stubKind;
-        if (procedureStubKind == ProcedureStubKind.Regular ||
-            procedureStubKind == ProcedureStubKind.NoSuchMethodForwarder) {
+        if (procedure.isGetter && !procedure.isAbstract) {
+          ProcedureStubKind procedureStubKind = procedure.stubKind;
+          if (procedureStubKind == ProcedureStubKind.Regular ||
+              procedureStubKind == ProcedureStubKind.NoSuchMethodForwarder) {
+            unpromotablePrivateFieldNames.add(procedure.name.text);
+          }
+        } else if (procedure.isSetter && procedure.isAbstractFieldAccessor) {
           unpromotablePrivateFieldNames.add(procedure.name.text);
         }
       }

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

@@ -1612,6 +1612,7 @@ class AbstractOrExternalFieldEncoding implements FieldEncoding {
           new FunctionNode(null, positionalParameters: [
             new VariableDeclaration(extensionThisName)..fileOffset
           ]),
+          isAbstractFieldAccessor: isAbstract,
           fileUri: fileUri,
           reference: getterReference)
         ..fileOffset = charOffset
@@ -1636,6 +1637,7 @@ class AbstractOrExternalFieldEncoding implements FieldEncoding {
                 returnType: const VoidType())
               ..fileOffset = charOffset
               ..fileEndOffset = charEndOffset,
+            isAbstractFieldAccessor: isAbstract,
             fileUri: fileUri,
             reference: setterReference)
           ..fileOffset = charOffset
@@ -1648,7 +1650,9 @@ class AbstractOrExternalFieldEncoding implements FieldEncoding {
     } else {
       _getter = new Procedure(
           dummyName, ProcedureKind.Getter, new FunctionNode(null),
-          fileUri: fileUri, reference: getterReference)
+          isAbstractFieldAccessor: isAbstract,
+          fileUri: fileUri,
+          reference: getterReference)
         ..fileOffset = charOffset
         ..fileEndOffset = charEndOffset
         ..isNonNullableByDefault = isNonNullableByDefault;
@@ -1667,6 +1671,7 @@ class AbstractOrExternalFieldEncoding implements FieldEncoding {
                 positionalParameters: [parameter], returnType: const VoidType())
               ..fileOffset = charOffset
               ..fileEndOffset = charEndOffset,
+            isAbstractFieldAccessor: isAbstract,
             fileUri: fileUri,
             reference: setterReference)
           ..fileOffset = charOffset

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

@@ -347,7 +347,10 @@ class OperationsCfe
     Set<String>? unpromotablePrivateFieldNames =
         this.unpromotablePrivateFieldNames;
     if (unpromotablePrivateFieldNames == null) return false;
-    if (property is! Field) return false;
+    if (property is Procedure && !property.isAbstractFieldAccessor) {
+      // We don't promote methods or explicit abstract getters.
+      return false;
+    }
     String name = property.name.text;
     if (!name.startsWith('_')) return false;
     return !unpromotablePrivateFieldNames.contains(name);

pkg/front_end/testcases/inference_update_2/abstract_field.dart

@@ -0,0 +1,65 @@
+// Copyright (c) 2022, the Dart project authors.  Please see the AUTHORS file
+// 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 that field promotion logic properly handles abstract fields.
+
+// SharedOptions=--enable-experiment=inference-update-2
+
+abstract class C {
+  abstract final int? _f1;
+  abstract int? _f2;
+}
+
+class D {
+  final int? _f1;
+  final int? _f2;
+
+  D(int i) : _f1 = i, _f2 = i;
+}
+
+void acceptsInt(int x) {}
+
+void testAbstractFinalFieldIsPromotable(C c) {
+  if (c._f1 != null) {
+    var x = c._f1;
+    // `x` has type `int` so this is ok
+    acceptsInt(x);
+  }
+}
+
+void testAbstractNonFinalFieldIsNotPromotable(C c) {
+  // Technically, it would be sound to promote an abstract non-final field, but
+  // there's no point because it's just going to be implemented by a concrete
+  // non-final field or a getter/setter pair, which will prevent promotion.  So
+  // we might as well prevent promotion even in the absence of an
+  // implementation.
+  if (c._f2 != null) {
+    var x = c._f2;
+    // `x` has type `int?` so this is ok
+    x = null;
+  }
+}
+
+void testAbstractFinalFieldDoesNotBlockPromotionElsewhere(D d) {
+  if (d._f1 != null) {
+    var x = d._f1;
+    // `x` has type `int` so this is ok
+    acceptsInt(x);
+  }
+}
+
+void testAbstractNonFinalFieldBlocksPromotionElsewhere(D d) {
+  // Technically, it would be sound if an abstract non-final field didn't block
+  // promotion, but there's no point because it's just going to be implemented
+  // by a concrete non-final field or a getter/setter pair, which will block
+  // promotion.  So we might as well block promotion even in the absence of an
+  // implementation.
+  if (d._f2 != null) {
+    var x = d._f2;
+    // `x` has type `int?` so this is ok
+    x = null;
+  }
+}
+
+main() {}

pkg/front_end/testcases/inference_update_2/abstract_field.dart.textual_outline.expect

@@ -0,0 +1,19 @@
+abstract class C {
+  abstract final int? _f1;
+  abstract int? _f2;
+}
+
+class D {
+  final int? _f1;
+  final int? _f2;
+  D(int i)
+      : _f1 = i,
+        _f2 = i;
+}
+
+void acceptsInt(int x) {}
+void testAbstractFinalFieldIsPromotable(C c) {}
+void testAbstractNonFinalFieldIsNotPromotable(C c) {}
+void testAbstractFinalFieldDoesNotBlockPromotionElsewhere(D d) {}
+void testAbstractNonFinalFieldBlocksPromotionElsewhere(D d) {}
+main() {}

pkg/front_end/testcases/inference_update_2/abstract_field.dart.textual_outline_modelled.expect

@@ -0,0 +1,19 @@
+abstract class C {
+  abstract final int? _f1;
+  abstract int? _f2;
+}
+
+class D {
+  D(int i)
+      : _f1 = i,
+        _f2 = i;
+  final int? _f1;
+  final int? _f2;
+}
+
+main() {}
+void acceptsInt(int x) {}
+void testAbstractFinalFieldDoesNotBlockPromotionElsewhere(D d) {}
+void testAbstractFinalFieldIsPromotable(C c) {}
+void testAbstractNonFinalFieldBlocksPromotionElsewhere(D d) {}
+void testAbstractNonFinalFieldIsNotPromotable(C c) {}

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

@@ -0,0 +1,45 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+abstract class C extends core::Object {
+  synthetic constructor •() → self::C
+    : super core::Object::•()
+    ;
+  abstract get _f1() → core::int?;
+  abstract get _f2() → core::int?;
+  abstract set _f2(core::int? #externalFieldValue) → void;
+}
+class D extends core::Object {
+  final field core::int? _f1;
+  final field core::int? _f2;
+  constructor •(core::int i) → self::D
+    : self::D::_f1 = i, self::D::_f2 = i, super core::Object::•()
+    ;
+}
+static method acceptsInt(core::int x) → void {}
+static method testAbstractFinalFieldIsPromotable(self::C c) → void {
+  if(!(c.{self::C::_f1}{core::int?} == null)) {
+    core::int x = c.{self::C::_f1}{core::int};
+    self::acceptsInt(x);
+  }
+}
+static method testAbstractNonFinalFieldIsNotPromotable(self::C c) → void {
+  if(!(c.{self::C::_f2}{core::int?} == null)) {
+    core::int? x = c.{self::C::_f2}{core::int?};
+    x = null;
+  }
+}
+static method testAbstractFinalFieldDoesNotBlockPromotionElsewhere(self::D d) → void {
+  if(!(d.{self::D::_f1}{core::int?} == null)) {
+    core::int x = d.{self::D::_f1}{core::int};
+    self::acceptsInt(x);
+  }
+}
+static method testAbstractNonFinalFieldBlocksPromotionElsewhere(self::D d) → void {
+  if(!(d.{self::D::_f2}{core::int?} == null)) {
+    core::int? x = d.{self::D::_f2}{core::int?};
+    x = null;
+  }
+}
+static method main() → dynamic {}

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

@@ -0,0 +1,45 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+abstract class C extends core::Object {
+  synthetic constructor •() → self::C
+    : super core::Object::•()
+    ;
+  abstract get _f1() → core::int?;
+  abstract get _f2() → core::int?;
+  abstract set _f2(core::int? #externalFieldValue) → void;
+}
+class D extends core::Object {
+  final field core::int? _f1;
+  final field core::int? _f2;
+  constructor •(core::int i) → self::D
+    : self::D::_f1 = i, self::D::_f2 = i, super core::Object::•()
+    ;
+}
+static method acceptsInt(core::int x) → void {}
+static method testAbstractFinalFieldIsPromotable(self::C c) → void {
+  if(!(c.{self::C::_f1}{core::int?} == null)) {
+    core::int x = c.{self::C::_f1}{core::int};
+    self::acceptsInt(x);
+  }
+}
+static method testAbstractNonFinalFieldIsNotPromotable(self::C c) → void {
+  if(!(c.{self::C::_f2}{core::int?} == null)) {
+    core::int? x = c.{self::C::_f2}{core::int?};
+    x = null;
+  }
+}
+static method testAbstractFinalFieldDoesNotBlockPromotionElsewhere(self::D d) → void {
+  if(!(d.{self::D::_f1}{core::int?} == null)) {
+    core::int x = d.{self::D::_f1}{core::int};
+    self::acceptsInt(x);
+  }
+}
+static method testAbstractNonFinalFieldBlocksPromotionElsewhere(self::D d) → void {
+  if(!(d.{self::D::_f2}{core::int?} == null)) {
+    core::int? x = d.{self::D::_f2}{core::int?};
+    x = null;
+  }
+}
+static method main() → dynamic {}

pkg/front_end/testcases/inference_update_2/abstract_field.dart.weak.outline.expect

@@ -0,0 +1,29 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+abstract class C extends core::Object {
+  synthetic constructor •() → self::C
+    ;
+  abstract get _f1() → core::int?;
+  abstract get _f2() → core::int?;
+  abstract set _f2(core::int? #externalFieldValue) → void;
+}
+class D extends core::Object {
+  final field core::int? _f1;
+  final field core::int? _f2;
+  constructor •(core::int i) → self::D
+    ;
+}
+static method acceptsInt(core::int x) → void
+  ;
+static method testAbstractFinalFieldIsPromotable(self::C c) → void
+  ;
+static method testAbstractNonFinalFieldIsNotPromotable(self::C c) → void
+  ;
+static method testAbstractFinalFieldDoesNotBlockPromotionElsewhere(self::D d) → void
+  ;
+static method testAbstractNonFinalFieldBlocksPromotionElsewhere(self::D d) → void
+  ;
+static method main() → dynamic
+  ;

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

@@ -0,0 +1,45 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+abstract class C extends core::Object {
+  synthetic constructor •() → self::C
+    : super core::Object::•()
+    ;
+  abstract get _f1() → core::int?;
+  abstract get _f2() → core::int?;
+  abstract set _f2(core::int? #externalFieldValue) → void;
+}
+class D extends core::Object {
+  final field core::int? _f1;
+  final field core::int? _f2;
+  constructor •(core::int i) → self::D
+    : self::D::_f1 = i, self::D::_f2 = i, super core::Object::•()
+    ;
+}
+static method acceptsInt(core::int x) → void {}
+static method testAbstractFinalFieldIsPromotable(self::C c) → void {
+  if(!(c.{self::C::_f1}{core::int?} == null)) {
+    core::int x = c.{self::C::_f1}{core::int};
+    self::acceptsInt(x);
+  }
+}
+static method testAbstractNonFinalFieldIsNotPromotable(self::C c) → void {
+  if(!(c.{self::C::_f2}{core::int?} == null)) {
+    core::int? x = c.{self::C::_f2}{core::int?};
+    x = null;
+  }
+}
+static method testAbstractFinalFieldDoesNotBlockPromotionElsewhere(self::D d) → void {
+  if(!(d.{self::D::_f1}{core::int?} == null)) {
+    core::int x = d.{self::D::_f1}{core::int};
+    self::acceptsInt(x);
+  }
+}
+static method testAbstractNonFinalFieldBlocksPromotionElsewhere(self::D d) → void {
+  if(!(d.{self::D::_f2}{core::int?} == null)) {
+    core::int? x = d.{self::D::_f2}{core::int?};
+    x = null;
+  }
+}
+static method main() → dynamic {}

pkg/front_end/testcases/inference_update_2/external_field.dart

@@ -0,0 +1,33 @@
+// Copyright (c) 2022, the Dart project authors.  Please see the AUTHORS file
+// 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 that field promotion logic properly handles external fields.  External
+// fields are not promotable because they are effectively just getters; the
+// language can't guarantee that they always return the same value.
+
+abstract class C {
+  external final int? _f1;
+}
+
+extension on C {
+  external final int? _f2;
+}
+
+void testExternalFieldInClass(C c) {
+  if (c._f1 != null) {
+    var x = c._f1;
+    // `x` has type `int?` so this is ok
+    x = null;
+  }
+}
+
+void testExternalFieldInExtension(C c) {
+  if (c._f2 != null) {
+    var x = c._f2;
+    // `x` has type `int?` so this is ok
+    x = null;
+  }
+}
+
+main() {}

pkg/front_end/testcases/inference_update_2/external_field.dart.textual_outline.expect

@@ -0,0 +1,11 @@
+abstract class C {
+  external final int? _f1;
+}
+
+extension on C {
+  external final int? _f2;
+}
+
+void testExternalFieldInClass(C c) {}
+void testExternalFieldInExtension(C c) {}
+main() {}