From 0eac43bf6f68d97b7795bd915f69ac37ff1510d2 Mon Sep 17 00:00:00 2001 From: Tess Strickland Date: Tue, 30 Jan 2024 12:48:49 +0000 Subject: [PATCH] [pkg/vm] Enable const functions during platform const field evaluation. This allows more complicated field initializers that are written using immediately invoked closures, like @pragma("vm:platform-const") final bool foo = () { ... do some stuff ... }(); to be properly evaluated by the VM constant evaluator. Also throws errors at compile time if the annotated member cannot be evaluated to a constant or if an invalid member (not an initialized static field or a static getter) is annotated. TEST=pkg/vm/test/transformations/vm_constant_evaluator_test Issue: https://github.com/dart-lang/sdk/issues/31969 Issue: https://github.com/dart-lang/sdk/issues/50473 Issue: https://github.com/flutter/flutter/issues/14233 Change-Id: I14be498bb5f7771f0f339baf7d3b1bec7df5903f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/348380 Reviewed-by: Johnni Winther Commit-Queue: Tess Strickland Reviewed-by: Alexander Markov --- .../src/fasta/kernel/constant_evaluator.dart | 2 +- .../unreachable_code_elimination.dart | 74 ++++++- .../vm_constant_evaluator.dart | 38 ++-- .../vm_constant_evaluator_test.dart | 186 +++++++++++++++--- .../errors/not_constant_field.dart | 12 ++ .../not_constant_field.dart.linux.expect | 6 + .../errors/not_constant_field.dart.options | 3 + .../errors/not_constant_getter.dart | 14 ++ .../not_constant_getter.dart.linux.expect | 6 + .../errors/not_constant_getter.dart.options | 3 + .../errors/not_getter.dart | 8 + .../errors/not_getter.dart.linux.expect | 4 + .../errors/not_getter.dart.options | 3 + .../errors/not_static_field.dart | 14 ++ .../errors/not_static_field.dart.linux.expect | 4 + .../errors/not_static_field.dart.options | 3 + .../errors/not_static_method.dart | 12 ++ .../not_static_method.dart.linux.expect | 4 + .../errors/not_static_method.dart.options | 3 + .../errors/possible_assert.dart | 17 ++ ...sible_assert.dart.linux.withAsserts.expect | 6 + .../errors/possible_assert.dart.options | 3 + .../{ => successes}/platform_testcases.dart | 18 +- ...atform_testcases.dart.android.debug.expect | 34 ++-- ...ases.dart.android.debug.withAsserts.expect | 39 ++-- .../platform_testcases.dart.android.expect | 49 +++-- ..._testcases.dart.android.withAsserts.expect | 34 ++-- ...atform_testcases.dart.fuchsia.debug.expect | 34 ++-- ...ases.dart.fuchsia.debug.withAsserts.expect | 39 ++-- .../platform_testcases.dart.fuchsia.expect | 49 +++-- ..._testcases.dart.fuchsia.withAsserts.expect | 34 ++-- .../platform_testcases.dart.ios.debug.expect | 34 ++-- ...estcases.dart.ios.debug.withAsserts.expect | 39 ++-- .../platform_testcases.dart.ios.expect | 49 +++-- ...form_testcases.dart.ios.withAsserts.expect | 34 ++-- ...platform_testcases.dart.linux.debug.expect | 34 ++-- ...tcases.dart.linux.debug.withAsserts.expect | 39 ++-- .../platform_testcases.dart.linux.expect | 49 +++-- ...rm_testcases.dart.linux.withAsserts.expect | 34 ++-- ...platform_testcases.dart.macos.debug.expect | 34 ++-- ...tcases.dart.macos.debug.withAsserts.expect | 39 ++-- .../platform_testcases.dart.macos.expect | 49 +++-- ...rm_testcases.dart.macos.withAsserts.expect | 34 ++-- ...atform_testcases.dart.windows.debug.expect | 34 ++-- ...ases.dart.windows.debug.withAsserts.expect | 39 ++-- .../platform_testcases.dart.windows.expect | 49 +++-- ..._testcases.dart.windows.withAsserts.expect | 34 ++-- 47 files changed, 876 insertions(+), 503 deletions(-) create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_constant_field.dart create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_constant_field.dart.linux.expect create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_constant_field.dart.options create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_constant_getter.dart create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_constant_getter.dart.linux.expect create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_constant_getter.dart.options create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_getter.dart create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_getter.dart.linux.expect create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_getter.dart.options create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_static_field.dart create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_static_field.dart.linux.expect create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_static_field.dart.options create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_static_method.dart create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_static_method.dart.linux.expect create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/not_static_method.dart.options create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/possible_assert.dart create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/possible_assert.dart.linux.withAsserts.expect create mode 100644 pkg/vm/testcases/transformations/vm_constant_evaluator/errors/possible_assert.dart.options rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart (93%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.android.debug.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.android.debug.withAsserts.expect (89%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.android.expect (85%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.android.withAsserts.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.fuchsia.debug.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.fuchsia.debug.withAsserts.expect (89%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.fuchsia.expect (85%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.fuchsia.withAsserts.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.ios.debug.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.ios.debug.withAsserts.expect (89%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.ios.expect (85%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.ios.withAsserts.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.linux.debug.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.linux.debug.withAsserts.expect (89%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.linux.expect (85%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.linux.withAsserts.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.macos.debug.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.macos.debug.withAsserts.expect (89%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.macos.expect (85%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.macos.withAsserts.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.windows.debug.expect (90%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.windows.debug.withAsserts.expect (89%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.windows.expect (85%) rename pkg/vm/testcases/transformations/vm_constant_evaluator/{ => successes}/platform_testcases.dart.windows.withAsserts.expect (90%) diff --git a/pkg/front_end/lib/src/fasta/kernel/constant_evaluator.dart b/pkg/front_end/lib/src/fasta/kernel/constant_evaluator.dart index b39706c09b98..f07659bd45cf 100644 --- a/pkg/front_end/lib/src/fasta/kernel/constant_evaluator.dart +++ b/pkg/front_end/lib/src/fasta/kernel/constant_evaluator.dart @@ -2451,7 +2451,7 @@ class ConstantEvaluator implements ExpressionVisitor { final bool enableTripleShift; final bool enableAsserts; - bool enableConstFunctions; + final bool enableConstFunctions; bool inExtensionTypeConstConstructor = false; final Map canonicalizationCache; diff --git a/pkg/vm/lib/transformations/unreachable_code_elimination.dart b/pkg/vm/lib/transformations/unreachable_code_elimination.dart index 4c6b788c64db..6230c32b8567 100644 --- a/pkg/vm/lib/transformations/unreachable_code_elimination.dart +++ b/pkg/vm/lib/transformations/unreachable_code_elimination.dart @@ -32,10 +32,56 @@ class SimpleUnreachableCodeElimination extends RemovingTransformer { SimpleUnreachableCodeElimination(this.constantEvaluator, {required this.enableAsserts, required this.soundNullSafety}); + bool _shouldEvaluateMember(Member node) => + constantEvaluator.hasTargetOS && constantEvaluator.isPlatformConst(node); + + Never _throwPlatformConstError(Member node, String message) { + final uri = constantEvaluator.getFileUri(node); + final offset = constantEvaluator.getFileOffset(uri, node); + throw PlatformConstError(message, node, uri, offset); + } + + void _checkPlatformConstMember(Member node) { + if (node is Field) { + if (!node.isStatic) { + _throwPlatformConstError(node, 'not a static field'); + } + // Static fields currently always have an initializer set, even if it's + // the implicit null initializer for a nullable field. + assert(node.initializer != null); + } else if (node is Procedure) { + if (!node.isStatic) { + _throwPlatformConstError(node, 'not a static method'); + } + if (!node.isGetter) { + _throwPlatformConstError(node, 'not a getter'); + } + } else { + _throwPlatformConstError(node, 'not a field or method'); + } + } + @override TreeNode defaultMember(Member node, TreeNode? removalSentinel) { _staticTypeContext = StaticTypeContext(node, constantEvaluator.typeEnvironment); + if (_shouldEvaluateMember(node)) { + _checkPlatformConstMember(node); + // Create a StaticGet to ensure the member is evaluated at least once, + // and then replace the field initializer or getter body with the result. + final staticGet = StaticGet(node)..fileOffset = node.fileOffset; + final result = + staticGet.accept1(this, cannotRemoveSentinel) as ConstantExpression; + if (node is Field) { + node.initializer = result + ..fileOffset = node.initializer!.fileOffset + ..parent = node; + } else if (node is Procedure) { + node.function.body = ReturnStatement(result) + ..fileOffset = node.function.body!.fileOffset + ..parent = node.function; + } + } final result = super.defaultMember(node, removalSentinel); _staticTypeContext = null; return result; @@ -205,20 +251,17 @@ class SimpleUnreachableCodeElimination extends RemovingTransformer { throw 'StaticGet from const field $target should be evaluated by front-end: $node'; } - if (!constantEvaluator.hasTargetOS || - !constantEvaluator.isPlatformConst(target)) { + if (!_shouldEvaluateMember(target)) { return super.visitStaticGet(node, removalSentinel); } - final result = constantEvaluator.evaluate(_staticTypeContext!, node); - - if (result is UnevaluatedConstant && - result.expression is InvalidExpression) { - return result.expression; + _checkPlatformConstMember(target); + final constant = constantEvaluator.evaluate(_staticTypeContext!, node); + if (constant is UnevaluatedConstant) { + _throwPlatformConstError(target, 'cannot evaluate to a constant'); } - final type = node.getStaticType(_staticTypeContext!); - return ConstantExpression(result, type)..fileOffset = node.fileOffset; + return ConstantExpression(constant, type)..fileOffset = node.fileOffset; } @override @@ -335,3 +378,16 @@ class ContinueSwitchStatementTargetCollector extends RecursiveVisitor { } } } + +class PlatformConstError extends Error { + final Object? message; + final Member member; + final Uri? uri; + final int offset; + + PlatformConstError(this.message, this.member, this.uri, this.offset); + + @override + String toString() => '${uri ?? ''}:$offset ' + 'Error for annotated member ${member.name}: $message'; +} diff --git a/pkg/vm/lib/transformations/vm_constant_evaluator.dart b/pkg/vm/lib/transformations/vm_constant_evaluator.dart index 2a09c0e13031..cdec0ad9ec8c 100644 --- a/pkg/vm/lib/transformations/vm_constant_evaluator.dart +++ b/pkg/vm/lib/transformations/vm_constant_evaluator.dart @@ -23,9 +23,9 @@ import 'pragma.dart'; /// fields and getters annotated with "vm:platform-const". /// /// To avoid restricting getters annotated with "vm:platform-const" to be just -/// a single return statement whose body is evaluated, we treat annotated -/// getters as const functions. If [enableConstFunctions] is false, then -/// only annotated getters are treated this way. +/// a single return statement whose body is evaluated, as well as handling +/// immediately-invoked closures wrapping complex initializing code in field +/// initializers, we enable constant evaluation of functions. class VMConstantEvaluator extends ConstantEvaluator { final TargetOS? _targetOS; final Map _constantFields = {}; @@ -43,7 +43,6 @@ class VMConstantEvaluator extends ConstantEvaluator { this._targetOS, this._pragmaParser, {bool enableTripleShift = false, - bool enableConstFunctions = false, bool enableAsserts = true, bool errorOnUnevaluatedConstant = false, EvaluationMode evaluationMode = EvaluationMode.weak}) @@ -51,7 +50,9 @@ class VMConstantEvaluator extends ConstantEvaluator { super(dartLibrarySupport, backend, component, environmentDefines, typeEnvironment, errorReporter, enableTripleShift: enableTripleShift, - enableConstFunctions: enableConstFunctions, + // We use evaluation of const functions for getters and immediately + // invoked closures in field initializers. + enableConstFunctions: true, enableAsserts: enableAsserts, errorOnUnevaluatedConstant: errorOnUnevaluatedConstant, evaluationMode: evaluationMode) { @@ -68,10 +69,10 @@ class VMConstantEvaluator extends ConstantEvaluator { Target target, Component component, TargetOS? targetOS, NnbdMode nnbdMode, {bool evaluateAnnotations = true, bool enableTripleShift = false, - bool enableConstFunctions = false, bool enableConstructorTearOff = false, bool enableAsserts = true, bool errorOnUnevaluatedConstant = false, + ErrorReporter? errorReporter, Map? environmentDefines, CoreTypes? coreTypes, ClassHierarchy? hierarchy}) { @@ -90,11 +91,10 @@ class VMConstantEvaluator extends ConstantEvaluator { component, environmentDefines, typeEnvironment, - const SimpleErrorReporter(), + errorReporter ?? const SimpleErrorReporter(), targetOS, ConstantPragmaAnnotationParser(coreTypes, target), enableTripleShift: enableTripleShift, - enableConstFunctions: enableConstFunctions, enableAsserts: enableAsserts, errorOnUnevaluatedConstant: errorOnUnevaluatedConstant, evaluationMode: EvaluationMode.fromNnbdMode(nnbdMode)); @@ -128,24 +128,14 @@ class VMConstantEvaluator extends ConstantEvaluator { } } - final initializer = target.initializer; - if (initializer == null) { - throw 'Cannot const evaluate annotated field with no initializer'; - } + // The base class only evaluates const fields, so the VM constant + // evaluator must manually request the initializer be evaluated. + // instead of just calling super.visitStaticGet(target). return withNewEnvironment( - () => evaluateExpressionInContext(target, initializer)); - } - - if (target is Procedure && target.kind == ProcedureKind.Getter) { - // Temporarily enable const functions and use the base class to evaluate - // the getter with appropriate caching/recursive evaluation checks. - final oldEnableConstFunctions = enableConstFunctions; - enableConstFunctions = true; - final result = super.visitStaticGet(node); - enableConstFunctions = oldEnableConstFunctions; - return result; + () => evaluateExpressionInContext(target, target.initializer!)); } - throw 'Expected annotated field with initializer or getter, got $target'; + // The base class already handles constant evaluation of a getter. + return super.visitStaticGet(node); } } diff --git a/pkg/vm/test/transformations/vm_constant_evaluator_test.dart b/pkg/vm/test/transformations/vm_constant_evaluator_test.dart index 7b944c542eeb..4b89dcf86f8b 100644 --- a/pkg/vm/test/transformations/vm_constant_evaluator_test.dart +++ b/pkg/vm/test/transformations/vm_constant_evaluator_test.dart @@ -4,7 +4,9 @@ import 'dart:io'; -import 'package:front_end/src/base/nnbd_mode.dart'; +import 'package:front_end/src/api_unstable/vm.dart'; +import 'package:front_end/src/fasta/kernel/constant_evaluator.dart' + show SimpleErrorReporter; import 'package:kernel/target/targets.dart'; import 'package:kernel/ast.dart'; import 'package:kernel/kernel.dart'; @@ -14,19 +16,46 @@ import 'package:test/test.dart'; import 'package:vm/target_os.dart'; import 'package:vm/target/vm.dart' show VmTarget; import 'package:vm/transformations/unreachable_code_elimination.dart' - show transformComponent; + show PlatformConstError, transformComponent; import 'package:vm/transformations/vm_constant_evaluator.dart'; import '../common_test_utils.dart'; final String pkgVmDir = Platform.script.resolve('../..').toFilePath(); +class TestErrorReporter extends SimpleErrorReporter { + final reportedMessages = []; + + TestErrorReporter(); + + @override + void reportMessage(Uri? uri, int offset, String message) { + final buffer = StringBuffer(); + if (offset >= 0) { + if (uri != null) { + buffer + ..write(uri.pathSegments.last) + ..write(':'); + } + buffer + ..write(offset) + ..write(' '); + } + buffer + ..write('Constant evaluation error: ') + ..write(message); + reportedMessages.add(buffer.toString()); + } +} + class TestCase { final TargetOS os; final bool debug; final bool enableAsserts; + final bool throws; - const TestCase(this.os, {required this.debug, required this.enableAsserts}); + const TestCase(this.os, + {required this.debug, required this.enableAsserts, this.throws = false}); String postfix() { String result = '.${os.name}'; @@ -40,6 +69,17 @@ class TestCase { } } +class TestOptions { + static const Option debug = Option('--debug', BoolValue(null)); + + static const Option enableAsserts = + Option('--enable-asserts', BoolValue(null)); + + static const Option targetOS = Option('--target-os', StringValue()); + + static const List