[flow analysis] Re-work handling of CFE null-aware guards.

The CFE desugars null-aware expressions such as `a?.b` into equivalent
`let` expressions involving a temporary guard variable (e.g., `let x =
a in x == null ? null : x.b`). This desugaring happens in the body
builder, prior to flow analysis. As a result, the expressions and
variables passed to flow analysis are those of the desugared `let`
expression rather than those of the code the user wrote. So a hack is
needed to ensure that flow analysis produces the correct result; the
hack involves promoting the temporary guard variable in the code path
where it is not null.

Prior to this change, the hack was done entirely in the CFE. Instead
of making a single pair of calls to
`FlowAnalysis.nullAwareAccess_rightBegin` and
`FlowAnalysis.nullAwareAccess_end` for every null-aware expression,
the CFE made two nested pairs of calls: one to promote the true target
of the null-aware access, and the other to promote the temporary guard
variable.

This had the advantage of keeping the hack entirely in the CFE, but
unfortunately it got in the way of fixing
dart-lang/language#4344, because it
prevented flow analysis from properly recognizing field accesses
inside null-aware expressions.

This change adds an optional `guardVariable` parameter to
`FlowAnalysis_nullAwareAccess_rightBegin`, and shifts the
responsibility for promoting the temporary guard variable to the flow
analysis engine itself. With this change, the CFE no longer needs to
make two nested pairs of calls to
`FlowAnalysis.nullAwareAccess_rightBegin` and
`FlowAnalysis.nullAwareAccess_end`, and flow analysis now has the
necessary information to recognize field accesses inside null-aware
expressions.

I will perform the actual fix for
dart-lang/language#4344 in a follow-up CL.

Note that in the future, I would like to change the CFE so that it
desugars null-aware accesses during type inference rather than during
the body builder; this will allow the expressions and variables passed
to flow analysis to be precisely those of the code the user wrote;
that in turn will give us extra confidence that flow analysis produces
the same results in the analyzer and the CFE.

Change-Id: Ie63619a3ca0f011c1ae8e3c0c76b5bc7c1ab8419
Bug: dart-lang/language#4344
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/427341
Reviewed-by: Johnni Winther <[email protected]>
Reviewed-by: Chloe Stefantsova <[email protected]>
Commit-Queue: Paul Berry <[email protected]>

Author: stereotype441

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

@@ -206,7 +206,7 @@ abstract class FlowAnalysis<
   Variable extends Object,
   Type extends Object
 >
-    implements FlowAnalysisNullShortingInterface<Expression, Type> {
+    implements FlowAnalysisNullShortingInterface<Expression, Variable, Type> {
   factory FlowAnalysis(
     FlowAnalysisOperations<Variable, Type> operations,
     AssignedVariables<Node, Variable> assignedVariables, {
@@ -1952,10 +1952,19 @@ class FlowAnalysisDebug<
   }
 
   @override
-  void nullAwareAccess_rightBegin(Expression? target, Type targetType) {
+  void nullAwareAccess_rightBegin(
+    Expression? target,
+    Type targetType, {
+    Variable? guardVariable,
+  }) {
     _wrap(
-      'nullAwareAccess_rightBegin($target, $targetType)',
-      () => _wrapped.nullAwareAccess_rightBegin(target, targetType),
+      'nullAwareAccess_rightBegin($target, $targetType, '
+      'guardVariable: $guardVariable)',
+      () => _wrapped.nullAwareAccess_rightBegin(
+        target,
+        targetType,
+        guardVariable: guardVariable,
+      ),
     );
   }
 
@@ -2493,6 +2502,7 @@ class FlowAnalysisDebug<
 /// relevant to it.
 abstract interface class FlowAnalysisNullShortingInterface<
   Expression extends Object,
+  Variable extends Object,
   Type extends Object
 > {
   /// Call this method after visiting an expression using `?.`.
@@ -2508,12 +2518,22 @@ abstract interface class FlowAnalysisNullShortingInterface<
   /// null-aware operator, and should be non-null even if the null-aware access
   /// starts a cascade section.
   ///
+  /// If the client desugars the null-aware access using a guard variable (e.g.,
+  /// if it desugars `a?.b` into `let x = a in x == null ? null : x.b`), it
+  /// should pass in the variable used for desugaring as [guardVariable]. Flow
+  /// analysis will ensure that this variable is promoted to the appropriate
+  /// type in the "not null" code path.
+  ///
   /// Note that [nullAwareAccess_end] should be called after the conclusion
   /// of any null-shorting that is caused by the `?.`.  So, for example, if the
   /// code being analyzed is `x?.y?.z(x)`, [nullAwareAccess_rightBegin] should
   /// be called once upon reaching each `?.`, but [nullAwareAccess_end] should
   /// not be called until after processing the method call to `z(x)`.
-  void nullAwareAccess_rightBegin(Expression? target, Type targetType);
+  void nullAwareAccess_rightBegin(
+    Expression? target,
+    Type targetType, {
+    Variable? guardVariable,
+  });
 }
 
 /// An instance of the [FlowModel] class represents the information gathered by
@@ -4793,7 +4813,8 @@ class TrivialVariableReference<Type extends Object> extends _Reference<Type> {
   @override
   String toString() =>
       'TrivialVariableReference(type: $_type, '
-      'promotionKey: $promotionKey, isThisOrSuper: $isThisOrSuper)';
+      'promotionKey: $promotionKey, isThisOrSuper: $isThisOrSuper, '
+      'ssaNode: $ssaNode)';
 }
 
 class WhyNotPromotedInfo {}
@@ -5908,7 +5929,11 @@ class _FlowAnalysisImpl<
   }
 
   @override
-  void nullAwareAccess_rightBegin(Expression? target, Type targetType) {
+  void nullAwareAccess_rightBegin(
+    Expression? target,
+    Type targetType, {
+    Variable? guardVariable,
+  }) {
     _current = _current.split();
     FlowModel<Type> shortcutControlPath = _current;
     _Reference<Type>? targetReference = _getExpressionReference(target);
@@ -5930,6 +5955,22 @@ class _FlowAnalysisImpl<
         break;
     }
     _stack.add(new _NullAwareAccessContext<Type>(shortcutControlPath));
+    if (guardVariable != null) {
+      // Promote the guard variable as well.
+      int promotionKey = promotionKeyStore.keyForVariable(guardVariable);
+      Type nonNullType = operations.promoteToNonNull(targetType);
+      _current = _current.updatePromotionInfo(
+        this,
+        promotionKey,
+        new PromotionModel(
+          promotedTypes: nonNullType == targetType ? null : [nonNullType],
+          tested: const [],
+          assigned: true,
+          unassigned: false,
+          ssaNode: new SsaNode(null),
+        ),
+      );
+    }
   }
 
   @override

pkg/_fe_analyzer_shared/lib/src/type_inference/null_shorting.dart

@@ -16,8 +16,13 @@ import '../flow_analysis/flow_analysis_operations.dart';
 /// The type parameter [Guard] should be instantiated with the data structure
 /// used by the client to desugar null-aware accesses. (The analyzer can
 /// instantiate this with `Null`, since it doesn't do desugaring.)
-mixin NullShortingMixin<Guard, Expression extends Object, Type extends Object>
-    on TypeAnalysisNullShortingInterface<Expression, Type> {
+mixin NullShortingMixin<
+  Guard,
+  Expression extends Object,
+  Variable extends Object,
+  Type extends Object
+>
+    on TypeAnalysisNullShortingInterface<Expression, Variable, Type> {
   /// Stack of [Guard] objects associated with null-shorting operations that
   /// haven't been terminated yet.
   final _guards = <Guard>[];
@@ -63,20 +68,36 @@ mixin NullShortingMixin<Guard, Expression extends Object, Type extends Object>
   /// desugar the null-aware access. It will be passed to
   /// [handleNullShortingStep] when the null shorting for this particular
   /// null-aware expression is terminated.
-  void startNullShorting(Guard guard, Expression target, Type targetType) {
+  ///
+  /// If the client desugars the null-aware access using a guard variable (e.g.,
+  /// if it desugars `a?.b` into `let x = a in x == null ? null : x.b`), it
+  /// should pass in the variable used for desugaring as [guardVariable]. Flow
+  /// analysis will ensure that this variable is promoted to the appropriate
+  /// type in the "not null" code path.
+  void startNullShorting(
+    Guard guard,
+    Expression target,
+    Type targetType, {
+    Variable? guardVariable,
+  }) {
     // Ensure the initializer of [_nullAwareVariable] is promoted to
     // non-nullable.
-    flow.nullAwareAccess_rightBegin(target, targetType);
+    flow.nullAwareAccess_rightBegin(
+      target,
+      targetType,
+      guardVariable: guardVariable,
+    );
     _guards.add(guard);
   }
 }
 
 abstract interface class TypeAnalysisNullShortingInterface<
   Expression extends Object,
+  Variable extends Object,
   Type extends Object
 > {
   /// Returns the client's [FlowAnalysis] object.
-  FlowAnalysisNullShortingInterface<Expression, Type> get flow;
+  FlowAnalysisNullShortingInterface<Expression, Variable, Type> get flow;
 
   /// Returns the number of null-shorting operations that haven't been
   /// terminated yet.

pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart

@@ -279,7 +279,12 @@ mixin TypeAnalyzer<
   TypeDeclarationType extends Object,
   TypeDeclaration extends Object
 >
-    implements TypeAnalysisNullShortingInterface<Expression, SharedTypeView> {
+    implements
+        TypeAnalysisNullShortingInterface<
+          Expression,
+          Variable,
+          SharedTypeView
+        > {
   /// Cached context types and their respective dot shorthand nodes.
   ///
   /// The [SharedTypeSchemaView] is used to resolve dot shorthand heads. We

pkg/_fe_analyzer_shared/test/mini_ast.dart

@@ -6245,7 +6245,7 @@ class _MiniAstTypeAnalyzer
           Type,
           String
         >,
-        NullShortingMixin<MiniIRTmp, Expression, SharedTypeView> {
+        NullShortingMixin<MiniIRTmp, Expression, Var, SharedTypeView> {
   final Harness _harness;
 
   @override

pkg/analyzer/lib/src/generated/resolver.dart

@@ -134,7 +134,12 @@ class ResolverVisitor extends ThrowingAstVisitor<void>
           InterfaceElementImpl2
         >,
         // TODO(paulberry): not yet used.
-        NullShortingMixin<Null, ExpressionImpl, SharedTypeView> {
+        NullShortingMixin<
+          Null,
+          ExpressionImpl,
+          PromotableElementImpl2,
+          SharedTypeView
+        > {
   /// Debug-only: if `true`, manipulations of [_rewriteStack] performed by
   /// [popRewrite], [pushRewrite], and [replaceExpression] will be printed.
   static const bool _debugRewriteStack = false;

pkg/front_end/lib/src/type_inference/inference_results.dart

@@ -389,8 +389,6 @@ class NullAwareGuard {
   ///
   Expression createExpression(
       DartType inferredType, Expression nullAwareAction) {
-    // End non-nullable promotion of the initializer of [_nullAwareVariable].
-    _inferrer.flowAnalysis.nullAwareAccess_end();
     Expression equalsNull = _inferrer.createEqualsNull(
         _nullAwareFileOffset, createVariableGet(_nullAwareVariable));
 

pkg/front_end/lib/src/type_inference/inference_visitor.dart

@@ -106,7 +106,8 @@ class InferenceVisitorImpl extends InferenceVisitorBase
     with
         TypeAnalyzer<TreeNode, Statement, Expression, VariableDeclaration,
             Pattern, InvalidExpression, TypeDeclarationType, TypeDeclaration>,
-        NullShortingMixin<NullAwareGuard, Expression, SharedTypeView>,
+        NullShortingMixin<NullAwareGuard, Expression, VariableDeclaration,
+            SharedTypeView>,
         StackChecker
     implements
         ExpressionVisitor1<ExpressionInferenceResult, DartType>,
@@ -245,14 +246,8 @@ class InferenceVisitorImpl extends InferenceVisitorBase
 
   void createNullAwareGuard(VariableDeclaration variable) {
     startNullShorting(new NullAwareGuard(variable, variable.fileOffset, this),
-        variable.initializer!, new SharedTypeView(variable.type));
-    // Ensure [variable] is promoted to non-nullable.
-    // TODO(johnniwinther): Avoid creating a [VariableGet] to promote the
-    // variable.
-    VariableGet read = new VariableGet(variable);
-    flowAnalysis.variableRead(read, variable);
-    flowAnalysis.nullAwareAccess_rightBegin(
-        read, new SharedTypeView(variable.type));
+        variable.initializer!, new SharedTypeView(variable.type),
+        guardVariable: variable);
   }
 
   @override
@@ -988,14 +983,8 @@ class InferenceVisitorImpl extends InferenceVisitorBase
       // Ensure the initializer of [_nullAwareVariable] is promoted to
       // non-nullable.
       flow.nullAwareAccess_rightBegin(
-          node.variable.initializer!, new SharedTypeView(node.variable.type));
-      // Ensure [variable] is promoted to non-nullable.
-      // TODO(johnniwinther): Avoid creating a [VariableGet] to promote the
-      // variable.
-      VariableGet read = new VariableGet(node.variable);
-      flowAnalysis.variableRead(read, node.variable);
-      flowAnalysis.nullAwareAccess_rightBegin(
-          read, new SharedTypeView(node.variable.type));
+          node.variable.initializer!, new SharedTypeView(node.variable.type),
+          guardVariable: node.variable);
     }
 
     Cascade? previousEnclosingCascade = _enclosingCascade;

pkg/front_end/lib/src/type_inference/type_inference_engine.dart

@@ -673,6 +673,8 @@ class OperationsCfe
       // use that if it exists.
       return new SharedTypeView(variable.lateType ?? variable.type);
     }
+    // TODO(paulberry): see if this code path can be eliminated.
+    // Coverage-ignore(suite): Not run.
     return new SharedTypeView(variable.type);
   }