[cfe] Ensure that shorting is stopped before expression usage

The creation of the shorting stop expression was done lazily
in NullAwareExpressionInferenceResult.expression. Since the shorting
calls flow analysis to stop null promotion doing this out of order
let to inconsistent flow analysis state.

This CL introduces an `inferNullAwareExpression` the allows to
caller to continue the shorting (if in an opt-in library). The
inferExpression method, used everywhere else, stops any shorting
currently in progress.

Closes flutter#43275

Change-Id: I448163e29bdb0de8e493e5c62aa0474d0c9a593f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/173729
Reviewed-by: Dmitry Stefantsov <[email protected]>
Commit-Queue: Johnni Winther <[email protected]>

Author: johnniwinther

pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart

@@ -2949,11 +2949,8 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
 
   @override
   void ifNullExpression_end() {
-    // TODO(paulberry): CFE sometimes calls ifNullExpression_end and
-    // nullAwareAccess_end out of order, so as a workaround we cast to the
-    // common base class.  See https://github.com/dart-lang/sdk/issues/43725.
-    _SimpleContext<Variable, Type> context =
-        _stack.removeLast() as _SimpleContext<Variable, Type>;
+    _IfNullExpressionContext<Variable, Type> context =
+        _stack.removeLast() as _IfNullExpressionContext<Variable, Type>;
     _current = _merge(_current, context._previous);
   }
 
@@ -3123,11 +3120,8 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
 
   @override
   void nullAwareAccess_end() {
-    // TODO(paulberry): CFE sometimes calls ifNullExpression_end and
-    // nullAwareAccess_end out of order, so as a workaround we cast to the
-    // common base class.  See https://github.com/dart-lang/sdk/issues/43725.
-    _SimpleContext<Variable, Type> context =
-        _stack.removeLast() as _SimpleContext<Variable, Type>;
+    _NullAwareAccessContext<Variable, Type> context =
+        _stack.removeLast() as _NullAwareAccessContext<Variable, Type>;
     _current = _merge(_current, context._previous);
   }
 

pkg/front_end/lib/src/fasta/kernel/inference_visitor.dart

@@ -1937,6 +1937,17 @@ class CompoundPropertySet extends InternalExpression {
   String toString() {
     return "CompoundPropertySet(${toStringInternal()})";
   }
+
+  @override
+  void toTextInternal(AstPrinter printer) {
+    printer.writeExpression(receiver);
+    printer.write('.');
+    printer.writeName(propertyName);
+    printer.write(' ');
+    printer.writeName(binaryName);
+    printer.write('= ');
+    printer.writeExpression(rhs);
+  }
 }
 
 /// Internal expression representing an compound property assignment.
@@ -2939,6 +2950,28 @@ class NullAwareCompoundSet extends InternalExpression {
   String toString() {
     return "NullAwareCompoundSet(${toStringInternal()})";
   }
+
+  @override
+  void toTextInternal(AstPrinter printer) {
+    printer.writeExpression(receiver);
+    printer.write('?.');
+    printer.writeName(propertyName);
+    if (forPostIncDec &&
+        rhs is IntLiteral &&
+        (rhs as IntLiteral).value == 1 &&
+        (binaryName == plusName || binaryName == minusName)) {
+      if (binaryName == plusName) {
+        printer.write('++');
+      } else {
+        printer.write('--');
+      }
+    } else {
+      printer.write(' ');
+      printer.writeName(binaryName);
+      printer.write('= ');
+      printer.writeExpression(rhs);
+    }
+  }
 }
 
 /// Internal expression representing an null-aware if-null property set.
@@ -3026,6 +3059,15 @@ class NullAwareIfNullSet extends InternalExpression {
   String toString() {
     return "NullAwareIfNullSet(${toStringInternal()})";
   }
+
+  @override
+  void toTextInternal(AstPrinter printer) {
+    printer.writeExpression(receiver);
+    printer.write('?.');
+    printer.writeName(name);
+    printer.write(' ??= ');
+    printer.writeExpression(value);
+  }
 }
 
 /// Internal expression representing a compound super index assignment.

pkg/front_end/lib/src/fasta/kernel/internal_ast.dart

@@ -1644,17 +1644,13 @@ class TypeInferrerImpl implements TypeInferrer {
       VariableDeclarationImpl variable) {
     assert(variable.isImplicitlyTyped);
     assert(variable.initializer != null);
-    ExpressionInferenceResult result = inferExpression(
+    ExpressionInferenceResult result = inferNullAwareExpression(
         variable.initializer, const UnknownType(), true,
         isVoidAllowed: true);
-    Link<NullAwareGuard> nullAwareGuards;
-    if (isNonNullableByDefault) {
-      variable.initializer = result.nullAwareAction..parent = variable;
-      nullAwareGuards = result.nullAwareGuards;
-    } else {
-      variable.initializer = result.expression..parent = variable;
-      nullAwareGuards = const Link<NullAwareGuard>();
-    }
+
+    Link<NullAwareGuard> nullAwareGuards = result.nullAwareGuards;
+    variable.initializer = result.nullAwareAction..parent = variable;
+
     DartType inferredType =
         inferDeclarationType(result.inferredType, forSyntheticVariable: true);
     instrumentation?.record(uriForInstrumentation, variable.fileOffset, 'type',
@@ -1713,7 +1709,7 @@ class TypeInferrerImpl implements TypeInferrer {
   ///
   /// Derived classes should override this method with logic that dispatches on
   /// the expression type and calls the appropriate specialized "infer" method.
-  ExpressionInferenceResult inferExpression(
+  ExpressionInferenceResult _inferExpression(
       Expression expression, DartType typeContext, bool typeNeeded,
       {bool isVoidAllowed: false, bool forEffect: false}) {
     registerIfUnreachableForTesting(expression);
@@ -1768,6 +1764,28 @@ class TypeInferrerImpl implements TypeInferrer {
     return result;
   }
 
+  ExpressionInferenceResult inferExpression(
+      Expression expression, DartType typeContext, bool typeNeeded,
+      {bool isVoidAllowed: false, bool forEffect: false}) {
+    ExpressionInferenceResult result = _inferExpression(
+        expression, typeContext, typeNeeded,
+        isVoidAllowed: isVoidAllowed, forEffect: forEffect);
+    return result.stopShorting();
+  }
+
+  ExpressionInferenceResult inferNullAwareExpression(
+      Expression expression, DartType typeContext, bool typeNeeded,
+      {bool isVoidAllowed: false, bool forEffect: false}) {
+    ExpressionInferenceResult result = _inferExpression(
+        expression, typeContext, typeNeeded,
+        isVoidAllowed: isVoidAllowed, forEffect: forEffect);
+    if (isNonNullableByDefault) {
+      return result;
+    } else {
+      return result.stopShorting();
+    }
+  }
+
   @override
   Expression inferFieldInitializer(
     InferenceHelper helper,
@@ -3965,6 +3983,8 @@ class ExpressionInferenceResult {
 
   DartType get nullAwareActionType => inferredType;
 
+  ExpressionInferenceResult stopShorting() => this;
+
   String toString() => 'ExpressionInferenceResult($inferredType,$expression)';
 }
 
@@ -4045,24 +4065,26 @@ class NullAwareExpressionInferenceResult implements ExpressionInferenceResult {
   @override
   final Expression nullAwareAction;
 
-  Expression _expression;
-
   NullAwareExpressionInferenceResult(this.inferredType,
       this.nullAwareActionType, this.nullAwareGuards, this.nullAwareAction)
       : assert(nullAwareGuards.isNotEmpty),
         assert(nullAwareAction != null);
 
   Expression get expression {
-    if (_expression == null) {
-      _expression = nullAwareAction;
-      Link<NullAwareGuard> nullAwareGuard = nullAwareGuards;
-      while (nullAwareGuard.isNotEmpty) {
-        _expression =
-            nullAwareGuard.head.createExpression(inferredType, _expression);
-        nullAwareGuard = nullAwareGuard.tail;
-      }
+    throw new UnsupportedError('Shorting must be explicitly stopped before'
+        'accessing the expression result of a '
+        'NullAwareExpressionInferenceResult');
+  }
+
+  ExpressionInferenceResult stopShorting() {
+    Expression expression = nullAwareAction;
+    Link<NullAwareGuard> nullAwareGuard = nullAwareGuards;
+    while (nullAwareGuard.isNotEmpty) {
+      expression =
+          nullAwareGuard.head.createExpression(inferredType, expression);
+      nullAwareGuard = nullAwareGuard.tail;
     }
-    return _expression;
+    return new ExpressionInferenceResult(inferredType, expression);
   }
 
   String toString() =>

pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart

@@ -1062,6 +1062,7 @@ stdlib
 stdout
 sticky
 stmt
+stopped
 str
 strategies
 streak

pkg/front_end/test/spell_checking_list_code.txt

@@ -581,7 +581,18 @@ foo ?? foo = 1''');
 
 void _testCompoundExtensionSet() {}
 
-void _testCompoundPropertySet() {}
+void _testCompoundPropertySet() {
+  testExpression(
+      new CompoundPropertySet(
+          new IntLiteral(0), new Name('foo'), new Name('+'), new IntLiteral(1),
+          readOffset: TreeNode.noOffset,
+          binaryOffset: TreeNode.noOffset,
+          writeOffset: TreeNode.noOffset,
+          readOnlyReceiver: false,
+          forEffect: false),
+      '''
+0.foo += 1''');
+}
 
 void _testPropertyPostIncDec() {}
 
@@ -607,9 +618,40 @@ void _testIfNullExtensionIndexSet() {}
 
 void _testCompoundIndexSet() {}
 
-void _testNullAwareCompoundSet() {}
+void _testNullAwareCompoundSet() {
+  testExpression(
+      new NullAwareCompoundSet(
+          new IntLiteral(0), new Name('foo'), new Name('+'), new IntLiteral(1),
+          readOffset: TreeNode.noOffset,
+          binaryOffset: TreeNode.noOffset,
+          writeOffset: TreeNode.noOffset,
+          forPostIncDec: false,
+          forEffect: false),
+      '''
+0?.foo += 1''');
+  testExpression(
+      new NullAwareCompoundSet(
+          new IntLiteral(0), new Name('foo'), new Name('+'), new IntLiteral(1),
+          readOffset: TreeNode.noOffset,
+          binaryOffset: TreeNode.noOffset,
+          writeOffset: TreeNode.noOffset,
+          forPostIncDec: true,
+          forEffect: false),
+      '''
+0?.foo++''');
+}
 
-void _testNullAwareIfNullSet() {}
+void _testNullAwareIfNullSet() {
+  testExpression(
+      new NullAwareIfNullSet(
+          new IntLiteral(0), new Name('foo'), new IntLiteral(1),
+          readOffset: TreeNode.noOffset,
+          testOffset: TreeNode.noOffset,
+          writeOffset: TreeNode.noOffset,
+          forEffect: false),
+      '''
+0?.foo ??= 1''');
+}
 
 void _testCompoundSuperIndexSet() {}
 

pkg/front_end/test/text_representation/internal_ast_text_representation_test.dart

@@ -1,3 +1,7 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// 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.
+
 class Class {
   final int length;
 

pkg/front_end/testcases/general/issue43290.dart

@@ -2,45 +2,45 @@ library;
 //
 // Problems in library:
 //
-// pkg/front_end/testcases/general/issue43290.dart:11:25: Error: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:15:25: Error: Not a constant expression.
 //     const Class(length: length);
 //                         ^^^^^^
 //
-// pkg/front_end/testcases/general/issue43290.dart:19:15: Error: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:23:15: Error: Not a constant expression.
 //     const a = length;
 //               ^^^^^^
 //
-// pkg/front_end/testcases/general/issue43290.dart:7:11: Error: Constant evaluation error:
+// pkg/front_end/testcases/general/issue43290.dart:11:11: Error: Constant evaluation error:
 //     const Class(length: this.length);
 //           ^
-// pkg/front_end/testcases/general/issue43290.dart:7:30: Context: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:11:30: Context: Not a constant expression.
 //     const Class(length: this.length);
 //                              ^
 //
-// pkg/front_end/testcases/general/issue43290.dart:11:11: Error: Constant evaluation error:
+// pkg/front_end/testcases/general/issue43290.dart:15:11: Error: Constant evaluation error:
 //     const Class(length: length);
 //           ^
-// pkg/front_end/testcases/general/issue43290.dart:11:25: Context: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:15:25: Context: Not a constant expression.
 //     const Class(length: length);
 //                         ^
 //
-// pkg/front_end/testcases/general/issue43290.dart:15:20: Error: Constant evaluation error:
+// pkg/front_end/testcases/general/issue43290.dart:19:20: Error: Constant evaluation error:
 //     const a = this.length;
 //                    ^
-// pkg/front_end/testcases/general/issue43290.dart:15:20: Context: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:19:20: Context: Not a constant expression.
 //     const a = this.length;
 //                    ^
-// pkg/front_end/testcases/general/issue43290.dart:15:11: Context: While analyzing:
+// pkg/front_end/testcases/general/issue43290.dart:19:11: Context: While analyzing:
 //     const a = this.length;
 //           ^
 //
-// pkg/front_end/testcases/general/issue43290.dart:19:15: Error: Constant evaluation error:
+// pkg/front_end/testcases/general/issue43290.dart:23:15: Error: Constant evaluation error:
 //     const a = length;
 //               ^
-// pkg/front_end/testcases/general/issue43290.dart:19:15: Context: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:23:15: Context: Not a constant expression.
 //     const a = length;
 //               ^
-// pkg/front_end/testcases/general/issue43290.dart:19:11: Context: While analyzing:
+// pkg/front_end/testcases/general/issue43290.dart:23:11: Context: While analyzing:
 //     const a = length;
 //           ^
 //

pkg/front_end/testcases/general/issue43290.dart.strong.expect

@@ -2,45 +2,45 @@ library;
 //
 // Problems in library:
 //
-// pkg/front_end/testcases/general/issue43290.dart:11:25: Error: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:15:25: Error: Not a constant expression.
 //     const Class(length: length);
 //                         ^^^^^^
 //
-// pkg/front_end/testcases/general/issue43290.dart:19:15: Error: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:23:15: Error: Not a constant expression.
 //     const a = length;
 //               ^^^^^^
 //
-// pkg/front_end/testcases/general/issue43290.dart:7:11: Error: Constant evaluation error:
+// pkg/front_end/testcases/general/issue43290.dart:11:11: Error: Constant evaluation error:
 //     const Class(length: this.length);
 //           ^
-// pkg/front_end/testcases/general/issue43290.dart:7:30: Context: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:11:30: Context: Not a constant expression.
 //     const Class(length: this.length);
 //                              ^
 //
-// pkg/front_end/testcases/general/issue43290.dart:11:11: Error: Constant evaluation error:
+// pkg/front_end/testcases/general/issue43290.dart:15:11: Error: Constant evaluation error:
 //     const Class(length: length);
 //           ^
-// pkg/front_end/testcases/general/issue43290.dart:11:25: Context: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:15:25: Context: Not a constant expression.
 //     const Class(length: length);
 //                         ^
 //
-// pkg/front_end/testcases/general/issue43290.dart:15:20: Error: Constant evaluation error:
+// pkg/front_end/testcases/general/issue43290.dart:19:20: Error: Constant evaluation error:
 //     const a = this.length;
 //                    ^
-// pkg/front_end/testcases/general/issue43290.dart:15:20: Context: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:19:20: Context: Not a constant expression.
 //     const a = this.length;
 //                    ^
-// pkg/front_end/testcases/general/issue43290.dart:15:11: Context: While analyzing:
+// pkg/front_end/testcases/general/issue43290.dart:19:11: Context: While analyzing:
 //     const a = this.length;
 //           ^
 //
-// pkg/front_end/testcases/general/issue43290.dart:19:15: Error: Constant evaluation error:
+// pkg/front_end/testcases/general/issue43290.dart:23:15: Error: Constant evaluation error:
 //     const a = length;
 //               ^
-// pkg/front_end/testcases/general/issue43290.dart:19:15: Context: Not a constant expression.
+// pkg/front_end/testcases/general/issue43290.dart:23:15: Context: Not a constant expression.
 //     const a = length;
 //               ^
-// pkg/front_end/testcases/general/issue43290.dart:19:11: Context: While analyzing:
+// pkg/front_end/testcases/general/issue43290.dart:23:11: Context: While analyzing:
 //     const a = length;
 //           ^
 //