[CFE] Keep const locals by default; expression evaluation can evaluate const locals

Note: Const locals are still off for VM aot and dart2js for the entry
points I've found in an attempt to retain the old behaviour there.
It might be better if those targets could remove such locals in a
whole-world analysis instead.

 * Keep const locals by default (except as noted above). Update the
   verifier to accept that. For the platforms this has increased the
   size by at most 6584 bytes. With this the VM will pass in any const
   locals as it does normal locals, but as the variable is never
   captured it will never pass a const local defined in a method when
   inside a local function in that method.
 * Change the dart scope calculation(s) to return the found variables
   instead of just the types of the found variables.
 * When the incremental compilers expression compilation - via the dart
   scope calculation - finds a const local that it wasn't told about, it
   will pass it on as an extra variable that it knows about, allowing
   for evaluating const locals in the case not covered by the first
   bullet.

With luck this can in future CLs be extended to know about other
variables that we're not told about, allowing to give a message saying
something like
"yes, we know what 'foo' is, but you can't currently use it" as wanted
in for instance #60316 and
#53996.

Tested: Existing tests for existing functionality; new tests for the new
Change-Id: I1ec24350273e6f81574bb2888f6bf46e3b8b1b47
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/445461
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>
Reviewed-by: Nicholas Shahan <[email protected]>
Reviewed-by: Daco Harkes <[email protected]>

Author: jensjoha

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

@@ -389,7 +389,8 @@ class Dart2jsConstantsBackend extends ConstantsBackend {
   @override
   final bool supportsUnevaluatedConstants;
 
-  const Dart2jsConstantsBackend({required this.supportsUnevaluatedConstants});
+  const Dart2jsConstantsBackend({required this.supportsUnevaluatedConstants})
+    : super(keepLocals: false);
 
   @override
   NumberSemantics get numberSemantics => NumberSemantics.js;

pkg/dart2wasm/lib/target.dart

@@ -41,7 +41,7 @@ enum Mode {
 }
 
 class Dart2WasmConstantsBackend extends ConstantsBackend {
-  const Dart2WasmConstantsBackend();
+  const Dart2WasmConstantsBackend() : super(keepLocals: false);
 
   @override
   bool get supportsUnevaluatedConstants => true;

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

@@ -125,12 +125,11 @@ class ExpressionCompiler {
       // extracted from the lowering.
       // See https://github.com/dart-lang/sdk/issues/55918
       var dartLateLocals = [
-        for (var name in dartScope.definitions.keys)
+        for (var name in dartScope.variables.keys)
           if (isLateLoweredLocalName(name)) name,
       ];
       for (var localName in dartLateLocals) {
-        dartScope.definitions[extractLocalName(localName)] = dartScope
-            .definitions
+        dartScope.variables[extractLocalName(localName)] = dartScope.variables
             .remove(localName)!;
       }
 
@@ -151,7 +150,7 @@ class ExpressionCompiler {
       // shorter Dart name. Since longer Dart names can't incorrectly match a
       // shorter JS name (JS names are always at least as long as the Dart
       // name), we process them from longest to shortest.
-      final dartNames = [...dartScope.definitions.keys]..sort(nameCompare);
+      final dartNames = [...dartScope.variables.keys]..sort(nameCompare);
 
       // Sort JS names so that the highest suffix value gets assigned to the
       // corresponding Dart name first. Since names are suffixed in ascending
@@ -245,15 +244,15 @@ class ExpressionCompiler {
         }
       }
 
-      dartScope.definitions.removeWhere(
-        (variable, type) =>
+      dartScope.variables.removeWhere(
+        (variableName, _) =>
             // Remove undefined js variables (this allows us to get a reference
             // error from chrome on evaluation).
-            !dartNameToJsValue.containsKey(variable) ||
+            !dartNameToJsValue.containsKey(variableName) ||
             // Remove wildcard method arguments which are lowered to have Dart
             // names that are invalid for Dart compilations.
             // Wildcard local variables are not appearing here at this time.
-            isWildcardLoweredFormalParameter(variable),
+            isWildcardLoweredFormalParameter(variableName),
       );
 
       // Wildcard type parameters already matched by this existing test.
@@ -265,7 +264,7 @@ class ExpressionCompiler {
       // captured variables optimized away in chrome)
       var localJsScope = [
         ...dartScope.typeParameters.map((parameter) => jsScope[parameter.name]),
-        ...dartScope.definitions.keys.map(
+        ...dartScope.variables.keys.map(
           (variable) => dartNameToJsValue[variable],
         ),
       ];
@@ -412,7 +411,7 @@ class ExpressionCompiler {
     }
     var procedure = await _compiler.compileExpression(
       expression,
-      scope.definitions,
+      scope.variablesAsMapToType(),
       scope.typeParameters,
       debugProcedureName,
       scope.library.importUri,

pkg/dev_compiler/test/scopes/scope_test.dart

@@ -109,7 +109,7 @@ class ScopeDataExtractor extends DdcDataExtractor<Features> {
             printer.writeTypeParameterName(typeParameter);
             features.addElement(Tags.typeParameter, printer.getText());
           }
-          for (var variable in scope.definitions.keys) {
+          for (var variable in scope.variables.keys) {
             features.addElement(Tags.variables, variable);
           }
           return features;

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

@@ -26,6 +26,7 @@ import 'package:kernel/kernel.dart'
     show
         Class,
         Component,
+        ConstantExpression,
         DartType,
         DynamicType,
         Expression,
@@ -1644,6 +1645,7 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
         return null;
       }
       LibraryBuilder libraryBuilder = compilationUnit.libraryBuilder;
+      List<VariableDeclarationImpl> extraKnownVariables = [];
       if (scriptUri != null && offset != TreeNode.noOffset) {
         Uri? scriptUriAsUri = Uri.tryParse(scriptUri);
         if (scriptUriAsUri != null) {
@@ -1667,19 +1669,35 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
           }
           DartScope foundScope = DartScopeBuilder2.findScopeFromOffsetAndClass(
               library, scriptUriAsUri, cls, offset);
+          final bool alwaysInlineConstants = lastGoodKernelTarget
+              .backendTarget.constantsBackend.alwaysInlineConstants;
           // For now, if any definition is (or contains) an Extension Type,
           // we'll overwrite the given (runtime?) definitions so we know about
           // the extension type. If any definition is said to be dynamic we'll
           // overwrite as well because that mostly means that the value is
           // currently null. This can also mean that the VM can't send over the
           // information - this for instance happens for function types.
-          for (MapEntry<String, DartType> def
-              in foundScope.definitions.entries) {
+          for (MapEntry<String, VariableDeclaration> def
+              in foundScope.variables.entries) {
             DartType? existingType = usedDefinitions[def.key];
-            if (existingType == null) continue;
-            if (existingType is DynamicType ||
-                _ExtensionTypeFinder.isOrContainsExtensionType(def.value)) {
-              usedDefinitions[def.key] = def.value;
+            if (existingType == null) {
+              // We found a variable, but we weren't told about it.
+              // For now we'll only do something special if it's a const
+              // variable that will be inlined.
+              if (alwaysInlineConstants &&
+                  def.value.isConst &&
+                  def.value.initializer is ConstantExpression) {
+                extraKnownVariables.add(new VariableDeclarationImpl(def.key,
+                    type: def.value.type,
+                    isConst: true,
+                    hasDeclaredInitializer: true,
+                    initializer: def.value.initializer)
+                  ..fileOffset = def.value.fileOffset);
+              }
+            } else if (existingType is DynamicType ||
+                _ExtensionTypeFinder.isOrContainsExtensionType(
+                    def.value.type)) {
+              usedDefinitions[def.key] = def.value.type;
             }
           }
         }
@@ -1920,7 +1938,8 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
               className ?? extensionName,
               (className != null && !isStatic) || extensionThis != null,
               procedure,
-              extensionThis);
+              extensionThis,
+              extraKnownVariables);
 
       parameters.body = new ReturnStatement(compiledExpression)
         ..parent = parameters;

pkg/front_end/lib/src/kernel/body_builder.dart

@@ -1587,7 +1587,10 @@ class BodyBuilder extends StackListenerImpl
 
   // Coverage-ignore(suite): Only used in expression compilation.
   Expression parseSingleExpression(
-      Parser parser, Token token, FunctionNode parameters) {
+      Parser parser,
+      Token token,
+      FunctionNode parameters,
+      List<VariableDeclarationImpl> extraKnownVariables) {
     int fileOffset = offsetForToken(token);
     List<NominalParameterBuilder>? typeParameterBuilders;
     for (TypeParameter typeParameter in parameters.typeParameters) {
@@ -1629,6 +1632,17 @@ class BodyBuilder extends StackListenerImpl
       wildcardVariablesEnabled: libraryFeatures.wildcardVariables.isEnabled,
     ));
 
+    if (extraKnownVariables.isNotEmpty) {
+      LocalScope extraKnownVariablesScope = _localScope.createNestedScope(
+          debugName: "expression compilation extra known variables scope",
+          kind: ScopeKind.ifElement);
+      enterLocalScope(extraKnownVariablesScope);
+      for (VariableDeclarationImpl extraVariable in extraKnownVariables) {
+        declareVariable(extraVariable, _localScope);
+        typeInferrer.assignedVariables.declare(extraVariable);
+      }
+    }
+
     Token endToken =
         parser.parseExpression(parser.syntheticPreviousToken(token));
 
@@ -1657,6 +1671,12 @@ class BodyBuilder extends StackListenerImpl
             initialized: true);
       }
     }
+    for (VariableDeclarationImpl extraVariable in extraKnownVariables) {
+      typeInferrer.flowAnalysis.declare(
+          extraVariable, new SharedTypeView(extraVariable.type),
+          initialized: true);
+    }
+
     InferredFunctionBody inferredFunctionBody = typeInferrer.inferFunctionBody(
         this, fileOffset, const DynamicType(), AsyncMarker.Sync, fakeReturn);
     assert(

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

@@ -407,7 +407,10 @@ class ConstantsTransformer extends RemovingTransformer {
             makeConstantExpression(constant, initializer)..parent = node;
 
         // If this constant is inlined, remove it.
-        if (!keepLocals && shouldInline(initializer)) {
+        if (!keepLocals &&
+            // Coverage-ignore(suite): Not run.
+            shouldInline(initializer)) {
+          // Coverage-ignore-block(suite): Not run.
           if (constant is! UnevaluatedConstant) {
             // If the constant is unevaluated we need to keep the expression,
             // so that, in the case the constant contains error but the local
@@ -590,6 +593,7 @@ class ConstantsTransformer extends RemovingTransformer {
       }
     }
     if (storeIndex < node.statements.length) {
+      // Coverage-ignore-block(suite): Not run.
       node.statements.length = storeIndex;
     }
     return node;

pkg/front_end/lib/src/source/source_loader.dart

@@ -19,6 +19,8 @@ import 'package:_fe_analyzer_shared/src/scanner/scanner.dart'
         ScannerResult,
         Token,
         scan;
+import 'package:front_end/src/kernel/internal_ast.dart'
+    show VariableDeclarationImpl;
 import 'package:kernel/ast.dart';
 import 'package:kernel/class_hierarchy.dart' show ClassHierarchy;
 import 'package:kernel/core_types.dart' show CoreTypes;
@@ -1246,7 +1248,8 @@ severity: $severity
       String? enclosingClassOrExtension,
       bool isClassInstanceMember,
       Procedure procedure,
-      VariableDeclaration? extensionThis) async {
+      VariableDeclaration? extensionThis,
+      List<VariableDeclarationImpl> extraKnownVariables) async {
     // TODO(johnniwinther): Support expression compilation in a specific
     //  compilation unit.
     LookupScope memberScope =
@@ -1311,7 +1314,8 @@ severity: $severity
             enableFeatureEnhancedParts:
                 libraryBuilder.libraryFeatures.enhancedParts.isEnabled),
         token,
-        procedure.function);
+        procedure.function,
+        extraKnownVariables);
   }
 
   DietListener createDietListener(SourceLibraryBuilder library,

pkg/front_end/test/scopes/scope_test.dart

@@ -85,7 +85,7 @@ class ScopeDataExtractor extends CfeDataExtractor<Features> {
           printer.writeTypeParameterName(typeParameter);
           features.addElement(Tags.typeParameter, printer.getText());
         }
-        for (String variable in scope.definitions.keys) {
+        for (String variable in scope.variables.keys) {
           features.addElement(Tags.variables, variable);
         }
         return features;

pkg/front_end/test/spell_checking_list_code.txt

@@ -875,6 +875,7 @@ indentations
 independently
 indexer
 indexing
+indicator
 indirection
 individual
 individually
@@ -1939,6 +1940,7 @@ tokenize
 tokenized
 tokenizer
 tokenizing
+told
 tolerant
 tolerate
 tolerated