dart2js: Use an abstract Name class for names in the generated JavaScript ast.

pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart

Index: pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart
diff --git a/pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart b/pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart
index ca49a9eb57fd98d5b6645b82a458e1465bee919a..8a81f455788ef2108d6cfcb42f05867e51e6e10b 100644
--- a/pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart
+++ b/pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart
@@ -25,7 +25,7 @@ class NsmEmitter extends CodeEmitterHelper {
 
     // Keep track of the JavaScript names we've already added so we
     // do not introduce duplicates (bad for code size).
-    Map<String, Selector> addedJsNames
+    Map<jsAst.Name, Selector> addedJsNames
         = generator.computeSelectorsForNsmHandlers();
 
     // Set flag used by generateMethod helper below.  If we have very few
@@ -33,7 +33,8 @@ class NsmEmitter extends CodeEmitterHelper {
     // them at runtime.
     bool haveVeryFewNoSuchMemberHandlers =
         (addedJsNames.length < VERY_FEW_NO_SUCH_METHOD_HANDLERS);
-    for (String jsName in addedJsNames.keys.toList()..sort()) {
+    List<jsAst.Name> names = addedJsNames.keys.toList()..sort();

[floitsch, 2015/06/22 17:43:45]

sort on a different line?

[herhut, 2015/06/23 13:26:32]

Done.

+    for (jsAst.Name jsName in names) {
       Selector selector = addedJsNames[jsName];
       String reflectionName = emitter.getReflectionName(selector, jsName);
 
@@ -57,7 +58,8 @@ class NsmEmitter extends CodeEmitterHelper {
           bool accessible =
               compiler.world.allFunctions.filter(selector, null).any(
               (Element e) => backend.isAccessibleByReflection(e));
-          addProperty('+$reflectionName', js(accessible ? '2' : '0'));
+          addProperty(namer.asName('+$reflectionName'),
+                      js(accessible ? '2' : '0'));
         }
       }
     }
@@ -66,23 +68,25 @@ class NsmEmitter extends CodeEmitterHelper {
   // Identify the noSuchMethod handlers that are so simple that we can
   // generate them programatically.
   bool isTrivialNsmHandler(
-      int type, List argNames, Selector selector, String internalName) {
+      int type, List argNames, Selector selector, jsAst.Name internalName) {
     if (!generateTrivialNsmHandlers) return false;
     // Check for interceptor calling convention.
     if (backend.isInterceptedName(selector.name)) {

[floitsch, 2015/06/22 17:43:45]

TBH I don't see why we do special casing for the intercepted names.
Maybe Peter remembers.

[herhut, 2015/06/23 13:26:31]

This is probably an artifact. I checked the decode and I am sure it does the
right thing in both minified and non-minified mode.

       // We can handle the calling convention used by intercepted names in the
       // diff encoding, but we don't use that for non-minified code.
       if (!compiler.enableMinification) return false;
-      String shortName = namer.invocationMirrorInternalName(selector);
-      if (shortName.length > MAX_MINIFIED_LENGTH_FOR_DIFF_ENCODING) {
-        return false;
-      }
+      // TODO(herhut): We can no longer answer the below with the late bound
+      //               names. However, names longer than 4 are bad anyway.
+      // jsAst.Name shortName = namer.invocationMirrorInternalName(selector);
+      // if (shortName.length > MAX_MINIFIED_LENGTH_FOR_DIFF_ENCODING) {
+      //   return false;
+      // }
     }
     // Check for named arguments.
     if (argNames.length != 0) return false;
     // Check for unexpected name (this doesn't really happen).
-    if (internalName.startsWith(namer.getterPrefix[0])) return type == 1;
-    if (internalName.startsWith(namer.setterPrefix[0])) return type == 2;
+    if (internalName is GetterName) return type == 1;
+    if (internalName is SetterName) return type == 2;
     return type == 0;
   }
 
@@ -128,14 +132,13 @@ class NsmEmitter extends CodeEmitterHelper {
   List<jsAst.Statement> buildTrivialNsmHandlers() {
     List<jsAst.Statement> statements = <jsAst.Statement>[];
     if (trivialNsmHandlers.length == 0) return statements;
-    // Sort by calling convention, JS name length and by JS name.
+    // Sort by calling convention and by JS name.
     trivialNsmHandlers.sort((a, b) {
       bool aIsIntercepted = backend.isInterceptedName(a.name);
       bool bIsIntercepted = backend.isInterceptedName(b.name);
       if (aIsIntercepted != bIsIntercepted) return aIsIntercepted ? -1 : 1;
-      String aName = namer.invocationMirrorInternalName(a);
-      String bName = namer.invocationMirrorInternalName(b);
-      if (aName.length != bName.length) return aName.length - bName.length;
+      jsAst.Name aName = namer.invocationMirrorInternalName(a);
+      jsAst.Name bName = namer.invocationMirrorInternalName(b);
       return aName.compareTo(bName);

[sra1, 2015/06/23 04:47:54]

How does this work?
jsAst.Name does not implement Comparable or define compareTo.
Does the type need to be _NamerName?

For the delta table:

1. It is essential for the that the sort takes place after deferred names are
assigned.
2. short names need to precede longer names.
3. setters and getters need to be interleaved with regular names.

What we need is length-then-lexicographic ordering, not the crazy _NamerName
compareTo ordering.

You need to defer this sort until the string is generated. get the final name
and do the original comparison on the final name:

    if (aName.length != bName.length) return aName.length - bName.length;	
    return aName.compareTo(bName);

[herhut, 2015/06/23 13:26:32]

Done.

     });
 
@@ -150,68 +153,19 @@ class NsmEmitter extends CodeEmitterHelper {
     }
 
     // Get the short names (JS names, perhaps minified).
-    Iterable<String> shorts = trivialNsmHandlers.map((selector) =>
-         namer.invocationMirrorInternalName(selector));
-    var diffEncoding = new StringBuffer();
-
-    // Treat string as a number in base 88 with digits in ASCII order from # to
-    // z.  The short name sorting is based on length, and uses ASCII order for
-    // equal length strings so this means that names are ascending.  The hash
-    // character, #, is never given as input, but we need it because it's the
-    // implicit leading zero (otherwise we could not code names with leading
-    // dollar signs).
-    int fromBase88(String x) {
-      int answer = 0;
-      for (int i = 0; i < x.length; i++) {
-        int c = x.codeUnitAt(i);
-        // No support for Unicode minified identifiers in JS.
-        assert(c >= $$ && c <= $z);
-        answer *= 88;
-        answer += c - $HASH;
-      }
-      return answer;
-    }
-
-    // Big endian encoding, A = 0, B = 1...
-    // A lower case letter terminates the number.
-    String toBase26(int x) {
-      int c = x;
-      var encodingChars = <int>[];
-      encodingChars.add($a + (c % 26));
-      while (true) {
-        c ~/= 26;
-        if (c == 0) break;
-        encodingChars.add($A + (c % 26));
-      }
-      return new String.fromCharCodes(encodingChars.reversed.toList());
-    }
+    Iterable<jsAst.Name> shorts = trivialNsmHandlers.map((selector) =>
+    namer.invocationMirrorInternalName(selector));

[floitsch, 2015/06/22 17:43:45]

indentation.

[herhut, 2015/06/23 13:26:32]

Done.

 
     bool minify = compiler.enableMinification;
     bool useDiffEncoding = minify && shorts.length > 30;
 
-    int previous = 0;
-    int nameCounter = 0;
-    for (String short in shorts) {
-      // Emit period that resets the diff base to zero when we switch to normal
-      // calling convention (this avoids the need to code negative diffs).
-      if (useDiffEncoding && nameCounter == firstNormalSelector) {
-        diffEncoding.write(".");
-        previous = 0;
-      }
-      if (short.length <= MAX_MINIFIED_LENGTH_FOR_DIFF_ENCODING &&
-          useDiffEncoding) {
-        int base63 = fromBase88(short);
-        int diff = base63 - previous;
-        previous = base63;
-        String base26Diff = toBase26(diff);
-        diffEncoding.write(base26Diff);
-      } else {
-        if (useDiffEncoding || diffEncoding.length != 0) {
-          diffEncoding.write(",");
-        }
-        diffEncoding.write(short);
-      }
-      nameCounter++;
+    jsAst.Expression diffEncoding;
+    if (useDiffEncoding) {
+      diffEncoding = new _DiffEncodedListOfNames(shorts, firstNormalSelector);
+    } else {
+      diffEncoding = jsAst.concatenateStrings(
+          jsAst.joinLiterals(shorts, jsAst.stringPart(",")),

[sra1, 2015/06/23 04:47:54]

This also needs to be deferred, since the sort order depends on the deferred
names.

I think you need several deferred expressions which use the same object to do
the processing (sorting, returning concatenated strings etc).

Plus side is that we can just return a deferred string for the short names list
too.

[herhut, 2015/06/23 13:26:31]

Done.

+          addQuotes: true);
     }
 
     // Startup code that loops over the method names and puts handlers on the
@@ -259,8 +213,8 @@ class NsmEmitter extends CodeEmitterHelper {
             }
             shortNames.splice.apply(shortNames, calculatedShortNames);
           }
-        }''', {'objectClass': js.string(namer.className(objectClass)),
-               'diffEncoding': js.string('$diffEncoding')}));
+        }''', {'objectClass': js.quoteName(namer.className(objectClass)),
+               'diffEncoding': diffEncoding}));
     } else {
       // No useDiffEncoding version.
       Iterable<String> longs = trivialNsmHandlers.map((selector) =>
@@ -268,8 +222,8 @@ class NsmEmitter extends CodeEmitterHelper {
       statements.add(js.statement(
           'var objectClassObject = processedClasses.collected[#objectClass],'
           '    shortNames = #diffEncoding.split(",")',
-          {'objectClass': js.string(namer.className(objectClass)),
-           'diffEncoding': js.string('$diffEncoding')}));
+          {'objectClass': js.quoteName(namer.className(objectClass)),
+           'diffEncoding': diffEncoding}));
       if (!minify) {
         statements.add(js.statement('var longNames = #longs.split(",")',
                 {'longs': js.string(longs.join(','))}));
@@ -324,3 +278,89 @@ class NsmEmitter extends CodeEmitterHelper {
     return statements;
   }
 }
+
+/// When pretty printed, this node computes a diff-encoded string for the list
+/// of given names.

[floitsch, 2015/06/22 17:43:45]

Refer or copy the documentation from buildTrivialNsmHandler

[herhut, 2015/06/23 13:26:32]

Done.

+class _DiffEncodedListOfNames extends jsAst.DeferredString
+                              implements AstContainer {
+  String _cachedValue;
+  jsAst.ArrayInitializer ast;
+  int firstNormalSelector;
+
+  _DiffEncodedListOfNames(Iterable<jsAst.Name> names,
+                          this.firstNormalSelector) {
+    ast = new jsAst.ArrayInitializer(names.toList());
+  }
+
+  _computeDiffEncoding() {
+    var diffEncoding = new StringBuffer();
+
+    // Treat string as a number in base 88 with digits in ASCII order from # to
+    // z.  The short name sorting is based on length, and uses ASCII order for
+    // equal length strings so this means that names are ascending.  The hash
+    // character, #, is never given as input, but we need it because it's the
+    // implicit leading zero (otherwise we could not code names with leading
+    // dollar signs).
+    int fromBase88(String x) {
+      int answer = 0;
+      for (int i = 0; i < x.length; i++) {
+        int c = x.codeUnitAt(i);
+        // No support for Unicode minified identifiers in JS.
+        assert(c >= $$ && c <= $z);
+        answer *= 88;
+        answer += c - $HASH;
+      }
+      return answer;
+    }
+
+    // Big endian encoding, A = 0, B = 1...
+    // A lower case letter terminates the number.
+    String toBase26(int x) {
+      int c = x;
+      var encodingChars = <int>[];
+      encodingChars.add($a + (c % 26));
+      while (true) {
+        c ~/= 26;
+        if (c == 0) break;
+        encodingChars.add($A + (c % 26));
+      }
+      return new String.fromCharCodes(encodingChars.reversed.toList());
+    }
+
+    Iterable<String> shorts = ast.elements.map((jsAst.Name name) => name.name);
+
+    int previous = 0;
+    int nameCounter = 0;
+    for (String short in shorts) {
+      // Emit period that resets the diff base to zero when we switch to normal
+      // calling convention (this avoids the need to code negative diffs).
+      if (nameCounter == firstNormalSelector) {
+        diffEncoding.write('.');
+        previous = 0;
+      }
+      if (short.length <= NsmEmitter.MAX_MINIFIED_LENGTH_FOR_DIFF_ENCODING) {
+        int base63 = fromBase88(short);
+        int diff = base63 - previous;
+        previous = base63;
+        String base26Diff = toBase26(diff);
+        diffEncoding.write(base26Diff);
+      } else {
+        if (diffEncoding.length != 0) {
+          diffEncoding.write(',');
+        }
+        diffEncoding.write(short);
+      }
+      nameCounter++;
+    }
+
+    _cachedValue = diffEncoding.toString();
+  }
+
+  String get value {
+    if (_cachedValue == null) {
+      _cachedValue = _computeDiffEncoding();
+    }
+
+    return _cachedValue;
+  }
+}