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>();
+  final NamingScope constantScope = new NamingScope();
 
-  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;
-    }
-  }
+  /// Used to store scopes for instances of [PrivatelyNamedJsEntity]
+  final Map<Entity, NamingScope> _privateNamingScopes =
+      new Map<Entity, 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),
         functionTypeNamer = new FunctionTypeNamer(compiler) {
@@ skipping 17 matching lines @@
   jsAst.Name get noSuchMethodName => invocationName(Selectors.noSuchMethod_);
 
   /**
    * Some closures must contain their name. The name is stored in
    * [STATIC_CLOSURE_NAME_NAME].
    */
   String get STATIC_CLOSURE_NAME_NAME => r'$name';
   String get closureInvocationSelectorName => Identifiers.call;
   bool get shouldMinify => false;
 
+  NamingScope _getPrivateScopeFor(PrivatelyNamedJSEntity entity) {
+    return _privateNamingScopes.putIfAbsent(entity.rootOfScope,
+                                            () => new NamingScope());
+  }
+
   /// Returns the string that is to be used as the result of a call to
   /// [JS_GET_NAME] at [node] with argument [name].
   jsAst.Name getNameForJsGetName(Node node, JsGetName name) {
     switch (name) {
       case JsGetName.GETTER_PREFIX: return asName(getterPrefix);
       case JsGetName.SETTER_PREFIX: return asName(setterPrefix);
       case JsGetName.CALL_PREFIX: return asName(callPrefix);
       case JsGetName.CALL_PREFIX0: return asName('${callPrefix}\$0');
       case JsGetName.CALL_PREFIX1: return asName('${callPrefix}\$1');
       case JsGetName.CALL_PREFIX2: return asName('${callPrefix}\$2');
@@ skipping 49 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 JSEntity) {
+      return _disambiguateInternalMember(element,
+                                         () => element.declaredEntity.name);
     }
 
     // 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 PrivatelyNamedJSEntity) {
+        NamingScope scope = _getPrivateScopeFor(element);
+        newName = getFreshName(scope, 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
+
+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);
+  }
 }