Cleanup the namer, and add a test with fields that used to clash with internal names used by the co…

sdk/lib/_internal/compiler/implementation/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.
  */
 class Namer implements ClosureNamer {
+
+  static const 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 =
+      const <String>["__PROTO__", "prototype", "constructor"];

[erikcorry, 2013/01/24 10:05:11]

This should surely be lower case __proto__, not upper case.  This was already
broken before.

[ngeoffray, 2013/01/24 10:33:17]

Done.

+
   static Set<String> _jsReserved = null;
   Set<String> get jsReserved {
     if (_jsReserved == null) {
       _jsReserved = new Set<String>();
-      _jsReserved.addAll(JsNames.javaScriptKeywords);
-      _jsReserved.addAll(JsNames.reservedPropertySymbols);
+      _jsReserved.addAll(javaScriptKeywords);
+      _jsReserved.addAll(reservedPropertySymbols);
     }
     return _jsReserved;
   }
 
   final String CURRENT_ISOLATE = r'$';
 
   /**
    * Map from top-level or static elements to their unique identifiers provided
    * by [getName].
    *
    * 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> instanceNameMap;
+  final Map<String, String> operatorNameMap;
   final Map<String, int> popularNameCounters;
 
   /**
    * A cache of names used for bailout methods. We make sure two
    * bailout methods cannot have the same name because if the two
    * bailout methods are in a class and a subclass, we would
    * call the wrong bailout method at runtime. To make it
    * simple, we don't keep track of inheritance and always avoid
    * similar names.
    */
   final Set<String> usedBailoutInstanceNames;
   final Map<Element, String> bailoutNames;
 
   final Map<Constant, String> constantNames;
 
   Namer(this.compiler)
       : globals = new Map<Element, String>(),
         shortPrivateNameOwners = new Map<String, LibraryElement>(),
         bailoutNames = new Map<Element, String>(),
         usedBailoutInstanceNames = new Set<String>(),
         usedGlobalNames = new Set<String>(),
         usedInstanceNames = new Set<String>(),
         instanceNameMap = new Map<String, String>(),
+        operatorNameMap = new Map<String, String>(),
         globalNameMap = new Map<String, String>(),
         constantNames = new Map<Constant, String>(),
         popularNameCounters = new Map<String, int>();
 
   String get isolateName => 'Isolate';
   String get isolatePropertiesName => r'$isolateProperties';
   /**
    * Some closures must contain their name. The name is stored in
    * [STATIC_CLOSURE_NAME_NAME].
    */
@@ skipping 17 matching lines @@
           // The minifier always constructs a new name, using the argument as
           // input to its hashing algorithm.  The given name does not need to be
           // valid.
           longName = stringConstant.value.slowToString();
         } else {
           longName = "C";
         }
       } else {
         longName = "CONSTANT";
       }
-      result = getFreshName(longName, usedGlobalNames);
+      result = getFreshName(longName, usedGlobalNames, true);
       constantNames[constant] = result;
     }
     return result;
   }
 
   String breakLabelName(LabelElement label) {
     return '\$${label.labelName}\$${label.target.nestingLevel}';
   }
 
   String implicitBreakLabelName(TargetElement target) {
@@ skipping 35 matching lines @@
 
     // If a library name does not start with the [LIBRARY_PREFIX] then our
     // assumptions about clashing with mangled private members do not hold.
     String libraryName = getName(library);
     assert(shouldMinify || libraryName.startsWith(LIBRARY_PREFIX));
     // TODO(erikcorry): Fix this with other manglings to avoid clashes.
     return '_lib$libraryName\$$nameString';
   }
 
   String instanceMethodName(FunctionElement element) {
-    SourceString name = Elements.operatorNameToIdentifier(element.name);
+    SourceString elementName = element.name;
+    SourceString name = operatorNameToIdentifier(elementName);
+    if (name != elementName) return getMappedOperatorName(name.slowToString());

[erikcorry, 2013/01/24 10:05:11]

This still has the issue that Kasper pointed out that the != operator does not
check the type of its arguments, so that a later type error here will go
unnoticed.  Suggest a custom equality method that you call here that will fail
in checked mode if both arguments are not SourceStrings.

[ngeoffray, 2013/01/24 10:33:17]

The two assignments below will check that they are both SourceString.

+
     LibraryElement library = element.getLibrary();
     if (element.kind == ElementKind.GENERATIVE_CONSTRUCTOR_BODY) {
       ConstructorBodyElement bodyElement = element;
       name = bodyElement.constructor.name;
     }
     FunctionSignature signature = element.computeSignature(compiler);
     String methodName =
         '${privateName(library, name)}\$${signature.parameterCount}';
     if (signature.optionalParametersAreNamed &&
         !signature.optionalParameters.isEmpty) {
       StringBuffer buffer = new StringBuffer();
       signature.orderedOptionalParameters.forEach((Element element) {
-        buffer.add('\$${JsNames.getValid(element.name.slowToString())}');
+        buffer.add('\$${safeName(element.name.slowToString())}');
       });
       methodName = '$methodName$buffer';
     }
     if (name == closureInvocationSelectorName) return methodName;
     return getMappedInstanceName(methodName);
   }
 
   String publicInstanceMethodNameByArity(SourceString name, int arity) {
-    name = Elements.operatorNameToIdentifier(name);
+    SourceString newName = operatorNameToIdentifier(name);
+    if (newName != name) return getMappedOperatorName(newName.slowToString());
     assert(!name.isPrivate());
     var base = name.slowToString();
     // We don't mangle the closure invoking function name because it
     // is generated by string concatenation in applyFunction from
     // js_helper.dart.
     var proposedName = '$base\$$arity';
     if (name == closureInvocationSelectorName) return proposedName;
     return getMappedInstanceName(proposedName);
   }
 
   String invocationName(Selector selector) {
     if (selector.isGetter()) {
       String proposedName = privateName(selector.library, selector.name);
       return 'get\$${getMappedInstanceName(proposedName)}';
     } else if (selector.isSetter()) {
       String proposedName = privateName(selector.library, selector.name);
       return 'set\$${getMappedInstanceName(proposedName)}';
     } else {
-      SourceString name = Elements.operatorNameToIdentifier(selector.name);
+      SourceString name = selector.name;
+      if (selector.kind == SelectorKind.OPERATOR
+          || selector.kind == SelectorKind.INDEX) {
+        name = operatorNameToIdentifier(name);
+        assert(name != selector.name);
+        return getMappedOperatorName(name.slowToString());
+      }
+      assert(name == operatorNameToIdentifier(name));
       StringBuffer buffer = new StringBuffer();
       for (SourceString argumentName in selector.getOrderedNamedArguments()) {
         buffer.add(r'$');
         argumentName.printOn(buffer);
       }
       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.
-      if (selector.isCall() && name == closureInvocationSelectorName) {
+      if (selector.isClosureCall()) {
         return "${name.slowToString()}$suffix";
+      } else {
+        String proposedName = privateName(selector.library, name);
+        return getMappedInstanceName('$proposedName$suffix');
       }
-      String proposedName = privateName(selector.library, name);
-      return getMappedInstanceName('$proposedName$suffix');
     }
   }
 
   /**
    * Returns the internal name used for an invocation mirror of this selector.
    */
   String invocationMirrorInternalName(Selector selector)
       => invocationName(selector);
 
   String instanceFieldName(Element element) {
     String proposedName = privateName(element.getLibrary(), element.name);
     return getMappedInstanceName(proposedName);
   }
 
   // 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 versions of the class and instance names, but the result is
   // minified once again, so that is not visible in the end result.
   String shadowedFieldName(Element fieldElement) {
     // Check for following situation: Native field ${fieldElement.name} has
     // fixed JSName ${fieldElement.nativeName()}, but a subclass shadows this
     // name.  We normally handle that by renaming the superclass field, but we
-    // can't do that because native fields have fixed JsNames.  In practice
-    // this can't happen because we can't inherit from native classes.
+    // can't do that because native fields have fixed JavaScript names.
+    // In practice this can't happen because we can't inherit from native
+    // classes.
     assert (!fieldElement.hasFixedBackendName());
 
     String libraryName = getName(fieldElement.getLibrary());
     String className = getName(fieldElement.getEnclosingClass());
     String instanceName = instanceFieldName(fieldElement);
     return getMappedInstanceName('$libraryName\$$className\$$instanceName');
   }
 
   String setterName(Element element) {
     // We dynamically create setters from the field-name. The setter name must
@@ skipping 26 matching lines @@
     // We dynamically create getters from the field-name. The getter name must
     // therefore be derived from the instance field-name.
     LibraryElement library = element.getLibrary();
     String name = getMappedInstanceName(privateName(library, element.name));
     return 'get\$$name';
   }
 
   String getMappedGlobalName(String proposedName) {
     var newName = globalNameMap[proposedName];
     if (newName == null) {
-      newName = getFreshName(proposedName, usedGlobalNames);
+      newName = getFreshName(proposedName, usedGlobalNames, true);

[erikcorry, 2013/01/24 10:05:11]

Can't we name ensureSafe so that it is clear at the call site what the flag
means?

[ngeoffray, 2013/01/24 10:33:17]

Done.

       globalNameMap[proposedName] = newName;
     }
     return newName;
   }
 
   String getMappedInstanceName(String proposedName) {
     var newName = instanceNameMap[proposedName];
     if (newName == null) {
-      newName = getFreshName(proposedName, usedInstanceNames);
+      newName = getFreshName(proposedName, usedInstanceNames, true);
       instanceNameMap[proposedName] = newName;
     }
     return newName;
   }
 
-  String getFreshName(String proposedName, Set<String> usedNames) {
+  String getMappedOperatorName(String proposedName) {
+    var newName = operatorNameMap[proposedName];
+    if (newName == null) {
+      newName = getFreshName(proposedName, usedInstanceNames, false);
+      operatorNameMap[proposedName] = newName;
+    }
+    return newName;
+  }
+
+  String getFreshName(String proposedName,
+                      Set<String> usedNames,
+                      bool ensureSafe) {
     var candidate;
-    proposedName = safeName(proposedName);
+    if (ensureSafe) {
+      proposedName = safeName(proposedName);
+    }
+    assert(!jsReserved.contains(proposedName));
     if (!usedNames.contains(proposedName)) {
       candidate = proposedName;
     } else {
       var counter = popularNameCounters[proposedName];
       var i = counter == null ? 0 : counter;
       while (usedNames.contains("$proposedName$i")) {
         i++;
       }
       popularNameCounters[proposedName] = i + 1;
       candidate = "$proposedName$i";
@@ skipping 119 matching lines @@
           kind == ElementKind.LIBRARY ||
           kind == ElementKind.MALFORMED_TYPE) {
         bool fixedName = false;
         if (kind == ElementKind.CLASS) {
           ClassElement classElement = element;
         }
         if (Elements.isInstanceField(element)) {
           fixedName = element.hasFixedBackendName();
         }
         String result =
-            fixedName ? guess : getFreshName(guess, usedGlobalNames);
+            fixedName ? guess : getFreshName(guess, usedGlobalNames, true);
         globals[element] = result;
         return result;
       }
       compiler.internalError('getName for unknown kind: ${element.kind}',
                               node: element.parseNode(compiler));
     }
   }
 
   String getLazyInitializerName(Element element) {
     assert(Elements.isStaticOrTopLevelField(element));
@@ skipping 10 matching lines @@
 
   String isolateBailoutAccess(Element element) {
     String newName = getMappedGlobalName('${getName(element)}\$bailout');
     return '$CURRENT_ISOLATE.$newName';
   }
 
   String isolateLazyInitializerAccess(Element element) {
     return "$CURRENT_ISOLATE.${getLazyInitializerName(element)}";
   }
 
+  String operatorIsPrefix() => r'$is';
+
   String operatorIs(Element element) {
-    // TODO(erikcorry): Reduce from is$x to ix when we are minifying.
-    return 'is\$${getName(element)}';
+    // TODO(erikcorry): Reduce from $isx to ix when we are minifying.
+    return '${operatorIsPrefix()}${getName(element)}';
   }
 
+  /*
+   * Returns a name that does not clash with reserved JS keywords,
+   * and also ensures it won't clash with other identifiers.
+   */
   String safeName(String name) {
-    if (jsReserved.contains(name) || name.startsWith('\$')) {
-      name = "\$$name";
-      assert(!jsReserved.contains(name));
+    if (jsReserved.contains(name) || name.startsWith(r'$')) {
+      name = '\$$name';
     }
+    assert(!jsReserved.contains(name));
     return name;
   }
+
+  SourceString operatorNameToIdentifier(SourceString name) {
+    if (name == null) return null;
+    String value = name.stringValue;
+    if (value == null) {
+      return name;
+    } else if (identical(value, '==')) {

[erikcorry, 2013/01/24 10:05:11]

Why do you want object identity instead of characterwise comparison here?

[ngeoffray, 2013/01/24 10:33:17]

I don't. It's due to me copy pasting the method from Elements.

I changed this new method to use == instead.

+      return const SourceString(r'$eq');
+    } else if (identical(value, '~')) {
+      return const SourceString(r'$not');
+    } else if (identical(value, '[]')) {
+      return const SourceString(r'$index');
+    } else if (identical(value, '[]=')) {
+      return const SourceString(r'$indexSet');
+    } else if (identical(value, '*')) {
+      return const SourceString(r'$mul');
+    } else if (identical(value, '/')) {
+      return const SourceString(r'$div');
+    } else if (identical(value, '%')) {
+      return const SourceString(r'$mod');
+    } else if (identical(value, '~/')) {
+      return const SourceString(r'$tdiv');
+    } else if (identical(value, '+')) {
+      return const SourceString(r'$add');
+    } else if (identical(value, '<<')) {
+      return const SourceString(r'$shl');
+    } else if (identical(value, '>>')) {
+      return const SourceString(r'$shr');
+    } else if (identical(value, '>=')) {
+      return const SourceString(r'$ge');
+    } else if (identical(value, '>')) {
+      return const SourceString(r'$gt');
+    } else if (identical(value, '<=')) {
+      return const SourceString(r'$le');
+    } else if (identical(value, '<')) {
+      return const SourceString(r'$lt');
+    } else if (identical(value, '&')) {
+      return const SourceString(r'$and');
+    } else if (identical(value, '^')) {
+      return const SourceString(r'$xor');
+    } else if (identical(value, '|')) {
+      return const SourceString(r'$or');
+    } else if (identical(value, '-')) {
+      return const SourceString(r'$sub');
+    } else if (identical(value, 'unary-')) {
+      return const SourceString(r'$negate');
+    } else {
+      return name;
+    }
+  }
 }