Migration: discard @nullable annotations on property accessors.

The migration tool already correctly infers nullability or
non-nullability based on the code generated by the built_value
package.  However, post-migration, code should no longer use
built_value's `@nullable` annotation, since the built_value package
gets its signal from the `?` type suffix instead.

The migration tool already has the ability to remove annotations in
certain rare circumstances; it's a relatively trivial matter to extend
it to remove `@nullable` annotations when appropriate.

Note that we don't just remove any `@nullable` annotation; we only
remove those annotations that resolve to the `nullable` constant
defined in the `built_value` package.

Fixes #43878.

Bug: https://github.com/dart-lang/sdk/issues/43878
Change-Id: I39c9b5e6fe4cd149b6d9549cd785d7b87c3185d6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206940
Reviewed-by: Samuel Rawlins <srawlins@google.com>

pkg/nnbd_migration/lib/nnbd_migration.dart

@@ -175,6 +175,12 @@ class NullabilityFixDescription {
           'Removed a null-aware assignment, because the target cannot be null',
       kind: NullabilityFixKind.removeDeadCode);
 
+  /// A built_value `@nullable` annotation has been discarded.
+  static const removeNullableAnnotation = NullabilityFixDescription._(
+    appliedMessage: 'Discarded a use of the built_value annotation @nullable',
+    kind: NullabilityFixKind.removeNullableAnnotation,
+  );
+
   /// A message used to indicate a fix has been applied.
   final String appliedMessage;
 
@@ -293,6 +299,7 @@ enum NullabilityFixKind {
   removeAs,
   removeDeadCode,
   removeLanguageVersionComment,
+  removeNullableAnnotation,
   replaceVar,
   typeNotMadeNullable,
   typeNotMadeNullableDueToHint,

pkg/nnbd_migration/lib/src/edit_plan.dart

@@ -838,6 +838,8 @@ class EditPlanner {
       return node.members;
     } else if (node is CompilationUnit) {
       return [...node.directives, ...node.declarations];
+    } else if (node is MethodDeclaration) {
+      return node.metadata;
     } else {
       return null;
     }
@@ -1542,7 +1544,8 @@ class _PassThroughBuilderImpl implements PassThroughBuilder {
     if (parent is Block ||
         parent is ClassDeclaration ||
         parent is CompilationUnit ||
-        parent is FormalParameter) {
+        parent is FormalParameter ||
+        parent is MethodDeclaration) {
       // These parent types don't use separators.
       return null;
     } else {

pkg/nnbd_migration/lib/src/fix_aggregator.dart

@@ -910,6 +910,34 @@ class NodeChangeForIfStatement extends NodeChange<IfStatement>
   }
 }
 
+/// Implementation of [NodeChange] specialized for operating on
+/// [MethodDeclaration] nodes.
+class NodeChangeForMethodDeclaration extends NodeChange<MethodDeclaration> {
+  /// If non-null, indicates a `@nullable` annotation which should be removed
+  /// from this node.
+  Annotation? annotationToRemove;
+
+  /// If [annotationToRemove] is non-null, the information that should be
+  /// contained in the edit.
+  AtomicEditInfo? removeAnnotationInfo;
+
+  NodeChangeForMethodDeclaration() : super._();
+
+  @override
+  Iterable<String> get _toStringParts =>
+      [if (annotationToRemove != null) 'annotationToRemove'];
+
+  @override
+  EditPlan _apply(MethodDeclaration node, FixAggregator aggregator) {
+    return aggregator.planner.passThrough(node, innerPlans: [
+      if (annotationToRemove != null)
+        aggregator.planner
+            .removeNode(annotationToRemove!, info: removeAnnotationInfo),
+      ...aggregator.innerPlansForNode(node),
+    ]);
+  }
+}
+
 /// Implementation of [NodeChange] specialized for operating on
 /// [MethodInvocation] nodes.
 class NodeChangeForMethodInvocation
@@ -1422,6 +1450,10 @@ class _NodeChangeVisitor extends GeneralizingAstVisitor<NodeChange<AstNode>> {
   @override
   NodeChange visitIfStatement(IfStatement node) => NodeChangeForIfStatement();
 
+  @override
+  NodeChange visitMethodDeclaration(MethodDeclaration node) =>
+      NodeChangeForMethodDeclaration();
+
   @override
   NodeChange visitMethodInvocation(MethodInvocation node) =>
       NodeChangeForMethodInvocation();

pkg/nnbd_migration/lib/src/fix_builder.dart

@@ -1200,6 +1200,31 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
     super.visitGenericFunctionType(node);
   }
 
+  @override
+  void visitMethodDeclaration(MethodDeclaration node) {
+    if (node.isGetter && node.isAbstract) {
+      for (var annotation in node.metadata) {
+        if (annotation.arguments == null) {
+          var element = annotation.element;
+          if (element is PropertyAccessorElement &&
+              element.name == 'nullable') {
+            if (element.enclosingElement is CompilationUnitElement) {
+              if (element.library.source.uri.toString() ==
+                  'package:built_value/built_value.dart') {
+                var info = AtomicEditInfo(
+                    NullabilityFixDescription.removeNullableAnnotation, {});
+                (_fixBuilder._getChange(node) as NodeChangeForMethodDeclaration)
+                  ..annotationToRemove = annotation
+                  ..removeAnnotationInfo = info;
+              }
+            }
+          }
+        }
+      }
+    }
+    super.visitMethodDeclaration(node);
+  }
+
   @override
   void visitTypeName(TypeName node) {
     var decoratedType = _fixBuilder._variables!

pkg/nnbd_migration/lib/src/front_end/info_builder.dart

@@ -265,6 +265,10 @@ class InfoBuilder {
       case NullabilityFixKind.addThen:
         // We don't offer any edits around addition of `.then` to a future.
         break;
+      case NullabilityFixKind.removeNullableAnnotation:
+        // We don't offer any edits around removal of built_value `@nullable`
+        // annotations.
+        break;
     }
     return edits;
   }

pkg/nnbd_migration/lib/src/front_end/migration_summary.dart

@@ -113,6 +113,8 @@ class MigrationSummary {
         return 'typeNotMadeNullable';
       case NullabilityFixKind.typeNotMadeNullableDueToHint:
         return 'typeNotMadeNullableDueToHint';
+      case NullabilityFixKind.removeNullableAnnotation:
+        return 'removeNullableAnnotation';
     }
   }
 }

pkg/nnbd_migration/lib/src/front_end/unit_renderer.dart

@@ -43,6 +43,7 @@ class UnitRenderer {
     NullabilityFixKind.removeAs,
     NullabilityFixKind.addLate,
     NullabilityFixKind.addLateDueToTestSetup,
+    NullabilityFixKind.removeNullableAnnotation,
     NullabilityFixKind.addLateDueToHint,
     NullabilityFixKind.addLateFinalDueToHint,
     NullabilityFixKind.checkExpressionDueToHint,
@@ -337,6 +338,8 @@ class UnitRenderer {
         return '$count type$s not made nullable';
       case NullabilityFixKind.typeNotMadeNullableDueToHint:
         return '$count type$s not made nullable due to hint$s';
+      case NullabilityFixKind.removeNullableAnnotation:
+        return '$count @nullable annotation$s removed';
     }
   }
 

pkg/nnbd_migration/test/api_test.dart

@@ -540,6 +540,58 @@ class _$Foo extends Foo {
   }
 }
 
+class FooBuilder implements Builder<Foo, FooBuilder> {
+  int get value => throw '';
+}
+''';
+    await _checkMultipleFileChanges(
+        {path1: file1, path2: file2}, {path1: expected1, path2: anything});
+  }
+
+  Future<void> test_built_value_nullable_getter() async {
+    addBuiltValuePackage();
+    var root = '$projectPath/lib';
+    var path1 = convertPath('$root/lib.dart');
+    var file1 = r'''
+import 'package:built_value/built_value.dart';
+
+part 'lib.g.dart';
+
+abstract class Foo implements Built<Foo, FooBuilder> {
+  @nullable
+  int get value;
+  Foo._();
+  factory Foo([void Function(FooBuilder) updates]) = _$Foo;
+}
+''';
+    var expected1 = r'''
+import 'package:built_value/built_value.dart';
+
+part 'lib.g.dart';
+
+abstract class Foo implements Built<Foo, FooBuilder> {
+  int? get value;
+  Foo._();
+  factory Foo([void Function(FooBuilder) updates]) = _$Foo;
+}
+''';
+    // Note: in a real-world scenario the generated file would be in a different
+    // directory but we don't need to simulate that detail for this test.  Also,
+    // the generated file would have a lot more code in it, but we don't need to
+    // simulate all the details of what is generated.
+    var path2 = convertPath('$root/lib.g.dart');
+    var file2 = r'''
+part of 'lib.dart';
+
+class _$Foo extends Foo {
+  @override
+  final int value;
+
+  factory _$Foo([void Function(FooBuilder) updates]) => throw '';
+
+  _$Foo._({this.value}) : super._();
+}
+
 class FooBuilder implements Builder<Foo, FooBuilder> {
   int get value => throw '';
 }

pkg/nnbd_migration/test/edit_plan_test.dart

@@ -903,6 +903,24 @@ f({
   int? x}) {}''');
   }
 
+  Future<void> test_remove_metadata_from_method_declaration() async {
+    await analyze('''
+class C {
+  @deprecated
+  f() {}
+}
+''');
+    var deprecated = findNode.annotation('@deprecated');
+    checkPlan(
+        planner!.passThrough(deprecated.parent,
+            innerPlans: [planner!.removeNode(deprecated)]),
+        '''
+class C {
+  f() {}
+}
+''');
+  }
+
   Future<void> test_remove_parameter() async {
     await analyze('f(int x, int y, int z) => null;');
     var parameter = findNode.simple('y').parent!;