[cfe] Handle un-inlined fields/locals in eager instantiation constantification

Closes #47108

TEST=existing

Change-Id: I590939161cde1316eede7d828dff5fe6ac2790fd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212577
Commit-Queue: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>

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

@@ -233,7 +233,7 @@ class Dart2jsTarget extends Target {
   }
 
   @override
-  ConstantsBackend constantsBackend(CoreTypes coreTypes) =>
+  ConstantsBackend get constantsBackend =>
       const Dart2jsConstantsBackend(supportsUnevaluatedConstants: true);
 }
 

pkg/dev_compiler/lib/src/kernel/constants.dart

@@ -98,8 +98,12 @@ class DevCompilerConstantsBackend extends ConstantsBackend {
   @override
   NumberSemantics get numberSemantics => NumberSemantics.js;
 
+  @override
+  bool get alwaysInlineConstants => false;
+
   @override
   bool shouldInlineConstant(ConstantExpression initializer) {
+    assert(!alwaysInlineConstants);
     var constant = initializer.constant;
     if (constant is StringConstant) {
       // Only inline small string constants, not large ones.

pkg/dev_compiler/lib/src/kernel/target.dart

@@ -243,8 +243,7 @@ class DevCompilerTarget extends Target {
   }
 
   @override
-  ConstantsBackend constantsBackend(CoreTypes coreTypes) =>
-      const DevCompilerConstantsBackend();
+  ConstantsBackend get constantsBackend => const DevCompilerConstantsBackend();
 }
 
 /// Analyzes a component to determine if any covariance checks in private

pkg/front_end/lib/src/base/processed_options.dart

@@ -848,6 +848,8 @@ class ProcessedOptions {
       return null;
     }
   }
+
+  CompilerOptions get rawOptionsForTesting => _raw;
 }
 
 /// A [FileSystem] that only allows access to files that have been explicitly

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

@@ -691,6 +691,14 @@ class ConstantsTransformer extends RemovingTransformer {
     Instantiation result =
         super.visitInstantiation(node, removalSentinel) as Instantiation;
     Expression expression = result.expression;
+    if (expression is StaticGet && expression.target.isConst) {
+      // Handle [StaticGet] of constant fields also when these are not inlined.
+      expression = (expression.target as Field).initializer!;
+    } else if (expression is VariableGet && expression.variable.isConst) {
+      // Handle [VariableGet] of constant locals also when these are not
+      // inlined.
+      expression = expression.variable.initializer!;
+    }
     if (expression is ConstantExpression) {
       if (result.typeArguments.every(isInstantiated)) {
         return evaluateAndTransformWithContext(node, result);
@@ -882,6 +890,9 @@ class ConstantsTransformer extends RemovingTransformer {
   }
 
   bool shouldInline(Expression initializer) {
+    if (backend.alwaysInlineConstants) {
+      return true;
+    }
     if (initializer is ConstantExpression) {
       return backend.shouldInlineConstant(initializer);
     }

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

@@ -1234,7 +1234,7 @@ class KernelTarget extends TargetImplementation {
     constants.ConstantEvaluationData constantEvaluationData =
         constants.transformLibraries(
             loader.libraries,
-            backendTarget.constantsBackend(loader.coreTypes),
+            backendTarget.constantsBackend,
             environmentDefines,
             environment,
             new KernelConstantErrorReporter(loader),
@@ -1290,7 +1290,7 @@ class KernelTarget extends TargetImplementation {
 
     constants.transformProcedure(
       procedure,
-      backendTarget.constantsBackend(loader.coreTypes),
+      backendTarget.constantsBackend,
       environmentDefines,
       environment,
       new KernelConstantErrorReporter(loader),

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

@@ -33,8 +33,8 @@ import 'redirecting_factory_body.dart'
 
 List<LocatedMessage> verifyComponent(Component component, Target target,
     {bool? isOutline, bool? afterConst, bool skipPlatform: false}) {
-  FastaVerifyingVisitor verifier =
-      new FastaVerifyingVisitor(target, isOutline, afterConst, skipPlatform);
+  FastaVerifyingVisitor verifier = new FastaVerifyingVisitor(target,
+      isOutline: isOutline, afterConst: afterConst, skipPlatform: skipPlatform);
   component.accept(verifier);
   return verifier.errors;
 }
@@ -47,9 +47,13 @@ class FastaVerifyingVisitor extends VerifyingVisitor {
   final List<TreeNode> treeNodeStack = <TreeNode>[];
   final bool skipPlatform;
 
-  FastaVerifyingVisitor(
-      this.target, bool? isOutline, bool? afterConst, this.skipPlatform)
-      : super(isOutline: isOutline, afterConst: afterConst);
+  FastaVerifyingVisitor(this.target,
+      {bool? isOutline, bool? afterConst, required this.skipPlatform})
+      : super(
+            isOutline: isOutline,
+            afterConst: afterConst,
+            constantsAreAlwaysInlined:
+                target.constantsBackend.alwaysInlineConstants);
 
   /// Invoked by all visit methods if the visited node is a [TreeNode].
   void enterTreeNode(TreeNode node) {

pkg/front_end/test/constant_evaluator_benchmark.dart

@@ -80,7 +80,7 @@ void benchmark(Component component, List<Library> libraries) {
         stopwatch.reset();
         CoreTypes coreTypes = new CoreTypes(component);
         ConstantsBackend constantsBackend =
-            target.backendTarget.constantsBackend(coreTypes);
+            target.backendTarget.constantsBackend;
         ClassHierarchy hierarchy = new ClassHierarchy(component, coreTypes);
         TypeEnvironment environment = new TypeEnvironment(coreTypes, hierarchy);
         if (verbose) {

pkg/front_end/test/fasta/testing/suite.dart

@@ -879,8 +879,7 @@ class StressConstantEvaluatorStep
   Future<Result<ComponentResult>> run(
       ComponentResult result, FastaContext context) async {
     KernelTarget target = result.sourceTarget;
-    ConstantsBackend constantsBackend =
-        target.backendTarget.constantsBackend(target.loader.coreTypes);
+    ConstantsBackend constantsBackend = target.backendTarget.constantsBackend;
     TypeEnvironment environment =
         new TypeEnvironment(target.loader.coreTypes, target.loader.hierarchy);
     StressConstantEvaluatorVisitor stressConstantEvaluatorVisitor =
@@ -1971,19 +1970,20 @@ class Verify extends Step<ComponentResult, ComponentResult, FastaContext> {
 
     Component component = result.component;
     StringBuffer messages = new StringBuffer();
-    ProcessedOptions options = new ProcessedOptions(
-        options: new CompilerOptions()
-          ..onDiagnostic = (DiagnosticMessage message) {
-            if (messages.isNotEmpty) {
-              messages.write("\n");
-            }
-            messages.writeAll(message.plainTextFormatted, "\n");
-          });
-    return await CompilerContext.runWithOptions(options,
-        (compilerContext) async {
+    void Function(DiagnosticMessage)? previousOnDiagnostics =
+        result.options.rawOptionsForTesting.onDiagnostic;
+    result.options.rawOptionsForTesting.onDiagnostic =
+        (DiagnosticMessage message) {
+      if (messages.isNotEmpty) {
+        messages.write("\n");
+      }
+      messages.writeAll(message.plainTextFormatted, "\n");
+    };
+    Result<ComponentResult> verifyResult = await CompilerContext.runWithOptions(
+        result.options, (compilerContext) async {
       compilerContext.uriToSource.addAll(component.uriToSource);
       List<LocatedMessage> verificationErrors = verifyComponent(
-          component, options.target,
+          component, result.options.target,
           isOutline: !fullCompile, skipPlatform: true);
       assert(verificationErrors.isEmpty || messages.isNotEmpty);
       if (messages.isEmpty) {
@@ -1993,6 +1993,8 @@ class Verify extends Step<ComponentResult, ComponentResult, FastaContext> {
             null, context.expectationSet["VerificationError"], "$messages");
       }
     }, errorOnMissingInput: false);
+    result.options.rawOptionsForTesting.onDiagnostic = previousOnDiagnostics;
+    return verifyResult;
   }
 }
 

pkg/front_end/test/issue_34856_test.dart

@@ -30,6 +30,7 @@ import 'package:front_end/src/fasta/kernel/utils.dart' show serializeComponent;
 import 'package:front_end/src/fasta/kernel/verifier.dart' show verifyComponent;
 
 import 'package:kernel/ast.dart' show Component;
+import 'package:kernel/target/targets.dart';
 
 const Map<String, String> files = const <String, String>{
   "repro.dart": """
@@ -88,7 +89,8 @@ Future<void> test() async {
   options = new CompilerOptions()
     ..fileSystem = fs
     ..additionalDills = <Uri>[base.resolve("lib.dart.dill")]
-    ..sdkSummary = platformDill;
+    ..sdkSummary = platformDill
+    ..target = new NoneTarget(new TargetFlags());
 
   List<Uri> inputs = <Uri>[base.resolve("repro.dart")];
 

pkg/front_end/testcases/dartdevc/issue47108.dart

@@ -0,0 +1,17 @@
+// Copyright (c) 2021, 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 C<T> {}
+
+const constructorTearOff = C.new;
+
+main() {
+  // These instantiations are in a const context so they appear in the const pool.
+  const instantiatedTearOff = constructorTearOff<int>;
+  const instantiatedTearOff2 = constructorTearOff<int>;
+  print(identical(instantiatedTearOff, instantiatedTearOff2)); // Prints true
+
+  // These instantiations are not in a const context so they don't appear in the const pool.
+  print(identical(constructorTearOff<String>, constructorTearOff<String>)); // Prints false
+}

pkg/front_end/testcases/dartdevc/issue47108.dart.strong.expect

@@ -0,0 +1,24 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+class C<T extends core::Object? = dynamic> extends core::Object {
+  synthetic constructor •() → self::C<self::C::T%>
+    : super core::Object::•()
+    ;
+  static method _#new#tearOff<T extends core::Object? = dynamic>() → self::C<self::C::_#new#tearOff::T%>
+    return new self::C::•<self::C::_#new#tearOff::T%>();
+}
+static const field <T extends core::Object? = dynamic>() → self::C<T%> constructorTearOff = #C1;
+static method main() → dynamic {
+  const () → self::C<core::int> instantiatedTearOff = #C2;
+  const () → self::C<core::int> instantiatedTearOff2 = #C2;
+  core::print(core::identical(instantiatedTearOff, instantiatedTearOff2));
+  core::print(core::identical(#C3, #C3));
+}
+
+constants  {
+  #C1 = static-tearoff self::C::_#new#tearOff
+  #C2 = instantiation self::C::_#new#tearOff <core::int>
+  #C3 = instantiation self::C::_#new#tearOff <core::String>
+}

pkg/front_end/testcases/dartdevc/issue47108.dart.strong.transformed.expect

@@ -0,0 +1,29 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+class C<T extends core::Object? = dynamic> extends core::Object {
+  synthetic constructor •() → self::C<self::C::T%>
+    : super core::Object::•()
+    ;
+  static method _#new#tearOff<T extends core::Object? = dynamic>() → self::C<self::C::_#new#tearOff::T%>
+    return new self::C::•<self::C::_#new#tearOff::T%>();
+}
+static const field <T extends core::Object? = dynamic>() → self::C<T%> constructorTearOff = #C1;
+static method main() → dynamic {
+  const () → self::C<core::int> instantiatedTearOff = #C2;
+  const () → self::C<core::int> instantiatedTearOff2 = #C2;
+  core::print(core::identical(instantiatedTearOff, instantiatedTearOff2));
+  core::print(core::identical(#C3, #C3));
+}
+
+constants  {
+  #C1 = static-tearoff self::C::_#new#tearOff
+  #C2 = instantiation self::C::_#new#tearOff <core::int>
+  #C3 = instantiation self::C::_#new#tearOff <core::String>
+}
+
+Extra constant evaluation status:
+Evaluated: StaticInvocation @ org-dartlang-testcase:///issue47108.dart:13:9 -> BoolConstant(true)
+Evaluated: StaticInvocation @ org-dartlang-testcase:///issue47108.dart:16:9 -> BoolConstant(true)
+Extra constant evaluation: evaluated: 5, effectively constant: 2

pkg/front_end/testcases/dartdevc/issue47108.dart.textual_outline.expect

@@ -0,0 +1,4 @@
+class C<T> {}
+
+const constructorTearOff = C.new;
+main() {}

pkg/front_end/testcases/dartdevc/issue47108.dart.textual_outline_modelled.expect

@@ -0,0 +1,4 @@
+class C<T> {}
+
+const constructorTearOff = C.new;
+main() {}

pkg/front_end/testcases/dartdevc/issue47108.dart.weak.expect

@@ -0,0 +1,24 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+class C<T extends core::Object? = dynamic> extends core::Object {
+  synthetic constructor •() → self::C<self::C::T%>
+    : super core::Object::•()
+    ;
+  static method _#new#tearOff<T extends core::Object? = dynamic>() → self::C<self::C::_#new#tearOff::T%>
+    return new self::C::•<self::C::_#new#tearOff::T%>();
+}
+static const field <T extends core::Object? = dynamic>() → self::C<T%> constructorTearOff = #C1;
+static method main() → dynamic {
+  const () → self::C<core::int> instantiatedTearOff = #C2;
+  const () → self::C<core::int> instantiatedTearOff2 = #C2;
+  core::print(core::identical(instantiatedTearOff, instantiatedTearOff2));
+  core::print(core::identical(#C3, #C3));
+}
+
+constants  {
+  #C1 = static-tearoff self::C::_#new#tearOff
+  #C2 = instantiation self::C::_#new#tearOff <core::int*>
+  #C3 = instantiation self::C::_#new#tearOff <core::String*>
+}

pkg/front_end/testcases/dartdevc/issue47108.dart.weak.outline.expect

@@ -0,0 +1,18 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+class C<T extends core::Object? = dynamic> extends core::Object {
+  synthetic constructor •() → self::C<self::C::T%>
+    ;
+  static method _#new#tearOff<T extends core::Object? = dynamic>() → self::C<self::C::_#new#tearOff::T%>
+    return new self::C::•<self::C::_#new#tearOff::T%>();
+}
+static const field <T extends core::Object? = dynamic>() → self::C<T%> constructorTearOff = self::C::_#new#tearOff;
+static method main() → dynamic
+  ;
+
+
+Extra constant evaluation status:
+Evaluated: StaticTearOff @ org-dartlang-testcase:///issue47108.dart:7:28 -> StaticTearOffConstant(C._#new#tearOff)
+Extra constant evaluation: evaluated: 2, effectively constant: 1

pkg/front_end/testcases/dartdevc/issue47108.dart.weak.transformed.expect

@@ -0,0 +1,29 @@
+library /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+
+class C<T extends core::Object? = dynamic> extends core::Object {
+  synthetic constructor •() → self::C<self::C::T%>
+    : super core::Object::•()
+    ;
+  static method _#new#tearOff<T extends core::Object? = dynamic>() → self::C<self::C::_#new#tearOff::T%>
+    return new self::C::•<self::C::_#new#tearOff::T%>();
+}
+static const field <T extends core::Object? = dynamic>() → self::C<T%> constructorTearOff = #C1;
+static method main() → dynamic {
+  const () → self::C<core::int> instantiatedTearOff = #C2;
+  const () → self::C<core::int> instantiatedTearOff2 = #C2;
+  core::print(core::identical(instantiatedTearOff, instantiatedTearOff2));
+  core::print(core::identical(#C3, #C3));
+}
+
+constants  {
+  #C1 = static-tearoff self::C::_#new#tearOff
+  #C2 = instantiation self::C::_#new#tearOff <core::int*>
+  #C3 = instantiation self::C::_#new#tearOff <core::String*>
+}
+
+Extra constant evaluation status:
+Evaluated: StaticInvocation @ org-dartlang-testcase:///issue47108.dart:13:9 -> BoolConstant(true)
+Evaluated: StaticInvocation @ org-dartlang-testcase:///issue47108.dart:16:9 -> BoolConstant(true)
+Extra constant evaluation: evaluated: 5, effectively constant: 2

pkg/kernel/lib/target/targets.dart

@@ -112,6 +112,10 @@ class ConstantsBackend {
   /// Number semantics to use for this backend.
   NumberSemantics get numberSemantics => NumberSemantics.vm;
 
+  /// If true, all constants are inlined. Otherwise [shouldInlineConstant] is
+  /// called to determine whether a constant expression should be inlined.
+  bool get alwaysInlineConstants => true;
+
   /// Inline control of constant variables. The given constant expression
   /// is the initializer of a [Field] or [VariableDeclaration] node.
   /// If this method returns `true`, the variable will be inlined at all
@@ -119,7 +123,11 @@ class ConstantsBackend {
   /// by the `keepFields` or `keepLocals` properties).
   /// This method must be deterministic, i.e. it must always return the same
   /// value for the same constant value and place in the AST.
-  bool shouldInlineConstant(ConstantExpression initializer) => true;
+  ///
+  /// This is only called if [alwaysInlineConstants] is `true`.
+  bool shouldInlineConstant(ConstantExpression initializer) =>
+      throw new UnsupportedError(
+          'Per-value constant inlining is not supported');
 
   /// Whether this target supports unevaluated constants.
   ///
@@ -445,7 +453,7 @@ abstract class Target {
   Class? concreteDoubleLiteralClass(CoreTypes coreTypes, double value) => null;
   Class? concreteStringLiteralClass(CoreTypes coreTypes, String value) => null;
 
-  ConstantsBackend constantsBackend(CoreTypes coreTypes);
+  ConstantsBackend get constantsBackend;
 }
 
 class NoneConstantsBackend extends ConstantsBackend {
@@ -517,7 +525,7 @@ class NoneTarget extends Target {
   }
 
   @override
-  ConstantsBackend constantsBackend(CoreTypes coreTypes) =>
+  ConstantsBackend get constantsBackend =>
       // TODO(johnniwinther): Should this vary with the use case?
       const NoneConstantsBackend(supportsUnevaluatedConstants: true);
 }
@@ -763,9 +771,7 @@ class TargetWrapper extends Target {
   }
 
   @override
-  ConstantsBackend constantsBackend(CoreTypes coreTypes) {
-    return _target.constantsBackend(coreTypes);
-  }
+  ConstantsBackend get constantsBackend => _target.constantsBackend;
 
   @override
   bool enableNative(Uri uri) {

pkg/kernel/lib/verifier.dart

@@ -8,9 +8,12 @@ import 'ast.dart';
 import 'transformations/flags.dart';
 import 'type_environment.dart' show StatefulStaticTypeContext, TypeEnvironment;
 
-void verifyComponent(Component component, {bool? isOutline, bool? afterConst}) {
+void verifyComponent(Component component,
+    {bool? isOutline, bool? afterConst, bool constantsAreAlwaysInlined: true}) {
   VerifyingVisitor.check(component,
-      isOutline: isOutline, afterConst: afterConst);
+      isOutline: isOutline,
+      afterConst: afterConst,
+      constantsAreAlwaysInlined: constantsAreAlwaysInlined);
 }
 
 class VerificationError {
@@ -67,6 +70,9 @@ class VerifyingVisitor extends RecursiveResultVisitor<void> {
   /// a verification error for anything that should have been removed by it.
   final bool afterConst;
 
+  /// If true, constant fields and local variables are expected to be inlined.
+  final bool constantsAreAlwaysInlined;
+
   AsyncMarker currentAsyncMarker = AsyncMarker.Sync;
 
   bool inCatchBlock = false;
@@ -88,12 +94,20 @@ class VerifyingVisitor extends RecursiveResultVisitor<void> {
   TreeNode? get currentClassOrExtensionOrMember =>
       currentMember ?? currentClass ?? currentExtension;
 
-  static void check(Component component, {bool? isOutline, bool? afterConst}) {
-    component.accept(
-        new VerifyingVisitor(isOutline: isOutline, afterConst: afterConst));
+  static void check(Component component,
+      {bool? isOutline,
+      bool? afterConst,
+      required bool constantsAreAlwaysInlined}) {
+    component.accept(new VerifyingVisitor(
+        isOutline: isOutline,
+        afterConst: afterConst,
+        constantsAreAlwaysInlined: constantsAreAlwaysInlined));
   }
 
-  VerifyingVisitor({bool? isOutline, bool? afterConst})
+  VerifyingVisitor(
+      {bool? isOutline,
+      bool? afterConst,
+      required this.constantsAreAlwaysInlined})
       : isOutline = isOutline ?? false,
         afterConst = afterConst ?? !(isOutline ?? false);
 
@@ -577,10 +591,12 @@ class VerifyingVisitor extends RecursiveResultVisitor<void> {
     declareVariable(node);
     if (afterConst && node.isConst) {
       Expression? initializer = node.initializer;
-      if (!(initializer is InvalidExpression ||
-          initializer is ConstantExpression &&
-              initializer.constant is UnevaluatedConstant)) {
-        problem(node, "Constant VariableDeclaration");
+      if (constantsAreAlwaysInlined) {
+        if (!(initializer is InvalidExpression ||
+            initializer is ConstantExpression &&
+                initializer.constant is UnevaluatedConstant)) {
+          problem(node, "Constant VariableDeclaration");
+        }
       }
     }
   }
@@ -589,7 +605,7 @@ class VerifyingVisitor extends RecursiveResultVisitor<void> {
   void visitVariableGet(VariableGet node) {
     checkVariableInScope(node.variable, node);
     visitChildren(node);
-    if (afterConst && node.variable.isConst) {
+    if (constantsAreAlwaysInlined && afterConst && node.variable.isConst) {
       problem(node, "VariableGet of const variable '${node.variable}'.");
     }
   }
@@ -621,7 +637,10 @@ class VerifyingVisitor extends RecursiveResultVisitor<void> {
     if (node.target.isInstanceMember) {
       problem(node, "StaticGet of '${node.target}' that's an instance member.");
     }
-    if (afterConst && node.target is Field && node.target.isConst) {
+    if (constantsAreAlwaysInlined &&
+        afterConst &&
+        node.target is Field &&
+        node.target.isConst) {
       problem(node, "StaticGet of const field '${node.target}'.");
     }
   }

pkg/vm/lib/target/vm.dart

@@ -488,7 +488,7 @@ class VmTarget extends Target {
   }
 
   @override
-  ConstantsBackend constantsBackend(CoreTypes coreTypes) => ConstantsBackend();
+  ConstantsBackend get constantsBackend => const ConstantsBackend();
 
   @override
   Map<String, String> updateEnvironmentDefines(Map<String, String> map) {