Skip to content

Commit

Permalink
[CFE/VM] Fix expression evaluation to not introduce new parameters be…
Browse files Browse the repository at this point in the history
…cause of extension types

To support expression evaluation on extension types - which are a static
thing and thus doesn't exist in the VM - on the CFE side we try to find
the available variables so that, for any variables that are found to be
extension types, we can treat them as such. This was introduced in
https://dart-review.googlesource.com/c/sdk/+/339900.

The problem is that this could accidentally _introduce_ new variables,
which would introduce new arguments to the created procedure causing
weird behavior in the VM as for instance seen in
#56911.

This CL makes sure to only set the type for already known variables (as
passed by the VM), thus not introducing new variables and fixing the
issues seen.

Fixes #56911

Change-Id: Ie8b8ae12438338e9c3d248d00cc5da81df5b8ece
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/400120
Reviewed-by: Johnni Winther <[email protected]>
Reviewed-by: Derek Xu <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>
  • Loading branch information
jensjoha authored and Commit Queue committed Dec 13, 2024
1 parent 9f111ce commit 91ba697
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 10 deletions.
1 change: 1 addition & 0 deletions pkg/front_end/lib/src/base/incremental_compiler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1871,6 +1871,7 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
// the extension type.
for (MapEntry<String, DartType> def
in foundScope.definitions.entries) {
if (!usedDefinitions.containsKey(def.key)) continue;
if (_ExtensionTypeFinder.isOrContainsExtensionType(def.value)) {
usedDefinitions[def.key] = def.value;
}
Expand Down
65 changes: 56 additions & 9 deletions pkg/front_end/test/expression_suite.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ import "package:kernel/ast.dart"
Member,
Procedure,
TreeNode,
TypeParameter;
TypeParameter,
VariableDeclaration;
import 'package:kernel/target/targets.dart' show TargetFlags;
import 'package:kernel/text/ast_to_text.dart' show Printer;
import "package:testing/src/log.dart" show splitLines;
Expand Down Expand Up @@ -72,7 +73,8 @@ class Context extends ChainContext {
const ReadTest(),
const CompileExpression(),
new MatchProcedureExpectations(".expect",
updateExpectations: updateExpectations)
updateExpectations: updateExpectations),
const OutputParametersMatches(),
];

ProcessedOptions get options => compilerContext.options;
Expand Down Expand Up @@ -194,7 +196,51 @@ class TestCase {
}
}

class MatchProcedureExpectations extends Step<List<TestCase>, Null, Context> {
class OutputParametersMatches
extends Step<List<TestCase>, List<TestCase>, Context> {
const OutputParametersMatches();

@override
String get name => "output parameters matches";

@override
Future<Result<List<TestCase>>> run(List<TestCase> tests, Context context) {
for (TestCase test in tests) {
for (CompilationResult result in test.results) {
Procedure? compiledProcedure = result.compiledProcedure;
if (compiledProcedure == null) continue;
if (compiledProcedure.function.namedParameters.isNotEmpty) {
return Future.value(
fail(tests, "Compiled expression contains named parameters."));
}
List<VariableDeclaration> positionals =
compiledProcedure.function.positionalParameters;
if (positionals.length != test.definitions.length) {
return Future.value(fail(
tests,
"Compiled expression contains a wrong number of "
"positional parameters: Expected ${test.definitions.length} "
"(${test.definitions.join(", ")}) "
"but had ${positionals.length} "
"(${positionals.map((p) => p.name).join(", ")})."));
}
for (int i = 0; i < positionals.length; i++) {
if (positionals[i].name != test.definitions[i]) {
return Future.value(fail(
tests,
"Compiled expression doesn't contain '${test.definitions[i]}' "
"but '${positionals[i].name}' as positional parameter $i."));
}
}
}
}

return Future.value(pass(tests));
}
}

class MatchProcedureExpectations
extends Step<List<TestCase>, List<TestCase>, Context> {
final String suffix;
final bool updateExpectations;

Expand All @@ -205,7 +251,8 @@ class MatchProcedureExpectations extends Step<List<TestCase>, Null, Context> {
String get name => "match expectations";

@override
Future<Result<Null>> run(List<TestCase> tests, Context context) async {
Future<Result<List<TestCase>>> run(
List<TestCase> tests, Context context) async {
String actual = "";
for (TestCase test in tests) {
String primary = test.results.first.printResult(test.entryPoint, context);
Expand All @@ -215,7 +262,7 @@ class MatchProcedureExpectations extends Step<List<TestCase>, Null, Context> {
test.results[i].printResult(test.entryPoint, context);
if (primary != secondary) {
return fail(
null,
tests,
"Multiple expectations don't match on $test:"
"\nFirst expectation:\n$actual\n"
"\nSecond expectation:\n$secondary\n");
Expand All @@ -231,19 +278,19 @@ class MatchProcedureExpectations extends Step<List<TestCase>, Null, Context> {
if (!updateExpectations) {
String diff = await runDiff(expectedFile.uri, actual);
return fail(
null, "$testUri doesn't match ${expectedFile.uri}\n$diff");
tests, "$testUri doesn't match ${expectedFile.uri}\n$diff");
}
} else {
return pass(null);
return pass(tests);
}
}
if (updateExpectations) {
await openWrite(expectedFile.uri, (IOSink sink) {
sink.writeln(actual.trim());
});
return pass(null);
return pass(tests);
} else {
return fail(null, """
return fail(tests, """
Please create file ${expectedFile.path} with this content:
$actual""");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Errors: {
}
static method /* from org-dartlang-debug:synthetic_debug_expression */ debugExpr(#lib1::ExtensionType% /* erasure=dart.core::String, declared=! */ input, dart.core::List<#lib1::ExtensionType% /* erasure=dart.core::String, declared=! */> list) → dynamic
static method /* from org-dartlang-debug:synthetic_debug_expression */ debugExpr(#lib1::ExtensionType% /* erasure=dart.core::String, declared=! */ input) → dynamic
return #lib1::ExtensionType|get#value(input);
1 change: 1 addition & 0 deletions pkg/pkg.status
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ vm_service/test/issue_25465_test: SkipByDesign # Debugger is disabled in AOT mod
vm_service/test/issue_27238_test: SkipByDesign # Debugger is disabled in AOT mode.
vm_service/test/issue_27287_test: SkipByDesign # Debugger is disabled in AOT mode.
vm_service/test/issue_30555_test: SkipByDesign # Debugger is disabled in AOT mode.
vm_service/test/issue_56911_test: SkipByDesign # Debugger is disabled in AOT mode.
vm_service/test/kill_paused_test: SkipByDesign # Debugger is disabled in AOT mode.
vm_service/test/library_dependency_test: SkipByDesign # Uses 'dart:mirrors' library.
vm_service/test/local_variable_declaration_test: SkipByDesign # Debugger is disabled in AOT mode.
Expand Down
96 changes: 96 additions & 0 deletions pkg/vm_service/test/issue_56911_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// Copyright (c) 2024, 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.

import 'dart:developer';
import 'package:test/test.dart';
import 'package:vm_service/vm_service.dart';
import 'common/service_test_common.dart';
import 'common/test_helper.dart';

// AUTOGENERATED START
//
// Update these constants by running:
//
// dart pkg/vm_service/test/update_line_numbers.dart <test.dart>
//
const LINE_A = 28;
const LINE_B = 32;
// AUTOGENERATED END

extension type ExtensionType._(String s) {
ExtensionType(int i) : this._('$i');
int get value => s.codeUnitAt(0);
}

void code() {
final list = [ExtensionType(0)];
debugger(); // LINE_A
print(list.single.value);
// ignore: avoid_function_literals_in_foreach_calls
list.forEach((ExtensionType input) {
debugger(); // LINE_B
print(input.value);
});
}

Future<void> Function(VmService, IsolateRef) test(
String topFrameName,
List<String> availableVariables,
List<(String evaluate, String evaluationResult)> evaluations,
) {
return (VmService service, IsolateRef isolateRef) async {
final isolateId = isolateRef.id!;
final stack = await service.getStack(isolateId);

// Make sure we are in the right place.
expect(stack.frames!.length, greaterThanOrEqualTo(1));
expect(stack.frames![0].function!.name, topFrameName);

// Check variables.
expect(
(stack.frames![0].vars ?? []).map((v) => v.name).toList(),
equals(availableVariables),
);

// Evaluate.
for (final (expression, expectedResult) in evaluations) {
final result = await service.evaluateInFrame(
isolateId,
/* frame = */ 0,
expression,
) as InstanceRef;
print(result.valueAsString);
expect(result.valueAsString, equals(expectedResult));
}
};
}

final tests = <IsolateTest>[
hasStoppedAtBreakpoint,
stoppedAtLine(LINE_A),
test('code', [
'list',
], [
('() { return list.single.value; }()', '48'),
('list.single.value', '48'),
]),
resumeIsolate,
hasStoppedAtBreakpoint,
stoppedAtLine(LINE_B),
test('<anonymous closure>', [
'input',
], [
('() { return input.value; }()', '48'),
('input.value', '48'),
]),
];

void main([args = const <String>[]]) => runIsolateTests(
args,
tests,
'issue_56911_test.dart',
testeeConcurrent: code,
pauseOnStart: false,
pauseOnExit: true,
);

0 comments on commit 91ba697

Please sign in to comment.