fixes #146, JS globals were not understood by the renamer

there are two basic strategies to fix: we either provide globals to the renamer, or it determines them based on use. I went with the second approach, as it fits more naturally with the current code generator.

[email protected]

Review URL: https://codereview.chromium.org/1099813006

Author: John Messerly

pkg/dev_compiler/lib/runtime/dart/collection.js

@@ -2859,7 +2859,7 @@ var collection;
   let _TypeTest = _TypeTest$();
   let _comparator = Symbol('_comparator');
   let _validKey = Symbol('_validKey');
-  let _internal = Symbol('_internal');
+  let _internal$ = Symbol('_internal');
   let SplayTreeMap$ = dart.generic(function(K, V) {
     class SplayTreeMap extends _SplayTree$(K) {
       SplayTreeMap(compare, isValidKey) {
@@ -2903,7 +2903,7 @@ var collection;
       [_compare](key1, key2) {
         return this[_comparator](key1, key2);
       }
-      [_internal]() {
+      [_internal$]() {
         this[_comparator] = null;
         this[_validKey] = null;
         super._SplayTree();
@@ -3064,7 +3064,7 @@ var collection;
     dart.defineNamedConstructor(SplayTreeMap, 'from');
     dart.defineNamedConstructor(SplayTreeMap, 'fromIterable');
     dart.defineNamedConstructor(SplayTreeMap, 'fromIterables');
-    dart.defineNamedConstructor(SplayTreeMap, _internal);
+    dart.defineNamedConstructor(SplayTreeMap, _internal$);
     return SplayTreeMap;
   });
   let SplayTreeMap = SplayTreeMap$();

pkg/dev_compiler/lib/runtime/dart/core.js

@@ -100,7 +100,7 @@ var core;
     return Comparable;
   });
   let Comparable = Comparable$();
-  let _internal = dart.JsSymbol('_internal');
+  let _internal$ = dart.JsSymbol('_internal');
   let _now = dart.JsSymbol('_now');
   let _brokenDownDateToMillisecondsSinceEpoch = dart.JsSymbol('_brokenDownDateToMillisecondsSinceEpoch');
   let _MAX_MILLISECONDS_SINCE_EPOCH = dart.JsSymbol('_MAX_MILLISECONDS_SINCE_EPOCH');
@@ -122,7 +122,7 @@ var core;
         second = 0;
       if (millisecond === void 0)
         millisecond = 0;
-      this[_internal](year, month, day, hour, minute, second, millisecond, false);
+      this[_internal$](year, month, day, hour, minute, second, millisecond, false);
     }
     utc(year, month, day, hour, minute, second, millisecond) {
       if (month === void 0)
@@ -137,7 +137,7 @@ var core;
         second = 0;
       if (millisecond === void 0)
         millisecond = 0;
-      this[_internal](year, month, day, hour, minute, second, millisecond, true);
+      this[_internal$](year, month, day, hour, minute, second, millisecond, true);
     }
     now() {
       this[_now]();
@@ -306,7 +306,7 @@ var core;
       let otherMs = other.millisecondsSinceEpoch;
       return new Duration({milliseconds: dart.notNull(ms) - dart.notNull(otherMs)});
     }
-    [_internal](year, month, day, hour, minute, second, millisecond, isUtc) {
+    [_internal$](year, month, day, hour, minute, second, millisecond, isUtc) {
       this.isUtc = typeof isUtc == 'boolean' ? isUtc : dart.throw_(new ArgumentError(isUtc));
       this.millisecondsSinceEpoch = dart.as(_js_helper.checkInt(_js_helper.Primitives.valueFromDecomposedDate(year, month, day, hour, minute, second, millisecond, isUtc)), int);
     }
@@ -356,7 +356,7 @@ var core;
   dart.defineNamedConstructor(DateTime, 'utc');
   dart.defineNamedConstructor(DateTime, 'now');
   dart.defineNamedConstructor(DateTime, 'fromMillisecondsSinceEpoch');
-  dart.defineNamedConstructor(DateTime, _internal);
+  dart.defineNamedConstructor(DateTime, _internal$);
   dart.defineNamedConstructor(DateTime, _now);
   DateTime.MONDAY = 1;
   DateTime.TUESDAY = 2;
@@ -1965,12 +1965,12 @@ var core;
       } else if (char == Uri[_NUMBER_SIGN]) {
         fragment = Uri[_makeFragment](uri, dart.notNull(index) + 1, uri.length);
       }
-      return new Uri[_internal](scheme, userinfo, host, port, path, query, fragment);
+      return new Uri[_internal$](scheme, userinfo, host, port, path, query, fragment);
     }
     static [_fail](uri, index, message) {
       throw new FormatException(message, uri, index);
     }
-    [_internal](scheme, userInfo, host, port, path, query, fragment) {
+    [_internal$](scheme, userInfo, host, port, path, query, fragment) {
       this.scheme = scheme;
       this[_userInfo] = userInfo;
       this[_host] = host;
@@ -2005,7 +2005,7 @@ var core;
       }
       let ensureLeadingSlash = host != null;
       path = Uri[_makePath](path, 0, Uri[_stringOrNullLength](path), pathSegments, ensureLeadingSlash, isFile);
-      return new Uri[_internal](scheme, userInfo, host, port, path, query, fragment);
+      return new Uri[_internal$](scheme, userInfo, host, port, path, query, fragment);
     }
     http(authority, unencodedPath, queryParameters) {
       if (queryParameters === void 0)
@@ -2215,7 +2215,7 @@ var core;
       } else if (this.hasFragment) {
         fragment = this.fragment;
       }
-      return new Uri[_internal](scheme, userInfo, host, port, path, query, fragment);
+      return new Uri[_internal$](scheme, userInfo, host, port, path, query, fragment);
     }
     get pathSegments() {
       if (this[_pathSegments] == null) {
@@ -2649,7 +2649,7 @@ var core;
         }
       }
       let fragment = reference.hasFragment ? reference.fragment : null;
-      return new Uri[_internal](targetScheme, targetUserInfo, targetHost, targetPort, targetPath, targetQuery, fragment);
+      return new Uri[_internal$](targetScheme, targetUserInfo, targetHost, targetPort, targetPath, targetQuery, fragment);
     }
     get hasAuthority() {
       return this[_host] != null;
@@ -3009,7 +3009,7 @@ var core;
       return dart.notNull(codeUnit) >= dart.notNull(Uri[_LOWER_CASE_A]) && dart.notNull(codeUnit) <= dart.notNull(Uri[_LOWER_CASE_Z]) || dart.notNull(codeUnit) >= dart.notNull(Uri[_UPPER_CASE_A]) && dart.notNull(codeUnit) <= dart.notNull(Uri[_UPPER_CASE_Z]);
     }
   }
-  dart.defineNamedConstructor(Uri, _internal);
+  dart.defineNamedConstructor(Uri, _internal$);
   dart.defineNamedConstructor(Uri, 'http');
   dart.defineNamedConstructor(Uri, 'https');
   dart.defineNamedConstructor(Uri, 'file');

pkg/dev_compiler/lib/src/codegen/js_names.dart

@@ -34,7 +34,7 @@ class JSNamer extends LocalNamer {
   JSNamer(Node node) : renames = new _RenameVisitor.build(node).renames;
 
   String getName(Identifier node) {
-    var rename = renames[renameKey(node)];
+    var rename = renames[identifierKey(node)];
     if (rename != null) return rename;
 
     assert(!needsRename(node));
@@ -45,81 +45,108 @@ class JSNamer extends LocalNamer {
   void leaveScope() {}
 }
 
+/// Represents a complete function scope in JS.
+///
+/// We don't currently track ES6 block scopes, because we don't represent them
+/// in js_ast yet.
 class _FunctionScope {
+  /// The parent scope.
   final _FunctionScope parent;
-  final names = new HashSet<String>();
+
+  /// All names declared in this scope.
+  final declared = new HashSet<Object>();
+
+  /// All names [declared] in this scope or its [parent]s, that is used in this
+  /// scope and/or children. This is exactly the set of variable names we must
+  /// not collide with inside this scope.
+  final used = new HashSet<String>();
+
+  /// Nested functions, these are visited after everything else so the names
+  /// they might need are in scope.
+  final functions = new List<FunctionExpression>();
+
   _FunctionScope(this.parent);
 }
 
 /// Collects all names used in the visited tree.
-class _RenameVisitor extends BaseVisitor {
+class _RenameVisitor extends VariableDeclarationVisitor {
   final pendingRenames = new Map<Object, Set<_FunctionScope>>();
   final renames = new HashMap<Object, String>();
 
-  _FunctionScope scope = new _FunctionScope(null);
+  final _FunctionScope rootScope = new _FunctionScope(null);
+  _FunctionScope scope;
 
   _RenameVisitor.build(Node root) {
+    scope = rootScope;
     root.accept(this);
+    _finishFunctions();
     _finishNames();
   }
 
+  declare(Identifier node) {
+    var id = identifierKey(node);
+    scope.declared.add(id);
+    _markUsed(node, id, scope);
+  }
+
   visitIdentifier(Identifier node) {
+    var id = identifierKey(node);
+
+    // Find where the node was declared.
+    var declScope = scope;
+    while (declScope != null && !declScope.declared.contains(id)) {
+      declScope = declScope.parent;
+    }
+    if (declScope == null) {
+      // Assume it comes from the global scope.
+      declScope = rootScope;
+      declScope.declared.add(id);
+    }
+    _markUsed(node, id, declScope);
+  }
+
+  _markUsed(Identifier node, Object id, _FunctionScope declScope) {
+    // If it needs rename, we can't add it to the used name set yet, instead we
+    // will record all scopes it is visible in.
+    Set<_FunctionScope> usedIn = null;
     if (needsRename(node)) {
-      // We can't assign the name yet, but we can add it to the list of things
-      // that need a name.
-      var id = renameKey(node);
-      pendingRenames.putIfAbsent(id, () => new HashSet()).add(scope);
-    } else {
-      scope.names.add(node.name);
+      usedIn = pendingRenames.putIfAbsent(id, () => new HashSet());
+    }
+
+    for (var s = scope, end = declScope.parent; s != end; s = s.parent) {
+      if (usedIn != null) {
+        usedIn.add(s);
+      } else {
+        s.used.add(node.name);
+      }
     }
   }
 
   visitFunctionExpression(FunctionExpression node) {
-    scope = new _FunctionScope(scope);
-    super.visitFunctionExpression(node);
-    scope = scope.parent;
+    // Visit nested functions after all identifiers are declared.
+    scope.functions.add(node);
+  }
+
+  void _finishFunctions() {
+    for (var f in scope.functions) {
+      scope = new _FunctionScope(scope);
+      super.visitFunctionExpression(f);
+      _finishFunctions();
+      scope = scope.parent;
+    }
   }
 
   void _finishNames() {
+    var allNames = new Set<String>();
     pendingRenames.forEach((id, scopes) {
-      var name = _findName(id, _allNamesInScope(scopes));
-      renames[id] = name;
-      for (var s in scopes) s.names.add(name);
-    });
-  }
+      allNames.clear();
+      for (var s in scopes) allNames.addAll(s.used);
 
-  // Given a set of scopes, populates [allNames] to include all names in those
-  // scopes as well as intermediate scopes. Returns the common parent of
-  // all scopes. For example:
-  //
-  // function outer(t) {
-  //   function middle(x) {
-  //     function inner() { return t; }
-  //     foo(x);
-  //   }
-  // }
-  //
-  // Here `t` is used in `inner` and `outer` but we need to include `middle`
-  // as well, so we know the rename of `t` to `x` is not valid.
-  static Set<String> _allNamesInScope(Set<_FunctionScope> scopes) {
-    // As we iterate, we'll add more scopes. We don't need to consider these
-    // as intermediate scopes can't introduce new intermediates.
-    var candidates = [];
-    var allScopes = scopes.toSet();
-    for (var scope in scopes) {
-      for (var p = scope.parent; p != null; p = p.parent) {
-        if (allScopes.contains(p)) {
-          allScopes.addAll(candidates);
-          break;
-        }
-        candidates.add(p);
-      }
-      // Discard these, we already added them or we didn't find a parent scope.
-      candidates.clear();
-    }
+      var name = _findName(id, allNames);
+      renames[id] = name;
 
-    // Now collect all names found.
-    return allScopes.expand((s) => s.names).toSet();
+      for (var s in scopes) s.used.add(name);
+    });
   }
 
   static String _findName(Object id, Set<String> usedNames) {
@@ -154,7 +181,7 @@ class _RenameVisitor extends BaseVisitor {
 bool needsRename(Identifier node) =>
     node is JSTemporary || node.allowRename && invalidJSVariableName(node.name);
 
-Object /*String|JSTemporary*/ renameKey(Identifier node) =>
+Object /*String|JSTemporary*/ identifierKey(Identifier node) =>
     node is JSTemporary ? node : node.name;
 
 /// Returns true for invalid JS variable names, such as keywords.

pkg/dev_compiler/lib/src/js/printer.dart

@@ -1385,3 +1385,40 @@ class MinifyRenamer implements LocalNamer {
     return newName;
   }
 }
+
+/// Like [BaseVisitor], but calls [declare] for [Identifier] declarations, and
+/// [visitIdentifier] otherwise.
+abstract class VariableDeclarationVisitor<T> extends BaseVisitor<T> {
+  declare(Identifier node);
+
+  visitFunctionExpression(FunctionExpression node) {
+    for (Identifier param in node.params) declare(param);
+    node.body.accept(this);
+  }
+
+  visitVariableInitialization(VariableInitialization node) {
+    declare(node.declaration);
+    if (node.value != null) node.value.accept(this);
+  }
+
+  visitCatch(Catch node) {
+    declare(node.declaration);
+    node.body.accept(this);
+  }
+
+  visitFunctionDeclaration(FunctionDeclaration node) {
+    declare(node.name);
+    node.function.accept(this);
+  }
+
+  visitNamedFunction(NamedFunction node) {
+    declare(node.name);
+    node.function.accept(this);
+  }
+
+  visitClassExpression(ClassExpression node) {
+    declare(node.name);
+    if (node.heritage != null) node.heritage.accept(this);
+    for (Method element in node.methods) element.accept(this);
+  }
+}