qualify core types: Object, Error, Symbol

lib/src/codegen/js_codegen.dart

Index: lib/src/codegen/js_codegen.dart
diff --git a/lib/src/codegen/js_codegen.dart b/lib/src/codegen/js_codegen.dart
index 048bb5c8508f6633a0b5a295e4f98eedc4740c58..87db3c5e1cc0cf239380317713201323b7610651 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -291,19 +291,11 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   }
 
   JS.Identifier _initSymbol(JS.Identifier id) {
-    var s = js.statement('let # = $_SYMBOL(#);', [id, js.string(id.name, "'")]);
+    var s = js.statement('let # = Symbol(#);', [id, js.string(id.name, "'")]);
     _moduleItems.add(s);
     return id;
   }
 
-  // TODO(jmesserly): this is a temporary workaround for `Symbol` in core,
-  // until we have better name tracking.
-  String get _SYMBOL {
-    var name = currentLibrary.name;
-    if (name == 'dart.core' || name == 'dart._internal') return 'dart.JsSymbol';
-    return 'Symbol';
-  }
-
   bool isPublic(String name) => !name.startsWith('_');
 
   @override
@@ -551,13 +543,34 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
           [_exportsVar, _propertyName(name), body, name]);
     }
 
-    if (isPublic(name)) _addExport(name);
-
     if (genericDef != null) {
       var dynType = fillDynamicTypeArgs(type, types);
       var genericInst = _emitTypeName(dynType, lowerGeneric: true);
-      return js.statement('{ #; let # = #; }', [genericDef, name, genericInst]);
+
+      if (_isJsBuiltinName(name)) {

[ochafik, 2015/09/15 18:05:17]

nit: store as a var above for reuse below?

+        genericInst = js.statement('#.# = #;',
+            [_libraryName(type.element.library), name, genericInst]);

[ochafik, 2015/09/15 18:05:17]

nit: isn't this guaranteed to be currentLibrary?

+      } else {
+        if (isPublic(name)) _addExport(name);
+        genericInst = js.statement('let # = #;', [name, genericInst]);
+      }
+
+      return new JS.Block([genericDef, genericInst]);
     }
+
+    if (_isJsBuiltinName(name)) {
+      var typeName = _emitTypeName(type) as JS.PropertyAccess;
+
+      // TODO(jmesserly): we could generate something cleaner, but since it only
+      // affects 3 types, doesn't seem like a big concern.
+      var classExpr = new JS.Call(
+          new JS.ArrowFun([], js.statement('{ #; return #; }', [body, name])),
+          []);
+      return new JS.Assignment(typeName, classExpr).toStatement();
+    } else if (isPublic(name)) {
+      _addExport(name);
+    }
+
     return body;
   }
 
@@ -655,7 +668,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     // Otherwise, emit the adapter method, which wraps the Dart iterator in
     // an ES6 iterator.
     return new JS.Method(
-        js.call('$_SYMBOL.iterator'),
+        js.call('Symbol.iterator'),
         js.call('function() { return new dart.JsIterator(this.#); }',
             [_emitMemberName('iterator', type: t)]) as JS.Fun);
   }
@@ -1655,13 +1668,30 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
 
     // Mutable top-level fields should always be qualified.
     bool mutableTopLevel = e is TopLevelVariableElement && !e.isConst;
-    if (e.library != currentLibrary || mutableTopLevel) {
+    if (e.library != currentLibrary ||
+        mutableTopLevel ||
+        _isJsBuiltinName(e.name)) {

[ochafik, 2015/09/15 18:05:17]

I think that check alone (in the context of "extends exports.Object") fixes the
Closure compilation error that occurs in core.js.

       return new JS.PropertyAccess(libName, name);
     }
 
     return idTable.putIfAbsent(e, () => new JS.MaybeQualifiedId(libName, name));
   }
 
+  /// Returns true if this name is Object, Error, or Symbol.
+  ///
+  /// We avoid using those names as locals so we don't shadow the global JS
+  /// names. While we could technically deal with this, it confuses other tools
+  /// like JS Compiler.
+  bool _isJsBuiltinName(String name) {
+    switch (name) {
+      case 'Object':
+      case 'Error':
+      case 'Symbol':

[ochafik, 2015/09/15 16:58:33]

Presumably also Map, Set, TypeError, RangeError, RegExp...

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects

[Jennifer Messerly, 2015/09/15 17:06:09]

Maybe I need to step back, I honestly don't have a good model of why we are
avoiding global names. What are the reasons?

In some sense, the point of ES6 modules are to bring sanity to the ES scoping
world.

Unless the code in question needs to use one of them, there should be no reason
to avoid that as a local variable name.

In general, we can't guarantee ES won't add more names in the future (e.g. Map
and Set are new). 

[ochafik, 2015/09/15 18:05:17]

Right now it's just to avoid errors in Closure with the following code, which I
believe your CL already fixes:

`class Object {}; class Sub extends Object {}`

Just defining `class Object {}` yields a mere warning, but extending `Object`
gives an error (extending `core.Object` apparently does the trick, but the
warning remains: http://goo.gl/AqXiqr).
Agreed, can't wait for https://github.com/whatwg/loader to succeed :-)
Note that we could also just ask the Closure Compiler team to remove those
warnings/errors when classes are redefined in a local scope (but again, I am /
was biased towards qualification, which is why I didn't ask them yet).
This is why I would advocate to generalize qualified names for all types, but
maybe then the overhead of these extra function blocks could start becoming an
issue.

[Jennifer Messerly, 2015/09/15 18:34:23]

ah, we try and qualify the minimum possible. In fact more is qualified at
present than is desirable. One of the biggest readability wins will be able to
unqualify names from other libraries--just like in Dart code (unless it uses
`as` in which case we'd keep it qualified)

+        return true;
+    }
+    return false;
+  }
+
   @override
   JS.Expression visitAssignmentExpression(AssignmentExpression node) {
     var left = node.leftHandSide;