Use better names for captured variables in closures.

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

 // Copyright (c) 2011, 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.
 
 part of js_backend;
 
 /**
  * Assigns JavaScript identifiers to Dart variables, class-names and members.
  *
  * Names are generated through three stages:
@@ skipping 352 matching lines @@
   static final RegExp NON_IDENTIFIER_CHAR = new RegExp(r'[^A-Za-z_0-9$]');
 
   final Compiler compiler;
 
   /// Used disambiguated names in the global namespace, issued by
   /// [_disambiguateGlobal], and [_disambiguateInternalGlobal].
   ///
   /// 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 =
       new HashMap<String, jsAst.Name>();
 
   /// 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 =
       new HashMap<Element, jsAst.Name>();
   final Map<String, jsAst.Name> userInstanceOperators =
       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>{};
 
   final Map<LibraryElement, String> libraryLongNames =
       new HashMap<LibraryElement, String>();
 
   final Map<ConstantValue, jsAst.Name> constantNames =
       new HashMap<ConstantValue, jsAst.Name>();
   final Map<ConstantValue, String> constantLongNames =
       <ConstantValue, String>{};
   ConstantCanonicalHasher constantHasher;
 
   /// Maps private names to a library that may use that name without prefixing
   /// itself. Used for building proposed names.
   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
   /// key into maps.
   final Map<LibraryElement, String> _libraryKeys =
       new HashMap<LibraryElement, String>();
 
   Namer(Compiler compiler)
       : compiler = compiler,
         constantHasher = new ConstantCanonicalHasher(compiler),
@@ skipping 87 matching lines @@
   ///
   /// Unique within the global-member namespace.
   jsAst.Name constantName(ConstantValue constant) {
     // In the current implementation it doesn't make sense to give names to
     // function constants since the function-implementation itself serves as
     // constant and can be accessed directly.
     assert(!constant.isFunction);
     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);
   }
 
   /// Proposed name for [constant].
   String constantLongName(ConstantValue constant) {
     String longName = constantLongNames[constant];
     if (longName == null) {
       longName = new ConstantNamingVisitor(compiler, constantHasher)
@@ skipping 233 matching lines @@
   /**
    * Returns the JavaScript property name used to store an instance field.
    */
   jsAst.Name instanceFieldPropertyName(FieldElement element) {
     ClassElement enclosingClass = element.enclosingClass;
 
     if (backend.hasFixedBackendName(element)) {
       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,
     // use a mangled field name to avoid potential clashes.
     // Note that if the class extends a native class, that native class might
     // have fields with fixed backend names, so we assume the worst and always
     // mangle the field names of classes extending native classes.
     // Methods on such classes are stored on the interceptor, not the instance,
     // so only fields have the potential to clash with a native property name.
     ClassWorld classWorld = compiler.world;
@@ skipping 55 matching lines @@
     jsAst.Name disambiguatedName = _disambiguateMember(originalName);
     return deriveGetterName(disambiguatedName);
   }
 
   /// Disambiguated name for a compiler-owned global variable.
   ///
   /// The resulting name is unique within the global-member namespace.
   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);
   }
 
   /// Returns the property name to use for a compiler-owner global variable,
   /// i.e. one that does not correspond to any element but is used as a utility
   /// global by code generation.
   ///
   /// [name] functions as both the proposed name for the global, and as a key
@@ skipping 26 matching lines @@
   /// Returns the disambiguated name for a top-level or static element.
   ///
   /// The resulting name is unique within the global-member namespace.
   jsAst.Name _disambiguateGlobal(Element element) {
     // TODO(asgerf): We can reuse more short names if we disambiguate with
     // a separate namespace for each of the global holder objects.
     element = element.declaration;
     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);
   }
 
   /// Returns the disambiguated name for an instance method or field
   /// with [originalName] in [library].
   ///
   /// [library] may be `null` if [originalName] is known to be public.
   ///
@@ skipping 19 matching lines @@
     // This avoids clashes since the original names cannot contain that symbol.
     String key = '$libraryKey@${originalName.text}@${suffixes.join('@')}';
     jsAst.Name newName = userInstanceMembers[key];
     if (newName == null) {
       String proposedName = privateName(originalName);
       if (!suffixes.isEmpty) {
         // In the proposed name, separate the name parts by '$', because the
         // 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;
     }
     return _newReference(newName);
   }
 
   /// Returns the disambiguated name for the instance member identified by
   /// [key].
   ///
   /// When a name for an element is requested by key, it may not be requested
   /// by element at the same time, as two different names would be returned.
   ///
   /// If key has not yet been registered, [proposeName] is used to generate
   /// a name proposal for the given key.
   ///
   /// [key] must not clash with valid instance names. This is typically
   /// achieved by using at least one character in [key] that is not valid in
   /// identifiers, for example the @ symbol.
   jsAst.Name _disambiguateMemberByKey(String key, String proposeName()) {
     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;
     }
     return _newReference(newName);
   }
 
   /// Forces the public instance member with [originalName] to have the given
   /// [disambiguatedName].
   ///
   /// The [originalName] must not have been disambiguated before, and the
   /// [disambiguatedName] must not have been used.
   ///
   /// Using [_disambiguateMember] with the given [originalName] and no suffixes
   /// will subsequently return [disambiguatedName].
   void reservePublicMemberName(String originalName,
                                String disambiguatedName) {
     // Build a key that corresponds to the one built in disambiguateMember.
     String libraryPrefix = ''; // Public names have an empty library prefix.
     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].
   ///
   /// This is used as the property name for fields, type variables,
   /// constructor bodies, and super-accessors.
   ///
   /// The resulting name is unique within the instance-member namespace.
   jsAst.Name _disambiguateInternalMember(Element element,
                                          String proposeName()) {
     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);
   }
 
   /// Disambiguated name for the given operator.
   ///
   /// [operatorIdentifier] must be the operator's identifier, e.g.
   /// `$add` and not `+`.
   ///
   /// The resulting name is unique within the instance-member namespace.
   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) {
       proposedName = _sanitizeForAnnotations(proposedName);
     }
     if (sanitizeForNatives) {
       proposedName = _sanitizeForNatives(proposedName);
     }
     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;
   }
 
   /// Returns an unused name.
   ///
   /// [proposedName] must be a valid JavaScript identifier.
   ///
   /// If [sanitizeForAnnotations] is `true`, then the result is guaranteed not
   /// to have the form of an annotated name.
   ///
   /// If [sanitizeForNatives] it `true`, then the result is guaranteed not to
   /// clash with a property name on a native object.
   ///
   /// Note that [MinifyNamer] overrides this method with one that produces
   /// minified names.
   jsAst.Name getFreshName(NamingScope scope,
                           String proposedName,
                           {bool sanitizeForAnnotations: false,
                            bool sanitizeForNatives: false}) {
     String candidate =
         _generateFreshStringForName(proposedName,
-                                    getUsedNames(scope),
-                                    getSuggestedNames(scope),
+                                    scope,
                                     sanitizeForAnnotations:
                                         sanitizeForAnnotations,
                                     sanitizeForNatives: sanitizeForNatives);
     return new StringBackedName(candidate);
   }
 
   /// Returns a variant of [name] that cannot clash with the annotated
   /// version of another name, that is, the resulting name can never be returned
   /// by [deriveGetterName], [deriveSetterName], [deriveCallMethodName],
   /// [operatorIs], or [substitutionName].
@@ skipping 314 matching lines @@
 
   String get functionTypeNamedParametersTag => r'named';
 
   Map<FunctionType, jsAst.Name> functionTypeNameMap =
       new HashMap<FunctionType, jsAst.Name>();
   final FunctionTypeNamer functionTypeNamer;
 
   jsAst.Name getFunctionTypeName(FunctionType functionType) {
     return functionTypeNameMap.putIfAbsent(functionType, () {
       String proposedName = functionTypeNamer.computeName(functionType);
-      return getFreshName(NamingScope.instance, proposedName);
+      return getFreshName(instanceScope, proposedName);
     });
   }
 
   jsAst.Name operatorIsType(DartType type) {
     if (type.isFunctionType) {
       // TODO(erikcorry): Reduce from $isx to ix when we are minifying.
       return new CompoundName([new StringBackedName(operatorIsPrefix),
                                _literalUnderscore,
                                getFunctionTypeName(type)]);
     }
@@ skipping 115 matching lines @@
 
   String get incrementalHelperName => r'$dart_unsafe_incremental_support';
 
   jsAst.Expression get accessIncrementalHelper {
     return js('self.${incrementalHelperName}');
   }
 
   void forgetElement(Element element) {
     jsAst.Name globalName = userGlobals[element];
     invariant(element, globalName != null, message: 'No global name.');
-    usedGlobalNames.remove(globalName);
     userGlobals.remove(element);
   }
 }
 
 /**
  * Generator of names for [ConstantValue] values.
  *
  * The names are stable under perturbations of the source.  The name is either a
  * short sequence of words, if this can be found from the constant, or a type
  * followed by a hash tag.
@@ skipping 435 matching lines @@
           sb.write('_');
         }
         sb.write('_');
         visit(parameter);
         first = true;
       }
     }
   }
 }
 
-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);
+  }
+}