[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 <[email protected]>
Reviewed-by: Sigmund Cherem <[email protected]>
Reviewed-by: Nicholas Shahan <[email protected]>

Author: natebiggs

pkg/dev_compiler/lib/src/compiler/js_names.dart

@@ -24,31 +24,31 @@ abstract class FixedNames {
 }
 
 /// 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.
+  final int _id;
+
   @override
-  dynamic get sourceInformation => null;
+  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
-  set sourceInformation(Object? obj) {}
+  int get hashCode => _id;
 
-  TemporaryId(super.name);
+  @override
+  bool operator ==(Object other) {
+    return other is TemporaryId && other._id == _id;
+  }
 }
 
 /// Creates a qualified identifier, without determining for sure if it needs to

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

@@ -97,6 +97,15 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
   @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 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     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 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     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 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
       // 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.

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

@@ -248,6 +248,15 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
   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 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     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 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     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 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
       // 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.

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

@@ -115,10 +115,80 @@ class ExpressionCompiler {
         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 @@ class ExpressionCompiler {
       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 @@ class ExpressionCompiler {
     }
   }
 
-  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) {