[ddc] Use TemporaryId when emitting all kernel VariableDeclaration references.
Some CFE lowerings (e.g. pattern lowerings) result in nested scopes containing VariableDeclarations with the same 'name'. The current DDC transform translates these to the exact same name in JS leading to incorrect semantics.
The `TemporaryId` mechanism automatically renames any variables with the same name that would shadow each other. So we re-use that here to ensure the variables all have a unique name if the CFE hasn't already given them one. If the name is already okay (i.e. not shadowing something else), the name in JS will appear unchanged.
Side note: In a future change perhaps we should rename `TemporaryId`. The general mechanism it implements is more useful than its original intended use.
Fixes: #59613
Change-Id: I708c72528d5df19af48dde01163d375a5588baae
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398504
Commit-Queue: Nate Biggs <natebiggs@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Nicholas Shahan <nshahan@google.com>
diff --git a/pkg/dev_compiler/lib/src/compiler/js_names.dart b/pkg/dev_compiler/lib/src/compiler/js_names.dart
index 33532da..f4db161 100644
--- a/pkg/dev_compiler/lib/src/compiler/js_names.dart
+++ b/pkg/dev_compiler/lib/src/compiler/js_names.dart
@@ -24,31 +24,31 @@
}
/// Unique instance for temporary variables. Will be renamed consistently
-/// across the entire file. Different instances will be named differently
-/// even if they have the same name, this makes it safe to use in code
-/// generation without needing global knowledge. See [TemporaryNamer].
+/// across the entire file. Instances with different IDs will be named
+/// differently even if they have the same name, this makes it safe to use in
+/// code generation without needing global knowledge. See [TemporaryNamer].
// TODO(jmesserly): move into js_ast? add a boolean to Identifier?
class TemporaryId extends Identifier {
- // TODO(jmesserly): by design, temporary identifier nodes are shared
- // throughout the AST, so any source information we attach in one location
- // be incorrect for another location (and overwrites previous data).
- //
- // If we want to track source information for temporary variables, we'll
- // need to separate the identity of the variable from its Identifier.
- //
- // In practice that makes temporaries more difficult to use: they're no longer
- // JS AST nodes, so `toIdentifier()` is required to put them in the JS AST.
- // And anywhere we currently use type `Identifier` to hold Identifier or
- // TemporaryId, those types would need to change to `Identifier Function()`.
- //
- // However we may need to fix this if we want hover to work well for things
- // like library prefixes and field-initializing formals.
- @override
- dynamic get sourceInformation => null;
- @override
- set sourceInformation(Object? obj) {}
+ final int _id;
- TemporaryId(super.name);
+ @override
+ TemporaryId withSourceInformation(dynamic sourceInformation) =>
+ TemporaryId.from(this)..sourceInformation = sourceInformation;
+
+ static int _idCounter = 0;
+
+ TemporaryId(super.name) : _id = _idCounter++;
+ TemporaryId.from(TemporaryId other)
+ : _id = other._id,
+ super(other.name);
+
+ @override
+ int get hashCode => _id;
+
+ @override
+ bool operator ==(Object other) {
+ return other is TemporaryId && other._id == _id;
+ }
}
/// Creates a qualified identifier, without determining for sure if it needs to
diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart
index f9cb5a8..3c06a23 100644
--- a/pkg/dev_compiler/lib/src/kernel/compiler.dart
+++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart
@@ -97,6 +97,15 @@
@override
final variableIdentifiers = <VariableDeclaration, js_ast.Identifier>{};
+ /// Identifiers for kernel variables with an analgous identifier in JS.
+ ///
+ /// [VariableDeclaration.name] is not necessarily a safe identifier for JS
+ /// transpiled code. The same name can be used in shadowing contexts. We map
+ /// each kernel variable to a [js_ast.TemporaryId] so that at code emission
+ /// time, declarations that would be shadowed are given a unique name. If
+ /// there is no risk of shadowing, the original name will be used.
+ final Map<VariableDeclaration, js_ast.TemporaryId> _variableTempIds = {};
+
/// Maps a library URI import, that is not in [_libraries], to the
/// corresponding Kernel summary module we imported it with.
///
@@ -4930,7 +4939,8 @@
var v = node.variable;
var id = _emitVariableRef(v);
if (id.name == v.name) {
- id.sourceInformation = _variableSpan(node.fileOffset, v.name!.length);
+ id = id.withSourceInformation(
+ _variableSpan(node.fileOffset, v.name!.length));
}
return id;
}
@@ -4964,7 +4974,7 @@
return null;
}
- js_ast.Identifier _emitVariableRef(VariableDeclaration v) {
+ js_ast.TemporaryId _emitVariableRef(VariableDeclaration v) {
if (_isTemporaryVariable(v)) {
var name = _debuggerFriendlyTemporaryVariableName(v);
name ??= 't\$${_tempVariables.length}';
@@ -4978,7 +4988,8 @@
// See https://github.com/dart-lang/sdk/issues/55918
name = extractLocalNameFromLateLoweredLocal(name);
}
- return _emitIdentifier(name);
+ return js_ast.TemporaryId.from(
+ _variableTempIds[v] ??= _emitTemporaryId(name));
}
/// Emits the declaration of a variable.
diff --git a/pkg/dev_compiler/lib/src/kernel/compiler_new.dart b/pkg/dev_compiler/lib/src/kernel/compiler_new.dart
index b5bc36f..eaf96de 100644
--- a/pkg/dev_compiler/lib/src/kernel/compiler_new.dart
+++ b/pkg/dev_compiler/lib/src/kernel/compiler_new.dart
@@ -248,6 +248,15 @@
Map<VariableDeclaration, js_ast.Identifier> get variableIdentifiers =>
_symbolData.variableIdentifiers;
+ /// Identifiers for kernel variables with an analgous identifier in JS.
+ ///
+ /// [VariableDeclaration.name] is not necessarily a safe identifier for JS
+ /// transpiled code. The same name can be used in shadowing contexts. We map
+ /// each kernel variable to a [js_ast.TemporaryId] so that at code emission
+ /// time, references that would be shadowed are given a unique name. If there
+ /// is no risk of shadowing, the original name will be used.
+ final Map<VariableDeclaration, js_ast.TemporaryId> _variableTempIds = {};
+
/// Maps a library URI import, that is not in [_libraries], to the
/// corresponding Kernel summary module we imported it with.
///
@@ -5250,7 +5259,8 @@
var v = node.variable;
var id = _emitVariableRef(v);
if (id.name == v.name) {
- id.sourceInformation = _variableSpan(node.fileOffset, v.name!.length);
+ id = id.withSourceInformation(
+ _variableSpan(node.fileOffset, v.name!.length));
}
return id;
}
@@ -5284,7 +5294,7 @@
return null;
}
- js_ast.Identifier _emitVariableRef(VariableDeclaration v) {
+ js_ast.TemporaryId _emitVariableRef(VariableDeclaration v) {
if (_isTemporaryVariable(v)) {
var name = _debuggerFriendlyTemporaryVariableName(v);
name ??= 't\$${_tempVariables.length}';
@@ -5298,7 +5308,8 @@
// See https://github.com/dart-lang/sdk/issues/55918
name = extractLocalNameFromLateLoweredLocal(name);
}
- return _emitIdentifier(name);
+ return js_ast.TemporaryId.from(
+ _variableTempIds[v] ??= _emitTemporaryId(name));
}
/// Emits the declaration of a variable.
diff --git a/pkg/dev_compiler/lib/src/kernel/expression_compiler.dart b/pkg/dev_compiler/lib/src/kernel/expression_compiler.dart
index 22abadc..b6326c4 100644
--- a/pkg/dev_compiler/lib/src/kernel/expression_compiler.dart
+++ b/pkg/dev_compiler/lib/src/kernel/expression_compiler.dart
@@ -115,10 +115,80 @@
dartScope.definitions[extractLocalName(localName)] =
dartScope.definitions.remove(localName)!;
}
+
+ // Create a mapping from Dart variable names in scope to the corresponding
+ // JS variable names. The Dart variable may have had a suffix of the
+ // form '$N' added to it where N is either the empty string or an
+ // integer >= 0.
+ final dartNameToJsName = <String, String>{};
+
+ int nameCompare(String a, String b) {
+ final lengthCmp = b.length.compareTo(a.length);
+ if (lengthCmp != 0) return lengthCmp;
+ return b.compareTo(a);
+ }
+
+ // Sort Dart names in case a user-defined name includes a '$'. The
+ // resulting normalized JS name might seem like a suffixed version of a
+ // 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);
+
+ // Sort JS names so that the highest suffix value gets assigned to the
+ // corresponding Dart name first. Since names are suffixed in ascending
+ // order as inner scopes are visited, the highest suffix value will be
+ // the one that matches the visible Dart name in a given scope.
+ final jsNames = [...jsScope.keys]..sort(nameCompare);
+
+ const removedSentinel = '';
+ const thisJsName = r'$this';
+
+ for (final dartName in dartNames) {
+ if (isExtensionThisName(dartName)) {
+ if (jsScope.containsKey(thisJsName)) {
+ dartNameToJsName[dartName] = thisJsName;
+ }
+ continue;
+ }
+ // Any name containing a '$' symbol will have that symbol expanded to
+ // '$36' in JS. We do a similar expansion here to normalize the names.
+ final jsNamePrefix =
+ js_ast.toJSIdentifier(dartName).replaceAll('\$', '\\\$');
+ print(jsNamePrefix);
+ final regexp = RegExp(r'^' + jsNamePrefix + r'(\$[0-9]*)?$');
+ for (var i = 0; i < jsNames.length; i++) {
+ final jsName = jsNames[i];
+ if (jsName == removedSentinel) continue;
+ if (jsName.length < dartName.length) break;
+ if (regexp.hasMatch(jsName)) {
+ dartNameToJsName[dartName] = jsName;
+ jsNames[i] = removedSentinel;
+
+ // Remove any additional JS names that match this name as these will
+ // correspond to shadowed Dart variables that are not visible in the
+ // current scope.
+ //
+ // Note: In some extreme cases this can match the wrong variable.
+ // This would require a combination of 36 nested variables with the
+ // same name and a similarly named variable with a $ in its name.
+ for (var j = i; j < jsNames.length; j++) {
+ final jsName = jsNames[j];
+ if (jsName == removedSentinel) continue;
+ if (jsName.length < dartName.length) break;
+ if (regexp.hasMatch(jsNames[j])) {
+ jsNames[j] = removedSentinel;
+ }
+ }
+ break;
+ }
+ }
+ }
+
// remove undefined js variables (this allows us to get a reference error
// from chrome on evaluation)
- dartScope.definitions.removeWhere((variable, type) =>
- !jsScope.containsKey(_dartNameToJsName(variable)));
+ dartScope.definitions.removeWhere(
+ (variable, type) => !dartNameToJsName.containsKey(variable));
dartScope.typeParameters
.removeWhere((parameter) => !jsScope.containsKey(parameter.name));
@@ -128,7 +198,7 @@
var localJsScope = [
...dartScope.typeParameters.map((parameter) => jsScope[parameter.name]),
...dartScope.definitions.keys
- .map((variable) => jsScope[_dartNameToJsName(variable)])
+ .map((variable) => jsScope[dartNameToJsName[variable]])
];
_log('Performed scope substitutions for expression');
@@ -177,12 +247,6 @@
}
}
- String? _dartNameToJsName(String? dartName) {
- if (dartName == null) return dartName;
- if (isExtensionThisName(dartName)) return r'$this';
- return dartName;
- }
-
DartScope? _findScopeAt(
Uri libraryUri, Uri? scriptFileUri, int line, int column) {
if (line < 0) {
diff --git a/pkg/dev_compiler/test/expression_compiler/expression_compiler_e2e_dart_3_0_test.dart b/pkg/dev_compiler/test/expression_compiler/expression_compiler_e2e_dart_3_0_test.dart
index 9e9fc3b..5780b6c 100644
--- a/pkg/dev_compiler/test/expression_compiler/expression_compiler_e2e_dart_3_0_test.dart
+++ b/pkg/dev_compiler/test/expression_compiler/expression_compiler_e2e_dart_3_0_test.dart
@@ -230,7 +230,7 @@
test('second case scope', () async {
await driver.checkScope(
breakpointId: 'bp2',
- expectedScope: {'a': '10', 'b': '\'20\'', 'obj': 'obj'});
+ expectedScope: {'a\$': '10', 'b\$': '\'20\'', 'obj': 'obj'});
});
test('default case scope', () async {
@@ -243,4 +243,200 @@
expectedScope: {'foo': 'foo', 'one': '1', 'ten': '10', 'zero': '0'});
});
});
+
+ group('Shadowing', () {
+ // Includes similar names that could collide due to renaming scheme.
+ const shadowingSource = r'''
+ void main() {
+ String a = '1';
+ String a$ = '2';
+ String a$0 = '3';
+ String a$360 = '4';
+ if (a case Object a) {
+ a = '5$a';
+ // Breakpoint: bp1
+ print(a);
+ }
+ if (a$ case Object a$) {
+ a$ = '10${a$}';
+ print(a$);
+ }
+ if (a$ case Object a$) {
+ a$ = '6${a$}';
+ // Breakpoint: bp2
+ print(a$);
+ }
+ if (a$0 case Object a$0) {
+ a$0 = '7${a$0}';
+ // Breakpoint: bp3
+ print(a$0);
+ }
+ if (a$360 case Object a$360) {
+ a$360 = '8${a$360}';
+ // Breakpoint: bp4
+ print(a$360);
+ }
+ // Breakpoint: bp5
+ print('$a, ${a$}, ${a$0}, ${a$360}');
+ }
+ ''';
+
+ setUpAll(() async {
+ await driver.initSource(setup, shadowingSource);
+ });
+
+ tearDownAll(() async {
+ await driver.cleanupTest();
+ });
+
+ group('bp1', () {
+ test('a', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp1',
+ expression: 'a.toString()',
+ expectedResult: '51');
+ });
+
+ test('a\$', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp1',
+ expression: 'a\$.toString()',
+ expectedResult: '2');
+ });
+
+ test('a\$0', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp1',
+ expression: 'a\$0.toString()',
+ expectedResult: '3');
+ });
+
+ test('a\$360', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp1',
+ expression: 'a\$360.toString()',
+ expectedResult: '4');
+ });
+ });
+
+ group('bp2', () {
+ test('a', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp2',
+ expression: 'a.toString()',
+ expectedResult: '1');
+ });
+
+ test('a\$', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp2',
+ expression: 'a\$.toString()',
+ expectedResult: '62');
+ });
+
+ test('a\$0', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp2',
+ expression: 'a\$0.toString()',
+ expectedResult: '3');
+ });
+
+ test('a\$360', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp2',
+ expression: 'a\$360.toString()',
+ expectedResult: '4');
+ });
+ });
+
+ group('bp3', () {
+ test('a', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp3',
+ expression: 'a.toString()',
+ expectedResult: '1');
+ });
+
+ test('a\$', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp3',
+ expression: 'a\$.toString()',
+ expectedResult: '2');
+ });
+
+ test('a\$0', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp3',
+ expression: 'a\$0.toString()',
+ expectedResult: '73');
+ });
+
+ test('a\$360', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp3',
+ expression: 'a\$360.toString()',
+ expectedResult: '4');
+ });
+ });
+
+ group('bp4', () {
+ test('a', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp4',
+ expression: 'a.toString()',
+ expectedResult: '1');
+ });
+
+ test('a\$', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp4',
+ expression: 'a\$.toString()',
+ expectedResult: '2');
+ });
+
+ test('a\$0', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp4',
+ expression: 'a\$0.toString()',
+ expectedResult: '3');
+ });
+
+ test('a\$360', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp4',
+ expression: 'a\$360.toString()',
+ expectedResult: '84');
+ });
+ });
+
+ group('bp5', () {
+ test('a', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp5',
+ expression: 'a.toString()',
+ expectedResult: '1');
+ });
+
+ test('a\$', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp5',
+ expression: 'a\$.toString()',
+ expectedResult: '2');
+ });
+
+ test('a\$0', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp5',
+ expression: 'a\$0.toString()',
+ expectedResult: '3');
+ });
+
+ test('a\$360', () async {
+ await driver.checkInFrame(
+ breakpointId: 'bp5',
+ expression: 'a\$360.toString()',
+ expectedResult: '4');
+ });
+ });
+ });
}
diff --git a/sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart b/sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart
index bc97b4d..7bc3669 100644
--- a/sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart
+++ b/sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart
@@ -177,7 +177,7 @@
T? get target {
var target = JS<T?>('', '#.deref()', _weakRef);
// Coerce to null if JavaScript returns undefined.
- if (JS<bool>('!', 'target === void 0')) return null;
+ if (JS<bool>('!', '# === void 0', target)) return null;
return target;
}
}
diff --git a/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart b/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart
index f8ed2f8..ceb1bcf 100644
--- a/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart
+++ b/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart
@@ -534,7 +534,7 @@
return noSuchMethod(
f,
InvocationImpl(
- JS('', 'f.name'),
+ JS('', '#.name', f),
args,
isMethod: true,
failureMessage: errorMessage,
@@ -551,7 +551,7 @@
null,
args,
named,
- JS('', 'f.name'),
+ JS('', '#.name', f),
);
dgcall(f, typeArgs, args, [named]) => _checkAndCall(
@@ -561,7 +561,7 @@
typeArgs,
args,
named,
- JS('', "f.name || 'call'"),
+ JS('', "#.name || 'call'", f),
);
/// Helper for REPL dynamic invocation variants that make a best effort to
diff --git a/tests/language/patterns/regress_59613_test.dart b/tests/language/patterns/regress_59613_test.dart
new file mode 100644
index 0000000..1a39c50
--- /dev/null
+++ b/tests/language/patterns/regress_59613_test.dart
@@ -0,0 +1,20 @@
+// 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.
+
+// Ensure backends handle nested scopes with VariableDeclaration nodes that
+// have the same 'name'.
+
+void main() {
+ for (final thing in [42.42, 'foo', Object()]) {
+ if (thing case final double thing) {
+ print('$thing is double');
+ } else if (thing case final String thing) {
+ print('$thing is String');
+ } else if (thing case final Object thing) {
+ print('$thing is Object');
+ } else {
+ throw '$thing is unknown ${thing.runtimeType}';
+ }
+ }
+}
