[nnbd_migration] track causations for substitution nodes

MichaelRFairhurst · MichaelRFairhurst · commit e34e734b56e8 · 2019-11-18T18:49:39.000Z
Change-Id: I7da4f2923d9b99b0bd6c16644a7adeaeda437340 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/125463 Reviewed-by: Paul Berry <paulberry@google.com>
diff --git a/pkg/analysis_server/lib/src/edit/nnbd_migration/info_builder.dart b/pkg/analysis_server/lib/src/edit/nnbd_migration/info_builder.dart
@@ -91,6 +91,26 @@ class InfoBuilder {
     return units;
   }
 
+  Iterable<EdgeInfo> upstreamTriggeredEdges(NullabilityNodeInfo node,
+      [List<RegionDetail> details]) {
+    var edges = <EdgeInfo>[];
+    for (EdgeInfo edge in node.upstreamEdges) {
+      if (node.isExactNullable && edge.sourceNode.isExactNullable) {
+        // When an exact nullable points here, the nullability propagated
+        // in the other direction.
+        continue;
+      }
+      if (edge.isTriggered) {
+        edges.add(edge);
+      }
+    }
+    for (final containerNode in node.outerCompoundNodes) {
+      edges.addAll(upstreamTriggeredEdges(containerNode, details));
+    }
+
+    return edges;
+  }
+
   /// Return detail text for a fix built from an edge with [node] as a
   /// destination.
   String _baseDescriptionForOrigin(
@@ -294,21 +314,14 @@ class InfoBuilder {
           }
         }
 
-        for (EdgeInfo edge in reason.upstreamEdges) {
-          if (edge.sourceNode.isExactNullable) {
-            // When an exact nullable points here, the nullability propagated
-            // in the other direction.
-            continue;
-          }
-          if (edge.isTriggered) {
-            EdgeOriginInfo origin = info.edgeOrigin[edge];
-            if (origin != null) {
-              details.add(_buildDetailForOrigin(
-                  origin, edge, fixInfo.fix.description.kind));
-            } else {
-              details.add(
-                  RegionDetail('upstream edge with no origin ($edge)', null));
-            }
+        for (EdgeInfo edge in upstreamTriggeredEdges(reason)) {
+          EdgeOriginInfo origin = info.edgeOrigin[edge];
+          if (origin != null) {
+            details.add(_buildDetailForOrigin(
+                origin, edge, fixInfo.fix.description.kind));
+          } else {
+            details.add(
+                RegionDetail('upstream edge with no origin ($edge)', null));
           }
         }
       } else if (reason is EdgeInfo) {
diff --git a/pkg/analysis_server/test/src/edit/nnbd_migration/info_builder_test.dart b/pkg/analysis_server/test/src/edit/nnbd_migration/info_builder_test.dart
@@ -208,8 +208,9 @@ void g() {
     List<RegionInfo> regions = unit.regions;
     expect(regions, hasLength(3));
     // regions[0] is the hard edge that f's parameter is non-nullable.
-    // TODO(mfairhurst): Diagnose why no detail is appearing here.
-    // assertRegion(region: regions[1], offset: 15, details: ["Null is assigned"]);
+    assertRegion(region: regions[1], offset: 15, details: [
+      "An explicit 'null' is assigned",
+    ]);
     assertRegion(
         region: regions[2],
         offset: 66,
diff --git a/pkg/nnbd_migration/lib/instrumentation.dart b/pkg/nnbd_migration/lib/instrumentation.dart
@@ -233,6 +233,9 @@ abstract class NullabilityMigrationInstrumentation {
 /// Information exposed to the migration client about a single node in the
 /// nullability graph.
 abstract class NullabilityNodeInfo implements FixReasonInfo {
+  /// List of compound nodes wrapping this node.
+  final List<NullabilityNodeInfo> outerCompoundNodes = <NullabilityNodeInfo>[];
+
   /// Some nodes get nullability from downstream, so the downstream edges are
   /// available to query as well.
   Iterable<EdgeInfo> get downstreamEdges;
diff --git a/pkg/nnbd_migration/lib/src/nullability_node.dart b/pkg/nnbd_migration/lib/src/nullability_node.dart
@@ -360,6 +360,8 @@ class NullabilityGraph {
         for (var edge in node._upstreamEdges) {
           pendingEdges.add(edge);
         }
+
+        // TODO(mfairhurst): should this propagate back up outerContainerNodes?
       }
     }
     while (pendingEdges.isNotEmpty) {
@@ -460,6 +462,9 @@ abstract class NullabilityNode implements NullabilityNodeInfo {
   /// List of edges that have this node as their destination.
   final _upstreamEdges = <NullabilityEdge>[];
 
+  /// List of compound nodes wrapping this node.
+  final List<NullabilityNode> outerCompoundNodes = <NullabilityNode>[];
+
   /// Creates a [NullabilityNode] representing the nullability of a variable
   /// whose type comes from an already-migrated library.
   factory NullabilityNode.forAlreadyMigrated() =>
@@ -583,7 +588,10 @@ class NullabilityNodeForLUB extends _NullabilityNodeCompound {
 
   final NullabilityNode right;
 
-  NullabilityNodeForLUB._(this.left, this.right);
+  NullabilityNodeForLUB._(this.left, this.right) {
+    left.outerCompoundNodes.add(this);
+    right.outerCompoundNodes.add(this);
+  }
 
   @override
   Iterable<NullabilityNode> get _components => [left, right];
@@ -602,7 +610,10 @@ class NullabilityNodeForSubstitution extends _NullabilityNodeCompound
   @override
   final NullabilityNode outerNode;
 
-  NullabilityNodeForSubstitution._(this.innerNode, this.outerNode);
+  NullabilityNodeForSubstitution._(this.innerNode, this.outerNode) {
+    innerNode.outerCompoundNodes.add(this);
+    outerNode.outerCompoundNodes.add(this);
+  }
 
   @override
   Iterable<NullabilityNode> get _components => [innerNode, outerNode];
diff --git a/pkg/nnbd_migration/test/nullability_node_test.dart b/pkg/nnbd_migration/test/nullability_node_test.dart
@@ -106,6 +106,15 @@ class NullabilityNodeTest {
     assertUnsatisfied([]);
   }
 
+  test_lubNode_relatesInBothDirections() {
+    final nodeA = newNode(1);
+    final nodeB = newNode(2);
+    final lubNode = lub(nodeA, nodeB);
+
+    expect(nodeA.outerCompoundNodes, [lubNode]);
+    expect(nodeB.outerCompoundNodes, [lubNode]);
+  }
+
   test_never_source() {
     // never -> 1
     var n1 = newNode(1);
@@ -600,6 +609,15 @@ class NullabilityNodeTest {
     expect(subst(n1, null), same(n1));
   }
 
+  test_substitutionNode_relatesInBothDirections() {
+    final nodeA = newNode(1);
+    final nodeB = newNode(2);
+    final substNode = subst(nodeA, nodeB);
+
+    expect(nodeA.outerCompoundNodes, [substNode]);
+    expect(nodeB.outerCompoundNodes, [substNode]);
+  }
+
   test_unconstrainted_node_non_nullable() {
     var n1 = newNode(1);
     propagate();
