NNBD i13n: Fix some navigation targets and text

Change-Id: Iba6befb86453353823c1702405057f83fff64456
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/120723
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

Author: srawlins

pkg/analysis_server/lib/src/edit/nnbd_migration/info_builder.dart

@@ -143,6 +143,8 @@ class InfoBuilder {
         return "This variable is initialized to null";
       }
       return "This variable is initialized to a nullable value";
+    } else if (parent is AsExpression) {
+      return "The value of the expression is nullable";
     }
     if (node is NullLiteral) {
       return "An explicit 'null' is assigned";
@@ -321,7 +323,25 @@ class InfoBuilder {
   /// Return the navigation target corresponding to the given [node] in the file
   /// with the given [filePath].
   NavigationTarget _targetForNode(String filePath, AstNode node) {
-    return _targetFor(filePath, node.offset, node.length);
+    AstNode parent = node.parent;
+    if (node is MethodDeclaration) {
+      // Rather than create a NavigationTarget for an entire method declaration
+      // (starting at its doc comment, ending at `}`, return a target pointing
+      // to the method's name.
+      return _targetFor(filePath, node.name.offset, node.name.length);
+    } else if (parent is ReturnStatement) {
+      // Rather than create a NavigationTarget for an entire expression, return
+      // a target pointing to the `return` token.
+      return _targetFor(
+          filePath, parent.returnKeyword.offset, parent.returnKeyword.length);
+    } else if (parent is ExpressionFunctionBody) {
+      // Rather than create a NavigationTarget for an entire expression function
+      // body, return a target pointing to the `=>` token.
+      return _targetFor(filePath, parent.functionDefinition.offset,
+          parent.functionDefinition.length);
+    } else {
+      return _targetFor(filePath, node.offset, node.length);
+    }
   }
 
   /// Return the unit info for the file at the given [path].

pkg/analysis_server/lib/src/edit/nnbd_migration/migration_info.dart

@@ -55,6 +55,9 @@ class NavigationTarget extends NavigationRegion {
         other.offset == offset &&
         other.length == length;
   }
+
+  @override
+  String toString() => 'NavigationTarget["$filePath", $offset, $length]';
 }
 
 /// An additional detail related to a region.

pkg/analysis_server/test/src/edit/nnbd_migration/info_builder_test.dart

@@ -28,6 +28,29 @@ class InfoBuilderTest extends AbstractAnalysisTest {
   /// not yet completed.
   List<UnitInfo> infos;
 
+  /// Assert various properties of the given [detail].
+  void assertDetail({@required RegionDetail detail, int offset, int length}) {
+    if (offset != null) {
+      expect(detail.target.offset, offset);
+    }
+    if (length != null) {
+      expect(detail.target.length, length);
+    }
+  }
+
+  /// Assert that some target in [targets] has various properties.
+  void assertInTargets(
+      {@required Iterable<NavigationTarget> targets, int offset, int length}) {
+    String failureReasons = [
+      if (offset != null) 'offset: $offset',
+      if (length != null) 'length: $length',
+    ].join(' and ');
+    expect(targets.any((t) {
+      return (offset == null || offset == t.offset) &&
+          (length == null || length == t.length);
+    }), isTrue, reason: 'Expected one of $targets to contain $failureReasons');
+  }
+
   /// Assert various properties of the given [region]. If an [offset] is
   /// provided but no [length] is provided, a default length of `1` will be
   /// used.
@@ -72,6 +95,52 @@ class InfoBuilderTest extends AbstractAnalysisTest {
     infos = await builder.explainMigration();
   }
 
+  test_asExpression() async {
+    addTestFile('''
+void f([num a]) {
+  int b = a as int;
+}
+''');
+    await buildInfo();
+    UnitInfo unit = infos[0];
+    expect(unit.content, '''
+void f([num? a]) {
+  int? b = a as int?;
+}
+''');
+    List<RegionInfo> regions = unit.regions;
+    expect(regions, hasLength(3));
+    assertRegion(region: regions[0], offset: 11);
+    assertRegion(
+        region: regions[1],
+        offset: 24,
+        details: ["This variable is initialized to a nullable value"]);
+    assertRegion(
+        region: regions[2],
+        offset: 38,
+        details: ["The value of the expression is nullable"]);
+  }
+
+  test_expressionFunctionReturnTarget() async {
+    addTestFile('''
+String g() => 1 == 2 ? "Hello" : null;
+''');
+    await buildInfo();
+    UnitInfo unit = infos[0];
+    expect(unit.content, '''
+String? g() => 1 == 2 ? "Hello" : null;
+''');
+    assertInTargets(targets: unit.targets, offset: 7, length: 1); // "g"
+    assertInTargets(targets: unit.targets, offset: 11, length: 2); // "=>"
+    List<RegionInfo> regions = unit.regions;
+    expect(regions, hasLength(1));
+    assertRegion(
+        region: regions[0],
+        offset: 6,
+        details: ["This function returns a nullable value"]);
+    assertDetail(detail: regions[0].details[0], offset: 11, length: 2);
+  }
+
   test_field_fieldFormalInitializer_optional() async {
     addTestFile('''
 class A {
@@ -357,6 +426,30 @@ void f([String? s]) {}
         details: ["This parameter has an implicit default value of 'null'"]);
   }
 
+  test_returnDetailTarget() async {
+    addTestFile('''
+String g() {
+  return 1 == 2 ? "Hello" : null;
+}
+''');
+    await buildInfo();
+    UnitInfo unit = infos[0];
+    expect(unit.content, '''
+String? g() {
+  return 1 == 2 ? "Hello" : null;
+}
+''');
+    assertInTargets(targets: unit.targets, offset: 7, length: 1); // "g"
+    assertInTargets(targets: unit.targets, offset: 15, length: 6); // "return"
+    List<RegionInfo> regions = unit.regions;
+    expect(regions, hasLength(1));
+    assertRegion(
+        region: regions[0],
+        offset: 6,
+        details: ["This function returns a nullable value"]);
+    assertDetail(detail: regions[0].details[0], offset: 15, length: 6);
+  }
+
   test_returnType_function_expression() async {
     addTestFile('''
 int _f = null;