Reapply class/method/field minification

sdk/lib/_internal/compiler/implementation/js_backend/namer.dart

Index: sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
diff --git a/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart b/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
index c4888b24b616c74b3b8c6622b1573bead56fc366..bf28c90e488770a7df38233817f8e5ec71969e11 100644
--- a/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
+++ b/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
@@ -8,8 +8,6 @@ part of js_backend;
  * Assigns JavaScript identifiers to Dart variables, class-names and members.
  */
 class Namer {
-  final Compiler compiler;
-
   static Set<String> _jsReserved = null;
   Set<String> get jsReserved {
     if (_jsReserved == null) {
@@ -20,15 +18,22 @@ class Namer {
     return _jsReserved;
   }
 
+  final String CURRENT_ISOLATE = r'$';
+
   /**
    * Map from top-level or static elements to their unique identifiers provided
    * by [getName].
    *
    * Invariant: Keys must be declaration elements.
    */
+  final Compiler compiler;
   final Map<Element, String> globals;
-  final Map<String, int> usedGlobals;
   final Map<String, LibraryElement> shortPrivateNameOwners;
+  final Set<String> usedGlobalNames;
+  final Set<String> usedInstanceNames;
+  final Map<String, String> instanceNameMap;
+  final Map<String, String> globalNameMap;
+  final Map<String, int> popularNameCounters;
 
   /**
    * A cache of names used for bailout methods. We make sure two
@@ -45,22 +50,27 @@ class Namer {
 
   Namer(this.compiler)
       : globals = new Map<Element, String>(),
-        usedGlobals = new Map<String, int>(),
         shortPrivateNameOwners = new Map<String, LibraryElement>(),
         bailoutNames = new Map<Element, String>(),
         usedBailoutInstanceNames = new Set<String>(),
-        constantNames = new Map<Constant, String>();
-
-  final String CURRENT_ISOLATE = r'$';
-  final String ISOLATE = 'Isolate';
-  final String ISOLATE_PROPERTIES = r"$isolateProperties";
+        usedGlobalNames = new Set<String>(),
+        usedInstanceNames = new Set<String>(),
+        instanceNameMap = new Map<String, String>(),
+        globalNameMap = new Map<String, String>(),
+        constantNames = new Map<Constant, String>(),
+        popularNameCounters = new Map<String, int>();
+
+  String get isolateName => 'Isolate';
+  String get isolatePropertiesName => r'$isolateProperties';
   /**
    * Some closures must contain their name. The name is stored in
    * [STATIC_CLOSURE_NAME_NAME].
    */
-  final String STATIC_CLOSURE_NAME_NAME = r'$name';
-  static const SourceString CLOSURE_INVOCATION_NAME =
-      Compiler.CALL_OPERATOR_NAME;
+  String get STATIC_CLOSURE_NAME_NAME => r'$name';
+  SourceString get closureInvocationSelector => Compiler.CALL_OPERATOR_NAME;

[ngeoffray, 2012/12/10 08:55:42]

We have the Selector class in the compiler, so I'd prefer keeping
closureInvocationName. Or closureInvocationSelectorName.

[erikcorry, 2012/12/10 12:37:40]

We already have something else called closureInvocationName in this class, so
closureInvocationSelectorName it is, though it reminds me a little of
http://en.wikipedia.org/wiki/Haddocks'_Eyes

+  bool get shouldMinify => false;
+
+  bool isReserved(String name) => name == isolateName;
 
   String constantName(Constant constant) {
     // In the current implementation it doesn't make sense to give names to
@@ -69,16 +79,29 @@ class Namer {
     assert(!constant.isFunction());
     String result = constantNames[constant];
     if (result == null) {
-      result = getFreshGlobalName("CTC");
+      String longName;
+      if (shouldMinify) {
+        if (constant.isString()) {
+          StringConstant stringConstant = constant;
+          // The minifier always constructs a new name, using the argument as
+          // input to its hashing algorithm.  The given name does not need to be
+          // valid.
+          longName = stringConstant.value.slowToString();
+        } else {
+          longName = "C";
+        }
+      } else {
+        longName = "CONSTANT";
+      }
+      result = getFreshName(longName, usedGlobalNames);
       constantNames[constant] = result;
     }
     return result;
   }
 
   String closureInvocationName(Selector selector) {
-    // TODO(floitsch): mangle, while not conflicting with instance names.
-    return instanceMethodInvocationName(null, CLOSURE_INVOCATION_NAME,
-                                        selector);
+    return
+        instanceMethodInvocationName(null, closureInvocationSelector, selector);

[ngeoffray, 2012/12/10 08:55:42]

The style we usually use is arguments on the new line.

[erikcorry, 2012/12/10 12:37:40]

Done.

   }
 
   String breakLabelName(LabelElement label) {
@@ -104,25 +127,32 @@ class Namer {
    * mangles the [name] so that each library has a unique name.
    */
   String privateName(LibraryElement lib, SourceString name) {
+    String result;
     if (name.isPrivate()) {
       String nameString = name.slowToString();
       // The first library asking for a short private name wins.
-      LibraryElement owner =
-          shortPrivateNameOwners.putIfAbsent(nameString, () => lib);
+      LibraryElement owner = shouldMinify ?
+          shortPrivateNameOwners.putIfAbsent(nameString, () => lib) :
+          lib;
       // If a private name could clash with a mangled private name we don't
       // use the short name. For example a private name "_lib3_foo" would
       // clash with "_foo" from "lib3".
-      if (identical(owner, lib) && !nameString.startsWith('_$LIBRARY_PREFIX')) {
-        return nameString;
+      if (owner == lib &&
+          !nameString.startsWith('_$LIBRARY_PREFIX') &&
+          !shouldMinify) {
+        result = nameString;
+      } else {
+        String libName = getName(lib);
+        // If a library name does not start with the [LIBRARY_PREFIX] then our
+        // assumptions about clashing with mangled private members do not hold.
+        assert(shouldMinify || libName.startsWith(LIBRARY_PREFIX));
+        // TODO(erikcorry): Fix this with other manglings to avoid clashes.
+        result = '_lib$libName\$$nameString';
       }
-      String libName = getName(lib);
-      // If a library name does not start with the [LIBRARY_PREFIX] then our
-      // assumptions about clashing with mangled private members do not hold.
-      assert(libName.startsWith(LIBRARY_PREFIX));
-      return '_$libName$nameString';
     } else {
-      return name.slowToString();
+      result = name.slowToString();
     }
+    return result;
   }
 
   String instanceMethodName(FunctionElement element) {
@@ -135,21 +165,27 @@ class Namer {
     FunctionSignature signature = element.computeSignature(compiler);
     String methodName =
         '${privateName(lib, name)}\$${signature.parameterCount}';
-    if (!signature.optionalParametersAreNamed) {
-      return methodName;
-    } else if (!signature.optionalParameters.isEmpty) {
+    if (signature.optionalParametersAreNamed &&
+        !signature.optionalParameters.isEmpty) {
       StringBuffer buffer = new StringBuffer();
       signature.orderedOptionalParameters.forEach((Element element) {
         buffer.add('\$${JsNames.getValid(element.name.slowToString())}');
       });
-      return '$methodName$buffer';
+      methodName = '$methodName$buffer';
     }
+    if (name == closureInvocationSelector) return methodName;
+    return getMappedInstanceName(methodName);
   }
 
   String publicInstanceMethodNameByArity(SourceString name, int arity) {
     name = Elements.operatorNameToIdentifier(name);
     assert(!name.isPrivate());
-    return '${name.slowToString()}\$$arity';
+    var base = name.slowToString();
+    // We don't mangle the closure invoking function name because it is
+    // generated in applyFunction.
+    var proposedName = '$base\$$arity';
+    if (base == closureInvocationSelector) return proposedName;
+    return getMappedInstanceName(proposedName);
   }
 
   String instanceMethodInvocationName(LibraryElement lib, SourceString name,
@@ -162,59 +198,108 @@ class Namer {
       buffer.add(r'$');
       argumentName.printOn(buffer);
     }
-    return '${privateName(lib, name)}\$${selector.argumentCount}$buffer';
+    if (name == closureInvocationSelector) {
+      // We don't mangle the closure invoking function name because it is
+      // generated in applyFunction.

[ngeoffray, 2012/12/10 08:55:42]

generated or used? What is applyFunction? Dart's Function.apply?

[erikcorry, 2012/12/10 12:37:40]

The name is generated (wiith string addition), then it gets used.  The
applyFunction function is found in js_helper.dart.  Comment clarified here and
below.

+      return '$closureInvocationSelector\$${selector.argumentCount}$buffer';
+    }
+    return getMappedInstanceName(
+        '${privateName(lib, name)}\$${selector.argumentCount}$buffer');
   }
 
   String instanceFieldName(LibraryElement libraryElement, SourceString name) {
     String proposedName = privateName(libraryElement, name);
-    return safeName(proposedName);
+    return getMappedInstanceName(proposedName);
   }
 
+  // Construct a new name for the element based on the library and class it is
+  // in.  The name here is not important, we just need to make sure it is
+  // unique.  If we are minifying, we actually construct the name from the
+  // minified versions of the class and instance names, but the result is
+  // minified once again, so that is not visible in the end result.
   String shadowedFieldName(Element fieldElement) {
+    // Check for following situation: Native field ${fieldElement.name} has
+    // fixed JSName ${fieldElement.nativeName()}, but a subclass shadows this
+    // name.  We normally handle that by renaming the superclass field, but we
+    // can't do that because native fields have fixed JSNames.  In practice
+    // this can't happen because we can't inherit from native classes.
+    assert (!fieldElement.isNative());
+
     ClassElement cls = fieldElement.getEnclosingClass();
     LibraryElement libraryElement = fieldElement.getLibrary();
     String libName = getName(libraryElement);
     String clsName = getName(cls);
     String instanceName = instanceFieldName(libraryElement, fieldElement.name);
-    return safeName('$libName\$$clsName\$$instanceName');
+    return getMappedInstanceName('$libName\$$clsName\$$instanceName');
   }
 
   String setterName(LibraryElement lib, SourceString name) {
     // We dynamically create setters from the field-name. The setter name must
     // therefore be derived from the instance field-name.
-    String fieldName = safeName(privateName(lib, name));
+    String fieldName = getMappedInstanceName(privateName(lib, name));
     return 'set\$$fieldName';
   }
 
+  String setterNameFromAccessorName(String name) {
+    // We dynamically create setters from the field-name. The setter name must
+    // therefore be derived from the instance field-name.
+    return 'set\$$name';
+  }
+
   String publicGetterName(SourceString name) {
     // We dynamically create getters from the field-name. The getter name must
     // therefore be derived from the instance field-name.
-    String fieldName = safeName(name.slowToString());
+    String fieldName = getMappedInstanceName(name.slowToString());
     return 'get\$$fieldName';
   }
 
+  String getterNameFromAccessorName(String name) {
+    // We dynamically create getters from the field-name. The getter name must
+    // therefore be derived from the instance field-name.
+    return 'get\$$name';
+  }
+
   String getterName(LibraryElement lib, SourceString name) {
     // We dynamically create getters from the field-name. The getter name must
     // therefore be derived from the instance field-name.
-    String fieldName = safeName(privateName(lib, name));
+    String fieldName = getMappedInstanceName(privateName(lib, name));
     return 'get\$$fieldName';
   }
 
-  String getFreshGlobalName(String proposedName) {
-    String name = proposedName;
-    int count = usedGlobals[name];
-    if (count != null) {
-      // Not the first time we see this name. Append a number to make it unique.
-      do {
-        name = '$proposedName${count++}';
-      } while (usedGlobals[name] != null);
-      // Record the count in case we see this name later. We
-      // frequently see names multiple times, as all our closures use
-      // the same name for their class.
-      usedGlobals[proposedName] = count;
+  String getMappedGlobalName(String proposedName) {
+    var newName = globalNameMap[proposedName];
+    if (newName == null) {
+      newName = getFreshName(proposedName, usedGlobalNames);
+      globalNameMap[proposedName] = newName;
     }
-    usedGlobals[name] = 0;
-    return name;
+    return newName;
+  }
+
+  String getMappedInstanceName(String proposedName) {
+    var newName = instanceNameMap[proposedName];
+    if (newName == null) {
+      newName = getFreshName(proposedName, usedInstanceNames);
+      instanceNameMap[proposedName] = newName;
+    }
+    return newName;
+  }
+
+  String getFreshName(String proposedName, Set<String> usedNames) {
+    var candidate;
+    proposedName = safeName(proposedName);
+    if (!usedNames.contains(proposedName)) {
+      candidate = proposedName;
+    } else {
+      var counter = popularNameCounters[proposedName];
+      var i = counter == null ? 0 : counter;
+      while (usedNames.contains("$proposedName$i")) {
+        i++;
+      }
+      popularNameCounters[proposedName] = i + 1;
+      candidate = "$proposedName$i";
+    }
+    usedNames.add(candidate);
+    return candidate;
   }
 
   static const String LIBRARY_PREFIX = "lib";
@@ -247,29 +332,38 @@ class Namer {
     } else {
       name = element.name.slowToString();
     }
-    return safeName(name);
+    return name;
   }
 
   String getSpecializedName(Element element, Collection<ClassElement> classes) {
+    // This gets the minified name, but it doesn't really make much difference.
+    // the important thing is that it is a unique name.

[ngeoffray, 2012/12/10 08:55:42]

the -> The

[erikcorry, 2012/12/10 12:37:40]

Done.

     StringBuffer buffer = new StringBuffer('${getName(element)}\$');
     for (ClassElement cls in classes) {
       buffer.add(getName(cls));
     }
-    return safeName(buffer.toString());
+    return getMappedGlobalName(buffer.toString());
   }
 
   String getBailoutName(Element element) {
     String name = bailoutNames[element];
     if (name != null) return name;
     bool global = !element.isInstanceMember();
+    // Despite the name of the variable, this gets the minified name when we
+    // are minifying, but it doesn't really make much difference.  The
+    // important thing is that it is a unique name.  We add $bailout and, if we
+    // are minifying, we minify the minified name and '$bailout'.
     String unminifiedName = '${getName(element)}\$bailout';
-    name = unminifiedName;
-    if (!global) {
+    if (global) {
+      name = getMappedGlobalName(unminifiedName);
+    } else {
+      name = unminifiedName;
       int i = 0;
       while (usedBailoutInstanceNames.contains(name)) {
         name = '$unminifiedName${i++}';
       }
       usedBailoutInstanceNames.add(name);
+      name = getMappedInstanceName(name);
     }
     bailoutNames[element] = name;
     return name;
@@ -307,21 +401,29 @@ class Namer {
 
       String guess = _computeGuess(element);
       ElementKind kind = element.kind;
-      if (identical(kind, ElementKind.VARIABLE) ||
-          identical(kind, ElementKind.PARAMETER)) {
+      if (kind == ElementKind.VARIABLE ||
+          kind == ElementKind.PARAMETER) {
         // The name is not guaranteed to be unique.
-        return guess;
+        return safeName(guess);
       }
-      if (identical(kind, ElementKind.GENERATIVE_CONSTRUCTOR) ||
-          identical(kind, ElementKind.FUNCTION) ||
-          identical(kind, ElementKind.CLASS) ||
-          identical(kind, ElementKind.FIELD) ||
-          identical(kind, ElementKind.GETTER) ||
-          identical(kind, ElementKind.SETTER) ||
-          identical(kind, ElementKind.TYPEDEF) ||
-          identical(kind, ElementKind.LIBRARY) ||
-          identical(kind, ElementKind.MALFORMED_TYPE)) {
-        String result = getFreshGlobalName(guess);
+      if (kind == ElementKind.GENERATIVE_CONSTRUCTOR ||
+          kind == ElementKind.FUNCTION ||
+          kind == ElementKind.CLASS ||
+          kind == ElementKind.FIELD ||
+          kind == ElementKind.GETTER ||
+          kind == ElementKind.SETTER ||
+          kind == ElementKind.TYPEDEF ||
+          kind == ElementKind.LIBRARY ||
+          kind == ElementKind.MALFORMED_TYPE) {
+        bool isNative = false;
+        if (kind == ElementKind.CLASS) {
+          ClassElement class_elt = element;

[ngeoffray, 2012/12/10 08:55:42]

class_elt -> classElement

[erikcorry, 2012/12/10 12:37:40]

Done.

+          isNative = class_elt.isNative();
+        }
+        if (Elements.isInstanceField(element)) {
+          isNative = element.isNative();
+        }
+        String result = isNative ? guess : getFreshName(guess, usedGlobalNames);
         globals[element] = result;
         return result;
       }
@@ -331,13 +433,12 @@ class Namer {
   }
 
   String getLazyInitializerName(Element element) {
-    // TODO(floitsch): mangle while not conflicting with other statics.
     assert(Elements.isStaticOrTopLevelField(element));
-    return "get\$${getName(element)}";
+    return getMappedGlobalName("get\$${getName(element)}");
   }
 
   String isolatePropertiesAccess(Element element) {
-    return "$ISOLATE.$ISOLATE_PROPERTIES.${getName(element)}";
+    return "$isolateName.$isolatePropertiesName.${getName(element)}";
   }
 
   String isolateAccess(Element element) {
@@ -345,7 +446,8 @@ class Namer {
   }
 
   String isolateBailoutAccess(Element element) {
-    return '${isolateAccess(element)}\$bailout';
+    String newName = getMappedGlobalName('${getName(element)}\$bailout');
+    return '$CURRENT_ISOLATE.$newName';
   }
 
   String isolateLazyInitializerAccess(Element element) {
@@ -353,6 +455,7 @@ class Namer {
   }
 
   String operatorIs(Element element) {
+    // TODO(erikcorry): Reduce from is$x to ix when we are minifying.
     return 'is\$${getName(element)}';
   }