qualify core types: Object, Error, Symbol

lib/src/codegen/js_codegen.dart

 // Copyright (c) 2015, 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.
 
 library dev_compiler.src.codegen.js_codegen;
 
 import 'dart:collection' show HashSet, HashMap, SplayTreeSet;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/ast.dart' hide ConstantEvaluator;
@@ skipping 273 matching lines @@
       if (hide != null) {
         hiddenNames.addAll(hide.hiddenNames.map((i) => js.string(i.name, "'")));
       }
       args.add(new JS.ArrayInitializer(shownNames));
       args.add(new JS.ArrayInitializer(hiddenNames));
     }
     _moduleItems.add(js.statement('dart.export(#);', [args]));
   }
 
   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
   visitAsExpression(AsExpression node) {
     var from = getStaticType(node.expression);
     var to = node.type.type;
 
     // Skip the cast if it's not needed.
     if (rules.isSubTypeOf(from, to)) return _visit(node.expression);
 
@@ skipping 227 matching lines @@
         return js.statement(
             '{ #; dart.defineLazyClassGeneric(#, #, { get: # }); }',
             [genericDef, _exportsVar, _propertyName(name), genericName]);
       }
 
       return js.statement(
           'dart.defineLazyClass(#, { get #() { #; return #; } });',
           [_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;
   }
 
   JS.Statement _emitGenericClassDef(ParameterizedType type, JS.Statement body) {
     var name = type.name;
     var genericName = '$name\$';
     var typeParams = type.typeParameters.map((p) => p.name);
     if (isPublic(name)) _exports.add(genericName);
     return js.statement('let # = dart.generic(function(#) { #; return #; });',
         [genericName, typeParams, body, name]);
@@ skipping 77 matching lines @@
     // Iterable, we know the adapter is already there, so we can skip it as a
     // simple code size optimization.
     var parent = t.lookUpGetterInSuperclass('iterator', t.element.library);
     if (parent != null) return null;
     var parentType = findSupertype(t, _implementsIterable);
     if (parentType != null) return null;
 
     // 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);
   }
 
   JS.Expression _instantiateAnnotation(Annotation node) {
     var element = node.element;
     if (element is ConstructorElement) {
       return _emitInstanceCreationExpression(element, element.returnType,
           node.constructorName, node.arguments, true);
     } else {
@@ skipping 979 matching lines @@
     return _maybeQualifiedName(element, _propertyName(name));
   }
 
   JS.Expression _maybeQualifiedName(Element e, JS.Expression name,
       [Map<Element, JS.MaybeQualifiedId> idTable]) {
     var libName = _libraryName(e.library);
     if (idTable == null) idTable = _qualifiedIds;
 
     // 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;
     var right = node.rightHandSide;
     if (node.operator.type == TokenType.EQ) return _emitSet(left, right);
     var op = node.operator.lexeme;
     assert(op.endsWith('='));
     op = op.substring(0, op.length - 1); // remove trailing '='
     return _emitOpAssign(left, right, op, node.staticElement, context: node);
   }
@@ skipping 1636 matching lines @@
 
 /// A special kind of element created by the compiler, signifying a temporary
 /// variable. These objects use instance equality, and should be shared
 /// everywhere in the tree where they are treated as the same variable.
 class TemporaryVariableElement extends LocalVariableElementImpl {
   TemporaryVariableElement.forNode(Identifier name) : super.forNode(name);
 
   int get hashCode => identityHashCode(this);
   bool operator ==(Object other) => identical(this, other);
 }