[analyzer] Change ResolverVisitor to always visit subexpressions using analyzeExpression.

Previously, when `ResolverVisitor` needed to provide a context to a
subexpression, it would always use `analyzeExpression` to visit it,
but when no context was needed, it would sometimes visit the
subexpression by calling `ExpressionImpl.accept`. This made the
`ResolverVisitor` more difficult to reason about, since there wasn't a
common method through which all subexpression visits were dispatched.

With change, any time the `ResolverVisitor` visits an expression in a
context that's inside a method body or initializer, it visits it using
`analyzeExpression`. To make sure I haven't missed anything, and to
prevent future regressions, I've added an assertion to
`_InferenceLogWriterImpl.enterExpression` that verifies that all
expressions visited by the `ResolverVisitor` are visited via
`analyzeExpression` when inside a top level declaration.

This change paves the way for allowing the analyzer to use the shared
implementation of null-shorting introduced in
https://dart-review.googlesource.com/c/sdk/+/399480.

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

Author: stereotype441

pkg/analyzer/analyzer_use_new_elements.txt

@@ -92,6 +92,7 @@ lib/src/dart/resolver/this_lookup.dart
 lib/src/dart/resolver/type_property_resolver.dart
 lib/src/dart/resolver/typed_literal_resolver.dart
 lib/src/dart/resolver/variable_declaration_resolver.dart
+lib/src/dart/resolver/yield_statement_resolver.dart
 lib/src/diagnostic/diagnostic_factory.dart
 lib/src/error/best_practices_verifier.dart
 lib/src/error/correct_override.dart

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

@@ -306,8 +306,12 @@ class BinaryExpressionResolver {
   }
 
   void _resolveUnsupportedOperator(BinaryExpressionImpl node) {
-    node.leftOperand.accept(_resolver);
-    node.rightOperand.accept(_resolver);
+    _resolver.analyzeExpression(
+        node.leftOperand, _resolver.operations.unknownType);
+    _resolver.popRewrite();
+    _resolver.analyzeExpression(
+        node.rightOperand, _resolver.operations.unknownType);
+    _resolver.popRewrite();
     node.recordStaticType(InvalidTypeImpl.instance, resolver: _resolver);
   }
 

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

@@ -23,6 +23,7 @@ import 'package:analyzer/src/dart/element/element.dart';
 import 'package:analyzer/src/dart/element/type.dart';
 import 'package:analyzer/src/dart/element/type_schema.dart';
 import 'package:analyzer/src/dart/element/type_system.dart' show TypeSystemImpl;
+import 'package:analyzer/src/generated/inference_log.dart';
 import 'package:analyzer/src/generated/variable_type_provider.dart';
 
 export 'package:_fe_analyzer_shared/src/type_inference/nullability_suffix.dart'
@@ -165,6 +166,7 @@ class FlowAnalysisHelper {
   /// will be visited.
   void bodyOrInitializer_enter(AstNode node, FormalParameterList? parameters,
       {void Function(AstVisitor<Object?> visitor)? visit}) {
+    inferenceLogWriter?.enterBodyOrInitializer(node);
     assert(flow == null);
     assignedVariables = computeAssignedVariables(node, parameters,
         retainDataForTesting: dataForTesting != null, visit: visit);
@@ -190,6 +192,7 @@ class FlowAnalysisHelper {
   /// This method is called whenever the [ResolverVisitor] leaves the body or
   /// initializer of a top level declaration.
   void bodyOrInitializer_exit() {
+    inferenceLogWriter?.exitBodyOrInitializer();
     // Set this.flow to null before doing any clean-up so that if an exception
     // is raised, the state is already updated correctly, and we don't have
     // cascading failures.

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

@@ -14,6 +14,7 @@ import 'package:analyzer/src/dart/element/type_system.dart';
 import 'package:analyzer/src/dart/resolver/assignment_expression_resolver.dart';
 import 'package:analyzer/src/dart/resolver/typed_literal_resolver.dart';
 import 'package:analyzer/src/error/codes.dart';
+import 'package:analyzer/src/generated/inference_log.dart';
 import 'package:analyzer/src/generated/resolver.dart';
 
 /// Helper for resolving [ForStatement]s and [ForElement]s.
@@ -129,6 +130,8 @@ class ForResolver {
     } else if (forEachParts is ForEachPartsWithIdentifier) {
       identifier = forEachParts.identifier;
       // TODO(scheglov): replace with lexical lookup
+      inferenceLogWriter?.setExpressionVisitCodePath(
+          identifier, ExpressionVisitCodePath.forEachIdentifier);
       identifier.accept(_resolver);
       AssignmentExpressionShared(
         resolver: _resolver,
@@ -199,7 +202,11 @@ class ForResolver {
     if (forParts is ForPartsWithDeclarations) {
       forParts.variables.accept(_resolver);
     } else if (forParts is ForPartsWithExpression) {
-      forParts.initialization?.accept(_resolver);
+      if (forParts.initialization case var initialization?) {
+        _resolver.analyzeExpression(
+            initialization, _resolver.operations.unknownType);
+        _resolver.popRewrite();
+      }
     } else if (forParts is ForPartsWithPattern) {
       forParts.variables.accept(_resolver);
     } else {
@@ -223,7 +230,10 @@ class ForResolver {
     visitBody();
 
     _resolver.flowAnalysis.flow?.for_updaterBegin();
-    forParts.updaters.accept(_resolver);
+    for (var updater in forParts.updaters) {
+      _resolver.analyzeExpression(updater, _resolver.operations.unknownType);
+      _resolver.popRewrite();
+    }
 
     _resolver.flowAnalysis.flow?.for_end();
   }

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

@@ -50,7 +50,8 @@ class FunctionReferenceResolver {
       var typeArguments = node.typeArguments;
       if (typeArguments != null) {
         // Something like `List.filled<int>`.
-        function.accept(_resolver);
+        _resolver.analyzeExpression(function, _resolver.operations.unknownType);
+        _resolver.popRewrite();
         // We can safely assume `function.constructorName.name` is non-null
         // because if no name had been given, the construct would have been
         // interpreted as a type literal (e.g. `List<int>`).
@@ -67,7 +68,8 @@ class FunctionReferenceResolver {
     } else {
       // TODO(srawlins): Handle `function` being a [SuperExpression].
 
-      function.accept(_resolver);
+      _resolver.analyzeExpression(function, _resolver.operations.unknownType);
+      function = _resolver.popRewrite()!;
       var functionType = function.staticType;
       if (functionType == null) {
         _resolveDisallowedExpression(node, functionType);
@@ -493,13 +495,15 @@ class FunctionReferenceResolver {
         CompileTimeErrorCode.DISALLOWED_TYPE_INSTANTIATION_EXPRESSION,
       );
     }
-    function.accept(_resolver);
+    _resolver.analyzeExpression(function, _resolver.operations.unknownType);
+    _resolver.popRewrite();
     node.recordStaticType(InvalidTypeImpl.instance, resolver: _resolver);
   }
 
   void _resolvePropertyAccessFunction(
       FunctionReferenceImpl node, PropertyAccessImpl function) {
-    function.accept(_resolver);
+    _resolver.analyzeExpression(function, _resolver.operations.unknownType);
+    _resolver.popRewrite();
     var callMethod = _getCallMethod(node, function.staticType);
     if (callMethod is MethodElement) {
       _resolveAsImplicitCallReference(node, callMethod);
@@ -617,16 +621,21 @@ class FunctionReferenceResolver {
         _resolveConstructorReference(node);
         return;
       } else if (element is InterfaceElement) {
-        node.function.accept(_resolver);
+        _resolver.analyzeExpression(
+            node.function, _resolver.operations.unknownType);
+        _resolver.popRewrite();
         _resolveDirectTypeLiteral(node, prefix, element);
         return;
       } else if (element is TypeAliasElement) {
-        prefix.accept(_resolver);
+        _resolver.analyzeExpression(prefix, _resolver.operations.unknownType);
+        _resolver.popRewrite();
         _resolveTypeAlias(node: node, element: element, typeAlias: prefix);
         return;
       }
     } else if (element is ExecutableElement) {
-      node.function.accept(_resolver);
+      _resolver.analyzeExpression(
+          node.function, _resolver.operations.unknownType);
+      _resolver.popRewrite();
       var callMethod = _getCallMethod(node, node.function.staticType);
       if (callMethod is MethodElement) {
         _resolveAsImplicitCallReference(node, callMethod);

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

@@ -27,7 +27,8 @@ class PrefixedIdentifierResolver {
     PrefixedIdentifierImpl node, {
     required DartType contextType,
   }) {
-    node.prefix.accept(_resolver);
+    _resolver.analyzeExpression(node.prefix, _resolver.operations.unknownType);
+    _resolver.popRewrite();
 
     var prefixElement = node.prefix.staticElement;
     if (prefixElement is! PrefixElement) {

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

@@ -183,7 +183,9 @@ class YieldStatementResolver {
   }
 
   void _resolve_notGenerator(YieldStatement node) {
-    node.expression.accept(_resolver);
+    _resolver.analyzeExpression(
+        node.expression, _resolver.operations.unknownType);
+    _resolver.popRewrite();
 
     _errorReporter.atNode(
       node,

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

@@ -6,6 +6,7 @@ import 'package:_fe_analyzer_shared/src/type_inference/shared_inference_log.dart
 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/dart/ast/ast.dart';
 import 'package:analyzer/src/generated/resolver.dart';
 
 final bool _assertionsEnabled = () {
@@ -14,6 +15,11 @@ final bool _assertionsEnabled = () {
   return enabled;
 }();
 
+/// Expando used by [_InferenceLogWriterImpl.setExpressionVisitCodePath] to
+/// record the code path that's being used by the [ResolverVisitor] to visit a
+/// subexpression.
+final _expressionVisitCodePaths = Expando<ExpressionVisitCodePath>();
+
 /// The [InferenceLogWriter] currently being used by the analyzer, if inference
 /// logging is active, otherwise `null`.
 InferenceLogWriter? _inferenceLogWriter;
@@ -52,6 +58,18 @@ void stopInferenceLogging() {
   _inferenceLogWriter = null;
 }
 
+/// Enum of all the possible code paths that can be used by the
+/// [ResolverVisitor] to visit expressions when inside the body or initializer
+/// of a top level construct.
+enum ExpressionVisitCodePath {
+  /// The expression is being visited via [ResolverVisitor.analyzeExpression].
+  analyzeExpression,
+
+  /// The expression is the identifier in a "for each" loop, so it is not a true
+  /// expression, and it is being visited directly using [Expression.accept].
+  forEachIdentifier
+}
+
 /// The [SharedInferenceLogWriter] interface, augmented with analyzer-specific
 /// functionality.
 abstract interface class InferenceLogWriter
@@ -62,12 +80,44 @@ abstract interface class InferenceLogWriter
   /// This is called from [ResolverVisitor.dispatchExpression], to verify that
   /// each expression's visit method property calls [enterExpression].
   void assertExpressionWasRecorded(Expression expression);
+
+  /// Called when type inference enters the body of a top level function or
+  /// method, or the initializer of a top level variable or field, or the
+  /// initializers and body of a constructor.
+  void enterBodyOrInitializer(AstNode node);
+
+  /// Called when type inference enters the body of a top level function or
+  /// method, or the initializer of a top level variable or field, or the
+  /// initializers and body of a constructor.
+  void exitBodyOrInitializer();
+
+  /// Records [source] as the code path that the [ResolverVisitor] is about to
+  /// use to visit the expression [node].
+  ///
+  /// An assertion in [enterExpression] verifies that when inside a method body
+  /// or initializer (see [enterBodyOrInitializer]), every call to
+  /// [enterExpression] is preceded by exactly one call to
+  /// [setExpressionVisitCodePath]. This ensures that the resolution process
+  /// doesn't ever try to resolve a subexpression more than once. It also
+  /// ensures that every code path that resolves subexpressions inside method
+  /// bodies and initializers calls this method, making it easy to statically
+  /// locate these code paths.
+  void setExpressionVisitCodePath(
+      Expression node, ExpressionVisitCodePath source);
 }
 
 /// The [SharedInferenceLogWriterImpl] implementation, augmented with
 /// analyzer-specific functionality.
 final class _InferenceLogWriterImpl extends SharedInferenceLogWriterImpl<
     DartType, DartType, TypeParameterElement> implements InferenceLogWriter {
+  /// Whether type inference is currently inside the body of a top level
+  /// function or method, or the initializer of a top level variable or field,
+  /// or the initializers and body of a constructor.
+  ///
+  /// When this value is `true`, flow analysis is active, and expressions must
+  /// be visited using [ResolverVisitor.analyzeExpression].
+  bool _inBodyOrInitializer = false;
+
   @override
   void assertExpressionWasRecorded(Object expression) {
     if (_recordedExpressions[expression] ?? false) return;
@@ -90,6 +140,12 @@ final class _InferenceLogWriterImpl extends SharedInferenceLogWriterImpl<
     super.enterAnnotation(node);
   }
 
+  @override
+  void enterBodyOrInitializer(AstNode node) {
+    assert(!_inBodyOrInitializer, 'Already in a body or initializer');
+    _inBodyOrInitializer = true;
+  }
+
   @override
   void enterElement(covariant CollectionElement node) {
     checkCall(
@@ -101,6 +157,10 @@ final class _InferenceLogWriterImpl extends SharedInferenceLogWriterImpl<
 
   @override
   void enterExpression(covariant Expression node, DartType contextType) {
+    assert(
+        !_inBodyOrInitializer || _expressionVisitCodePaths[node] != null,
+        'When in a body or initializer, setExpressionVisitSource should be '
+        'called prior to enterExpression. Not called for $node.');
     checkCall(
         method: 'enterExpression',
         arguments: [node, contextType],
@@ -147,6 +207,20 @@ final class _InferenceLogWriterImpl extends SharedInferenceLogWriterImpl<
     super.enterStatement(node);
   }
 
+  @override
+  void exitBodyOrInitializer() {
+    assert(_inBodyOrInitializer, 'Not in a method body or initializer');
+    _inBodyOrInitializer = false;
+  }
+
+  @override
+  void setExpressionVisitCodePath(
+      Expression node, ExpressionVisitCodePath source) {
+    assert(_expressionVisitCodePaths[node] == null,
+        'An expression visit source was already set for $node');
+    _expressionVisitCodePaths[node] = source;
+  }
+
   /// Returns the nearest ancestor of [node] for which a call to `enter...`
   /// should have been made.
   ///