Migration: expose fix reasons through the instrumentation interface.

Change-Id: I0f8ba9aa58d098b431fad338d46a2f14d87e43b4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/117686
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Paul Berry <[email protected]>

Author: stereotype441

pkg/nnbd_migration/lib/instrumentation.dart

@@ -6,6 +6,7 @@ import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/src/generated/source.dart';
+import 'package:nnbd_migration/nnbd_migration.dart';
 import 'package:nnbd_migration/nullability_state.dart';
 
 /// Information exposed to the migration client about the set of nullability
@@ -42,7 +43,7 @@ abstract class DecoratedTypeInfo {
 /// A graph edge represents a dependency relationship between two types being
 /// migrated, suggesting that if one type (the source) is made nullable, it may
 /// be desirable to make the other type (the destination) nullable as well.
-abstract class EdgeInfo {
+abstract class EdgeInfo implements FixReasonInfo {
   /// Information about the graph node that this edge "points to".
   NullabilityNodeInfo get destinationNode;
 
@@ -96,6 +97,10 @@ abstract class EdgeOriginInfo {
   Source get source;
 }
 
+/// Interface used by the migration engine to expose information to its client
+/// about a reason for a modification to the source file.
+abstract class FixReasonInfo {}
+
 /// Interface used by the migration engine to expose information to its client
 /// about the decisions made during migration, and how those decisions relate to
 /// the input source code.
@@ -112,6 +117,9 @@ abstract class NullabilityMigrationInstrumentation {
   /// of the element.
   void externalDecoratedType(Element element, DecoratedTypeInfo decoratedType);
 
+  /// Called whenever a fix is decided upon, to report the reasons for the fix.
+  void fix(SingleNullabilityFix fix, Iterable<FixReasonInfo> reasons);
+
   /// Called whenever the migration engine creates a graph edge between
   /// nullability nodes, to report information about the edge that was created,
   /// and why it was created.
@@ -162,7 +170,7 @@ abstract class NullabilityMigrationInstrumentation {
 
 /// Information exposed to the migration client about a single node in the
 /// nullability graph.
-abstract class NullabilityNodeInfo {
+abstract class NullabilityNodeInfo implements FixReasonInfo {
   /// Indicates whether the node is immutable.  The only immutable nodes in the
   /// nullability graph are the nodes `never` and `always` that are used as the
   /// starting points for nullability propagation.

pkg/nnbd_migration/lib/src/conditional_discard.dart

@@ -2,6 +2,7 @@
 // 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:nnbd_migration/instrumentation.dart';
 import 'package:nnbd_migration/src/nullability_node.dart';
 
 /// Container for information gathered during nullability migration about a
@@ -48,4 +49,10 @@ class ConditionalDiscard {
   /// Indicates whether the code path that results from the condition evaluating
   /// to `true` is reachable after migration.
   bool get keepTrue => trueGuard == null || trueGuard.isNullable;
+
+  @override
+  Iterable<FixReasonInfo> get reasons sync* {
+    if (!keepTrue) yield falseGuard;
+    if (!keepFalse) yield trueGuard;
+  }
 }

pkg/nnbd_migration/lib/src/expression_checks.dart

@@ -5,6 +5,7 @@
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart';
+import 'package:nnbd_migration/instrumentation.dart';
 import 'package:nnbd_migration/nnbd_migration.dart';
 import 'package:nnbd_migration/src/edge_origin.dart';
 import 'package:nnbd_migration/src/nullability_node.dart';
@@ -65,6 +66,13 @@ class ExpressionChecks extends PotentialModification {
     // reified to contain only non-null ints.
     return isEmpty ? [] : [SourceEdit(offset, 0, '!')];
   }
+
+  @override
+  Iterable<FixReasonInfo> get reasons sync* {
+    for (var edge in edges) {
+      if (!edge.isSatisfied) yield edge;
+    }
+  }
 }
 
 /// [EdgeOrigin] object associated with [ExpressionChecks].  This is a separate

pkg/nnbd_migration/lib/src/nullability_migration_impl.dart

@@ -55,7 +55,7 @@ class NullabilityMigrationImpl implements NullabilityMigration {
     // it, we can't report on every unsatisfied edge.  We need to figure out a
     // way to report unsatisfied edges that isn't too overwhelming.
     if (_variables != null) {
-      broadcast(_variables, listener);
+      broadcast(_variables, listener, _instrumentation);
     }
   }
 
@@ -77,7 +77,9 @@ class NullabilityMigrationImpl implements NullabilityMigration {
 
   @visibleForTesting
   static void broadcast(
-      Variables variables, NullabilityMigrationListener listener) {
+      Variables variables,
+      NullabilityMigrationListener listener,
+      NullabilityMigrationInstrumentation instrumentation) {
     for (var entry in variables.getPotentialModifications().entries) {
       var source = entry.key;
       final lineInfo = LineInfo.fromContent(source.contents.data);
@@ -89,6 +91,7 @@ class NullabilityMigrationImpl implements NullabilityMigration {
         var fix =
             _SingleNullabilityFix(source, potentialModification, lineInfo);
         listener.addFix(fix);
+        instrumentation?.fix(fix, potentialModification.reasons);
         for (var edit in modifications) {
           listener.addEdit(fix, edit);
         }

pkg/nnbd_migration/lib/src/potential_modification.dart

@@ -5,6 +5,7 @@
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart' show SourceEdit;
+import 'package:nnbd_migration/instrumentation.dart';
 import 'package:nnbd_migration/nnbd_migration.dart';
 import 'package:nnbd_migration/src/conditional_discard.dart';
 import 'package:nnbd_migration/src/nullability_node.dart';
@@ -87,6 +88,9 @@ class ConditionalModification extends PotentialModification {
     }
     return result;
   }
+
+  @override
+  Iterable<FixReasonInfo> get reasons => discard.reasons;
 }
 
 /// Records information about the possible addition of an import to the source
@@ -125,6 +129,13 @@ class PotentiallyAddImport extends PotentialModification {
   Iterable<SourceEdit> get modifications =>
       isEmpty ? const [] : [SourceEdit(_offset, 0, "import '$importPath';\n")];
 
+  @override
+  Iterable<FixReasonInfo> get reasons sync* {
+    for (var usage in _usages) {
+      if (!usage.isEmpty) yield* usage.reasons;
+    }
+  }
+
   void addUsage(PotentialModification usage) {
     _usages.add(usage);
   }
@@ -149,6 +160,9 @@ class PotentiallyAddQuestionSuffix extends PotentialModification {
   @override
   Iterable<SourceEdit> get modifications =>
       isEmpty ? [] : [SourceEdit(_offset, 0, '?')];
+
+  @override
+  Iterable<FixReasonInfo> get reasons => [node];
 }
 
 /// Records information about the possible addition of a `@required` annotation
@@ -184,6 +198,9 @@ class PotentiallyAddRequired extends PotentialModification {
   @override
   Iterable<SourceEdit> get modifications =>
       isEmpty ? const [] : [SourceEdit(_offset, 0, '@required ')];
+
+  @override
+  Iterable<FixReasonInfo> get reasons => [_node];
 }
 
 /// Interface used by data structures representing potential modifications to
@@ -197,6 +214,9 @@ abstract class PotentialModification {
   /// Gets the individual migrations that need to be done, considering the
   /// solution to the constraint equations.
   Iterable<SourceEdit> get modifications;
+
+  /// Gets the reasons for this potential modification.
+  Iterable<FixReasonInfo> get reasons;
 }
 
 /// Helper object used by [ConditionalModification] to keep track of AST nodes

pkg/nnbd_migration/test/instrumentation_test.dart

@@ -40,6 +40,11 @@ class _InstrumentationClient implements NullabilityMigrationInstrumentation {
     test.externalDecoratedType[element] = decoratedType;
   }
 
+  @override
+  void fix(SingleNullabilityFix fix, Iterable<FixReasonInfo> reasons) {
+    test.fixes[fix] = reasons.toList();
+  }
+
   @override
   void graphEdge(EdgeInfo edge, EdgeOriginInfo originInfo) {
     expect(test.edgeOrigin, isNot(contains(edge)));
@@ -93,6 +98,8 @@ class _InstrumentationTest extends AbstractContextTest {
 
   final List<EdgeInfo> edges = [];
 
+  Map<SingleNullabilityFix, List<FixReasonInfo>> fixes = {};
+
   final Map<AstNode, DecoratedTypeInfo> implicitReturnType = {};
 
   final Map<AstNode, DecoratedTypeInfo> implicitType = {};
@@ -148,6 +155,47 @@ main() {
         'void Function(Object)');
   }
 
+  test_fix_reason_edge() async {
+    await analyze('''
+void f(int x) {
+  print(x.isEven);
+}
+void g(int y, bool b) {
+  if (b) {
+    f(y);
+  }
+}
+main() {
+  g(null, false);
+}
+''');
+    var yUsage = findNode.simple('y);');
+    var entry =
+        fixes.entries.where((e) => e.key.location.offset == yUsage.end).single;
+    var reasons = entry.value;
+    expect(reasons, hasLength(1));
+    var edge = reasons[0] as EdgeInfo;
+    expect(edge.sourceNode,
+        same(explicitTypeNullability[findNode.typeAnnotation('int y')]));
+    expect(edge.destinationNode,
+        same(explicitTypeNullability[findNode.typeAnnotation('int x')]));
+    expect(edge.isSatisfied, false);
+    expect(edgeOrigin[edge].node, same(yUsage));
+  }
+
+  test_fix_reason_node() async {
+    await analyze('''
+int x = null;
+''');
+    var entries = fixes.entries.toList();
+    expect(entries, hasLength(1));
+    var intAnnotation = findNode.typeAnnotation('int');
+    expect(entries.single.key.location.offset, intAnnotation.end);
+    var reasons = entries.single.value;
+    expect(reasons, hasLength(1));
+    expect(reasons.single, same(explicitTypeNullability[intAnnotation]));
+  }
+
   test_graphEdge() async {
     await analyze('''
 int f(int x) => x;

pkg/nnbd_migration/test/nullability_migration_impl_test.dart

@@ -39,7 +39,7 @@ class NullabilityMigrationImplTest {
 
     when(innerModification.isEmpty).thenReturn(false);
 
-    NullabilityMigrationImpl.broadcast(variables, listener);
+    NullabilityMigrationImpl.broadcast(variables, listener, null);
 
     final fix = verify(listener.addFix(captureAny)).captured.single
         as SingleNullabilityFix;
@@ -68,7 +68,7 @@ class NullabilityMigrationImplTest {
 
     when(potentialModification.modifications).thenReturn([]);
 
-    NullabilityMigrationImpl.broadcast(variables, listener);
+    NullabilityMigrationImpl.broadcast(variables, listener, null);
 
     verifyNever(listener.addFix(any));
     verifyNever(listener.reportException(any, any, any, any));
@@ -80,7 +80,7 @@ class NullabilityMigrationImplTest {
     final source = SourceMock('');
     when(variables.getPotentialModifications()).thenReturn({source: []});
 
-    NullabilityMigrationImpl.broadcast(variables, listener);
+    NullabilityMigrationImpl.broadcast(variables, listener, null);
 
     verifyNever(listener.addFix(any));
     verifyNever(listener.reportException(any, any, any, any));