[dart2js] Eliminate a use of a tree node in impact data(1).

This CL moves the error processing for the ConstConstructorInvocation
logic to when we build impact data, and in doing so allows us to
eliminate one use of a tree node. This should not change any logic.

Change-Id: I69ba69cabedb4d73ec718f552d451647c7f0c51b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/239843
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Joshua Litt <joshualitt@google.com>

Author: joshualitt

pkg/compiler/lib/src/ir/impact.dart

@@ -182,13 +182,12 @@ abstract class ImpactRegistry {
   void registerRuntimeTypeUse(ir.Expression node, RuntimeTypeUseKind kind,
       ir.DartType receiverType, ir.DartType argumentType);
 
-  // TODO(johnniwinther): Remove these when CFE provides constants.
   void registerConstructorNode(ir.Constructor node);
   void registerFieldNode(ir.Field node);
   void registerProcedureNode(ir.Procedure node);
   void registerStaticInvocationNode(ir.StaticInvocation node);
   void registerSwitchStatementNode(ir.SwitchStatement node);
-  void registerConstConstructorInvocationNode(ir.ConstructorInvocation node);
+  void registerConstSymbolConstructorInvocationNode();
 }
 
 class ImpactBuilderData {

pkg/compiler/lib/src/ir/impact_data.dart

@@ -9,7 +9,11 @@ import 'package:kernel/class_hierarchy.dart' as ir;
 import 'package:kernel/type_environment.dart' as ir;
 
 import '../common.dart';
+import '../common/elements.dart';
+import '../elements/entities.dart';
+import '../elements/types.dart';
 import '../ir/scope.dart';
+import '../kernel/element_map.dart';
 import '../serialization/serialization.dart';
 import '../util/enumset.dart';
 import 'constants.dart';
@@ -21,6 +25,7 @@ import 'util.dart';
 /// Visitor that builds an [ImpactData] object for the world impact.
 class ImpactBuilder extends StaticTypeVisitor implements ImpactRegistry {
   final ImpactData _data = ImpactData();
+  final KernelToElementMap _elementMap;
 
   @override
   final VariableScopeModel variableScopeModel;
@@ -34,12 +39,24 @@ class ImpactBuilder extends StaticTypeVisitor implements ImpactRegistry {
   @override
   final inferEffectivelyFinalVariableTypes;
 
-  ImpactBuilder(this.staticTypeContext, StaticTypeCacheImpl staticTypeCache,
-      ir.ClassHierarchy classHierarchy, this.variableScopeModel,
-      {this.useAsserts = false, this.inferEffectivelyFinalVariableTypes = true})
+  ImpactBuilder(
+      this._elementMap,
+      this.staticTypeContext,
+      StaticTypeCacheImpl staticTypeCache,
+      ir.ClassHierarchy classHierarchy,
+      this.variableScopeModel,
+      {this.useAsserts = false,
+      this.inferEffectivelyFinalVariableTypes = true})
       : super(
             staticTypeContext.typeEnvironment, classHierarchy, staticTypeCache);
 
+  CommonElements get _commonElements => _elementMap.commonElements;
+
+  DiagnosticReporter get _reporter => _elementMap.reporter;
+
+  String _typeToString(DartType type) =>
+      type.toStructuredText(_elementMap.types, _elementMap.options);
+
   /// Return the named arguments names as a list of strings.
   List<String> _getNamedArguments(ir.Arguments arguments) =>
       arguments.named.map((n) => n.name).toList();
@@ -313,6 +330,23 @@ class ImpactBuilder extends StaticTypeVisitor implements ImpactRegistry {
     registerProcedureNode(procedure);
   }
 
+  void _handleConstConstructorInvocation(ir.ConstructorInvocation node) {
+    assert(node.isConst);
+    ConstructorEntity constructor = _elementMap.getConstructor(node.target);
+    if (_commonElements.isSymbolConstructor(constructor)) {
+      DartType argumentType = _elementMap.getDartType(
+          node.arguments.positional.first.getStaticType(staticTypeContext));
+      // TODO(joshualitt): Does the CFE check this for us?
+      if (argumentType != _commonElements.stringType) {
+        // TODO(het): Get the actual span for the Symbol constructor argument
+        _reporter.reportErrorMessage(CURRENT_ELEMENT_SPANNABLE,
+            MessageKind.STRING_EXPECTED, {'type': _typeToString(argumentType)});
+        return;
+      }
+      registerConstSymbolConstructorInvocationNode();
+    }
+  }
+
   @override
   void handleConstructorInvocation(ir.ConstructorInvocation node,
       ArgumentTypes argumentTypes, ir.DartType resultType) {
@@ -325,7 +359,7 @@ class ImpactBuilder extends StaticTypeVisitor implements ImpactRegistry {
         getDeferredImport(node),
         isConst: node.isConst);
     if (node.isConst) {
-      registerConstConstructorInvocationNode(node);
+      _handleConstConstructorInvocation(node);
     }
   }
 
@@ -959,13 +993,15 @@ class ImpactBuilder extends StaticTypeVisitor implements ImpactRegistry {
   }
 
   @override
-  void registerConstConstructorInvocationNode(ir.ConstructorInvocation node) {
-    _data._constConstructorInvocationNodes ??= [];
-    _data._constConstructorInvocationNodes.add(node);
+  void registerConstSymbolConstructorInvocationNode() {
+    _data._hasConstSymbolConstructorInvocation = true;
   }
 }
 
 /// Data object that contains the world impact data derived purely from kernel.
+/// It is critical that all of the data in this class be invariant to changes in
+/// the AST that occur after modular compilation and before deserializing the
+/// impact data.
 class ImpactData {
   static const String tag = 'ImpactData';
 
@@ -1012,7 +1048,7 @@ class ImpactData {
   List<ir.Procedure> _procedureNodes;
   List<ir.SwitchStatement> _switchStatementNodes;
   List<ir.StaticInvocation> _staticInvocationNodes;
-  List<ir.ConstructorInvocation> _constConstructorInvocationNodes;
+  bool _hasConstSymbolConstructorInvocation = false;
 
   ImpactData();
 
@@ -1109,8 +1145,7 @@ class ImpactData {
         source.readTreeNodes<ir.SwitchStatement>(emptyAsNull: true);
     _staticInvocationNodes =
         source.readTreeNodes<ir.StaticInvocation>(emptyAsNull: true);
-    _constConstructorInvocationNodes =
-        source.readTreeNodes<ir.ConstructorInvocation>(emptyAsNull: true);
+    _hasConstSymbolConstructorInvocation = source.readBool();
     source.end(tag);
   }
 
@@ -1186,13 +1221,12 @@ class ImpactData {
     sink.writeList(_runtimeTypeUses, (_RuntimeTypeUse o) => o.toDataSink(sink),
         allowNull: true);
 
-    // TODO(johnniwinther): Remove these when CFE provides constants.
     sink.writeMemberNodes(_constructorNodes, allowNull: true);
     sink.writeMemberNodes(_fieldNodes, allowNull: true);
     sink.writeMemberNodes(_procedureNodes, allowNull: true);
     sink.writeTreeNodes(_switchStatementNodes, allowNull: true);
     sink.writeTreeNodes(_staticInvocationNodes, allowNull: true);
-    sink.writeTreeNodes(_constConstructorInvocationNodes, allowNull: true);
+    sink.writeBool(_hasConstSymbolConstructorInvocation);
 
     sink.end(tag);
   }
@@ -1527,10 +1561,8 @@ class ImpactData {
         registry.registerStaticInvocationNode(data);
       }
     }
-    if (_constConstructorInvocationNodes != null) {
-      for (ir.ConstructorInvocation data in _constConstructorInvocationNodes) {
-        registry.registerConstConstructorInvocationNode(data);
-      }
+    if (_hasConstSymbolConstructorInvocation) {
+      registry.registerConstSymbolConstructorInvocationNode();
     }
   }
 }

pkg/compiler/lib/src/ir/modular.dart

@@ -5,7 +5,6 @@
 // @dart = 2.10
 
 import 'package:kernel/ast.dart' as ir;
-import 'package:kernel/class_hierarchy.dart' as ir;
 import 'package:kernel/type_environment.dart' as ir;
 
 import 'package:front_end/src/api_unstable/dart2js.dart' as ir
@@ -17,7 +16,7 @@ import '../diagnostics/source_span.dart';
 import '../ir/impact_data.dart';
 import '../ir/static_type.dart';
 import '../js_backend/annotations.dart';
-import '../options.dart';
+import '../kernel/element_map.dart';
 import '../serialization/serialization.dart';
 import '../util/enumset.dart';
 import 'annotations.dart';
@@ -73,19 +72,20 @@ class ModuleData {
 }
 
 /// Compute [ModularMemberData] from the IR.
-ModularMemberData computeModularMemberData(ir.Member node,
-    {CompilerOptions options,
-    ir.TypeEnvironment typeEnvironment,
-    ir.ClassHierarchy classHierarchy,
+ModularMemberData computeModularMemberData(
+    KernelToElementMap elementMap,
+    ir.Member node,
     ScopeModel scopeModel,
-    EnumSet<PragmaAnnotation> annotations}) {
+    EnumSet<PragmaAnnotation> annotations) {
   var staticTypeCache = StaticTypeCacheImpl();
   var impactBuilderData = ImpactBuilder(
-          ir.StaticTypeContext(node, typeEnvironment, cache: staticTypeCache),
+          elementMap,
+          ir.StaticTypeContext(node, elementMap.typeEnvironment,
+              cache: staticTypeCache),
           staticTypeCache,
-          classHierarchy,
+          elementMap.classHierarchy,
           scopeModel.variableScopeModel,
-          useAsserts: options.enableUserAssertions,
+          useAsserts: elementMap.options.enableUserAssertions,
           inferEffectivelyFinalVariableTypes:
               !annotations.contains(PragmaAnnotation.disableFinal))
       .computeImpact(node);

pkg/compiler/lib/src/kernel/kernel_impact.dart

@@ -348,22 +348,8 @@ class KernelImpactConverter implements ImpactRegistry {
   }
 
   @override
-  void registerConstConstructorInvocationNode(ir.ConstructorInvocation node) {
-    assert(node.isConst);
-    ConstructorEntity constructor = elementMap.getConstructor(node.target);
-    if (commonElements.isSymbolConstructor(constructor)) {
-      ConstantValue value = elementMap.getConstantValue(
-          staticTypeContext, node.arguments.positional.first);
-      if (!value.isString) {
-        // TODO(het): Get the actual span for the Symbol constructor argument
-        reporter.reportErrorMessage(
-            CURRENT_ELEMENT_SPANNABLE,
-            MessageKind.STRING_EXPECTED,
-            {'type': typeToString(value.getType(elementMap.commonElements))});
-        return;
-      }
-      registerBackendImpact(_impacts.constSymbol);
-    }
+  void registerConstSymbolConstructorInvocationNode() {
+    registerBackendImpact(_impacts.constSymbol);
   }
 
   @override

pkg/compiler/lib/src/kernel/kernel_strategy.dart

@@ -440,12 +440,8 @@ class KernelModularStrategy extends ModularStrategy {
     ScopeModel scopeModel = _compilerTask.measureSubtask(
         'closures', () => ScopeModel.from(node, _elementMap.constantEvaluator));
     return _compilerTask.measureSubtask('worldImpact', () {
-      return computeModularMemberData(node,
-          options: _elementMap.options,
-          typeEnvironment: _elementMap.typeEnvironment,
-          classHierarchy: _elementMap.classHierarchy,
-          scopeModel: scopeModel,
-          annotations: annotations);
+      return computeModularMemberData(
+          _elementMap, node, scopeModel, annotations);
     });
   }
 }

pkg/compiler/lib/src/phase/modular_analysis.dart

@@ -63,13 +63,9 @@ ModuleData run(Input input) {
     final scopeModel = ScopeModel.from(member, elementMap.constantEvaluator);
     final annotations = processMemberAnnotations(
         options, reporter, member, computePragmaAnnotationDataFromIr(member));
-    result[member] = computeModularMemberData(member,
-            options: options,
-            typeEnvironment: elementMap.typeEnvironment,
-            classHierarchy: elementMap.classHierarchy,
-            scopeModel: scopeModel,
-            annotations: annotations)
-        .impactBuilderData;
+    result[member] =
+        computeModularMemberData(elementMap, member, scopeModel, annotations)
+            .impactBuilderData;
   }
 
   for (final library in input.component.libraries) {