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

pkg/compiler/lib/src/js_backend/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.
+ *
+ * Names are generated through three stages:
+ *
+ * 1. Original names and proposed names
+ * 2. Disambiguated names (also known as "mangled names")
+ * 3. Annotated names
+ *
+ * Original names are names taken directly from the input.
+ *
+ * Proposed names are either original names or synthesized names for input
+ * elements that do not have original names.
+ *
+ * Disambiguated names are derived from the above, but are mangled to ensure
+ * uniqueness within some namespace (e.g. as fields on the same JS object).
+ * In [MinifyNamer], disambiguated names are also minified.
+ *
+ * Annotated names are names generated from a disambiguated name. Annnotated
+ * names must be computable at runtime by prefixing/suffixing constant strings
+ * onto the disambiguated name.
+ *
+ * For example, some entity called `x` might be associated with these names:
+ *
+ *     Original name: `x`
+ *
+ *     Disambiguated name: `x1` (if something else was called `x`)
+ *
+ *     Annotated names: `x1`     (field name)
+ *                      `get$x1` (getter name)
+ *                      `set$x1` (setter name)
+ *                      `x1$2`   (if `x` is a method with 2 parameters)
+ *
+ * The [Namer] can choose the disambiguated names, and to some degree the
+ * prefix/suffix constants used to construct annotated names. It cannot choose
+ * annotated names with total freedom, for example, it cannot choose that the
+ * getter for `x1` should be called `getX` -- the annotated names are always
+ * built by concatenation.
+ *
+ * Disambiguated names must be chosen such that none of the annotated names can
+ * clash with each other. This may happen even if the disambiguated names are
+ * distinct, for example, suppose a field `x` and `get$x` exists in the input:
+ *
+ *     Original names: `x` and `get$x`
+ *
+ *     Disambiguated names: `x` and `get$x` (the two names a different)
+ *
+ *     Annotated names: `x` (field for `x`)
+ *                      `get$x` (getter for `x`)
+ *                      `get$x` (field for `get$x`)
+ *                      `get$get$x` (getter for `get$x`)
+ *
+ * The getter for `x` clashes with the field name for `get$x`, so the
+ * disambiguated names are invalid.
+ *
+ * Additionally, disambiguated names must be chosen such that all annotated
+ * names are valid JavaScript identifiers and do not coincide with a native
+ * JavaScript property such as `__proto__`.
+ *
+ * The following annotated names are generated for instance members, where
+ * <NAME> denotes the disambiguated name.
+ *
+ * 0. The disambiguated name can itself be seen as a annotated name.

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

as an annotated name.

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

Done.

+ *
+ * 1. Multiple annotated names exist per method name, encoding arity and named
+ *    parameters with the pattern:
+ *
+ *       <NAME>$<N>$namedParam1...$namedParam<M>
+ *
+ *    where <N> is the number of parameters (required and optional) and <M> is
+ *    the number of named parameters, and namedParam<n> are the names of the
+ *    named parameters in alphabetical order.
+ *
+ *    Note that this convention is not encapsulated in the [Namer], and is
+ *    hardcoded into other components, such as [Element] and [Selector].

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

:(

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

Clarified that it's just the ordering of parameters that's hardcoded this way. I
made it sound worse than it was.

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

ok. that's not a big deal.

+ *
+ * 2. The getter/setter for a field, or tear-off getter for a method:
+ *
+ *        get$<NAME>
+ *        set$<NAME>
+ *
+ *    (The [getterPrefix] and [setterPrefix] are different in [MinifyNamer]).
+ *
+ * 3. The `is` and operator uses the following names:
+ *
+ *        $is<NAME>
+ *        $as<NAME>
+ *
+ * For local variables, the [Namer] only provides *proposed names*. These names
+ * must be disambiguated elsewhere.
  */
 class Namer implements ClosureNamer {
 
-  static const javaScriptKeywords = const <String>[
+  static const List<String> javaScriptKeywords = const <String>[
     // These are current keywords.
     "break", "delete", "function", "return", "typeof", "case", "do", "if",
     "switch", "var", "catch", "else", "in", "this", "void", "continue",
     "false", "instanceof", "throw", "while", "debugger", "finally", "new",
     "true", "with", "default", "for", "null", "try",
 
     // These are future keywords.
     "abstract", "double", "goto", "native", "static", "boolean", "enum",
     "implements", "package", "super", "byte", "export", "import", "private",
     "synchronized", "char", "extends", "int", "protected", "throws",
     "class", "final", "interface", "public", "transient", "const", "float",
     "long", "short", "volatile"
   ];
 
-  static const reservedPropertySymbols =
+  static const List<String> reservedPropertySymbols =
       const <String>["__proto__", "prototype", "constructor", "call"];
 
   // Symbols that we might be using in our JS snippets.
-  static const reservedGlobalSymbols = const <String>[
+  static const List<String> reservedGlobalSymbols = const <String>[
     // Section references are from Ecma-262
     // (http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf)
 
     // 15.1.1 Value Properties of the Global Object
     "NaN", "Infinity", "undefined",
 
     // 15.1.2 Function Properties of the Global Object
     "eval", "parseInt", "parseFloat", "isNaN", "isFinite",
 
     // 15.1.3 URI Handling Function Properties
@@ skipping 99 matching lines @@
     "Table", "TableCell", "TableRow", "TableSelection", "Text", "TextArea",
     "UIEvent", "Window", "XMLHttpRequest", "XMLSerializer",
     "XPathException", "XPathResult", "XSLTProcessor",
 
     // These keywords trigger the loading of the java-plugin. For the
     // next-generation plugin, this results in starting a new Java process.
     "java", "Packages", "netscape", "sun", "JavaObject", "JavaClass",
     "JavaArray", "JavaMember",
   ];
 
-  static const reservedGlobalObjectNames = const <String>[
+  static const List<String> reservedGlobalObjectNames = const <String>[
       "A",
       "B",
       "C", // Global object for *C*onstants.
       "D",
       "E",
       "F",
       "G",
       "H", // Global object for internal (*H*elper) libraries.
       // I is used for used for the Isolate function.
       "J", // Global object for the interceptor library.
       "K",
       "L",
       "M",
       "N",
       "O",
       "P", // Global object for other *P*latform libraries.
       "Q",
       "R",
       "S",
       "T",
       "U",
       "V",
       "W", // Global object for *W*eb libraries (dart:html).
       "X",
       "Y",
       "Z",
   ];
 
-  static const reservedGlobalHelperFunctions = const <String>[
+  static const List<String> reservedGlobalHelperFunctions = const <String>[
       "init",
       "Isolate",
   ];
 
-  static final userGlobalObjects = new List.from(reservedGlobalObjectNames)
+  static final List<String> userGlobalObjects =
+      new List.from(reservedGlobalObjectNames)
       ..remove('C')
       ..remove('H')
       ..remove('J')
       ..remove('P')
       ..remove('W');
 
   Set<String> _jsReserved = null;
   /// Names that cannot be used by members, top level and static
   /// methods.
   Set<String> get jsReserved {
@@ skipping 38 matching lines @@
 
   final String classDescriptorProperty = r'^';
 
   // Name of property in a class description for the native dispatch metadata.
   final String nativeSpecProperty = '%';
 
   static final RegExp IDENTIFIER = new RegExp(r'^[A-Za-z_$][A-Za-z0-9_$]*$');
   static final RegExp NON_IDENTIFIER_CHAR = new RegExp(r'[^A-Za-z_0-9$]');
 
   /**
    * Map from top-level or static elements to their unique identifiers provided

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

This comment is on the wrong field.

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

Done.

    * by [getName].

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

`getName` doesn't exist anymore. Was replaced with `getNameX` which you just
removed as well...

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

disambiguateGlobal is the closest we have now

    *
    * Invariant: Keys must be declaration elements.
    */
   final Compiler compiler;
   final Map<Element, String> globals;
   final Map<String, LibraryElement> shortPrivateNameOwners;
 
   final Set<String> usedGlobalNames;
   final Set<String> usedInstanceNames;
-  final Map<String, String> globalNameMap;
+  final Map<String, String> internGlobals;

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

add comment.
internGlobals -> internalGlobals?

Many other fields have "Names" as suffix. Not this one...
Unify?

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

Done.

   final Map<String, String> suggestedGlobalNames;

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

I would group the fields:

globals
usedGlobalNames
suggestedGlobalNames

instanceMembers
usedInstanceNames
suggestedInstanceNames

Add comments to all fields that are not completely obvious.

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

Grouped and renamed, to be more streamlined. I added a comment on the usedX set
for each group and hope it is clear the following maps all are part of the
mapping into that namespace.

-  final Map<String, String> instanceNameMap;
+  final Map<LibraryElement, Map<String, String>> instanceMembers;

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

Add a comment explaining that `null` is used as key for public members.

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

The map works differently now, see disambiguateMember.

I decided to omit declaration-site comments for these maps since they are
repetitive and verbose, and most of them are only referenced from one method, so
it should be easy to determine their use if it is not clear from the name and
grouping.

   final Map<String, String> suggestedInstanceNames;
+  final Map<Element, String> directAccessMap = <Element, String>{};

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

comment.

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

see above.

 
   final Map<String, String> operatorNameMap;
   final Map<String, int> popularNameCounters;
 
   final Map<ConstantValue, String> constantNames;
   final Map<ConstantValue, String> constantLongNames;
   ConstantCanonicalHasher constantHasher;
 
   // All alphanumeric characters.
   static const String _alphaNumeric =
       'abcdefghijklmnopqrstuvwxyzABZDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
 
   Namer(Compiler compiler)
       : compiler = compiler,

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

nit: Namer(this.compiler);

You could move all assignments (except the constantHasher and functionTypeNamer)
to the field-declarations. Since they would almost never fit on one line anyway,
I'm ok with leaving them here.

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

Done.

         globals = new Map<Element, String>(),
         shortPrivateNameOwners = new Map<String, LibraryElement>(),
         usedGlobalNames = new Set<String>(),
         usedInstanceNames = new Set<String>(),
-        instanceNameMap = new Map<String, String>(),
+        instanceMembers = new Map<LibraryElement, Map<String, String>>(),
         operatorNameMap = new Map<String, String>(),
-        globalNameMap = new Map<String, String>(),
+        internGlobals = new Map<String, String>(),
         suggestedGlobalNames = new Map<String, String>(),
         suggestedInstanceNames = new Map<String, String>(),
         popularNameCounters = new Map<String, int>(),
         constantNames = new Map<ConstantValue, String>(),
         constantLongNames = new Map<ConstantValue, String>(),
         constantHasher = new ConstantCanonicalHasher(compiler),
         functionTypeNamer = new FunctionTypeNamer(compiler);
 
   JavaScriptBackend get backend => compiler.backend;
 
@@ skipping 16 matching lines @@
       case 'REFLECTABLE': return reflectableField;
       case 'CLASS_DESCRIPTOR_PROPERTY': return classDescriptorProperty;
       default:
         compiler.reportError(
             node, MessageKind.GENERIC,
             {'text': 'Error: Namer has no name for "$name".'});
         return 'BROKEN';
     }
   }
 
+  /// Disambiguated name for [constant].
+  ///
+  /// Unique within the global-member namespace.

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

Is that necessary?
After all, we always put constants on the C object where they don't conflict
with anything else.

If in doubt, just leave a TODO.
See also comment in disambiguateGlobal.

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

You are right, but this is how it worked to begin with. We can do more
fine-grained disambiguation of globals, but I deliberately left it this way to
limit the scope of the CL.

I left a TODO to clarfy that this is just how the implementation works atm.

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

TODO is fine.

   String constantName(ConstantValue constant) {
     // In the current implementation it doesn't make sense to give names to
     // function constants since the function-implementation itself serves as
     // constant and can be accessed directly.
     assert(!constant.isFunction);
     String result = constantNames[constant];
     if (result == null) {
       String longName = constantLongName(constant);
       result = getFreshName(longName, usedGlobalNames, suggestedGlobalNames,
                             ensureSafe: true);
       constantNames[constant] = result;
     }
     return result;
   }
 
-  // The long name is unminified and may have collisions.
+  /// Proposed name for [constant].
   String constantLongName(ConstantValue constant) {

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

This looks like a function that shouldn't be called from the outside. Make it
private?

Same for all other private methods.

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

The emitters currently call it for some sort of compression optimization.

I've made some other methods private, though, in particular the disambiguateXXX
methods.

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

Acknowledged.

     String longName = constantLongNames[constant];
     if (longName == null) {
       longName = new ConstantNamingVisitor(compiler, constantHasher)
           .getName(constant);
       constantLongNames[constant] = longName;
     }
     return longName;
   }
 
   String breakLabelName(LabelDefinition label) {
     return '\$${label.labelName}\$${label.target.nestingLevel}';
   }
 
   String implicitBreakLabelName(JumpTarget target) {
     return '\$${target.nestingLevel}';
   }
 
   // We sometimes handle continue targets differently from break targets,
   // so we have special continue-only labels.
   String continueLabelName(LabelDefinition label) {
     return 'c\$${label.labelName}\$${label.target.nestingLevel}';
   }
 
   String implicitContinueLabelName(JumpTarget target) {
     return 'c\$${target.nestingLevel}';
   }
 
   /**
-   * If the [name] is not private returns [:name:]. Otherwise
-   * mangles the [name] so that each library has a unique name.
+   * If the [originalName] is not private returns [originalName]. Otherwise
+   * mangles the [originalName] so that each library has its own distinguished
+   * version of the name.
+   *
+   * Although the name is not guaranteed to be unique within any namespace,
+   * clashes are very unlikely in practice. Therefore, it can be used in cases
+   * where uniqueness is nice but not a strict requirement.
+   *
+   * The resulting name is a *proposed name* and is never minified.
    */
-  String privateName(LibraryElement library, String name) {
+  String privateName(LibraryElement library, String originalName) {
     // Public names are easy.
-    String nameString = name;
-    if (!isPrivateName(name)) return nameString;
+    if (!isPrivateName(originalName)) return originalName;
 
     // The first library asking for a short private name wins.
     LibraryElement owner =
-        shortPrivateNameOwners.putIfAbsent(nameString, () => library);
+        shortPrivateNameOwners.putIfAbsent(originalName, () => library);
 
-    if (owner == library && !nameString.contains('\$')) {
+    if (owner == library && !originalName.contains('\$')) {
       // Since the name doesn't contain $ it doesn't clash with any
       // of the private names that have the library name as the prefix.
-      return nameString;
+      return originalName;
     } else {
       // Make sure to return a private name that starts with _ so it
       // cannot clash with any public names.
-      String libraryName = getNameOfLibrary(library);
-      return '_$libraryName\$$nameString';
+      String libraryName = disambiguateGlobal(library);
+      return '_$libraryName\$${originalName}';

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

Add comment why this is not unique.

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

Done.

     }
   }
 
-  String instanceMethodName(FunctionElement element) {
-    // TODO(ahe): Could this be: return invocationName(new
-    // Selector.fromElement(element))?
-    String elementName = element.name;
-    String name = operatorNameToIdentifier(elementName);
-    if (name != elementName) return getMappedOperatorName(name);
-
-    LibraryElement library = element.library;
-    if (element.isGenerativeConstructorBody) {
-      name = Elements.reconstructConstructorNameSourceString(element);
+  /// Annotated name for [method] encoding arity and named parameters.
+  String instanceMethodName(FunctionElement method) {
+    if (method.isGenerativeConstructorBody) {
+      return disambiguateDirectAccess(method,
+          () => Elements.reconstructConstructorNameSourceString(method));
     }
-    FunctionSignature signature = element.functionSignature;
-    // We don't mangle the closure invoking function name because it
-    // is generated by string concatenation in applyFunction from
-    // js_helper.dart. To keep code size down, we potentially shorten
-    // the prefix though.
-    String methodName;
-    if (name == closureInvocationSelectorName) {
-      methodName = '$callPrefix\$${signature.parameterCount}';
-    } else {
-      methodName = '${privateName(library, name)}\$${signature.parameterCount}';
-    }
-    if (signature.optionalParametersAreNamed &&
-        !signature.optionalParameters.isEmpty) {
-      StringBuffer buffer = new StringBuffer();
-      signature.orderedOptionalParameters.forEach((Element element) {
-        buffer.write('\$${safeName(element.name)}');
-      });
-      methodName = '$methodName$buffer';
-    }
-    if (name == closureInvocationSelectorName) return methodName;
-    return getMappedInstanceName(methodName);
+    return invocationName(new Selector.fromElement(method));
   }
 
-  String publicInstanceMethodNameByArity(String name, int arity) {
-    String newName = operatorNameToIdentifier(name);
-    if (newName != name) return getMappedOperatorName(newName);
-    assert(!isPrivateName(name));
-    // We don't mangle the closure invoking function name because it
-    // is generated by string concatenation in applyFunction from
-    // js_helper.dart. To keep code size down, we potentially shorten
-    // the prefix though.
-    if (name == closureInvocationSelectorName) return '$callPrefix\$$arity';
-
-    return getMappedInstanceName('$name\$$arity');
+  /// Annotated name for a public method with the given [originalName]
+  /// and [arity] and no named parameters.

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

Maybe mention that it handles operators?

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

Not anymore.

+  String publicInstanceMethodNameByArity(String originalName, int arity) {
+    String opName = operatorNameToIdentifier(originalName);
+    if (opName != originalName) return disambiguateOperator(opName);
+    assert(!isPrivateName(originalName));
+    String disambiguatedName = disambiguateMember(null, originalName);
+    Selector selector = new Selector.call(originalName, null, arity);
+    return deriveMethodName(disambiguatedName, selector);
   }
 
+  /// Returns the annotated name for a method with the given name,
+  /// encoding arity and named parameters with the pattern:
+  ///
+  ///     <NAME>$<N>$namedParam1...$namedParam<M>
+  ///
+  String deriveMethodName(String disambiguatedName, Selector selector) {
+    assert(selector.isCall);
+    // TODO(asgerf): Avoid clash when named parameters contain '$' symbols.

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

Create a failing test.

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

Done. And the issue has been fixed.

+    StringBuffer buffer = new StringBuffer();
+    for (String argumentName in selector.getOrderedNamedArguments()) {
+      buffer.write('\$${safeName(argumentName)}');
+    }
+    return '$disambiguatedName\$${selector.argumentCount}$buffer';
+  }
+
+  /// Annotated name for the member being invoked by [selector].
   String invocationName(Selector selector) {
-    if (selector.isGetter) {
-      String proposedName = privateName(selector.library, selector.name);
-      return '$getterPrefix${getMappedInstanceName(proposedName)}';
-    } else if (selector.isSetter) {
-      String proposedName = privateName(selector.library, selector.name);
-      return '$setterPrefix${getMappedInstanceName(proposedName)}';
-    } else {
-      String name = selector.name;
-      if (selector.kind == SelectorKind.OPERATOR
-          || selector.kind == SelectorKind.INDEX) {
-        name = operatorNameToIdentifier(name);
-        assert(name != selector.name);
-        return getMappedOperatorName(name);
-      }
-      assert(name == operatorNameToIdentifier(name));
-      StringBuffer buffer = new StringBuffer();
-      for (String argumentName in selector.getOrderedNamedArguments()) {
-        buffer.write('\$${safeName(argumentName)}');
-      }
-      String suffix = '\$${selector.argumentCount}$buffer';
-      // We don't mangle the closure invoking function name because it
-      // is generated by string concatenation in applyFunction from
-      // js_helper.dart. We potentially shorten the prefix though.
-      if (selector.isClosureCall) {
-        return "$callPrefix$suffix";
-      } else {
-        String proposedName = privateName(selector.library, name);
-        return getMappedInstanceName('$proposedName$suffix');
-      }
+    LibraryElement library = selector.library;
+    switch (selector.kind) {
+      case SelectorKind.GETTER:
+        String disambiguatedName = disambiguateMember(library, selector.name);
+        return deriveGetterName(disambiguatedName);
+
+      case SelectorKind.SETTER:
+        String disambiguatedName = disambiguateMember(library, selector.name);
+        return deriveSetterName(disambiguatedName);
+
+      case SelectorKind.OPERATOR:
+      case SelectorKind.INDEX:
+        String operatorIdentifier = operatorNameToIdentifier(selector.name);
+        String disambiguatedName = disambiguateOperator(operatorIdentifier);
+        return disambiguatedName; // Operators are not annotated.
+
+      case SelectorKind.CALL:
+        String disambiguatedName = disambiguateMember(library, selector.name);
+        return deriveMethodName(disambiguatedName, selector);
+
+      default: throw 'Unexpected selector kind: ${selector.kind}';

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

throw an internal error. (we tend to avoid throwing strings).

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

Done.

     }
   }
 
   /**
    * Returns the internal name used for an invocation mirror of this selector.

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

What's an "internal" name?

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

I don't know what this function is used for.

    */
   String invocationMirrorInternalName(Selector selector)
       => invocationName(selector);
 
   /**
-   * Returns name of accessor (root to getter and setter) for a static or
-   * instance field.
+   * Returns the disambiguated name for the given field, used for constructing
+   * the getter and setter names.
    */
   String fieldAccessorName(Element element) {
     return element.isInstanceMember
-        ? instanceFieldAccessorName(element)
-        : getNameOfField(element);
+        ? disambiguateMember(element.library, element.name)
+        : disambiguateGlobal(element);
   }
 
   /**
    * Returns name of the JavaScript property used to store a static or instance
    * field.
    */
   String fieldPropertyName(Element element) {
     return element.isInstanceMember
         ? instanceFieldPropertyName(element)
-        : getNameOfField(element);
+        : disambiguateGlobal(element);
   }
 
   /**
-   * Returns name of accessor (root to getter and setter) for an instance field.
+   * Returns name of the JavaScript property used to store the given type
+   * variable.

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

Also mention that this is inside a class and thus at the same level as instance
members. (if I'm right).

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

Done.

    */
-  String instanceFieldAccessorName(Element element) {
-    String proposedName = privateName(element.library, element.name);
-    return getMappedInstanceName(proposedName);
-  }
-
   String readTypeVariableName(TypeVariableElement element) {

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

Why is this prefixed "read" ?

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

Don't know. Changed to typeVariableName.

-    return '\$tv_${instanceFieldAccessorName(element)}';
+    return disambiguateDirectAccess(element, () => element.name);
   }
 
   /**
-   * Returns name of the JavaScript property used to store an instance field.
+   * Returns a JavaScript property name used to store [element] on one
+   * of the global objects.
+   *
+   * Should be used together with [globalObjectFor], which denotes the object
+   * on which the returned property name should be used.
+   */
+  String globalPropertyName(Element element) {
+    return disambiguateGlobal(element);
+  }
+
+  /**
+   * Returns the JavaScript property name used to store an instance field.
    */
   String instanceFieldPropertyName(Element element) {
     if (element.hasFixedBackendName) {
       return element.fixedBackendName;

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

Does this potentially lead to clashes?

What if there is another member that ends up being assigned the
fixedBackendName.

If it's not possible, please comment (and/or assert).

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

Turns out clashes are possible. Added custom_element_name_clash_test.dart.

I couldn't find a way to get a list of all fixed backend names, so I gave up on
just blacklisting them.

Instead, we now take extra care for property names on classes that extend native
classes. We prepend a $ in front of such names, and assert that fixed backend
names cannot contain a $.

     }
-    // If a class is used anywhere as a mixin, we must make the name unique so
-    // that it does not accidentally shadow.  Also, the mixin name must be
-    // constant over all mixins.
+
+    // If the name of the field might clash with another field,
+    // use a mangled field name to avoid potential clashes.
     ClassWorld classWorld = compiler.world;
     if (classWorld.isUsedAsMixin(element.enclosingClass) ||
         shadowingAnotherField(element)) {
-      // 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 version of the class name, but the result is minified once
-      // again, so that is not visible in the end result.
-      String libraryName = getNameOfLibrary(element.library);
-      String className = getNameOfClass(element.enclosingClass);
-      String instanceName = privateName(element.library, element.name);
-      return getMappedInstanceName('$libraryName\$$className\$$instanceName');
-    }
-
-    String proposedName = privateName(element.library, element.name);
-    return getMappedInstanceName(proposedName);
-  }
-
+      return disambiguateDirectAccess(element,
+          () => '${element.enclosingClass.name}_${element.name}');

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

Is this enough?
What if I have a mixin class A_b with field c and then another mixin class A
with field "b_c", and both have their fields shadowed by a class that mixes in A
and A_b.

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

The string we're building is only a proposed name. The disambiguated name is
still guaranteed to be unique to the element we're passing in.

I've rewritten it a bit to make it more clear at call-site that the string is
only proposed.

+    }
+
+    // No inheritance-related name clashes exist, so the accessor and property

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

I don't understand this comment.
Do you mean: no inheritance related clashes exist, so accessors may use the same
naming algorithm as instance members?

If yes, why is that the case?

From what I can tell we can still have a subclass that overrides the field's
name with a getter or method. I'm not sure if that's an issue or not...

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

Done following offline discussion.

+    // name can be shared. This generates nicer field names since otherwise
+    // one of them would have to be mangled.
+    return disambiguateMember(element.library, element.name);
+  }
 
   bool shadowingAnotherField(Element element) {

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

isShadowingASuperField ?

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

Done.

     return element.enclosingClass.hasFieldShadowedBy(element);
   }
 
-  String setterName(Element element) {
+  /// Annotated name for the setter of [element].
+  String setterForElement(Element element) {
     // We dynamically create setters from the field-name. The setter name must
     // therefore be derived from the instance field-name.
-    LibraryElement library = element.library;
-    String name = getMappedInstanceName(privateName(library, element.name));
-    return '$setterPrefix$name';
-  }
-
-  String setterNameFromAccessorName(String name) {
+    String name = disambiguateMember(element.library, element.name);
+    return deriveSetterName(name);
+  }
+
+  /// Annotated name for the setter of any member with [disambiguatedName].
+  String deriveSetterName(String disambiguatedName) {
     // We dynamically create setters from the field-name. The setter name must
     // therefore be derived from the instance field-name.
-    return '$setterPrefix$name';
-  }
-
-  String getterNameFromAccessorName(String name) {
+    return '$setterPrefix$disambiguatedName';
+  }
+
+  /// Annotated name for the setter of any member with [disambiguatedName].
+  String deriveGetterName(String disambiguatedName) {
     // We dynamically create getters from the field-name. The getter name must
     // therefore be derived from the instance field-name.
-    return '$getterPrefix$name';
-  }
-
-  String getterName(Element element) {
+    return '$getterPrefix$disambiguatedName';
+  }
+
+  /// Annotated name for the getter of [element].
+  String getterForElement(Element element) {

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

nit: either put getter and setter next to each other, or getterForElement and
deriveGetterName (in the same order). That is:

getterForElement
setterForElement
deriveGetterName
deriveSetterName

or
getterForElement
deriveGetterName
setterForElement
deriveSetterName

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

I agree. I kept it like this to avoid messing up the diff. Will reorder when
getting close to a final version.

     // We dynamically create getters from the field-name. The getter name must
     // therefore be derived from the instance field-name.
-    LibraryElement library = element.library;
-    String name = getMappedInstanceName(privateName(library, element.name));
-    return '$getterPrefix$name';
-  }
-
-  String getMappedGlobalName(String proposedName, {bool ensureSafe: true}) {
-    var newName = globalNameMap[proposedName];
-    if (newName == null) {
+    String name = disambiguateMember(element.library, element.name);
+    return deriveGetterName(name);
+  }
+
+  /// Property name for the getter of an instance member with [originalName]
+  /// in [library].
+  ///
+  /// [library] may be `null` if [originalName] is known to be public.
+  String getterForMember(LibraryElement library, String originalName) {
+    String disambiguatedName = disambiguateMember(library, originalName);
+    return deriveGetterName(disambiguatedName);
+  }
+
+  /// Disambiguated name for a compiler-owned global variable.
+  ///
+  /// The resulting name is unique within the global-member namespace.
+  String disambiguateInternGlobal(String name, {bool ensureSafe: true}) {

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

disambiguateInternalGlobal ?
disambiguateCompilerGlobal ?
disambiguateSynthesizedGlobal ?

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

Comment what "ensureSafe" means.

+    String newName = internGlobals[name];
+    if (newName == null) {
+      newName = getFreshName(name, usedGlobalNames,
+                             suggestedGlobalNames, ensureSafe: ensureSafe);
+      internGlobals[name] = newName;
+    }
+    return newName;
+  }
+
+  /// Returns the property name to use for a compiler-owner global variable,
+  /// i.e. one that does not correspond to any element but is used as a utility
+  /// global by code generation.
+  ///
+  /// [name] functions as both the proposed name for the global, and as a key
+  /// identifying the global. The [name] should not contain `$` symbols, since

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

Make this a "must" and assert it.

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

Done.

+  /// the [Namer] uses those names internally.
+  ///
+  /// This provides an easy mechanism for avoiding a name-clash with user-space
+  /// globals, although the callers of must still take care not to accidentally

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

-of-

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

Done.

+  /// pass in the same [name] for two different intern globals.
+  String internGlobal(String name, {bool ensureSafe: true}) {

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

internGlobal -> internalGlobal ?

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

Done.

+    return disambiguateInternGlobal(name, ensureSafe: ensureSafe);
+  }
+
+  /// Returns the disambiguated name for a top-level or static element.
+  ///
+  /// The resulting name is unique within the global-member namespace.

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

Same as for the constant: we might be able to only disambiguate within the
element's holder.

I know that classes need to be unique (for going from type to constructor) but
I'm not sure anything else needs to. (Unless, of course, reflection destroys
everything...)

Also note: in the new emitter we are considering removing the global objects.

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

Again, probably yes. But this CL is big enough already.

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

Didn't expect more than a TODO.

+  String disambiguateGlobal(Element element) {
+    element = element.declaration;
+    String newName = globals[element];
+    if (newName == null) {
+      String proposedName = _proposeNameForGlobal(element);
       newName = getFreshName(proposedName, usedGlobalNames,
-                             suggestedGlobalNames, ensureSafe: ensureSafe);
-      globalNameMap[proposedName] = newName;
-    }
-    return newName;
-  }
-
-  String getMappedInstanceName(String proposedName) {
-    var newName = instanceNameMap[proposedName];
-    if (newName == null) {
-      newName = getFreshName(proposedName, usedInstanceNames,
-                             suggestedInstanceNames, ensureSafe: true);
-      instanceNameMap[proposedName] = newName;
-    }
-    return newName;
-  }
-
-  String getMappedOperatorName(String proposedName) {
-    var newName = operatorNameMap[proposedName];
-    if (newName == null) {
-      newName = getFreshName(proposedName, usedInstanceNames,
+                             suggestedGlobalNames, ensureSafe: true);
+      globals[element] = newName;
+    }
+    return newName;
+  }
+
+  /// Returns the disambiguated name for an instance method or field
+  /// with [originalName] in [library].
+  ///
+  /// [library] may be `null` if [originalName] is known to be public.
+  ///
+  /// This is the name used for deriving names of accessors (getters and
+  /// setters) and annotated method names (encoding arity and named parameters).
+  ///
+  /// The resulting name, and its associated annotated names, are unique within
+  /// the instance-member namespace.
+  String disambiguateMember(LibraryElement library, String originalName) {
+    if (originalName == closureInvocationSelectorName) {

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

assert that library is not null if the originalName is not public.

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

Done.

+      // We hardcode the disambiguated name of 'call'.
+      return callPrefix;
+    }
+    if (!isPrivateName(originalName)) {
+      library = null;
+    }
+    Map<String, String> nameMap =
+        instanceMembers.putIfAbsent(library, () => new Map<String,String>());
+    String newName = nameMap[originalName];
+    if (newName == null) {
+      String proposedName = privateName(library, originalName);
+      newName = getFreshName(proposedName,
+                             usedInstanceNames, suggestedInstanceNames,
+                             ensureSafe: true,
+                             sanitizeForAnnotations: true);
+      nameMap[originalName] = newName;
+    }
+    return newName;
+  }
+
+  /// Disambiguated name unique to [element].
+  ///
+  /// This is used as the property name for fields, type variables,
+  /// constructor bodies, and super-accessors.
+  ///
+  /// The resulting name is unique within the instance-member namespace.
+  String disambiguateDirectAccess(Element element, String proposeName()) {

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

Explain how this is different from disambiguateMember.
Basically: two elements could have the same name (even private name) and still
get different disambiguated names here, whereas disambiguateMember only looks at
the name.
Maybe best explained with an example.

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

I've changed it to disambiguateInternalMember. I hope this makes it clear that
it just generates names that don't clash with anything else.

+    String newName = directAccessMap[element];
+    if (newName == null) {
+      String name = proposeName();
+      newName = getFreshName(name,
+                             usedInstanceNames, suggestedInstanceNames,
+                             ensureSafe: true,
+                             sanitizeForAnnotations: true);
+      directAccessMap[element] = newName;
+    }
+    return newName;
+  }
+
+  /// Disambiguated name for the given operator.
+  ///
+  /// [operatorIdentifier] must be the operator's identifier, e.g.
+  /// `$add` and not `+`.
+  ///
+  /// The resulting name is unique within the instance-member namespace.
+  String disambiguateOperator(String operatorIdentifier) {
+    String newName = operatorNameMap[operatorIdentifier];
+    if (newName == null) {
+      newName = getFreshName(operatorIdentifier, usedInstanceNames,
                              suggestedInstanceNames, ensureSafe: false);
-      operatorNameMap[proposedName] = newName;
-    }
-    return newName;
-  }
-
+      operatorNameMap[operatorIdentifier] = newName;
+    }
+    return newName;
+  }
+
+  /// Returns an unused name.
+  ///
+  /// [proposedName] must be a valid JavaScript identifier.
+  ///
+  /// If [ensureSafe] is `false`, then [proposedName] must not be a keyword.
+  ///
+  /// If [sanitizeForAnnotations] is `true`, then the result is guaranteed not
+  /// to have the form of an annotated name.
+  ///
+  /// Note that [MinifyNamer] overrides this method with one that produces
+  /// minified names.
   String getFreshName(String proposedName,
                       Set<String> usedNames,
                       Map<String, String> suggestedNames,
-                      {bool ensureSafe: true}) {
-    var candidate;
+                      {bool ensureSafe: true,
+                       bool sanitizeForAnnotations: false}) {
+    if (sanitizeForAnnotations) {
+      proposedName = _sanitizeForAnnotations(proposedName);
+    }
     if (ensureSafe) {
       proposedName = safeName(proposedName);
     }
     assert(!jsReserved.contains(proposedName));
+    String candidate;
     if (!usedNames.contains(proposedName)) {
       candidate = proposedName;
     } else {
-      var counter = popularNameCounters[proposedName];
-      var i = counter == null ? 0 : counter;
+      int counter = popularNameCounters[proposedName];
+      int i = counter == null ? 0 : counter;

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

(counter == null)

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

Done.

       while (usedNames.contains("$proposedName$i")) {
         i++;
       }
       popularNameCounters[proposedName] = i + 1;
       candidate = "$proposedName$i";
     }
     usedNames.add(candidate);
     return candidate;
   }
 
+  /// Returns a variant of [name] that cannot clash with the annotated
+  /// version of another name, that is, the resulting name can never be returned
+  /// by [deriveGetterName], [deriveSetterName], [deriveMethodName],
+  /// [operatorIs], or [substitutionName].
+  ///
+  /// For example, a name `get$x` would be converted to `$get$x` to ensure it
+  /// cannot clash with the getter for `x`.

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

Add '_isAnnotated' method and assert that every annotated method satisfies it,
and the result of this function doesn't.

You could then also add:
String input = name;
...
assert(_isAnnotated(input) || input == name);
return name;

This would make sure that we don't strip too much.

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

I don't think it's worth it anymore, since it would now just be a duplicate of
the method body, which is now just four checks.

+  ///
+  /// We don't want to register all potential annotated names in
+  /// [usedInstanceNames] (there are too many), so we use this step to avoid
+  /// clashes between annotated and unannotated names.
+  String _sanitizeForAnnotations(String name) {
+    // Ensure name does not contain a $ followed by a digit, since this could
+    // clash with the suffix we append on method names.
+    for (int i = name.indexOf(r'$'); i != -1; i = name.indexOf(r'$', i+1)) {
+      if (i + 1 < name.length && isDigit(name.codeUnitAt(i+1))) {
+        // Strip off everything after the $ to be safe.
+        name = name.substring(0, i + 1);
+        break;
+      }
+    }
+    // Ensure name does not clash with a getter or setter of another name, or
+    // one of the other special names that start with `$`, such as `$is`.
+    if (name.startsWith(r'$') ||
+        name.startsWith(getterPrefix) ||
+        name.startsWith(setterPrefix)) {
+      name = '\$$name';
+    }
+    return name;
+  }
+
   String getClosureVariableName(String name, int id) {
     return "${name}_$id";
   }
 
   /**
-   * Returns a preferred JS-id for the given top-level or static element.
+   * Returns a proposed name for the given top-level or static element.
    * The returned id is guaranteed to be a valid JS-id.
    */
-  String _computeGuess(Element element) {
+  String _proposeNameForGlobal(Element element) {
     assert(!element.isInstanceMember);
     String name;
     if (element.isGenerativeConstructor) {
       name = "${element.enclosingClass.name}\$"
              "${element.name}";
     } else if (element.isFactoryConstructor) {
       // TODO(johnniwinther): Change factory name encoding as to not include
       // the class-name twice.
       String className = element.enclosingClass.name;
       name = '${className}_${Elements.reconstructConstructorName(element)}';
@@ skipping 51 matching lines @@
     // There is one dispatch mechanism for all native classes.
     if (classes.any((cls) => Elements.isNativeOrExtendsNative(cls))) {
       names.add("x");
     }
     // Sort the names of the classes after abbreviating them to ensure
     // the suffix is stable and predictable for the suggested names.
     names.sort();
     return names.join();
   }
 
-  String getInterceptorName(Element element, Iterable<ClassElement> classes) {
+  /// Property name used for `getInterceptor` or one of its specializations.

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

as written in another file:
"nameOfGetInterceptorFunctionFor" (or similar).

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

Done.

+  String getInterceptorName(Iterable<ClassElement> classes) {
+    FunctionElement getInterceptor = backend.getInterceptorMethod;
     if (classes.contains(backend.jsInterceptorClass)) {
       // If the base Interceptor class is in the set of intercepted classes, we
       // need to go through the generic getInterceptorMethod, since any subclass
       // of the base Interceptor could match.
-      return getNameOfInstanceMember(element);
+      // The unspecialized getInterceptor method can also be accessed through
+      // its element, so we treat this as a user-space global instead of an
+      // intern global.
+      return disambiguateGlobal(getInterceptor);
     }
     String suffix = getInterceptorSuffix(classes);
-    return getMappedGlobalName("${element.name}\$$suffix");
+    return disambiguateInternGlobal("${getInterceptor.name}\$$suffix");
   }
 
+  /// Property name used for the one-shot interceptor method for the given
+  /// [selector] and return-type specialization.
   String getOneShotInterceptorName(Selector selector,
                                    Iterable<ClassElement> classes) {
     // The one-shot name is a global name derived from the invocation name.  To
     // avoid instability we would like the names to be unique and not clash with
     // other global names.
 
     String root = invocationName(selector);  // Is already safe.
 
     if (classes.contains(backend.jsInterceptorClass)) {
       // If the base Interceptor class is in the set of intercepted classes,
       // this is the most general specialization which uses the generic
       // getInterceptor method.  To keep the name short, we add '$' only to
-      // distinguish from global getters or setters; operators and methods can't
-      // clash.
+      // distinguish from other intern globals that don't contain '$' symbols.
       // TODO(sra): Find a way to get the simple name when Object is not in the
       // set of classes for most general variant, e.g. "$lt$n" could be "$lt".
       if (selector.isGetter || selector.isSetter) root = '$root\$';

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

Why does a getter and setter need the "$" ?
The old comment explained the reasoning, although it didn't make it clear why
operators and methods couldn't clash.
I would have interpreted this as 'all other internal globals, except getters and
setters, contain a "$"', but that doesn't seem to be true.

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

Rephrased comment.

-      return getMappedGlobalName(root, ensureSafe: false);
+      return disambiguateInternGlobal(root, ensureSafe: false);
     } else {
       String suffix = getInterceptorSuffix(classes);
-      return getMappedGlobalName("$root\$$suffix", ensureSafe: false);
+      return disambiguateInternGlobal("$root\$$suffix", ensureSafe: false);
     }
   }
 
-  /// Returns the runtime name for [element].  The result is not safe as an id.
-  String getRuntimeTypeName(Element element) {
+  /// Returns the runtime name for [element].
+  ///
+  /// This name is used as the basis for deriving `is` and `as` property names
+  /// for the given type.
+  ///
+  /// The result is not always safe as a property name unless prefixing
+  /// [operatorIsPrefix] or [operatorAsPrefix]. If this is a function type,
+  /// then by convention, an underscore must also separate [operatorIsPrefix]
+  /// from the type name.

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

Yeah for weird arbitrary rules...

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

Still better than undocumented arbitrary rules...

+  String getRuntimeTypeName(TypeDeclarationElement element) {
     if (element == null) return 'dynamic';
-    return getNameForRti(element);
+    // The returned name affects both the global and instance member namespaces:
+    //
+    // - If given a class, this must coincide with the class name, which
+    //   is also the GLOBAL property name of its constructor.
+    //
+    // - The result is used to derive `$isX` and `$asX` names, which are used
+    //   as INSTANCE property names.
+    //
+    // To prevent clashes in both namespaces at once, we disambiguate the name
+    // as a global here, and in [_sanitizeForAnnotations] we ensure that
+    // ordinary instance members cannot start with a '$' character.
+    return disambiguateGlobal(element);
   }
 
-  /**
-   * Returns a preferred JS-id for the given element. The returned id is
-   * guaranteed to be a valid JS-id. Globals and static fields are furthermore
-   * guaranteed to be unique.
-   *
-   * For accessing statics consider calling [elementAccess] instead.
-   */
-  // TODO(ahe): This is an internal method to the Namer (and its subclasses)
-  // and should not be call from outside.
-  String getNameX(Element element) {
-    if (element.isInstanceMember) {
-      if (element.kind == ElementKind.GENERATIVE_CONSTRUCTOR_BODY
-          || element.kind == ElementKind.FUNCTION) {
-        return instanceMethodName(element);
-      } else if (element.kind == ElementKind.GETTER) {
-        return getterName(element);
-      } else if (element.kind == ElementKind.SETTER) {
-        return setterName(element);
-      } else if (element.kind == ElementKind.FIELD) {
-        compiler.internalError(element,
-            'Use instanceFieldPropertyName or instanceFieldAccessorName.');
-        return null;
-      } else {
-        compiler.internalError(element,
-            'getName for bad kind: ${element.kind}.');
-        return null;
-      }
-    } else {
-      // Use declaration element to ensure invariant on [globals].
-      element = element.declaration;
-      // Dealing with a top-level or static element.
-      String cached = globals[element];
-      if (cached != null) return cached;
+  /// Returns the disambiguated name of [class_].
+  ///
+  /// This is both the *runtime type* of the class (see [getRuntimeTypeName])
+  /// and a global property name in which to store its JS constructor.
+  String getNameOfClass(ClassElement class_) => disambiguateGlobal(class_);
 
-      String guess = _computeGuess(element);
-      ElementKind kind = element.kind;
-      if (kind == ElementKind.VARIABLE ||
-          kind == ElementKind.PARAMETER) {
-        // The name is not guaranteed to be unique.
-        return safeName(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) {
-        bool fixedName = false;
-        if (Elements.isInstanceField(element)) {
-          fixedName = element.hasFixedBackendName;
-        }
-        String result = fixedName
-            ? guess
-            : getFreshName(guess, usedGlobalNames, suggestedGlobalNames,
-                           ensureSafe: true);
-        globals[element] = result;
-        return result;
-      }
-      compiler.internalError(element,
-          'getName for unknown kind: ${element.kind}.');
-      return null;
-    }
+  /// Property name on which [member] can be accessed directly,
+  /// without clashing with another JS property name.

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

Might help to give a small example when this is necessary:

foo(x, [y]) has a stub method foo$1(x) that redirects to this.foo$2(x, null);

However, if the method is overridden by another foo, method we must make sure
that we reach the right `foo$2` method. By giving it an alias we can redirect to
`aliased$2(x, null)`.

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

Done, although I don't understand your example, so I chose a different one.

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

Interesting. I didn't know that we added the alias for such simple cases (since
we could just grab the prototype and use `B.prototype.foo.call(this, ...)`.

My example is more complex (and was the reason we introduced aliases, since the
prototype approach wasn't working any more):

class A {
  foo([x]);
}

class B extends A {
  foo([x]) => super.foo();
}

Assume that A was compiled to:
A.prototype.foo$0 = function() { return this.foo$1(null); };
A.prototype.foo$1 = function(x) { ... };

with the prototype approach we would compile B.foo as follows:

B.prototype.foo$1 = function(x) { return A.prototype.foo$0.call(this); }

However, A.foo$0 redirects to this.foo$1 which would then again be B.foo$1. To
solve this issue one could either change foo$0 to:
A.prototype.foo$0 = function() { return A.prototype.foo$1.call(this, null); },
or simply give foo$1 another name.

Note: no need to add this as comment. Just FYI.

+  String getNameOfAliasedSuperMember(Element member) {
+    assert(!member.isField); // Fields do not need super aliases.
+    return disambiguateDirectAccess(member,
+        () => '\$super\$${member.enclosingClass.name}\$${member.name}');
   }
 
-  String getNameForRti(Element element) => getNameX(element);
-
-  String getNameOfLibrary(LibraryElement library) => getNameX(library);
-
-  String getNameOfClass(ClassElement cls) => getNameX(cls);
-
-  String getNameOfField(VariableElement field) => getNameX(field);
-
-  // TODO(ahe): Remove this method. Use get getNameOfMember instead.
-  String getNameOfInstanceMember(Element member) => getNameX(member);
-
-  String getNameOfAliasedSuperMember(Element member) {
-    ClassElement superClass = member.enclosingClass;
-    String className = getNameOfClass(superClass);
-    String memberName = getNameOfMember(member);
-    String proposal = "$superPrefix$className\$$memberName";
-    // TODO(herhut): Use field naming constraints (unique wrt. inheritance).
-    return getMappedInstanceName(proposal);
+  /// Property name in which to store the given static or instance [method].
+  /// For instance methods, this includes the suffix encoding arity and named
+  /// parameters.
+  ///
+  /// The name is not necessarily unique to [method], since a static method
+  /// may share its name with an instance method.

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

But is it unique within their respective categories?
Is the returned name a proposed name or a disambiguated one?

(it looks like it's the disambiguated one, but add comment).

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

Ideally, the users of Namer shouldn't have to know about the different kinds of
names (proposed, disambiguated, annotated).

This is why it just says "property name". It's what the caller should care
about.

If we were to go completely formal on this, I'd say some annotated names are
just a disambiguated name with an empty annotation. This is one of those names.
But I think writing this would cause more confusion than clarity.

+  String getNameOfMethod(Element method) {

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

Why is this prefixed with "get"?

Most of the other methods in this class have: "methodName" or similar naming
conventions. Some drop the "name" suffix (although it feels inconsistent).

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

I agree. I've changed it to "methodName".

+    return method.isInstanceMember
+        ? instanceMethodName(method)
+        : globalPropertyName(method);
   }
 
-  String getNameOfMember(Element member) => getNameX(member);
-
-  String getNameOfGlobalField(VariableElement field) => getNameX(field);
-
   /// Returns true if [element] is stored on current isolate ('$').  We intend
   /// to store only mutable static state in [currentIsolate], constants are
   /// stored in 'C', and functions, accessors, classes, etc. are stored in one
   /// of the other objects in [reservedGlobalObjectNames].
   bool isPropertyOfCurrentIsolate(Element element) {
     // TODO(ahe): Make sure this method's documentation is always true and
     // remove the word "intend".
     return
         // TODO(ahe): Re-write these tests to be positive (so it only returns
         // true for static/top-level mutable fields). Right now, a number of
@@ skipping 15 matching lines @@
     if (library.isPlatformLibrary) {
       if ('${library.canonicalUri}' == 'dart:html') return 'W';
       return 'P';
     }
     return userGlobalObjects[
         library.getLibraryOrScriptName().hashCode % userGlobalObjects.length];
   }
 
   String getLazyInitializerName(Element element) {
     assert(Elements.isStaticOrTopLevelField(element));
-    return getMappedGlobalName("$getterPrefix${getNameX(element)}");
+    return disambiguateInternGlobal("$getterPrefix${globalPropertyName(element)}");

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

long line.

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

Done.

   }
 
   String getStaticClosureName(Element element) {
     assert(Elements.isStaticOrTopLevelFunction(element));
-    return getMappedGlobalName("${getNameX(element)}\$closure");
+    return disambiguateInternGlobal("${globalPropertyName(element)}\$closure");
   }
 
   // This name is used as part of the name of a TypeConstant
   String uniqueNameForTypeConstantElement(Element element) {
     // TODO(sra): If we replace the period with an identifier character,
     // TypeConstants will have better names in unminified code.
-    return "${globalObjectFor(element)}.${getNameX(element)}";
+    return "${globalObjectFor(element)}.${globalPropertyName(element)}";
   }
 
   String globalObjectForConstant(ConstantValue constant) => 'C';
 
   String get operatorIsPrefix => r'$is';
 
   String get operatorAsPrefix => r'$as';
 
   String get operatorSignature => r'$signature';
 
@@ skipping 25 matching lines @@
   }
 
   String operatorIsType(DartType type) {
     if (type.isFunctionType) {
       // TODO(erikcorry): Reduce from $isx to ix when we are minifying.
       return '${operatorIsPrefix}_${getFunctionTypeName(type)}';
     }
     return operatorIs(type.element);
   }
 
-  String operatorIs(Element element) {
+  String operatorIs(ClassElement element) {
     // TODO(erikcorry): Reduce from $isx to ix when we are minifying.
     return '${operatorIsPrefix}${getRuntimeTypeName(element)}';
   }
 
   /*
    * Returns a name that does not clash with reserved JS keywords,
    * and also ensures it won't clash with other identifiers.

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

with other reserved identifiers (like "__proto__").

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

Actually I think that was a stray comment from when _safeName also was the guard
against clashes with annotated names.

    */
   String _safeName(String name, Set<String> reserved) {
-    if (reserved.contains(name) || name.startsWith(r'$')) {
+    if (reserved.contains(name)) {
       name = '\$$name';
     }
     assert(!reserved.contains(name));
     return name;
   }
 
   String substitutionName(Element element) {
     // TODO(ahe): Creating a string here is unfortunate. It is slow (due to

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

I don't know what Peter means.
The cost in string concatenation for substitutions should be noise.
I don't see what "segmentation of '$'" means.

Unless you think there is a reason we should keep the TODO, please delete it.

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

Done.

     // string concatenation in the implementation), and may prevent
     // segmentation of '$'.
-    return '${operatorAsPrefix}${getNameForRti(element)}';
+    return '${operatorAsPrefix}${getRuntimeTypeName(element)}';
   }
 
   String safeName(String name) => _safeName(name, jsReserved);
   String safeVariableName(String name) => _safeName(name, jsVariableReserved);
 
   String operatorNameToIdentifier(String name) {
     if (name == null) return null;
     if (name == '==') {
       return r'$eq';
     } else if (name == '~') {
@@ skipping 462 matching lines @@
         if (!first) {
           sb.write('_');
         }
         sb.write('_');
         visit(parameter);
         first = true;
       }
     }
   }
 }