Migration: fix hard crash in trial_migration when migrating package:async.

There were two problems:

1. An info object isn't being properly passed to
   NodeChangeForExpression, causing an assertion failure during the
   FixAggregator stage.  I've modified the assertion check so that it
   happens earlier, during the FixBuilder, so that the exception is
   caught by permissive mode logic and doesn't cause a hard crash.
   I'll work on fixing the assertion in a follow-up CL.

2. The hack described in https://github.com/dart-lang/sdk/issues/40536
   was causing a bogus assertion failure in
   _PassThroughBuilderImpl._checkParenLogic.  I've weakened the
   assertion slightly so that for now, the hack won't cause trouble.
   See the issue for more information about what's happening--this
   should be cleaned up eventually but it's probably not high
   priority.

Bug: https://github.com/dart-lang/sdk/issues/40533
Change-Id: Iee832fbd07e54b02d2c52f79a097ff9d973cda9a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134980
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

pkg/nnbd_migration/lib/src/edit_plan.dart

@@ -1116,6 +1116,8 @@ class _PassThroughBuilderImpl implements PassThroughBuilder {
     if (node is FunctionExpression && node.body is ExpressionFunctionBody) {
       // To avoid ambiguities when adding `as Type` after a function expression,
       // assume assignment precedence.
+      // TODO(paulberry): this is a hack - see
+      // https://github.com/dart-lang/sdk/issues/40536
       precedence = Precedence.assignment;
     } else if (node is Expression) {
       precedence = node.precedence;
@@ -1306,10 +1308,17 @@ class _PassThroughBuilderImpl implements PassThroughBuilder {
 
   static bool _checkParenLogic(EditPlan innerPlan, bool parensNeeded) {
     if (innerPlan is _SimpleEditPlan && innerPlan._innerChanges == null) {
-      assert(
-          !parensNeeded,
-          "Code prior to fixes didn't need parens here, "
-          "shouldn't need parens now.");
+      if (innerPlan.sourceNode is FunctionExpression) {
+        // Skip parentheses check for function expressions; it produces false
+        // failures when examining an expression like `x ?? (y) => z`, due to
+        // https://github.com/dart-lang/sdk/issues/40536.
+        // TODO(paulberry): fix this.
+      } else {
+        assert(
+            !parensNeeded,
+            "Code prior to fixes didn't need parens here, "
+            "shouldn't need parens now.");
+      }
     }
     return true;
   }

pkg/nnbd_migration/lib/src/fix_aggregator.dart

@@ -263,23 +263,38 @@ class NodeChangeForDefaultFormalParameter
 /// Implementation of [NodeChange] specialized for operating on [Expression]
 /// nodes.
 class NodeChangeForExpression<N extends Expression> extends NodeChange<N> {
-  /// Indicates whether the expression should be null checked.
-  bool addNullCheck = false;
+  bool _addsNullCheck = false;
 
-  /// If [addNullCheck] is `true`, the information that should be contained in
-  /// the edit that adds the null check.
-  AtomicEditInfo addNullCheckInfo;
+  AtomicEditInfo _addNullCheckInfo;
 
-  /// Indicates whether the expression should be cast to a different type using
-  /// `as`.
-  String introduceAsType;
+  String _introducesAsType;
 
-  /// If [introduceAsType] is not `null`, the information that should be
-  /// contained in the edit that introduces the cast.
-  AtomicEditInfo introduceAsInfo;
+  AtomicEditInfo _introduceAsInfo;
 
   NodeChangeForExpression() : super._();
 
+  /// Indicates whether [addNullCheck] has been called.
+  bool get addsNullCheck => _addsNullCheck;
+
+  /// Causes a null check to be added to this expression, with the given [info].
+  void addNullCheck(AtomicEditInfo info) {
+    assert(!_addsNullCheck);
+    assert(_introducesAsType == null);
+    _addsNullCheck = true;
+    _addNullCheckInfo = info;
+  }
+
+  /// Causes a cast to the given [type] to be added to this expression, with
+  /// the given [info].
+  void introduceAs(String type, AtomicEditInfo info) {
+    assert(!_addsNullCheck);
+    assert(_introducesAsType == null);
+    assert(type != null);
+    assert(info != null);
+    _introducesAsType = type;
+    _introduceAsInfo = info;
+  }
+
   @override
   EditPlan _apply(N node, FixAggregator aggregator) {
     var innerPlan = aggregator.innerPlanForNode(node);
@@ -291,14 +306,13 @@ class NodeChangeForExpression<N extends Expression> extends NodeChange<N> {
   /// Otherwise returns [innerPlan] unchanged.
   NodeProducingEditPlan _applyExpression(
       FixAggregator aggregator, NodeProducingEditPlan innerPlan) {
-    assert((introduceAsType == null) == (introduceAsInfo == null));
-    if (addNullCheck) {
-      assert(introduceAsInfo == null);
+    if (_addsNullCheck) {
       return aggregator.planner
-          .addUnaryPostfix(innerPlan, TokenType.BANG, info: addNullCheckInfo);
-    } else if (introduceAsInfo != null) {
-      return aggregator.planner
-          .addBinaryPostfix(innerPlan, TokenType.AS, introduceAsType);
+          .addUnaryPostfix(innerPlan, TokenType.BANG, info: _addNullCheckInfo);
+    } else if (_introducesAsType != null) {
+      return aggregator.planner.addBinaryPostfix(
+          innerPlan, TokenType.AS, _introducesAsType,
+          info: _introduceAsInfo);
     } else {
       return innerPlan;
     }

pkg/nnbd_migration/lib/src/fix_builder.dart

@@ -432,8 +432,7 @@ class MigrationResolutionHooksImpl implements MigrationResolutionHooks {
             NullabilityFixDescription.checkExpression, checks.edges)
         : null;
     (_fixBuilder._getChange(node) as NodeChangeForExpression)
-      ..introduceAsType = contextType.getDisplayString(withNullability: true)
-      ..introduceAsInfo = info;
+        .introduceAs(contextType.getDisplayString(withNullability: true), info);
     _flowAnalysis.asExpression_end(node, contextType);
     return contextType;
   }
@@ -446,8 +445,7 @@ class MigrationResolutionHooksImpl implements MigrationResolutionHooks {
             NullabilityFixDescription.checkExpression, checks.edges)
         : null;
     (_fixBuilder._getChange(node) as NodeChangeForExpression)
-      ..addNullCheck = true
-      ..addNullCheckInfo = info;
+        .addNullCheck(info);
     _flowAnalysis.nonNullAssert_end(node);
     return _fixBuilder._typeSystem.promoteToNonNull(type as TypeImpl);
   }

pkg/nnbd_migration/test/edit_plan_test.dart

@@ -1361,6 +1361,10 @@ void g(x) => E(x).f();
 ''');
   }
 
+  Future<void> test_precedence_functionExpression_ifNotNull() async {
+    await checkPrecedence('f(b, c) => ((a) => b) ?? c;');
+  }
+
   Future<void> test_precedence_functionExpressionInvocation() async {
     await checkPrecedence('''
 f(g) => g[0](1);
@@ -1368,6 +1372,11 @@ h(x) => (x + 2)(3);
 ''');
   }
 
+  @FailingTest(issue: 'https://github.com/dart-lang/sdk/issues/40536')
+  Future<void> test_precedence_ifNotNull_functionExpression() async {
+    await checkPrecedence('f(a, c) => a ?? (b) => c;');
+  }
+
   Future<void> test_precedence_is() async {
     await checkPrecedence('''
 f(a) => (a as num) is int;

pkg/nnbd_migration/test/fix_aggregator_test.dart

@@ -36,9 +36,10 @@ class FixAggregatorTest extends FixAggregatorTestBase {
     var aRef = findNode.simple('a +');
     var bRef = findNode.simple('b;');
     var previewInfo = run({
-      aRef: NodeChangeForExpression()..addNullCheck = true,
-      bRef: NodeChangeForExpression()..addNullCheck = true,
-      findNode.binary('a + b'): NodeChangeForExpression()..addNullCheck = true
+      aRef: NodeChangeForExpression()..addNullCheck(_MockInfo()),
+      bRef: NodeChangeForExpression()..addNullCheck(_MockInfo()),
+      findNode.binary('a + b'): NodeChangeForExpression()
+        ..addNullCheck(_MockInfo())
     });
     expect(previewInfo.applyTo(code), 'f(a, b) => (a! + b!)!;');
   }
@@ -53,7 +54,7 @@ f(int i, int/*?*/ j) {
       findNode.statement('if'): NodeChangeForIfStatement()
         ..conditionValue = true,
       findNode.simple('j.isEven'): NodeChangeForExpression()
-        ..addNullCheck = true
+        ..addNullCheck(_MockInfo())
     });
     expect(previewInfo.applyTo(code), '''
 f(int i, int/*?*/ j) {
@@ -74,7 +75,7 @@ f(int i, int/*?*/ j) {
       findNode.statement('if'): NodeChangeForIfStatement()
         ..conditionValue = true,
       findNode.simple('j.isEven'): NodeChangeForExpression()
-        ..addNullCheck = true
+        ..addNullCheck(_MockInfo())
     });
     expect(previewInfo.applyTo(code), '''
 f(int i, int/*?*/ j) {
@@ -327,11 +328,8 @@ void f(int i, String callback()) {
     // leave them.
     await analyze('f(a, c) => a..b = (throw c..d);');
     var cd = findNode.cascade('c..d');
-    var previewInfo = run({
-      cd: NodeChangeForExpression()
-        ..introduceAsType = 'int'
-        ..introduceAsInfo = _MockInfo()
-    });
+    var previewInfo =
+        run({cd: NodeChangeForExpression()..introduceAs('int', _MockInfo())});
     expect(
         previewInfo.applyTo(code), 'f(a, c) => a..b = (throw (c..d) as int);');
   }
@@ -339,22 +337,16 @@ void f(int i, String callback()) {
   Future<void> test_introduceAs_no_parens() async {
     await analyze('f(a, b) => a | b;');
     var expr = findNode.binary('a | b');
-    var previewInfo = run({
-      expr: NodeChangeForExpression()
-        ..introduceAsType = 'int'
-        ..introduceAsInfo = _MockInfo()
-    });
+    var previewInfo =
+        run({expr: NodeChangeForExpression()..introduceAs('int', _MockInfo())});
     expect(previewInfo.applyTo(code), 'f(a, b) => a | b as int;');
   }
 
   Future<void> test_introduceAs_parens() async {
     await analyze('f(a, b) => a < b;');
     var expr = findNode.binary('a < b');
-    var previewInfo = run({
-      expr: NodeChangeForExpression()
-        ..introduceAsType = 'bool'
-        ..introduceAsInfo = _MockInfo()
-    });
+    var previewInfo = run(
+        {expr: NodeChangeForExpression()..introduceAs('bool', _MockInfo())});
     expect(previewInfo.applyTo(code), 'f(a, b) => (a < b) as bool;');
   }
 
@@ -380,7 +372,7 @@ void f(int i, String callback()) {
     await analyze('f(a) => a++;');
     var expr = findNode.postfix('a++');
     var previewInfo =
-        run({expr: NodeChangeForExpression()..addNullCheck = true});
+        run({expr: NodeChangeForExpression()..addNullCheck(_MockInfo())});
     expect(previewInfo.applyTo(code), 'f(a) => a++!;');
   }
 
@@ -388,7 +380,7 @@ void f(int i, String callback()) {
     await analyze('f(a) => -a;');
     var expr = findNode.prefix('-a');
     var previewInfo =
-        run({expr: NodeChangeForExpression()..addNullCheck = true});
+        run({expr: NodeChangeForExpression()..addNullCheck(_MockInfo())});
     expect(previewInfo.applyTo(code), 'f(a) => (-a)!;');
   }
 
@@ -524,7 +516,7 @@ void f(int i, String callback()) {
     var previewInfo = run({
       methodInvocation: NodeChangeForMethodInvocation()
         ..removeNullAwareness = true,
-      argument: NodeChangeForExpression()..addNullCheck = true
+      argument: NodeChangeForExpression()..addNullCheck(_MockInfo())
     });
     expect(previewInfo.applyTo(code), 'f(x) => x.m(x!);');
   }
@@ -658,7 +650,7 @@ f({required int x}) {}
             ..addExplicitType =
                 MockDartType(toStringValueWithNullability: 'int'),
       findNode.integerLiteral('0'): NodeChangeForExpression()
-        ..addNullCheck = true
+        ..addNullCheck(_MockInfo())
     });
     expect(previewInfo.applyTo(code), 'int x = 0!;');
   }

pkg/nnbd_migration/test/fix_builder_test.dart

@@ -47,7 +47,7 @@ class FixBuilderTest extends EdgeBuilderTestBase {
       .having((c) => c.makeNullable, 'makeNullable', isNotNull);
 
   static final isNullCheck = TypeMatcher<NodeChangeForExpression>()
-      .having((c) => c.addNullCheck, 'addNullCheck', true);
+      .having((c) => c.addsNullCheck, 'addsNullCheck', true);
 
   static final isRemoveNullAwareness =
       TypeMatcher<NodeChangeForPropertyAccess>()
@@ -2292,7 +2292,7 @@ int/*!*/ f(C/*!*/ c) => c?.i;
     var propertyAccess = findNode.propertyAccess('?.');
     visitSubexpression(propertyAccess, 'int', changes: {
       propertyAccess: TypeMatcher<NodeChangeForPropertyAccess>()
-          .having((c) => c.addNullCheck, 'addNullCheck', true)
+          .having((c) => c.addsNullCheck, 'addsNullCheck', true)
           .having((c) => c.removeNullAwareness, 'removeNullAwareness', true)
     });
   }