dart2js: Refactoring, documentation, and a few bugfixes in Namer class.

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

Index: pkg/compiler/lib/src/js_backend/minify_namer.dart
diff --git a/pkg/compiler/lib/src/js_backend/minify_namer.dart b/pkg/compiler/lib/src/js_backend/minify_namer.dart
index 4641afdd5fe6fd5cbfe1bb187b86c03eb3060a83..dae2c2d84d879aff5031972053bbcd14b55da838 100644
--- a/pkg/compiler/lib/src/js_backend/minify_namer.dart
+++ b/pkg/compiler/lib/src/js_backend/minify_namer.dart
@@ -31,9 +31,9 @@ class MinifyNamer extends Namer {
   String getFreshName(String proposedName,
                       Set<String> usedNames,
                       Map<String, String> suggestedNames,
-                      {bool ensureSafe: true}) {
-    var freshName;
-    var suggestion = suggestedNames[proposedName];
+                      {bool ensureSafe, bool sanitizeForAnnotations}) {

[floitsch, 2015/01/30 21:03:36]

assert that ensureSafe and sanitizeForAnnotations are not null ?

[asgerf, 2015/02/03 17:39:11]

Since the minifying namer ignores these, I don't see why.

[floitsch, 2015/02/04 00:10:10]

argh.
Of course.

+    String freshName;
+    String suggestion = suggestedNames[proposedName];
     if (suggestion != null && !usedNames.contains(suggestion)) {
       freshName = suggestion;
     } else {
@@ -44,18 +44,23 @@ class MinifyNamer extends Namer {
     return freshName;
   }
 
-  String getClosureVariableName(String name, int id) {
+  String getClosureVariableName(String _, int id) {
     if (id < ALPHABET_CHARACTERS) {
-      return new String.fromCharCodes([_letterNumber(id)]);
+      String name = new String.fromCharCodes([_letterNumber(id)]);
+      // The names 'g' and 's' are the getter/setter for the 'call' property.
+      // These cannot be used as a closure fields.
+      if (!_hasBannedPrefix(name)) return name;

[sra1, 2015/01/30 22:48:27]

Where are 'g' and 's' defined?
Is is 'accidental' that these names correspond to the getter/setter prefix?

Are you sure this is a problem - AFAIK this function names the fields of a scope
'box' which can never be treated as an object, so there should be no collision.

[asgerf, 2015/02/03 17:39:11]

I was concerned because this is also used to name closure fields (as well as box
fields), and these fields can clash with regular property names.

It turns out this method doesn't actually decide the field name --
instanceFieldPropertyName does, and that will only issue 1-letter names if the
proposed name is Q, a, b, c, d, e, f, r, x, y, or z.

Anyways, after some other changes in how methods are disambiguated, the check is
certainly not necessary anymore and has been removed again.

I added some documentation to getClosureVariableName in the Namer class (which
is overridden here) which should clarify get the role of this method.

     }
-    return "${getMappedInstanceName('closure')}_$id";
+    // Fall back to a slightly longer name.
+    String basename = disambiguateMember(null, 'closure');
+    return '${basename}_$id';

[sra1, 2015/01/30 22:48:27]

So will we get something like 'xy_6' where previously we got 'g'?

[asgerf, 2015/02/03 17:39:11]

As it turns out we never got 'g' because it's not a "reserved backend name".
It's pretty counter-intuitive that "reserved" names are used, while unreserved
names are not, but apparently that's how minifying namer work at the moment.

It can certainly be improved, but I think this CL is big enough already.

   }
 
   // From issue 7554.  These should not be used on objects (as instance
   // variables) because they clash with names from the DOM. However, it is
   // OK to use them as fields, as we only access fields directly if we know
   // the receiver type.
-  static const _reservedNativeProperties = const <String>[
+  static const List<String> _reservedNativeProperties = const <String>[
       'Q', 'a', 'b', 'c', 'd', 'e', 'f', 'r', 'x', 'y', 'z',
       // 2-letter:
       'ch', 'cx', 'cy', 'db', 'dx', 'dy', 'fr', 'fx', 'fy', 'go', 'id', 'k1',
@@ -80,9 +85,11 @@ class MinifyNamer extends Namer {
       'Text', 'time', 'type', 'view', 'warn', 'wrap', 'ZERO'];
 
   void reserveBackendNames() {
-    for (var name in _reservedNativeProperties) {
+    Map<String,String> publicMembers =
+        instanceMembers.putIfAbsent(null, () => new Map<String, String>());
+    for (String name in _reservedNativeProperties) {
       if (name.length < 2) {
-        instanceNameMap[name] = name;
+        publicMembers[name] = name;
       }
       usedInstanceNames.add(name);
       // Getter and setter names are autogenerated by prepending 'g' and 's' to
@@ -144,7 +151,7 @@ class MinifyNamer extends Namer {
         assert(c != $Z);
         c = (c == $z) ? $A : c + 1;
         letter = new String.fromCharCodes([c]);
-      } while (used.contains(letter));
+      } while (used.contains(letter) || _hasBannedPrefix(letter));

[sra1, 2015/01/30 22:48:27]

Was this a problem?  How did it trigger?
It seems a shame to lose 'g' just because 'gX' means something.  It is probably
worthwhile strengthening the prefix check in runtime code to permit some common
method to be minified to 'g'.

I don't think 'g' a problem for a global name.

[asgerf, 2015/02/03 17:39:11]

Done. The check is no longer necessary.

It was a problem because 'g' was also the getter for 'call', so an expression "x
+ y" would return x if x was a closure. But 'g' should not actually be the
getter for 'call', that was a misunderstanding on my part, which has been fixed
now.

       assert(suggestionMap[name] == null);
       suggestionMap[name] = letter;
     }
@@ -166,12 +173,12 @@ class MinifyNamer extends Namer {
     // in a predictable order determined by the proposed name.  This is in order
     // to make the renamer stable: small changes in the input should nornally
     // result in relatively small changes in the output.
-    for (var n = 1; n <= 3; n++) {
+    for (int n = 1; n <= 3; n++) {
       int h = hash;
       while (h > 10) {
-        var codes = <int>[_letterNumber(h)];
+        List<int> codes = <int>[_letterNumber(h)];
         int h2 = h ~/ ALPHABET_CHARACTERS;
-        for (var i = 1; i < n; i++) {
+        for (int i = 1; i < n; i++) {
           codes.add(_alphaNumericNumber(h2));
           h2 ~/= ALPHANUMERIC_CHARACTERS;
         }
@@ -213,9 +220,9 @@ class MinifyNamer extends Namer {
   /// If we can't find a hash based name in the three-letter space, then base
   /// the name on a letter and a counter.
   String _badName(int hash, Set<String> usedNames) {
-    var startLetter = new String.fromCharCodes([_letterNumber(hash)]);
-    var name;
-    var i = 0;
+    String startLetter = new String.fromCharCodes([_letterNumber(hash)]);
+    String name;
+    int i = 0;
     do {
       name = "$startLetter${i++}";
     } while (usedNames.contains(name));
@@ -422,7 +429,7 @@ class _MixinFieldNamingScope extends _FieldNamingScope {
     : super.inherit(container, superScope, registry);
 
   String _nextName() {
-    var proposed = super._nextName();
+    String proposed = super._nextName();
     return proposed + r'$';
   }
 }