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

Index: sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
===================================================================
--- sdk/lib/_internal/compiler/implementation/js_backend/namer.dart	(revision 17449)
+++ sdk/lib/_internal/compiler/implementation/js_backend/namer.dart	(working copy)
@@ -8,12 +8,31 @@
  * 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;
   }
@@ -33,6 +52,7 @@
   final Set<String> usedInstanceNames;
   final Map<String, String> globalNameMap;
   final Map<String, String> instanceNameMap;
+  final Map<String, String> operatorNameMap;
   final Map<String, int> popularNameCounters;
 
   /**
@@ -56,6 +76,7 @@
         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>();
@@ -93,7 +114,7 @@
       } else {
         longName = "CONSTANT";
       }
-      result = getFreshName(longName, usedGlobalNames);
+      result = getFreshName(longName, usedGlobalNames, true);
       constantNames[constant] = result;
     }
     return result;
@@ -149,7 +170,10 @@
   }
 
   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;
@@ -162,7 +186,7 @@
         !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';
     }
@@ -171,7 +195,8 @@
   }
 
   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
@@ -190,7 +215,14 @@
       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'$');
@@ -200,11 +232,12 @@
       // 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');
     }
   }
 
@@ -228,8 +261,9 @@
     // 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());
@@ -276,7 +310,7 @@
   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;
@@ -285,15 +319,29 @@
   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 {
@@ -433,7 +481,7 @@
           fixedName = element.hasFixedBackendName();
         }
         String result =
-            fixedName ? guess : getFreshName(guess, usedGlobalNames);
+            fixedName ? guess : getFreshName(guess, usedGlobalNames, true);
         globals[element] = result;
         return result;
       }
@@ -464,16 +512,72 @@
     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;
+    }
+  }
 }