Version 3.3.0-207.0.dev

Merge 87c791e into dev

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/analysis_server/lib/src/services/correction/dart/convert_to_null_aware.dart

@@ -7,7 +7,7 @@ import 'package:analysis_server/src/services/correction/dart/abstract_producer.d
 import 'package:analysis_server/src/services/correction/fix.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart';
-import 'package:analyzer/dart/element/element.dart';
+import 'package:analyzer/source/source_range.dart';
 import 'package:analyzer_plugin/utilities/assist/assist.dart';
 import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
 import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
@@ -43,29 +43,24 @@ class ConvertToNullAware extends ResolvedCorrectionProducer {
       return;
     }
     var condition = targetNode.condition.unParenthesized;
-    SimpleIdentifier identifier;
+    String conditionText;
     Expression nullExpression;
     Expression nonNullExpression;
-    int periodOffset;
 
     if (condition is BinaryExpression) {
       //
       // Identify the variable being compared to `null`, or return if the
-      // condition isn't a simple comparison of `null` to a variable's value.
+      // condition isn't a simple comparison of `null` to another expression.
       //
       var leftOperand = condition.leftOperand;
       var rightOperand = condition.rightOperand;
-      if (leftOperand is NullLiteral && rightOperand is SimpleIdentifier) {
-        identifier = rightOperand;
-      } else if (rightOperand is NullLiteral &&
-          leftOperand is SimpleIdentifier) {
-        identifier = leftOperand;
+      if (leftOperand is NullLiteral && rightOperand is! NullLiteral) {
+        conditionText = rightOperand.toString();
+      } else if (rightOperand is NullLiteral && leftOperand is! NullLiteral) {
+        conditionText = leftOperand.toString();
       } else {
         return;
       }
-      if (identifier.staticElement is! LocalElement) {
-        return;
-      }
       //
       // Identify the expression executed when the variable is `null` and when
       // it is non-`null`. Return if the `null` expression isn't a null literal
@@ -84,30 +79,51 @@ class ConvertToNullAware extends ResolvedCorrectionProducer {
       if (nullExpression.unParenthesized is! NullLiteral) {
         return;
       }
-      var unwrappedExpression = nonNullExpression.unParenthesized;
-      Expression? target;
+      Expression? resultExpression = nonNullExpression.unParenthesized;
       Token? operator;
-      if (unwrappedExpression is MethodInvocation) {
-        target = unwrappedExpression.target;
-        operator = unwrappedExpression.operator;
-      } else if (unwrappedExpression is PrefixedIdentifier) {
-        target = unwrappedExpression.prefix;
-        operator = unwrappedExpression.period;
-      } else {
-        return;
+      while (true) {
+        switch (resultExpression) {
+          case PrefixedIdentifier():
+            operator = resultExpression.period;
+            resultExpression = resultExpression.prefix;
+          case MethodInvocation():
+            operator = resultExpression.operator;
+            resultExpression = resultExpression.target;
+          case PostfixExpression()
+              when resultExpression.operator.type == TokenType.BANG:
+            // (Operator remains unaffected.)
+            resultExpression = resultExpression.operand;
+          case PropertyAccess():
+            operator = resultExpression.operator;
+            resultExpression = resultExpression.target;
+          default:
+            return;
+        }
+        if (resultExpression.toString() == conditionText) {
+          break;
+        }
       }
-      if (operator == null || operator.type != TokenType.PERIOD) {
+      if (resultExpression == null) {
         return;
       }
-      if (!(target is SimpleIdentifier &&
-          target.staticElement == identifier.staticElement)) {
+
+      SourceRange operatorRange;
+      var optionalQuestionMark = '';
+      if (operator != null) {
+        if (operator.type == TokenType.PERIOD) {
+          optionalQuestionMark = '?';
+        }
+        operatorRange = range.endStart(resultExpression, operator);
+      } else if (resultExpression.parent is PostfixExpression) {
+        // The case where the expression is just `foo!`.
+        operatorRange =
+            range.endEnd(resultExpression, resultExpression.parent!);
+      } else {
         return;
       }
-      periodOffset = operator.offset;
-
       await builder.addDartFileEdit(file, (builder) {
         builder.addDeletion(range.startStart(targetNode, nonNullExpression));
-        builder.addSimpleInsertion(periodOffset, '?');
+        builder.addSimpleReplacement(operatorRange, optionalQuestionMark);
         builder.addDeletion(range.endEnd(nonNullExpression, targetNode));
       });
     }

pkg/analysis_server/test/src/services/correction/assist/convert_to_null_aware_test.dart

@@ -78,7 +78,12 @@ abstract class A {
 }
 int? f(A? a1) => a1 == null ? null : a1?.m();
 ''');
-    await assertNoAssistAt('? n');
+    await assertHasAssistAt('? n', '''
+abstract class A {
+  int m();
+}
+int? f(A? a1) => a1?.m();
+''');
   }
 
   Future<void> test_equal_nullOnLeft() async {