NNBD i13n: Proper messages for nullable values in collection literals

Change-Id: I5eccfeb7d88f3f4adb57f3734f170dce82c1fd2d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/121906
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Paul Berry <[email protected]>

Author: srawlins

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

@@ -104,16 +104,17 @@ class InfoBuilder {
       return "This field is initialized by a field formal parameter and a "
           "nullable value is passed as an argument";
     }
+    String nullableValue =
+        node is NullLiteral ? "an explicit 'null'" : "a nullable value";
+    String nullableValue2 =
+        node is NullLiteral ? "An explicit 'null'" : "A nullable value";
     AstNode parent = node.parent;
     if (parent is ArgumentList) {
-      if (node is NullLiteral) {
-        return "An explicit 'null' is passed as an argument";
-      }
-      return "A nullable value is explicitly passed as an argument";
+      return "$nullableValue2 is passed as an argument";
     }
 
-    /// If the [node] is the return expression for a function body, return the
-    /// function body. Otherwise return `null`.
+    /// If the [node] is inside the return expression for a function body,
+    /// return the function body. Otherwise return `null`.
     FunctionBody findFunctionBody() {
       if (parent is ExpressionFunctionBody) {
         return parent;
@@ -126,37 +127,51 @@ class InfoBuilder {
       }
     }
 
+    /// If the [node] is inside a collection literal, return it. Otherwise
+    /// return `null`.
+    TypedLiteral findCollectionLiteral() {
+      AstNode ancestor = parent;
+      // Walk up collection elements, except for collection literals.
+      while (ancestor is CollectionElement && ancestor is! TypedLiteral) {
+        ancestor = ancestor.parent;
+      }
+      return (ancestor is TypedLiteral) ? ancestor : null;
+    }
+
+    CompilationUnit unit = node.thisOrAncestorOfType<CompilationUnit>();
+    int lineNumber = unit.lineInfo.getLocation(node.offset).lineNumber;
     FunctionBody functionBody = findFunctionBody();
     if (functionBody != null) {
-      CompilationUnit unit = node.thisOrAncestorOfType<CompilationUnit>();
-      int lineNumber = unit.lineInfo.getLocation(node.offset).lineNumber;
       AstNode function = functionBody.parent;
       if (function is MethodDeclaration) {
         if (function.isGetter) {
-          return "This getter returns a nullable value on line $lineNumber";
+          return "This getter returns $nullableValue on line $lineNumber";
         }
-        return "This method returns a nullable value on line $lineNumber";
+        return "This method returns $nullableValue on line $lineNumber";
+      }
+      return "This function returns $nullableValue on line $lineNumber";
+    }
+
+    TypedLiteral collectionLiteral = findCollectionLiteral();
+    if (collectionLiteral != null) {
+      if (collectionLiteral is ListLiteral) {
+        return "This list is initialized with $nullableValue on line "
+            "$lineNumber";
+      } else if (collectionLiteral is SetOrMapLiteral) {
+        var mapOrSet = collectionLiteral.isMap ? 'map' : 'set';
+        return "This $mapOrSet is initialized with $nullableValue on line "
+            "$lineNumber";
       }
-      return "This function returns a nullable value on line $lineNumber";
     } else if (parent is VariableDeclaration) {
       AstNode grandparent = parent.parent?.parent;
       if (grandparent is FieldDeclaration) {
-        if (node is NullLiteral) {
-          return "This field is initialized to null";
-        }
-        return "This field is initialized to a nullable value";
+        return "This field is initialized to $nullableValue";
       }
-      if (node is NullLiteral) {
-        return "This variable is initialized to null";
-      }
-      return "This variable is initialized to a nullable value";
+      return "This variable is initialized to $nullableValue";
     } else if (parent is AsExpression) {
       return "The value of the expression is nullable";
     }
-    if (node is NullLiteral) {
-      return "An explicit 'null' is assigned";
-    }
-    return "A nullable value is assigned";
+    return "$nullableValue2 is assigned";
   }
 
   /// Return details for a fix built from the given [edge], or `null` if the

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

@@ -221,13 +221,79 @@ class A {
     assertRegion(
         region: regions[0],
         offset: 15,
-        details: ["This field is initialized to null"]);
+        details: ["This field is initialized to an explicit 'null'"]);
     assertRegion(
         region: regions[1],
         offset: 33,
         details: ["This field is initialized to a nullable value"]);
   }
 
+  test_listAndSetLiteralTypeArgument() async {
+    // TODO(srawlins): Simplify this test with `var x` once #38341 is fixed.
+    addTestFile('''
+void f() {
+  String s = null;
+  List<String> x = <String>["hello", s];
+  Set<String> y = <String>{"hello", s};
+''');
+    await buildInfo();
+    expect(infos, hasLength(1));
+    UnitInfo unit = infos[0];
+    expect(unit.content, '''
+void f() {
+  String? s = null;
+  List<String?> x = <String?>["hello", s];
+  Set<String?> y = <String?>{"hello", s};
+''');
+    List<RegionInfo> regions = unit.regions;
+    expect(regions, hasLength(5));
+    // regions[0] is the `String? s` fix.
+    // regions[1] is the `List<String?> x` fix.
+    assertRegion(
+        region: regions[2],
+        offset: 58,
+        details: ["This list is initialized with a nullable value on line 3"]);
+    assertDetail(detail: regions[2].details[0], offset: 67, length: 1);
+    // regions[3] is the `Set<String?> y` fix.
+    assertRegion(
+        region: regions[4],
+        offset: 100,
+        details: ["This set is initialized with a nullable value on line 4"]);
+    assertDetail(detail: regions[4].details[0], offset: 107, length: 1);
+  }
+
+  test_listLiteralTypeArgument_collectionIf() async {
+    // TODO(srawlins): Simplify this test with `var x` once #38341 is fixed.
+    addTestFile('''
+void f() {
+  String s = null;
+  List<String> x = <String>[
+    "hello",
+    if (1 == 2) s
+  ];
+''');
+    await buildInfo();
+    expect(infos, hasLength(1));
+    UnitInfo unit = infos[0];
+    expect(unit.content, '''
+void f() {
+  String? s = null;
+  List<String?> x = <String?>[
+    "hello",
+    if (1 == 2) s
+  ];
+''');
+    List<RegionInfo> regions = unit.regions;
+    expect(regions, hasLength(3));
+    // regions[0] is the `String? s` fix.
+    // regions[1] is the `List<String?> x` fix.
+    assertRegion(
+        region: regions[2],
+        offset: 58,
+        details: ["This list is initialized with a nullable value on line 5"]);
+    assertDetail(detail: regions[2].details[0], offset: 88, length: 1);
+  }
+
   test_localVariable() async {
     addTestFile('''
 void f() {
@@ -250,13 +316,47 @@ void f() {
     assertRegion(
         region: regions[0],
         offset: 16,
-        details: ["This variable is initialized to null"]);
+        details: ["This variable is initialized to an explicit 'null'"]);
     assertRegion(
         region: regions[1],
         offset: 35,
         details: ["This variable is initialized to a nullable value"]);
   }
 
+  test_mapLiteralTypeArgument() async {
+    // TODO(srawlins): Simplify this test with `var x` once #38341 is fixed.
+    addTestFile('''
+void f() {
+  String s = null;
+  Map<String, bool> x = <String, bool>{"hello": false, s: true};
+  Map<bool, String> y = <bool, String>{false: "hello", true: s};
+''');
+    await buildInfo();
+    expect(infos, hasLength(1));
+    UnitInfo unit = infos[0];
+    expect(unit.content, '''
+void f() {
+  String? s = null;
+  Map<String?, bool> x = <String?, bool>{"hello": false, s: true};
+  Map<bool, String?> y = <bool, String?>{false: "hello", true: s};
+''');
+    List<RegionInfo> regions = unit.regions;
+    expect(regions, hasLength(5));
+    // regions[0] is the `String? s` fix.
+    // regions[1] is the `Map<String?, bool> x` fix.
+    assertRegion(
+        region: regions[2],
+        offset: 63,
+        details: ["This map is initialized with a nullable value on line 3"]);
+    assertDetail(detail: regions[2].details[0], offset: 85, length: 1);
+    // regions[3] is the `Map<bool, String?> y` fix.
+    assertRegion(
+        region: regions[4],
+        offset: 136,
+        details: ["This map is initialized with a nullable value on line 4"]);
+    assertDetail(detail: regions[4].details[0], offset: 156, length: 1);
+  }
+
   test_parameter_fromInvocation_explicit() async {
     addTestFile('''
 void f(String s) {}
@@ -562,7 +662,7 @@ int? f() => _f;
     assertRegion(
         region: regions[0],
         offset: 3,
-        details: ["This variable is initialized to null"]);
+        details: ["This variable is initialized to an explicit 'null'"]);
     assertRegion(
         region: regions[1],
         offset: 19,
@@ -595,7 +695,7 @@ class A {
     assertRegion(
         region: regions[0],
         offset: 15,
-        details: ["This field is initialized to null"]);
+        details: ["This field is initialized to an explicit 'null'"]);
     assertRegion(
         region: regions[1],
         offset: 33,
@@ -624,13 +724,47 @@ class A {
     assertRegion(
         region: regions[0],
         offset: 15,
-        details: ["This field is initialized to null"]);
+        details: ["This field is initialized to an explicit 'null'"]);
     assertRegion(
         region: regions[1],
         offset: 33,
         details: ["This getter returns a nullable value on line 3"]);
   }
 
+  test_setLiteralTypeArgument_nestedList() async {
+    // TODO(srawlins): Simplify this test with `var x` once #38341 is fixed.
+    addTestFile('''
+void f() {
+  String s = null;
+  Set<List<String>> x = <List<String>>{
+    ["hello"],
+    if (1 == 2) [s]
+  };
+''');
+    await buildInfo();
+    expect(infos, hasLength(1));
+    UnitInfo unit = infos[0];
+    expect(unit.content, '''
+void f() {
+  String? s = null;
+  Set<List<String?>> x = <List<String?>>{
+    ["hello"],
+    if (1 == 2) [s]
+  };
+''');
+    List<RegionInfo> regions = unit.regions;
+    expect(regions, hasLength(3));
+    // regions[0] is the `String? s` fix.
+    // regions[1] is the `Set<List<String?>> x` fix.
+    assertRegion(
+        region: regions[2],
+        offset: 68,
+        details: ["This set is initialized with a nullable value on line 5"]);
+    // TODO(srawlins): Actually, this is marking the `[s]`, but I think only
+    //  `s` should be marked. Minor bug for now.
+    assertDetail(detail: regions[2].details[0], offset: 101, length: 3);
+  }
+
   test_topLevelVariable() async {
     addTestFile('''
 int _f = null;
@@ -649,7 +783,7 @@ int? _f2 = _f;
     assertRegion(
         region: regions[0],
         offset: 3,
-        details: ["This variable is initialized to null"]);
+        details: ["This variable is initialized to an explicit 'null'"]);
     assertRegion(
         region: regions[1],
         offset: 19,