Refactor and simplify the dart2dart renamer.

sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart

Index: sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
diff --git a/sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart b/sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
index ff5b28a05e2af362616b1a05d976464ffcd8fee1..229adc187b6bad8291f5ecbb9a96e37dccf69251 100644
--- a/sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
+++ b/sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
@@ -8,85 +8,92 @@ Comparator get _compareNodes =>
     compareBy((n) => n.getBeginToken().charOffset);
 
 typedef String _Renamer(Renamable renamable);
+typedef String _NameGenerator(String orginial);
 

[jgruber1, 2014/08/07 15:15:05]

orginial -> original

[sigurdm, 2014/08/14 09:39:17]

Thanks!

-abstract class Renamable {
+abstract class Renamable implements Comparable {
   final int RENAMABLE_TYPE_ELEMENT = 1;
   final int RENAMABLE_TYPE_MEMBER = 2;
   final int RENAMABLE_TYPE_LOCAL = 3;
 
   final Set<Node> nodes;
-  final _Renamer renamer;
 
-  Renamable(this.nodes, this.renamer);
+  Renamable(this.nodes);
   int compareTo(Renamable other) {
     int nodesDiff = other.nodes.length.compareTo(this.nodes.length);
     if (nodesDiff != 0) return nodesDiff;
-    int typeDiff = this.getTypeId().compareTo(other.getTypeId());
+    int typeDiff = this.kind.compareTo(other.kind);
     return typeDiff != 0 ? typeDiff : compareInternals(other);
   }
 
   int compareInternals(Renamable other);
-  int getTypeId();
-
-  String rename() => renamer(this);
+  int get kind;
 }
 
 class ElementRenamable extends Renamable {
   final Element element;
 
-  ElementRenamable(this.element, Set<Node> nodes, _Renamer renamer)
-      : super(nodes, renamer);
+  ElementRenamable(this.element, Set<Node> nodes)
+      : super(nodes);
 
   int compareInternals(ElementRenamable other) =>
       compareElements(this.element, other.element);
-  int getTypeId() => RENAMABLE_TYPE_ELEMENT;
+  int get kind => RENAMABLE_TYPE_ELEMENT;
 }
 
 class MemberRenamable extends Renamable {
   final String identifier;
-  MemberRenamable(this.identifier, Set<Node> nodes, _Renamer renamer)
-      : super(nodes, renamer);
+  MemberRenamable(this.identifier, Set<Node> nodes)
+      : super(nodes);
   int compareInternals(MemberRenamable other) =>
       this.identifier.compareTo(other.identifier);
-  int getTypeId() => RENAMABLE_TYPE_MEMBER;
+  int get kind => RENAMABLE_TYPE_MEMBER;
 }
 
 class LocalRenamable extends Renamable {
-  LocalRenamable(Set<Node> nodes, _Renamer renamer) : super(nodes, renamer);
+  LocalRenamable(Set<Node> nodes)
+      : super(nodes);
   int compareInternals(LocalRenamable other) =>
       _compareNodes(sorted(this.nodes, _compareNodes)[0],
           sorted(other.nodes, _compareNodes)[0]);
-  int getTypeId() => RENAMABLE_TYPE_LOCAL;
+  int get kind => RENAMABLE_TYPE_LOCAL;
 }
 
 /**
  * Renames only top-level elements that would let to ambiguity if not renamed.
  */

[jgruber1, 2014/08/07 15:15:05]

let -> lead, and empty line between doc and class, class decl and vars.

[sigurdm, 2014/08/14 09:39:16]

Done.

-void renamePlaceholders(
-    Compiler compiler,
-    PlaceholderCollector placeholderCollector,
-    Map<Node, String> renames,
-    Map<LibraryElement, String> imports,
-    Set<String> fixedMemberNames,
-    Map<Element, LibraryElement> reexportingLibraries,
-    bool cutDeclarationTypes,
-    {bool uniqueGlobalNaming: false}) {
-  final Map<LibraryElement, Map<String, String>> renamed
-      = new Map<LibraryElement, Map<String, String>>();
 
-  renameNodes(Iterable<Node> nodes, renamer) {
+class PlaceholderRenamer {
+
+  final Compiler compiler;
+  final Map<Node, String> renames = new Map<Node, String>();
+  final Set<String> fixedMemberNames;
+  Map<Element, LibraryElement> reexportingLibraries;
+  final bool cutDeclarationTypes;

[jgruber1, 2014/08/07 15:15:04]

Can this be final?

[sigurdm, 2014/08/14 09:39:16]

Done.

+
+  final Set<LibraryElement> platformImports = new Set<LibraryElement>();
+
+  PlaceholderRenamer(this.compiler, this.fixedMemberNames,
+      this.reexportingLibraries, {this.cutDeclarationTypes}) {
+
+  }
+
+  _renameNodes(Iterable<Node> nodes, String renamer(Node node)) {
     for (Node node in sorted(nodes, _compareNodes)) {

[jgruber1, 2014/08/07 15:15:05]

Add a return type?

[sigurdm, 2014/08/14 09:39:16]

Done.

       renames[node] = renamer(node);
     }
   }
 
-  sortedForEach(Map<Element, dynamic> map, f) {
+  _sortedForEach(Map<Element, dynamic> map, f) {
     for (Element element in sortElements(map.keys)) {

[jgruber1, 2014/08/07 15:15:05]

Ditto, and 'Function f'?

[sigurdm, 2014/08/14 09:39:16]

Done.

       f(element, map[element]);
     }
   }
 
-  String renameType(DartType type, Function renameElement) {
+  final Map<LibraryElement, Map<String, String>> renamed
+      = new Map<LibraryElement, Map<String, String>>();
+

[jgruber1, 2014/08/07 15:15:04]

Maybe move this before functions in this class?

[sigurdm, 2014/08/14 09:39:16]

Done.

+
+  String _renameType(DartType type, Function renameElement) {
     if (type.isDynamic) return 'dynamic';
     // TODO(smok): Do not rename type if it is in platform library or
     // js-helpers.
@@ -94,18 +101,25 @@ void renamePlaceholders(
     if (type is GenericType && !type.treatAsRaw) {
       result.write('<');
       List<DartType> arguments = type.typeArguments;
-      result.write(renameType(arguments.first, renameElement));
+      result.write(_renameType(arguments.first, renameElement));
       for (int index = 1; index < arguments.length; index++) {
         result.write(',');
-        result.write(renameType(arguments[index], renameElement));
+        result.write(_renameType(arguments[index], renameElement));
       }
       result.write('>');
     }
     return result.toString();
   }
 
-  String renameConstructor(Element element, ConstructorPlaceholder placeholder,
-      Function renameString, Function renameElement) {
+  /// Gives a new name, if it was not renamed before in [library].
+  rename(library, originalName) {
+    return renamed.putIfAbsent(library, () => {})

[jgruber1, 2014/08/07 15:15:05]

Types, and make this private to stay consistent. Is it possible to make the name
more descriptive? From the description, I don't know what 'it' is.

[sigurdm, 2014/08/14 09:39:17]

Done.

+       .putIfAbsent(originalName,
+            () => generateUniqueName(originalName));
+  }
+
+  String _renameConstructor(Element element,
+                            ConstructorPlaceholder placeholder) {
     assert(element.isConstructor);
     StringBuffer result = new StringBuffer();
     String name = element.name;
@@ -113,21 +127,21 @@ void renamePlaceholders(
       // Named constructor or factory. Is there a more reliable way to check
       // this case?
       if (!placeholder.isRedirectingCall) {
-        result.write(renameType(placeholder.type, renameElement));
+        result.write(_renameType(placeholder.type, _renameElement));
         result.write('.');
       }
       if (!element.library.isPlatformLibrary) {
-        name = renameString(element.library, name);
+        name = rename(element.library, name);
       }
       result.write(name);
     } else {
       assert(!placeholder.isRedirectingCall);
-      result.write(renameType(placeholder.type, renameElement));
+      result.write(_renameType(placeholder.type, _renameElement));
     }
     return result.toString();
   }
 
-  Function makeElementRenamer(rename, generateUniqueName) => (element) {
+  _renameElement(element) {
     assert(Elements.isErroneousElement(element) ||

[jgruber1, 2014/08/07 15:15:05]

Types.

[sigurdm, 2014/08/14 09:39:17]

Done.

            Elements.isStaticOrTopLevel(element) ||
            element is TypeVariableElement);
@@ -143,173 +157,167 @@ void renamePlaceholders(
         library = reexportingLibraries[element];
       }
       if (!library.isInternalLibrary) {
-        final prefix =
-            imports.putIfAbsent(library, () => generateUniqueName('p'));
-        return '$prefix.$originalName';
+        platformImports.add(library);
+        return originalName;
       }
     }
 
     return rename(library, originalName);
-  };
-
-  Function makeRenamer(generateUniqueName) =>
-      (library, originalName) =>
-          renamed.putIfAbsent(library, () => {})
-              .putIfAbsent(originalName,
-                  () => generateUniqueName(originalName));
-
-  // Renamer function that takes library and original name and returns a new
-  // name for given identifier.
-  Function rename;
-  Function renameElement;
-  // A function that takes original identifier name and generates a new unique
-  // identifier.
-  Function generateUniqueName;
-
-  Set<String> allNamedParameterIdentifiers = new Set<String>();
-  for (var functionScope in placeholderCollector.functionScopes.values) {
-    allNamedParameterIdentifiers.addAll(functionScope.parameterIdentifiers);
   }
 
-  if (compiler.enableMinification) {
-    MinifyingGenerator generator = new MinifyingGenerator();
-    Set<String> forbiddenIdentifiers = new Set<String>.from(['main']);
-    forbiddenIdentifiers.addAll(Keyword.keywords.keys);
-    forbiddenIdentifiers.addAll(fixedMemberNames);
-    generateUniqueName = (_) =>
-        generator.generate((name) =>
-            forbiddenIdentifiers.contains(name)
-            || allNamedParameterIdentifiers.contains(name));
-    rename = makeRenamer(generateUniqueName);
-    renameElement = makeElementRenamer(rename, generateUniqueName);
-
-    List<Set<Node>> allLocals = new List<Set<Node>>();
-
-    // Build a list sorted by usage of local nodes that will be renamed to
-    // the same identifier. So the top-used local variables in all functions
-    // will be renamed first and will all share the same new identifier.
+  MinifyingGenerator generator = new MinifyingGenerator();
+  _NameGenerator generateUniqueName;

[jgruber1, 2014/08/07 15:15:05]

Move these before functions and mark generator final. Maybe generateUniqueName
can be removed entirely since it is only called from functions down the
computeRenamings call stack.

[sigurdm, 2014/08/14 09:39:16]

Done.

+
+  void computeRenamings(PlaceholderCollector placeholderCollector) {

[jgruber1, 2014/08/07 15:15:05]

Maybe a comment on what this does, especially since it's called from the backend
and fairly long.

[sigurdm, 2014/08/14 09:39:17]

Done.

+    Set<String> allNamedParameterIdentifiers = new Set<String>();
     for (var functionScope in placeholderCollector.functionScopes.values) {
-      // Add current sorted local identifiers to the whole sorted list
-      // of all local identifiers for all functions.
-      List<LocalPlaceholder> currentSortedPlaceholders =
-          sorted(functionScope.localPlaceholders,
-              compareBy((LocalPlaceholder ph) => -ph.nodes.length));
-      List<Set<Node>> currentSortedNodes =
-          currentSortedPlaceholders.map((ph) => ph.nodes).toList();
-      // Make room in all sorted locals list for new stuff.
-      while (currentSortedNodes.length > allLocals.length) {
-        allLocals.add(new Set<Node>());
-      }
-      for (int i = 0; i < currentSortedNodes.length; i++) {
-        allLocals[i].addAll(currentSortedNodes[i]);
-      }
+      allNamedParameterIdentifiers.addAll(functionScope.parameterIdentifiers);
     }
 
-    // Rename elements, members and locals together based on their usage count,
-    // otherwise when we rename elements first there will be no good identifiers
-    // left for members even if they are used often.
-    String elementRenamer(ElementRenamable elementRenamable) =>
-        renameElement(elementRenamable.element);
-    String memberRenamer(MemberRenamable memberRenamable) =>
-        generator.generate(forbiddenIdentifiers.contains);
-    Function localRenamer = generateUniqueName;
-    List<Renamable> renamables = [];
-    placeholderCollector.elementNodes.forEach(
-        (Element element, Set<Node> nodes) {
-      renamables.add(new ElementRenamable(element, nodes, elementRenamer));
-    });
-    placeholderCollector.memberPlaceholders.forEach(
-        (String memberName, Set<Identifier> identifiers) {
-      renamables.add(
-          new MemberRenamable(memberName, identifiers, memberRenamer));
-    });
-    for (Set<Node> localIdentifiers in allLocals) {
-      renamables.add(new LocalRenamable(localIdentifiers, localRenamer));
-    }
-    renamables.sort((Renamable renamable1, Renamable renamable2) =>
-        renamable1.compareTo(renamable2));
-    for (Renamable renamable in renamables) {
-      String newName = renamable.rename();
-      renameNodes(renamable.nodes, (_) => newName);
+    Set<String> forbiddenIdentifiers = new Set<String>.from(fixedMemberNames);
+    forbiddenIdentifiers.addAll(Keyword.keywords.keys);
+    forbiddenIdentifiers.add('main');
+
+    String generateUniqueMinifiedName() {
+      return generator.generate((name) =>
+          forbiddenIdentifiers.contains(name)
+          || allNamedParameterIdentifiers.contains(name));
     }
-  } else {
-    // Never rename anything to 'main'.
-    final usedTopLevelOrMemberIdentifiers = new Set<String>();
-    usedTopLevelOrMemberIdentifiers.add('main');
-    usedTopLevelOrMemberIdentifiers.addAll(fixedMemberNames);
-    generateUniqueName = (originalName) {
+
+    generateUniqueNonminifiedName(originalName) {
       String newName = conservativeGenerator(
           originalName, (name) =>
-              usedTopLevelOrMemberIdentifiers.contains(name)
+              forbiddenIdentifiers.contains(name)
               || allNamedParameterIdentifiers.contains(name));
-      usedTopLevelOrMemberIdentifiers.add(newName);
+      forbiddenIdentifiers.add(newName);
       return newName;
-    };
-    rename = makeRenamer(generateUniqueName);
-    renameElement = makeElementRenamer(rename, generateUniqueName);
-    // Rename elements.
-    sortedForEach(placeholderCollector.elementNodes,
-        (Element element, Set<Node> nodes) {
-      renameNodes(nodes, (_) => renameElement(element));
-    });
+    }
+    generateUniqueName = compiler.enableMinification

[jgruber1, 2014/08/07 15:15:04]

Name generation closures can be created and assigned within the if
(enableMinification) .. else .. statement below.

[sigurdm, 2014/08/14 09:39:17]

Done.

+        ? (_) => generateUniqueMinifiedName()
+        : generateUniqueNonminifiedName;
+
+    if (compiler.enableMinification) {
+      // Build a list sorted by usage of local nodes that will be renamed to

[jgruber1, 2014/08/07 15:15:05]

Maybe it'd be easier to read if the then/else branches were refactored into
separate functions.

[sigurdm, 2014/08/14 09:39:17]

Great idea!

+      // the same identifier. So the top-used local variables in all functions
+      // will be renamed first and will all share the same new identifier.
+      int maxLength = placeholderCollector.functionScopes.values.fold(0,
+          (a, b) => max(a, b.localPlaceholders.length));
+
+      List<Set<Node>> allLocals = new List<Set<Node>>
+          .generate(maxLength, (i) => new Set<Node>());
+

[jgruber1, 2014/08/07 15:15:05]

Nit: (_) =>

[sigurdm, 2014/08/14 09:39:16]

Done.

+      for (FunctionScope functionScope
+          in placeholderCollector.functionScopes.values) {
+        // Add current sorted local identifiers to the whole sorted list
+        // of all local identifiers for all functions.
+        List<LocalPlaceholder> currentSortedPlaceholders =
+            sorted(functionScope.localPlaceholders,
+                compareBy((LocalPlaceholder ph) => -ph.nodes.length));
+
+        List<Set<Node>> currentSortedNodes =
+            currentSortedPlaceholders.map((ph) => ph.nodes).toList();
+

[jgruber1, 2014/08/07 15:15:04]

Type? Not sure how strict we are on this.

[sigurdm, 2014/08/14 09:39:16]

I think we don't demand it, but there is really no reason not to.

+        for (int i = 0; i < currentSortedNodes.length; i++) {
+          allLocals[i].addAll(currentSortedNodes[i]);
+        }
+      }
 
-    // Rename locals.
-    sortedForEach(placeholderCollector.functionScopes,
-        (functionElement, functionScope) {
-      Set<LocalPlaceholder> placeholders = functionScope.localPlaceholders;
-      Set<String> memberIdentifiers = new Set<String>();
-      if (functionElement.enclosingClass != null) {
-        functionElement.enclosingClass.forEachMember(
-            (enclosingClass, member) {
-              memberIdentifiers.add(member.name);
-            });
+      // Rename elements, members and locals together based on their usage
+      // count, otherwise when we rename elements first there will be no good
+      // identifiers left for members even if they are used often.
+      List<Renamable> renamables = [];

[jgruber1, 2014/08/07 15:15:05]

<Renamable>[]

[sigurdm, 2014/08/14 09:39:16]

Done.

+      placeholderCollector.elementNodes.forEach(
+          (Element element, Set<Node> nodes) {
+        renamables.add(new ElementRenamable(element, nodes));
+      });
+      placeholderCollector.memberPlaceholders.forEach(
+          (String memberName, Set<Identifier> identifiers) {
+        renamables.add(
+            new MemberRenamable(memberName, identifiers));
+      });
+      for (Set<Node> localIdentifiers in allLocals) {
+        renamables.add(new LocalRenamable(localIdentifiers));
       }
-      Set<String> usedLocalIdentifiers = new Set<String>();
-      for (LocalPlaceholder placeholder in placeholders) {
-        String nextId =
-            conservativeGenerator(placeholder.identifier, (name) =>
-                functionScope.parameterIdentifiers.contains(name)
-                || usedTopLevelOrMemberIdentifiers.contains(name)
-                || usedLocalIdentifiers.contains(name)
-                || memberIdentifiers.contains(name));
-        usedLocalIdentifiers.add(nextId);
-        renameNodes(placeholder.nodes, (_) => nextId);
+      renamables.sort((Renamable renamable1, Renamable renamable2) =>

[jgruber1, 2014/08/07 15:15:05]

Is the explicit comparator necessary? The way I understand it, it is used by
default.

[sigurdm, 2014/08/14 09:39:16]

Done.

+          renamable1.compareTo(renamable2));
+      for (Renamable renamable in renamables) {
+        String newName;
+        if (renamable is ElementRenamable) {

[jgruber1, 2014/08/07 15:15:04]

Use .kind instead? Maybe a final else branch with throw/assert(false)?

[sigurdm, 2014/08/14 09:39:17]

I like the is-check because of type promotion.
Done

+          newName = _renameElement(renamable.element);
+          print("${renamable.element}, $newName");
+        } else if (renamable is MemberRenamable) {
+          newName = generator.generate(forbiddenIdentifiers.contains);
+        } else if (renamable is LocalRenamable) {
+          newName = generateUniqueMinifiedName();
+        }
+        _renameNodes(renamable.nodes, (_) => newName);
       }
-    });
+    } else {
 

[jgruber1, 2014/08/07 15:15:05]

Nit: Whitespace.

[sigurdm, 2014/08/14 09:39:16]

Done.

-    final usedMemberIdentifiers = new Set<String>.from(fixedMemberNames);
-    // Do not rename members to top-levels, that allows to avoid renaming
-    // members to constructors.
-    usedMemberIdentifiers.addAll(usedTopLevelOrMemberIdentifiers);
-    placeholderCollector.memberPlaceholders.forEach((identifier, nodes) {
-      String newIdentifier = conservativeGenerator(
-          identifier, usedMemberIdentifiers.contains);
-      renameNodes(nodes, (_) => newIdentifier);
-    });
-  }
 
-  // Rename constructors.
-  sortedForEach(placeholderCollector.constructorPlaceholders,
-      (Element constructor, List<ConstructorPlaceholder> placeholders) {
-        for (ConstructorPlaceholder ph in placeholders) {
-          renames[ph.node] =
-              renameConstructor(constructor, ph, rename, renameElement);
+      // Rename elements.
+      _sortedForEach(placeholderCollector.elementNodes,
+          (Element element, Set<Node> nodes) {
+        _renameNodes(nodes, (_) => _renameElement(element));
+      });
+
+      // Rename locals.
+      _sortedForEach(placeholderCollector.functionScopes,
+          (functionElement, functionScope) {
+        Set<LocalPlaceholder> placeholders = functionScope.localPlaceholders;
+        Set<String> memberIdentifiers = new Set<String>();
+        if (functionElement.enclosingClass != null) {
+          functionElement.enclosingClass.forEachMember(
+              (enclosingClass, member) {
+                memberIdentifiers.add(member.name);
+              });
         }
-  });
-  sortedForEach(placeholderCollector.privateNodes, (library, nodes) {
-    renameNodes(nodes, (node) => rename(library, node.source));
-  });
-  renameNodes(placeholderCollector.unresolvedNodes,
-      (_) => generateUniqueName('Unresolved'));
-  renameNodes(placeholderCollector.nullNodes, (_) => '');
-  if (cutDeclarationTypes) {
-    for (DeclarationTypePlaceholder placeholder in
-         placeholderCollector.declarationTypePlaceholders) {
-      renames[placeholder.typeNode] = placeholder.requiresVar ? 'var' : '';
+        Set<String> usedLocalIdentifiers = new Set<String>();
+        for (LocalPlaceholder placeholder in placeholders) {
+          String nextId =
+              conservativeGenerator(placeholder.identifier, (name) =>
+                  functionScope.parameterIdentifiers.contains(name)
+                  || forbiddenIdentifiers.contains(name)
+                  || usedLocalIdentifiers.contains(name)
+                  || memberIdentifiers.contains(name));
+          usedLocalIdentifiers.add(nextId);
+          _renameNodes(placeholder.nodes, (_) => nextId);
+        }
+      });
+
+      // Do not rename members to top-levels, that allows to avoid renaming
+      // members to constructors.
+      placeholderCollector.memberPlaceholders.forEach((identifier, nodes) {
+        String newIdentifier = conservativeGenerator(
+            identifier, forbiddenIdentifiers.contains);
+        _renameNodes(nodes, (_) => newIdentifier);
+      });
+    }
+
+    // Rename constructors.
+    _sortedForEach(placeholderCollector.constructorPlaceholders,
+        (Element constructor, List<ConstructorPlaceholder> placeholders) {
+          for (ConstructorPlaceholder placeholder in placeholders) {
+            renames[placeholder.node] =
+                _renameConstructor(constructor, placeholder);
+          }
+    });
+    _sortedForEach(placeholderCollector.privateNodes, (library, nodes) {
+      _renameNodes(nodes, (node) => rename(library, node.source));
+    });
+    _renameNodes(placeholderCollector.unresolvedNodes,
+        (_) => generateUniqueName('Unresolved'));
+    _renameNodes(placeholderCollector.nullNodes, (_) => '');
+    if (cutDeclarationTypes) {
+      for (DeclarationTypePlaceholder placeholder in
+           placeholderCollector.declarationTypePlaceholders) {
+        renames[placeholder.typeNode] = placeholder.requiresVar ? 'var' : '';
+      }
     }
   }
 }
 
+
 /**
  * Generates mini ID based on index.
  * In other words, it converts index to visual representation
@@ -341,7 +349,7 @@ String conservativeGenerator(String name, bool isForbidden(String name)) {
   String result = name;
   int index = 0;
   while (isForbidden(result)) {
-    result = '${generateMiniId(index++)}_$name';
+    result = '${name}_${generateMiniId(index++)}';
   }
   return result;
 }