Migration: respect built_value @nullable annotations even when there is no codegen

Previously, when the migration tool encountered a built_value
`@nullable` annotation, it removed it, because when the built_value
code generator is consuming a library with null safety enabled, it
relies on the presence of a `?` to determine nullability, rather than
an annotation.  However, there was no logic to actually ensure that
the `?` would actually get introduced, because I thought the code
generated by built_value would always create the conditions necessary
to convince the migration tool to introduce the `?` using its normal
graph traversal algorithm.

It turns out this is not the case: when `@nullable` appears in an
interface class that is used by other built_value classes, but is not
itself a built_value class, the migration tool sometimes doesn't have
enough information to figure out that it needs to add the `?`.

So in this CL, I'm doing what I probably should have done in the first
time: adding the necessary logic to the migration tool to ensure that
the `@nullable` annotation gets translated into a `?` regardless of
whether it is required to by generated code.

Bug: https://buganizer.corp.google.com/issues/217863427
Change-Id: I9efe5241634389981a4c56e764bac91b3350c4fb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/233003
Reviewed-by: Samuel Rawlins <[email protected]>
Commit-Queue: Paul Berry <[email protected]>

Author: stereotype441

pkg/nnbd_migration/lib/instrumentation.dart

@@ -242,6 +242,7 @@ enum EdgeOriginKind {
   alwaysNullableType,
   angularAnnotation,
   argumentErrorCheckNotNull,
+  builtValueNullableAnnotation,
   callTearOff,
   compoundAssignment,
   // See [DummyOrigin].

pkg/nnbd_migration/lib/src/edge_builder.dart

@@ -29,6 +29,7 @@ import 'package:nnbd_migration/src/edge_origin.dart';
 import 'package:nnbd_migration/src/expression_checks.dart';
 import 'package:nnbd_migration/src/nullability_node.dart';
 import 'package:nnbd_migration/src/nullability_node_target.dart';
+import 'package:nnbd_migration/src/utilities/built_value_transformer.dart';
 import 'package:nnbd_migration/src/utilities/completeness_tracker.dart';
 import 'package:nnbd_migration/src/utilities/hint_utils.dart';
 import 'package:nnbd_migration/src/utilities/permissive_mode.dart';
@@ -1330,6 +1331,14 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
 
   @override
   DecoratedType? visitMethodDeclaration(MethodDeclaration node) {
+    if (BuiltValueTransformer.findNullableAnnotation(node) != null) {
+      _graph.makeNullable(
+          _variables!
+              .decoratedElementType(node.declaredElement!.declaration)
+              .returnType!
+              .node!,
+          BuiltValueNullableOrigin(source, node));
+    }
     _handleExecutableDeclaration(node, node.declaredElement!, node.metadata,
         node.returnType, node.parameters, null, node.body, null);
     _dispatch(node.typeParameters);

pkg/nnbd_migration/lib/src/edge_origin.dart

@@ -98,6 +98,17 @@ class ArgumentErrorCheckNotNullOrigin extends EdgeOrigin {
   EdgeOriginKind get kind => EdgeOriginKind.argumentErrorCheckNotNull;
 }
 
+/// Edge origin resulting from a use of built_value's `@nullable` annotation.
+class BuiltValueNullableOrigin extends EdgeOrigin {
+  BuiltValueNullableOrigin(Source? source, AstNode node) : super(source, node);
+
+  @override
+  String get description => 'method is marked with the `@nullable` annotation';
+
+  @override
+  EdgeOriginKind get kind => EdgeOriginKind.builtValueNullableAnnotation;
+}
+
 /// An edge origin used for edges that originated because of a tear-off of
 /// `call` on a function type.
 class CallTearOffOrigin extends EdgeOrigin {

pkg/nnbd_migration/lib/src/fix_builder.dart

@@ -35,6 +35,7 @@ import 'package:nnbd_migration/src/decorated_type.dart';
 import 'package:nnbd_migration/src/edit_plan.dart';
 import 'package:nnbd_migration/src/fix_aggregator.dart';
 import 'package:nnbd_migration/src/nullability_node.dart';
+import 'package:nnbd_migration/src/utilities/built_value_transformer.dart';
 import 'package:nnbd_migration/src/utilities/permissive_mode.dart';
 import 'package:nnbd_migration/src/utilities/resolution_utils.dart';
 import 'package:nnbd_migration/src/utilities/where_or_null_transformer.dart';
@@ -1245,25 +1246,13 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
 
   @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;
-              }
-            }
-          }
-        }
-      }
+    var nullableAnnotation = BuiltValueTransformer.findNullableAnnotation(node);
+    if (nullableAnnotation != null) {
+      var info = AtomicEditInfo(
+          NullabilityFixDescription.removeNullableAnnotation, {});
+      (_fixBuilder._getChange(node) as NodeChangeForMethodDeclaration)
+        ..annotationToRemove = nullableAnnotation
+        ..removeAnnotationInfo = info;
     }
     super.visitMethodDeclaration(node);
   }

pkg/nnbd_migration/lib/src/utilities/built_value_transformer.dart

@@ -0,0 +1,28 @@
+// 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.
+
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/element/element.dart';
+
+class BuiltValueTransformer {
+  static Annotation? findNullableAnnotation(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') {
+                return annotation;
+              }
+            }
+          }
+        }
+      }
+    }
+    return null;
+  }
+}

pkg/nnbd_migration/test/api_test.dart

@@ -977,6 +977,26 @@ class FooBuilder implements Builder<Foo, FooBuilder> {
         {path1: file1, path2: file2}, {path1: expected1, path2: anything});
   }
 
+  Future<void> test_built_value_nullable_getter_interface_only() async {
+    addBuiltValuePackage();
+    var content = '''
+import 'package:built_value/built_value.dart';
+
+abstract class Foo {
+  @nullable
+  int get value;
+}
+''';
+    var expected = '''
+import 'package:built_value/built_value.dart';
+
+abstract class Foo {
+  int? get value;
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   Future<void> test_call_already_migrated_extension() async {
     var content = '''
 import 'already_migrated.dart';