Use better names for captured variables in closures.

pkg/compiler/lib/src/js_backend/namer.dart

Index: pkg/compiler/lib/src/js_backend/namer.dart
diff --git a/pkg/compiler/lib/src/js_backend/namer.dart b/pkg/compiler/lib/src/js_backend/namer.dart
index 26dd37605963a441cf14f8e5ab68a69565dfa0d9..7f47f1ad6a7ccb6b54575c742f7d75f138a19159 100644
--- a/pkg/compiler/lib/src/js_backend/namer.dart
+++ b/pkg/compiler/lib/src/js_backend/namer.dart
@@ -370,7 +370,7 @@ class Namer {
   /// Although global names are distributed across a number of global objects,
   /// (see [globalObjectFor]), we currently use a single namespace for all these
   /// names.
-  final Set<String> usedGlobalNames = new Set<String>();
+  final NamingScope globalScope = new NamingScope();
   final Map<Element, jsAst.Name> userGlobals =
       new HashMap<Element, jsAst.Name>();
   final Map<String, jsAst.Name> internalGlobals =
@@ -379,7 +379,7 @@ class Namer {
   /// Used disambiguated names in the instance namespace, issued by
   /// [_disambiguateMember], [_disambiguateInternalMember],
   /// [_disambiguateOperator], and [reservePublicMemberName].
-  final Set<String> usedInstanceNames = new Set<String>();
+  final NamingScope instanceScope = new NamingScope();
   final Map<String, jsAst.Name> userInstanceMembers =
       new HashMap<String, jsAst.Name>();
   final Map<Element, jsAst.Name> internalInstanceMembers =
@@ -388,18 +388,7 @@ class Namer {
       new HashMap<String, jsAst.Name>();
 
   /// Used to disambiguate names for constants in [constantName].
-  final Set<String> usedConstantNames = new Set<String>();
-
-  Set<String> getUsedNames(NamingScope scope) {
-    if (scope == NamingScope.global) {
-      return usedGlobalNames;
-    } else if (scope == NamingScope.instance){
-      return usedInstanceNames;
-    } else {
-      assert(scope == NamingScope.constant);
-      return usedConstantNames;
-    }
-  }
+  final NamingScope constantScope = new NamingScope();
 
   final Map<String, int> popularNameCounters = <String, int>{};
 
@@ -417,27 +406,10 @@ class Namer {
   final Map<String, LibraryElement> shortPrivateNameOwners =
       <String, LibraryElement>{};
 
-  /// Maps proposed names to *suggested* disambiguated names.
-  ///
-  /// Suggested names are hints to the [MinifyNamer], suggesting that a specific
-  /// names be given to the first item with the given proposed name.
-  ///
-  /// This is currently used in [MinifyNamer] to assign very short minified
-  /// names to things that tend to be used very often.
+
   final Map<String, String> suggestedGlobalNames = <String, String>{};
   final Map<String, String> suggestedInstanceNames = <String, String>{};
 
-  Map<String, String> getSuggestedNames(NamingScope scope) {
-    if (scope == NamingScope.global) {
-      return suggestedGlobalNames;
-    } else if (scope == NamingScope.instance) {
-      return suggestedInstanceNames;
-    } else {
-      assert(scope == NamingScope.constant);
-      return const {};
-    }
-  }
-
 
   /// Used to store unique keys for library names. Keys are not used as names,
   /// nor are they visible in the output. The only serve as an internal
@@ -545,7 +517,7 @@ class Namer {
     jsAst.Name result = constantNames[constant];
     if (result == null) {
       String longName = constantLongName(constant);
-      result = getFreshName(NamingScope.constant, longName);
+      result = getFreshName(constantScope, longName);
       constantNames[constant] = result;
     }
     return _newReference(result);
@@ -799,14 +771,16 @@ class Namer {
       return new StringBackedName(backend.getFixedBackendName(element));
     }
 
-    // Instances of BoxFieldElement are special. They are already created with
-    // a unique and safe name. However, as boxes are not really instances of
-    // classes, the usual naming scheme that tries to avoid name clashes with
-    // super classes does not apply. We still do not mark the name as a
-    // fixedBackendName, as we want to allow other namers to do something more
-    // clever with them.
-    if (element is BoxFieldElement) {
-      return new StringBackedName(element.name);
+    // Some elements, like e.g. instances of BoxFieldElement are special.
+    // They are created with a unique and safe name for the element model.
+    // While their name is unique, it is not very readable. So we try to
+    // preserve the original, proposed name.
+    // However, as boxes are not really instances of classes, the usual naming
+    // scheme that tries to avoid name clashes with super classes does not
+    // apply. So we can directly grab a name.
+    if (element is NameProposingEntity) {

[sra1, 2015/11/04 02:27:49]

Are the names generated here going to be stable?
The order is not as clear as in closure conversion where fields are sorted.

[herhut, 2015/11/04 15:25:48]

This is typically the order in which the model is built, which should be
somewhat deterministic. IIRC that is also the order in which code is emitted.
Also, this is more in line with how it works for other fields, as well.

+      var proposing = entity;

[Siggi Cherem (dart-lang), 2015/11/03 17:21:34]

remove this extra var? with the spec's type propagation of is-conditionals I
believe `element.proposeName` should be allowed here without warnings.

[sra1, 2015/11/04 02:27:48]

What Siggi says.

'entity' is not in scope.
Had to fix it to get to look at swarm, 

[herhut, 2015/11/04 15:25:48]

Sorry, last minute editing change.

[herhut, 2015/11/04 15:25:48]

Did not work for me but with the new hierarchy it seems to be fine.

+      return _disambiguateInternalMember(element, proposing.proposeName);

[Siggi Cherem (dart-lang), 2015/11/03 17:21:34]

(related to my other comment, I'd assume the only change is that here you'd
tear-off the getter `() => element.proposedName`)

[herhut, 2015/11/04 15:25:48]

It would have been, now I need to create a little closure anyway so I changed
it.

     }
 
     // If the name of the field might clash with another field,
@@ -882,7 +856,7 @@ class Namer {
   jsAst.Name _disambiguateInternalGlobal(String name) {
     jsAst.Name newName = internalGlobals[name];
     if (newName == null) {
-      newName = getFreshName(NamingScope.global, name);
+      newName = getFreshName(globalScope, name);
       internalGlobals[name] = newName;
     }
     return _newReference(newName);
@@ -929,7 +903,7 @@ class Namer {
     jsAst.Name newName = userGlobals[element];
     if (newName == null) {
       String proposedName = _proposeNameForGlobal(element);
-      newName = getFreshName(NamingScope.global, proposedName);
+      newName = getFreshName(globalScope, proposedName);
       userGlobals[element] = newName;
     }
     return _newReference(newName);
@@ -969,7 +943,7 @@ class Namer {
         // proposed name must be a valid identifier, but not necessarily unique.
         proposedName += r'$' + suffixes.join(r'$');
       }
-      newName = getFreshName(NamingScope.instance, proposedName,
+      newName = getFreshName(instanceScope, proposedName,
                              sanitizeForAnnotations: true);
       userInstanceMembers[key] = newName;
     }
@@ -992,7 +966,7 @@ class Namer {
     jsAst.Name newName = userInstanceMembers[key];
     if (newName == null) {
       String name = proposeName();
-      newName = getFreshName(NamingScope.instance, name,
+      newName = getFreshName(instanceScope, name,
                              sanitizeForAnnotations: true);
       userInstanceMembers[key] = newName;
     }
@@ -1014,9 +988,9 @@ class Namer {
     String suffix = ''; // We don't need any suffixes.
     String key = '$libraryPrefix@$originalName@$suffix';
     assert(!userInstanceMembers.containsKey(key));
-    assert(!usedInstanceNames.contains(disambiguatedName));
+    assert(!instanceScope.isUsed(disambiguatedName));
     userInstanceMembers[key] = new StringBackedName(disambiguatedName);
-    usedInstanceNames.add(disambiguatedName);
+    instanceScope.registerUse(disambiguatedName);
   }
 
   /// Disambiguated name unique to [element].
@@ -1030,11 +1004,21 @@ class Namer {
     jsAst.Name newName = internalInstanceMembers[element];
     if (newName == null) {
       String name = proposeName();
-      bool mayClashNative = _isUserClassExtendingNative(element.enclosingClass);
-      newName = getFreshName(NamingScope.instance, name,
-                             sanitizeForAnnotations: true,
-                             sanitizeForNatives: mayClashNative);
-      internalInstanceMembers[element] = newName;
+
+      if (element is ScopeBearingEntity) {
+        var scopeProvider = element;

[sra1, 2015/11/04 02:27:49]

Don't use var to avoid static type checks.

[herhut, 2015/11/04 15:25:48]

Done.

+        newName = getFreshName(scopeProvider.namingScope, name,
+                               sanitizeForAnnotations: true,
+                               sanitizeForNatives: false);
+        internalInstanceMembers[element] = newName;
+      } else {
+        bool mayClashNative =
+            _isUserClassExtendingNative(element.enclosingClass);
+        newName = getFreshName(instanceScope, name,
+                               sanitizeForAnnotations: true,
+                               sanitizeForNatives: mayClashNative);
+        internalInstanceMembers[element] = newName;
+      }
     }
     return _newReference(newName);
   }
@@ -1048,15 +1032,14 @@ class Namer {
   jsAst.Name _disambiguateOperator(String operatorIdentifier) {
     jsAst.Name newName = userInstanceOperators[operatorIdentifier];
     if (newName == null) {
-      newName = getFreshName(NamingScope.instance, operatorIdentifier);
+      newName = getFreshName(instanceScope, operatorIdentifier);
       userInstanceOperators[operatorIdentifier] = newName;
     }
     return _newReference(newName);
   }
 
   String _generateFreshStringForName(String proposedName,
-      Set<String> usedNames,
-      Map<String, String> suggestedNames,
+      NamingScope scope,
       {bool sanitizeForAnnotations: false,
        bool sanitizeForNatives: false}) {
     if (sanitizeForAnnotations) {
@@ -1067,18 +1050,18 @@ class Namer {
     }
     proposedName = _sanitizeForKeywords(proposedName);
     String candidate;
-    if (!usedNames.contains(proposedName)) {
+    if (scope.isUnused(proposedName)) {
       candidate = proposedName;
     } else {
       int counter = popularNameCounters[proposedName];
       int i = (counter == null) ? 0 : counter;
-      while (usedNames.contains("$proposedName$i")) {
+      while (scope.isUsed("$proposedName$i")) {
         i++;
       }
       popularNameCounters[proposedName] = i + 1;
       candidate = "$proposedName$i";
     }
-    usedNames.add(candidate);
+    scope.registerUse(candidate);
     return candidate;
   }
 
@@ -1100,8 +1083,7 @@ class Namer {
                            bool sanitizeForNatives: false}) {
     String candidate =
         _generateFreshStringForName(proposedName,
-                                    getUsedNames(scope),
-                                    getSuggestedNames(scope),
+                                    scope,
                                     sanitizeForAnnotations:
                                         sanitizeForAnnotations,
                                     sanitizeForNatives: sanitizeForNatives);
@@ -1436,7 +1418,7 @@ class Namer {
   jsAst.Name getFunctionTypeName(FunctionType functionType) {
     return functionTypeNameMap.putIfAbsent(functionType, () {
       String proposedName = functionTypeNamer.computeName(functionType);
-      return getFreshName(NamingScope.instance, proposedName);
+      return getFreshName(instanceScope, proposedName);
     });
   }
 
@@ -1572,7 +1554,6 @@ class Namer {
   void forgetElement(Element element) {
     jsAst.Name globalName = userGlobals[element];
     invariant(element, globalName != null, message: 'No global name.');
-    usedGlobalNames.remove(globalName);
     userGlobals.remove(element);
   }
 }
@@ -2028,8 +2009,36 @@ class FunctionTypeNamer extends BaseDartTypeVisitor {
   }
 }
 
-enum NamingScope {
-  global,
-  instance,
-  constant
+abstract class NameProposingEntity implements Entity {
+  String proposeName();
+}
+
+abstract class ScopeBearingEntity implements Entity {
+  NamingScope get namingScope;
+}
+
+class NamingScope {
+  /// Maps proposed names to *suggested* disambiguated names.
+  ///
+  /// Suggested names are hints to the [MinifyNamer], suggesting that a specific
+  /// names be given to the first item with the given proposed name.
+  ///
+  /// This is currently used in [MinifyNamer] to assign very short minified
+  /// names to things that tend to be used very often.
+  final Map<String, String> _suggestedNames = new Map<String, String>();
+  final Set<String> _usedNames = new Set<String>();
+
+  bool isUsed(String name) => _usedNames.contains(name);
+  bool isUnused(String name) => !_usedNames.contains(name);
+  bool registerUse(String name) => _usedNames.add(name);
+
+  String suggestName(String original) => _suggestedNames[original];
+  void addSuggestion(String original, String suggestion) {
+    assert(!_suggestedNames.containsKey(original));
+    _suggestedNames[original] = suggestion;
+  }
+  bool hasSuggestion(String original) => _suggestedNames.containsKey(original);
+  bool isSuggestion(String candidate) {
+    return _suggestedNames.containsValue(candidate);
+  }
 }