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

pkg/compiler/lib/src/js_backend/minify_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.
  */
 class MinifyNamer extends Namer {
@@ skipping 13 matching lines @@
   final ALPHABET_CHARACTERS = 52;  // a-zA-Z.
   final ALPHANUMERIC_CHARACTERS = 62;  // a-zA-Z0-9.
 
   _FieldNamingRegistry fieldRegistry;
 
   // You can pass an invalid identifier to this and unlike its non-minifying
   // counterpart it will never return the proposedName as the new fresh name.
   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 {
       freshName = _getUnusedName(proposedName, usedNames,
           suggestedNames.values);
     }
     usedNames.add(freshName);
     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',
       'k2', 'k3', 'k4', 'r1', 'r2', 'rx', 'ry', 'x1', 'x2', 'y1', 'y2',
       // 3-letter:
       'add', 'all', 'alt', 'arc', 'CCW', 'cmp', 'dir', 'end', 'get', 'in1',
       'in2', 'INT', 'key', 'log', 'low', 'm11', 'm12', 'm13', 'm14', 'm21',
       'm22', 'm23', 'm24', 'm31', 'm32', 'm33', 'm34', 'm41', 'm42', 'm43',
       'm44', 'max', 'min', 'now', 'ONE', 'put', 'red', 'rel', 'rev', 'RGB',
       'sdp', 'set', 'src', 'tag', 'top', 'uid', 'uri', 'url', 'URL',
       // 4-letter:
       'abbr', 'atob', 'Attr', 'axes', 'axis', 'back', 'BACK', 'beta', 'bias',
       'Blob', 'blue', 'blur', 'BLUR', 'body', 'BOOL', 'BOTH', 'btoa', 'BYTE',
       'cite', 'clip', 'code', 'cols', 'cues', 'data', 'DECR', 'DONE', 'face',
       'file', 'File', 'fill', 'find', 'font', 'form', 'gain', 'hash', 'head',
       'high', 'hint', 'host', 'href', 'HRTF', 'IDLE', 'INCR', 'info', 'INIT',
       'isId', 'item', 'KEEP', 'kind', 'knee', 'lang', 'left', 'LESS', 'line',
       'link', 'list', 'load', 'loop', 'mode', 'name', 'Node', 'None', 'NONE',
       'only', 'open', 'OPEN', 'ping', 'play', 'port', 'rect', 'Rect', 'refX',
       'refY', 'RGBA', 'root', 'rows', 'save', 'seed', 'seek', 'self', 'send',
       'show', 'SINE', 'size', 'span', 'stat', 'step', 'stop', 'tags', 'text',
       '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
       // field names.  Therefore there are some field names we don't want to
       // use.  It is implicit in the next line that the banned prefix is
       // only one character.
       if (_hasBannedPrefix(name)) usedInstanceNames.add(name.substring(1));
     }
 
     // These popular names are present in most programs and deserve
@@ skipping 41 matching lines @@
   void _populateSuggestedNames(Map<String, String> suggestionMap,
                                Set<String> used,
                                List<String> suggestions) {
     int c = $a - 1;
     String letter;
     for (String name in suggestions) {
       do {
         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;
     }
   }
 
 
   // This gets a minified name based on a hash of the proposed name.  This
   // is slightly less efficient than just getting the next name in a series,
   // but it means that small changes in the input program will give smallish
   // changes in the output, which can be useful for diffing etc.
   String _getUnusedName(String proposedName, Set<String> usedNames,
                         Iterable<String> suggestions) {
     int hash = _calculateHash(proposedName);
     // Avoid very small hashes that won't try many names.
     hash = hash < 1000 ? hash * 314159 : hash;  // Yes, it's prime.
 
     // Try other n-character names based on the hash.  We try one to three
     // character identifiers.  For each length we try around 10 different names
     // 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;
         }
         final candidate = new String.fromCharCodes(codes);
         if (!usedNames.contains(candidate) &&
             !jsReserved.contains(candidate) &&
             !_hasBannedPrefix(candidate) &&
             (n != 1 || !suggestions.contains(candidate))) {
           return candidate;
         }
@@ skipping 21 matching lines @@
       h &= 0xffffffff;
       h ^= h >> 6;
       h &= 0xffffffff;
     }
     return h;
   }
 
   /// 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));
     // We don't need to check for banned prefix because the name is in the form
     // xnnn, where nnn is a number.  There can be no getter or setter called
     // gnnn since that would imply a numeric field name.
     return name;
   }
 
   int _letterNumber(int x) {
@@ skipping 186 matching lines @@
   Map<Entity, String> get names => registry.globalNames;
 
   _MixinFieldNamingScope.mixin(ClassElement cls, _FieldNamingRegistry registry)
     : super.rootScope(cls, registry);
 
   _MixinFieldNamingScope.mixedIn(MixinApplicationElement container,
       _FieldNamingScope superScope, _FieldNamingRegistry registry)
     : super.inherit(container, superScope, registry);
 
   String _nextName() {
-    var proposed = super._nextName();
+    String proposed = super._nextName();
     return proposed + r'$';
   }
 }
 
 /**
  * [BoxFieldElement] fields work differently in that they do not belong to an
  * actual class but an anonymous box associated to a [Local]. As there is no
  * inheritance chain, we do not need to compute fields a priori but can assign
  * names on the fly.
  */
 class _BoxFieldNamingScope extends _FieldNamingScope {
   _BoxFieldNamingScope(Local box, _FieldNamingRegistry registry) :
     super.rootScope(box, registry);
 
   bool containsField(_) => true;
 
   String operator[](Element field) {
     if (!names.containsKey(field)) add(field);
     return names[field];
   }
 }