Flow analysis: don't capture type information in equalityOperand_end.

There are two pieces of information flow analysis needs to know about
an equality test operand (i.e., an operands of `==`, `!=`, or
`identical()`):

- Their static types. This is used for reachability (e.g., flow
  analysis knows that if `f()` has type `Null`, then the body of `if
  (f() != null)` is unreachable).

- Whether they take the form of a null literal or a reference to
  something promotable. This is used to determine when an `if` test
  should promote a something to a non-nullable type.

Previous to this change, both pieces of information were captured by
`FlowAnalysis.equalityOperand_end` into an `ExpressionInfo` object,
and then those objects were passed into
`FlowAnalysis.equalityOperation_end`.

With this change, the client is now responsible for passing the static
types of the operands as separate arguments to
`FlowAnalysis.equalityOperation_end`, and the only information
captured by `equalityOperand_end` is whether the operand is a null
literal or a reference to something promotable.

This has two advantages:

- It avoids unnecessary allocations when analyzing code that doesn't
  have flow analysis consequences, since flow analysis no longer needs
  to allocate an `ExpressionInfo` for every equality test operand; it
  only has to allocate them for null literals and references to things
  that are promotable (which is a much smaller number of allocations).

- It means that `FlowAnalysis.equalityOperation_end` no longer needs
  to use the `type` field of `ExpressionInfo`. This helps build toward
  an eventual goal I have of removing this field, so that
  `ExpressionInfo` will simply be a container for a pair of flow
  models (one representing the flow state if the expression is `true`,
  one representing the flow state if the expression is `false`). I
  believe this will make flow analysis easier to reason about, and
  will help build toward a long term goal of cleaning up bugs in the
  "why not promoted" logic.

Making this change required adding a little bit of plumbing to the
analyzer, so that when analyzing an invocation of `identical`, it
keeps track of both the `ExpressionInfo` and the static type of the
operands; previously it just had to keep track of an `ExpressionInfo`
for each operand. The performance impact of this additional tracking
should be negligible, since this tracking doesn't happen for
invocations of anything other than `identical`.

Change-Id: I3e5473af095f3c8a747e9f527d7e14a21269dc95
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/389361
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Kallen Tu <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>

Author: stereotype441

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

@@ -405,20 +405,21 @@ abstract class FlowAnalysis<Node extends Object, Statement extends Node,
   ///
   /// Returns information about the expression that will later be needed by
   /// [equalityOperation_end].
-  ///
-  /// Note: the return type is nullable because legacy type promotion doesn't
-  /// need to record information about equality operands.
-  ExpressionInfo<Type>? equalityOperand_end(Expression operand, Type type);
+  ExpressionInfo<Type>? equalityOperand_end(Expression operand);
 
   /// Call this method just after visiting the operands of a binary `==` or `!=`
   /// expression, or an invocation of `identical`.
   ///
   /// [leftOperandInfo] and [rightOperandInfo] should be the values returned by
-  /// [equalityOperand_end].
+  /// [equalityOperand_end] for the left and right operands. [leftOperandType]
+  /// and [rightOperandType] should be the static types of the left and right
+  /// operands.
   void equalityOperation_end(
       Expression wholeExpression,
       ExpressionInfo<Type>? leftOperandInfo,
+      Type leftOperandType,
       ExpressionInfo<Type>? rightOperandInfo,
+      Type rightOperandType,
       {bool notEqual = false});
 
   /// Call this method after processing a relational pattern that uses an
@@ -1412,22 +1413,25 @@ class FlowAnalysisDebug<Node extends Object, Statement extends Node,
   }
 
   @override
-  ExpressionInfo<Type>? equalityOperand_end(Expression operand, Type type) =>
-      _wrap('equalityOperand_end($operand, $type)',
-          () => _wrapped.equalityOperand_end(operand, type),
-          isQuery: true);
+  ExpressionInfo<Type>? equalityOperand_end(Expression operand) => _wrap(
+      'equalityOperand_end($operand)',
+      () => _wrapped.equalityOperand_end(operand),
+      isQuery: true);
 
   @override
   void equalityOperation_end(
       Expression wholeExpression,
       ExpressionInfo<Type>? leftOperandInfo,
+      Type leftOperandType,
       ExpressionInfo<Type>? rightOperandInfo,
+      Type rightOperandType,
       {bool notEqual = false}) {
     _wrap(
         'equalityOperation_end($wholeExpression, $leftOperandInfo, '
-        '$rightOperandInfo, notEqual: $notEqual)',
-        () => _wrapped.equalityOperation_end(
-            wholeExpression, leftOperandInfo, rightOperandInfo,
+        '$leftOperandType, $rightOperandInfo, $rightOperandType, notEqual: '
+        '$notEqual)',
+        () => _wrapped.equalityOperation_end(wholeExpression, leftOperandInfo,
+            leftOperandType, rightOperandInfo, rightOperandType,
             notEqual: notEqual));
   }
 
@@ -4689,23 +4693,24 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
   }
 
   @override
-  ExpressionInfo<Type> equalityOperand_end(Expression operand, Type type) =>
-      _getExpressionInfo(operand) ??
-      new ExpressionInfo<Type>.trivial(model: _current, type: type);
+  ExpressionInfo<Type>? equalityOperand_end(Expression operand) =>
+      _getExpressionInfo(operand);
 
   @override
   void equalityOperation_end(
       Expression wholeExpression,
       ExpressionInfo<Type>? leftOperandInfo,
+      Type leftOperandType,
       ExpressionInfo<Type>? rightOperandInfo,
+      Type rightOperandType,
       {bool notEqual = false}) {
     // Note: leftOperandInfo and rightOperandInfo are nullable in the base class
     // to account for the fact that legacy type promotion doesn't record
     // information about legacy operands.  But since we are currently in full
     // (post null safety) flow analysis logic, we can safely assume that they
     // are not null.
-    _EqualityCheckResult equalityCheckResult =
-        _equalityCheck(leftOperandInfo!, rightOperandInfo!);
+    _EqualityCheckResult equalityCheckResult = _equalityCheck(
+        leftOperandInfo, leftOperandType, rightOperandInfo, rightOperandType);
     if (equalityCheckResult is _GuaranteedEqual) {
       // Both operands are known by flow analysis to compare equal, so the whole
       // expression behaves equivalently to a boolean (either `true` or `false`
@@ -5760,13 +5765,13 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
   }
 
   /// Analyzes an equality check between the operands described by
-  /// [lhsInfo] and [rhsInfo].
-  _EqualityCheckResult _equalityCheck(
-      ExpressionInfo<Type> lhsInfo, ExpressionInfo<Type> rhsInfo) {
+  /// [lhsInfo] and [rhsInfo], having static types [lhsType] and [rhsType].
+  _EqualityCheckResult _equalityCheck(ExpressionInfo<Type>? lhsInfo,
+      Type lhsType, ExpressionInfo<Type>? rhsInfo, Type rhsType) {
     TypeClassification leftOperandTypeClassification =
-        operations.classifyType(lhsInfo._type);
+        operations.classifyType(lhsType);
     TypeClassification rightOperandTypeClassification =
-        operations.classifyType(rhsInfo._type);
+        operations.classifyType(rhsType);
     if (leftOperandTypeClassification == TypeClassification.nullOrEquivalent &&
         rightOperandTypeClassification == TypeClassification.nullOrEquivalent) {
       return const _GuaranteedEqual();
@@ -5781,11 +5786,11 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
       // analysis behavior to depend on mode, so we conservatively assume that
       // either result is possible.
       return const _NoEqualityInformation();
-    } else if (lhsInfo.isNull) {
+    } else if (lhsInfo != null && lhsInfo.isNull) {
       return new _EqualityCheckIsNullCheck(
           rhsInfo is _Reference<Type> ? rhsInfo : null,
           isReferenceOnRight: true);
-    } else if (rhsInfo.isNull) {
+    } else if (rhsInfo != null && rhsInfo.isNull) {
       return new _EqualityCheckIsNullCheck(
           lhsInfo is _Reference<Type> ? lhsInfo : null,
           isReferenceOnRight: false);
@@ -5967,8 +5972,8 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
     _Reference<Type> newReference = context
         .createReference(matchedValueType, _current)
         .addPreviousInfo(context._matchedValueInfo, this, _current);
-    _EqualityCheckResult equalityCheckResult =
-        _equalityCheck(newReference, equalityOperand_end(operand, operandType));
+    _EqualityCheckResult equalityCheckResult = _equalityCheck(newReference,
+        matchedValueType, _getExpressionInfo(operand), operandType);
     if (equalityCheckResult is _NoEqualityInformation) {
       // We have no information so we have to assume the pattern might or
       // might not match.
@@ -6560,14 +6565,15 @@ class _LegacyTypePromotion<Node extends Object, Statement extends Node,
   void doStatement_end(Expression condition) {}
 
   @override
-  ExpressionInfo<Type>? equalityOperand_end(Expression operand, Type type) =>
-      null;
+  ExpressionInfo<Type>? equalityOperand_end(Expression operand) => null;
 
   @override
   void equalityOperation_end(
       Expression wholeExpression,
       ExpressionInfo<Type>? leftOperandInfo,
+      Type leftOperandType,
       ExpressionInfo<Type>? rightOperandInfo,
+      Type rightOperandType,
       {bool notEqual = false}) {}
 
   @override

pkg/_fe_analyzer_shared/test/mini_ast.dart

@@ -5529,14 +5529,14 @@ class _MiniAstTypeAnalyzer
     var leftType = analyzeExpression(lhs, operations.unknownType);
     ExpressionInfo<SharedTypeView<Type>>? leftInfo;
     if (isEquals) {
-      leftInfo = flow.equalityOperand_end(lhs, leftType);
+      leftInfo = flow.equalityOperand_end(lhs);
     } else if (isLogical) {
       flow.logicalBinaryOp_rightBegin(lhs, node, isAnd: isAnd);
     }
     var rightType = analyzeExpression(rhs, operations.unknownType);
     if (isEquals) {
       flow.equalityOperation_end(
-          node, leftInfo, flow.equalityOperand_end(rhs, rightType),
+          node, leftInfo, leftType, flow.equalityOperand_end(rhs), rightType,
           notEqual: isNot);
     } else if (isLogical) {
       flow.logicalBinaryOp_end(node, rhs, isAnd: isAnd);

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

@@ -99,8 +99,7 @@ class BinaryExpressionResolver {
     ExpressionInfo<SharedTypeView<DartType>>? leftInfo;
     var leftExtensionOverride = left is ExtensionOverride;
     if (!leftExtensionOverride) {
-      leftInfo =
-          flow?.equalityOperand_end(left, SharedTypeView(left.typeOrThrow));
+      leftInfo = flow?.equalityOperand_end(left);
     }
 
     _resolver.analyzeExpression(
@@ -109,8 +108,12 @@ class BinaryExpressionResolver {
     var whyNotPromoted = flowAnalysis.flow?.whyNotPromoted(right);
 
     if (!leftExtensionOverride) {
-      flow?.equalityOperation_end(node, leftInfo,
-          flow.equalityOperand_end(right, SharedTypeView(right.typeOrThrow)),
+      flow?.equalityOperation_end(
+          node,
+          leftInfo,
+          SharedTypeView(left.typeOrThrow),
+          flow.equalityOperand_end(right),
+          SharedTypeView(right.typeOrThrow),
           notEqual: notEqual);
     }
 

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

@@ -241,12 +241,12 @@ abstract class FullInvocationInferrer<Node extends AstNodeImpl>
           rawType.typeFormals, inferrer.choosePreliminaryTypes());
     }
 
-    List<ExpressionInfo<SharedTypeView<DartType>>?>? identicalInfo =
+    List<_IdenticalArgumentInfo?>? identicalArgumentInfo =
         _isIdentical ? [] : null;
     var parameterMap = _computeParameterMap(rawType?.parameters ?? const []);
     var deferredFunctionLiterals = _visitArguments(
         parameterMap: parameterMap,
-        identicalInfo: identicalInfo,
+        identicalArgumentInfo: identicalArgumentInfo,
         substitution: substitution,
         inferrer: inferrer);
     if (deferredFunctionLiterals != null) {
@@ -264,7 +264,7 @@ abstract class FullInvocationInferrer<Node extends AstNodeImpl>
         }
         _resolveDeferredFunctionLiterals(
             deferredFunctionLiterals: stage,
-            identicalInfo: identicalInfo,
+            identicalArgumentInfo: identicalArgumentInfo,
             substitution: substitution,
             inferrer: inferrer);
         isFirstStage = false;
@@ -289,7 +289,7 @@ abstract class FullInvocationInferrer<Node extends AstNodeImpl>
     }
     var returnType = _refineReturnType(
         InvocationInferrer.computeInvokeReturnType(invokeType));
-    _recordIdenticalInfo(identicalInfo);
+    _recordIdenticalArgumentInfo(identicalArgumentInfo);
     return returnType;
   }
 
@@ -472,19 +472,25 @@ class InvocationInferrer<Node extends AstNodeImpl> {
 
   /// If the invocation being processed is a call to `identical`, informs flow
   /// analysis about it, so that it can do appropriate promotions.
-  void _recordIdenticalInfo(
-      List<ExpressionInfo<SharedTypeView<DartType>>?>? identicalInfo) {
+  void _recordIdenticalArgumentInfo(
+      List<_IdenticalArgumentInfo?>? identicalArgumentInfo) {
     var flow = resolver.flowAnalysis.flow;
-    if (identicalInfo != null) {
-      flow?.equalityOperation_end(argumentList.parent as Expression,
-          identicalInfo[0], identicalInfo[1]);
+    if (identicalArgumentInfo != null) {
+      var leftOperandInfo = identicalArgumentInfo[0]!;
+      var rightOperandInfo = identicalArgumentInfo[1]!;
+      flow?.equalityOperation_end(
+          argumentList.parent as Expression,
+          leftOperandInfo.expressionInfo,
+          SharedTypeView<DartType>(leftOperandInfo.staticType),
+          rightOperandInfo.expressionInfo,
+          SharedTypeView<DartType>(rightOperandInfo.staticType));
     }
   }
 
   /// Resolves any function literals that were deferred by [_visitArguments].
   void _resolveDeferredFunctionLiterals(
       {required List<_DeferredParamInfo> deferredFunctionLiterals,
-      List<ExpressionInfo<SharedTypeView<DartType>>?>? identicalInfo,
+      List<_IdenticalArgumentInfo?>? identicalArgumentInfo,
       Substitution? substitution,
       GenericInferrer? inferrer}) {
     var flow = resolver.flowAnalysis.flow;
@@ -506,8 +512,9 @@ class InvocationInferrer<Node extends AstNodeImpl> {
           argument, SharedTypeSchemaView(parameterContextType));
       argument = resolver.popRewrite()!;
       if (flow != null) {
-        identicalInfo?[deferredArgument.index] = flow.equalityOperand_end(
-            argument, SharedTypeView(argument.typeOrThrow));
+        identicalArgumentInfo?[deferredArgument.index] = _IdenticalArgumentInfo(
+            expressionInfo: flow.equalityOperand_end(argument),
+            staticType: argument.typeOrThrow);
       }
       if (parameter != null) {
         inferrer?.constrainArgument(
@@ -522,7 +529,7 @@ class InvocationInferrer<Node extends AstNodeImpl> {
   /// returned.
   List<_DeferredParamInfo>? _visitArguments(
       {required Map<Object, ParameterElement> parameterMap,
-      List<ExpressionInfo<SharedTypeView<DartType>>?>? identicalInfo,
+      List<_IdenticalArgumentInfo?>? identicalArgumentInfo,
       Substitution? substitution,
       GenericInferrer? inferrer}) {
     assert(whyNotPromotedList.isEmpty);
@@ -549,7 +556,7 @@ class InvocationInferrer<Node extends AstNodeImpl> {
           value is FunctionExpressionImpl) {
         (deferredFunctionLiterals ??= [])
             .add(_DeferredParamInfo(parameter, value, i, parameterKey));
-        identicalInfo?.add(null);
+        identicalArgumentInfo?.add(null);
         // The "why not promoted" list isn't really relevant for function
         // literals because promoting a function literal doesn't even make
         // sense.  So we store an innocuous value in the list.
@@ -569,8 +576,9 @@ class InvocationInferrer<Node extends AstNodeImpl> {
             argument, SharedTypeSchemaView(parameterContextType));
         argument = resolver.popRewrite()!;
         if (flow != null) {
-          identicalInfo?.add(flow.equalityOperand_end(
-              argument, SharedTypeView(argument.typeOrThrow)));
+          identicalArgumentInfo?.add(_IdenticalArgumentInfo(
+              expressionInfo: flow.equalityOperand_end(argument),
+              staticType: argument.typeOrThrow));
           whyNotPromotedList.add(flow.whyNotPromoted(argument));
         }
         if (parameter != null) {
@@ -712,6 +720,20 @@ class _FunctionLiteralDependencies extends FunctionLiteralDependencies<
   }
 }
 
+/// Information tracked by [InvocationInferrer] about an argument passed to the
+/// `identical` function in `dart:core`.
+class _IdenticalArgumentInfo {
+  /// The [ExpressionInfo] returned by [FlowAnalysis.equalityOperand_end] for
+  /// the argument.
+  final ExpressionInfo<SharedTypeView<DartType>>? expressionInfo;
+
+  /// The static type of the argument.
+  final DartType staticType;
+
+  _IdenticalArgumentInfo(
+      {required this.expressionInfo, required this.staticType});
+}
+
 /// Information about an invocation argument that may or may not have already
 /// been resolved, as part of the deferred resolution mechanism for the
 /// `inference-update-1` feature.

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

@@ -6744,7 +6744,7 @@ class InferenceVisitorImpl extends InferenceVisitorBase
       int fileOffset, Expression left, DartType leftType, Expression right,
       {required bool isNot}) {
     ExpressionInfo<SharedTypeView<DartType>>? equalityInfo =
-        flowAnalysis.equalityOperand_end(left, new SharedTypeView(leftType));
+        flowAnalysis.equalityOperand_end(left);
 
     Expression? equals;
     ExpressionInferenceResult rightResult =
@@ -6762,8 +6762,9 @@ class InferenceVisitorImpl extends InferenceVisitorBase
       flowAnalysis.equalityOperation_end(
           equals,
           equalityInfo,
-          flowAnalysis.equalityOperand_end(rightResult.expression,
-              new SharedTypeView(rightResult.inferredType)),
+          new SharedTypeView(leftType),
+          flowAnalysis.equalityOperand_end(rightResult.expression),
+          new SharedTypeView(rightResult.inferredType),
           notEqual: isNot);
       return new ExpressionInferenceResult(
           coreTypes.boolRawType(Nullability.nonNullable), equals);
@@ -6811,8 +6812,9 @@ class InferenceVisitorImpl extends InferenceVisitorBase
     flowAnalysis.equalityOperation_end(
         equals,
         equalityInfo,
-        flowAnalysis.equalityOperand_end(
-            right, new SharedTypeView(rightResult.inferredType)),
+        new SharedTypeView(leftType),
+        flowAnalysis.equalityOperand_end(right),
+        new SharedTypeView(rightResult.inferredType),
         notEqual: isNot);
     return new ExpressionInferenceResult(
         equalsTarget.isNever

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

@@ -1802,8 +1802,7 @@ abstract class InferenceVisitorBase implements InferenceVisitor {
         }
         Expression expression =
             _hoist(result.expression, inferredType, hoistedExpressions);
-        identicalInfo?.add(flowAnalysis.equalityOperand_end(
-            expression, new SharedTypeView(inferredType)));
+        identicalInfo?.add(flowAnalysis.equalityOperand_end(expression));
         if (isExpression) {
           arguments.positional[index] = expression..parent = arguments;
         } else {
@@ -1843,8 +1842,7 @@ abstract class InferenceVisitorBase implements InferenceVisitor {
           DartType inferredType = _computeInferredType(result);
           Expression expression = result.expression;
           identicalInfo?[deferredArgument.evaluationOrderIndex] =
-              flowAnalysis.equalityOperand_end(
-                  expression, new SharedTypeView(inferredType));
+              flowAnalysis.equalityOperand_end(expression);
           if (deferredArgument.isNamed) {
             NamedExpression namedArgument =
                 arguments.named[deferredArgument.index];
@@ -1862,7 +1860,11 @@ abstract class InferenceVisitorBase implements InferenceVisitor {
     }
     if (identicalInfo != null) {
       flowAnalysis.equalityOperation_end(
-          arguments.parent as Expression, identicalInfo[0], identicalInfo[1]);
+          arguments.parent as Expression,
+          identicalInfo[0],
+          new SharedTypeView(actualTypes[0]),
+          identicalInfo[1],
+          new SharedTypeView(actualTypes[1]));
     }
     assert(
         positionalIndex == arguments.positional.length,