Use field names that are unique only wrt. the inheritance chain in minified mode.

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 a3182791dc285c6182fea6b83224b135a3336050..86bf2f7113642a9327663c92c0601a3f5b3003d4 100644
--- a/pkg/compiler/lib/src/js_backend/minify_namer.dart
+++ b/pkg/compiler/lib/src/js_backend/minify_namer.dart
@@ -10,6 +10,7 @@ part of js_backend;
 class MinifyNamer extends Namer {
   MinifyNamer(Compiler compiler) : super(compiler) {
     reserveBackendNames();
+    fieldRegistry = new _FieldNamingRegistry(this);
   }
 
   String get isolateName => 'I';
@@ -20,8 +21,10 @@ class MinifyNamer extends Namer {
   final String setterPrefix = 's';
   final String callPrefix = ''; // this will create function names $<n>
 
-  static const ALPHABET_CHARACTERS = 52;  // a-zA-Z.
-  static const ALPHANUMERIC_CHARACTERS = 62;  // a-zA-Z0-9.
+  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.
@@ -47,34 +50,36 @@ class MinifyNamer extends Namer {
     return "${getMappedInstanceName('closure')}_$id";
   }
 
+  // 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>[
+      '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() {
-    // From issue 7554.  These should not be used on objects (as instance
-    // variables) because they clash with names from the DOM.
-    const 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'];
-
-    for (var name in reservedNativeProperties) {
+    for (var name in _reservedNativeProperties) {
       if (name.length < 2) {
         instanceNameMap[name] = name;
       }
@@ -222,4 +227,200 @@ class MinifyNamer extends Namer {
     return $0 + x - 52;
   }
 
+  String instanceFieldPropertyName(Element element) {
+    if (element.hasFixedBackendName) {
+      return element.fixedBackendName;
+    }
+
+    _FieldNamingScope names;
+    if (element is BoxFieldElement) {
+      names = new _FieldNamingScope.forBox(element.box, fieldRegistry);
+    } else {
+      ClassElement cls = element is ClosureFieldElement
+          ? element.closureClass : element.enclosingClass;
+      names = new _FieldNamingScope.forClass(cls, compiler.world,
+          fieldRegistry);
+    }
+
+    // The inheritance scope based naming did not yield a name. For instance,
+    // this could be because the field belongs to a mixin.
+    if (!names.containsField(element)) {
+      return super.instanceFieldPropertyName(element);
+    }
+
+    return names[element];
+  }
+}
+
+/**
+ * Encapsulates the global state of field naming.
+ */
+class _FieldNamingRegistry {
+  final MinifyNamer namer;
+
+  final Map<Entity, _FieldNamingScope> scopes =
+      new Map<Entity, _FieldNamingScope>();
+
+  final Map<Entity, String> globalNames = new Map<Entity, String>();
+
+  int globalCount = 0;
+
+  final List<String> nameStore = new List<String>();
+
+  _FieldNamingRegistry(this.namer);
+
+  String getName(int count) {
+    if (count >= nameStore.length) {
+      // Try reuse short reserved names (which are safe for fields). Otherwise

[floitsch, 2014/11/19 13:04:26]

Start by saying that the namer doesn't use reserved names, but some of them are
short and we thus want to shortcut the namer for them.

[herhut, 2014/11/20 10:36:37]

Done.

+      // go through the normal namer.
+      if (count < MinifyNamer._reservedNativeProperties.length &&
+          MinifyNamer._reservedNativeProperties[count].length <= 2) {
+        nameStore.add(MinifyNamer._reservedNativeProperties[count]);
+      } else {
+        nameStore.add(namer.getFreshName("field$count",
+            namer.usedInstanceNames, namer.suggestedInstanceNames,
+            ensureSafe: true));
+      }
+    }
+
+    return nameStore[count];
+  }
+}
+
+/**
+ * A [_FieldNamingScope] encodes a node in the inheritance tree of the current
+ * class hierarchy. The root node typically is the node corresponding to the
+ * `Object` class. It is used to assign a unique name to each field of a class.
+ * Unique here means unique wrt. all fields along the path back to the root.
+ * This is achieved at construction time via the [_count] field that counts the
+ * number of fields on the path to the root node that have been encountered so
+ * far.
+ *
+ * Obviously, this only works if no fields are added to a parent node after its
+ * children have added their first field.
+ */
+class _FieldNamingScope {
+  final _FieldNamingScope superScope;
+  final Entity container;
+  final Map<Element, String> names = new Maplet<Element, String>();
+  final _FieldNamingRegistry registry;
+  int _count;

[floitsch, 2014/11/19 13:04:26]

Document _count.
Apparently it serves as both maximum and local count. That's confusing.

[herhut, 2014/11/20 10:36:37]

Done.

+
+  // The maxium count along the superclass chain.

[floitsch, 2014/11/19 13:04:26]

maximum

[herhut, 2014/11/20 10:36:37]

Done.

+  int get maxCount => _count;
+
+  // The local count. This is separate to be able to override it in subclasses.

[floitsch, 2014/11/19 13:04:26]

What's a "local" count?
Why is it the same as the maxCount.

[herhut, 2014/11/20 10:36:37]

Done.

+  int get _localCount => _count;
+  void set _localCount(int val) { _count = val; }
+
+  factory _FieldNamingScope.forClass(ClassElement cls, ClassWorld world,
+      _FieldNamingRegistry registry) {
+    _FieldNamingScope computeFieldNames() {

[floitsch, 2014/11/19 13:04:25]

new line before nested function.

[herhut, 2014/11/20 10:36:37]

Done.

+      _FieldNamingScope result;
+      // For a minin class, we have to generate globally unique names

[floitsch, 2014/11/19 13:04:26]

"mixin".
finish with a ".".

Nit:
I'm not sure how the comment helps here. It's already on the
MixinFieldNamingScope. Here we just need to know that it needs to be treated
specially (which is clear from the code).

[herhut, 2014/11/20 10:36:37]

Done.

+      if (world.isUsedAsMixin(cls)) {
+        result = new _MixinFieldNamingScope.mixin(cls, registry);
+      } else {
+        ClassElement superClass = cls.superclass;

[floitsch, 2014/11/19 13:04:26]

apparently we generally use "superclass" and not "superClass".

[herhut, 2014/11/20 10:36:37]

Done.

+        if (superClass != null) {
+          _FieldNamingScope superScope = new _FieldNamingScope.forClass(
+              superClass, world, registry);
+          // Mixin applications use the global scope.

[floitsch, 2014/11/19 13:04:26]

Same nit for comment: it's not clear how the "new _MixinFieldNamingScope"
satisfies the "use the global scope". Rather move that comment to where it is
relevant (where it probably already is).

[herhut, 2014/11/20 10:36:37]

Done.

+          if (cls.isMixinApplication) {
+            result = new _MixinFieldNamingScope.mixedin(cls, superScope,

[floitsch, 2014/11/19 13:04:26]

mixedIn ?

[herhut, 2014/11/20 10:36:37]

Done.

+                registry);
+          } else {
+            result = new _FieldNamingScope.inherit(cls, superScope, registry);
+          }
+        } else {

[floitsch, 2014/11/19 13:04:26]

I think it's easier to read if this is the default case. Then the actual
condition is close to the 'else'.

if (superClass == null) {
  result = new _FieldNamingScope.rootScope(cls, registry);
} else {
  superClass = cls.superclass;
  ...
}

Maybe even avoid the 'superClass' local.

if (world.isUsedAsMixin(cls)) {
  result = new _MixinFieldNamingScope.mixin(cls, registry);
} else if (cls.superclass == null) {
  result = new _FieldNamingScope.rootScope(cls, registry);
} else {
  _FieldNamingScope superScope =
      new _FieldNamingScope.forClass(cls.superclass, world, registry);
  if (cls.isMixinApplication) {
    result = new _MixinFieldNamingScope.mixedin(cls, superScope, registry);
  } else {
    result = new _FieldNamingScope.inherit(cls, superScope, registry);
  }
}


Finally: couldn't the mixedIn and inherit constructors just construct the
super-scope themselves?

[herhut, 2014/11/20 10:36:37]

I have reordered them.
Then I would also have to pass [world] to them and I prefer to keep the
dependency to world in the factory.

+          result = new _FieldNamingScope.rootScope(cls, registry);
+        }
+      }
+
+      cls.forEachInstanceField((cls, field) => result.add(field));
+      return result;
+    }
+
+    return registry.scopes.putIfAbsent(cls, computeFieldNames);

[floitsch, 2014/11/19 13:04:26]

I prefer the more common pattern:

_FieldNamingScope cached = registry.scopes[cls];
if (cached != null) return cached;

<code that was inside computeFieldNames>
registry[cls] = result;
return result;

[herhut, 2014/11/20 10:36:37]

Done.

+  }
+
+  factory _FieldNamingScope.forBox(Local box, _FieldNamingRegistry registry) {
+    return registry.scopes.putIfAbsent(box,
+        () => new _BoxFieldNamingScope(box, registry));
+  }
+
+  _FieldNamingScope.rootScope(this.container, this.registry)
+    : superScope = null,
+      _count = 0;
+
+  _FieldNamingScope.inherit(this.container, this.superScope, this.registry) {
+    _count = superScope.maxCount;
+  }
+
+  _isValueUnused(String name) {

[floitsch, 2014/11/19 13:04:26]

comment what values we are talking about.
I would have thought that this is "isNameUnused".

[herhut, 2014/11/20 10:36:38]

Done.

+    return !names.values.contains(name) &&
+        ((superScope == null) || superScope._isValueUnused(name));
+  }
+
+  String _nextName() => registry.getName(_localCount++);
+
+  String operator[](Element field) {
+    String name = names[field];
+    if (name == null && superScope != null) return superScope[field];
+    return name;
+  }
+
+  void add(Element field) {
+    if (names.containsKey(field)) return;
+
+    String value = _nextName();
+    assert(invariant(field, _isValueUnused(value)));
+    names[field] = value;
+  }
+
+  bool containsField(Element field) => names.containsKey(field);
+}
+
+/**
+ * Field names for mixins have two constraints: They need to be unique in the
+ * hierarchy of each application of a mixin and they need to be the same for
+ * all applications of a mixin. To achieve this, we use global naming for
+ * mixins and add a `$` at the end to ensure they are unqiue across all field

[floitsch, 2014/11/19 13:04:26]

how does the "$" guarantee that it's unique across all field names?
add assert here and in other locations:

assert(isReserved(name)); // or isReservedForMixin(name);

and in the other uses:
assert(!isReserved(name));  // or !isReservedForMixin(name);

[herhut, 2014/11/20 10:36:37]

I have expanded the comment to explain this. 
The uniqueness is checked when adding a field (the add method does so by
asserting on isNameUnused).

+ * names.
+ */
+class _MixinFieldNamingScope extends _FieldNamingScope {
+  int get _localCount => registry.globalCount;
+  void set _localCount(int val) { registry.globalCount = val; }
+
+  Map<Element, 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();
+    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];
+  }
 }