Generate all runtime files from dart.

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 1ca3993e1a8c9303368e813f868f5389a17da4ea..e2bb50047502b2e78f5abe54d1862de01e92e131 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -38,7 +38,6 @@ import 'js_module_item_order.dart';
 import 'js_names.dart';
 import 'js_printer.dart' show writeJsLibrary;
 import 'side_effect_analysis.dart';
-import 'package:collection/equality.dart';
 
 // Various dynamic helpers we call.
 // If renaming these, make sure to check other places like the
@@ -52,8 +51,6 @@ const DSETINDEX = 'dsetindex';
 const DCALL = 'dcall';
 const DSEND = 'dsend';
 
-const ListEquality _listEquality = const ListEquality();
-
 class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   final AbstractCompiler compiler;
   final CodegenOptions options;
@@ -94,6 +91,9 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   final _dartxVar = new JS.Identifier('dartx');
   final _exportsVar = new JS.TemporaryId('exports');
   final _runtimeLibVar = new JS.Identifier('dart');
+  final _utilsLibVar = new JS.TemporaryId('utils');
+  final _classesLibVar = new JS.TemporaryId('classes');
+  final _rttiLibVar = new JS.TemporaryId('rtti');
   final _namedArgTemp = new JS.TemporaryId('opts');
 
   final TypeProvider _types;
@@ -108,7 +108,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   /// The default value of the module object. See [visitLibraryDirective].
   String _jsModuleValue;
 
-  bool _isDartUtils;
+  bool _isDartRuntime;
 
   Map<String, DartType> _objectMembers;
 
@@ -123,10 +123,24 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     var src = context.sourceFactory.forUri('dart:_interceptors');
     var interceptors = context.computeLibraryElement(src);
     _jsArray = interceptors.getType('JSArray');
-    _isDartUtils = currentLibrary.source.uri.toString() == 'dart:_utils';
+
+    _isDartRuntime = _runtimeLibUris.contains(_getLibUri(currentLibrary));
 
     _objectMembers = getObjectMemberMap(types);
   }
+  String _getLibUri(LibraryElement lib) => lib.source.uri.toString();
+
+  static final _runtimeLibUris = new Set<String>.from([

[Jennifer Messerly, 2016/01/07 20:18:04]

ideally we wouldn't need to hard code these, or generate code differently when
we are compiling these. If possible, I'd prefer to express the relevant
differences in the library files themselves. (Perhaps an annotation if needed.)

Otherwise when I look at the SDK source, I have to know which libraries will be
treated differently.

(Even a naming convention would be better than a hard coded list)

[ochafik, 2016/01/13 13:13:20]

Dropped this but kept _isDartRuntime (testing on dart:_runtime only) to decide
on function tagging + module decl.

+    'dart:_utils',
+    'dart:_runtime',
+    'dart:_operations',
+    'dart:_errors',
+    'dart:_classes',
+    'dart:_generators',
+    'dart:_operations',
+    'dart:_types',
+    'dart:_rtti'
+  ]);
 
   TypeProvider get types => rules.provider;
 
@@ -196,39 +210,59 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     // TODO(jmesserly): it would be great to run the renamer on the body,
     // then figure out if we really need each of these parameters.
     // See ES6 modules: https://github.com/dart-lang/dev_compiler/issues/34
-    var params = [_exportsVar, _runtimeLibVar];
-    var processImport =
-        (LibraryElement library, JS.TemporaryId temp, List list) {
-      params.add(temp);
-      list.add(js.string(compiler.getModuleName(library.source.uri), "'"));
-    };
+    var params = [_exportsVar];
+    var lazyParams = [];
 
-    var needsDartRuntime = !_isDartUtils;
+    var libUri = _getLibUri(currentLibrary);
 
     var imports = <JS.Expression>[];
+    var lazyImports = <JS.Expression>[];
     var moduleStatements = <JS.Statement>[];
-    if (needsDartRuntime) {
-      imports.add(js.string('dart/_runtime'));
+
+    addImport(String name, JS.Expression libVar, {bool eager: true}) {
+      (eager ? imports : lazyImports).add(js.string(name, "'"));
+      (eager ? params : lazyParams).add(libVar);
+    }
+
+    if (!_isDartRuntime) {
+      addImport('dart/_runtime', _runtimeLibVar);

[Jennifer Messerly, 2016/01/07 20:18:04]

can we just import this library explicitly?

    // in the input .dart file
    library dart._rtti;
    import 'dart:_runtime';

[ochafik, 2016/01/13 13:13:20]

That's mainly for the "mere mortal" files

 
       var dartxImport =
           js.statement("let # = #.dartx;", [_dartxVar, _runtimeLibVar]);
       moduleStatements.add(dartxImport);
+    } else {
+      if (libUri == 'dart:_generators') {
+        addImport('dart/_classes', _classesLibVar);

[Jennifer Messerly, 2016/01/07 20:18:04]

same here, can we import this explicitly?

[ochafik, 2016/01/13 13:13:20]

Dropped now that all runtime is in dart:_runtime

+      }
     }
     moduleStatements.addAll(_moduleItems);
 
-    _imports.forEach((library, temp) {
-      if (_loader.libraryIsLoaded(library)) {
-        processImport(library, temp, imports);
-      }
-    });
+    bool shouldImportEagerly(lib) {
+      var otherLibUri = _getLibUri(lib);
 
-    var lazyImports = <JS.Expression>[];
-    _imports.forEach((library, temp) {
-      if (!_loader.libraryIsLoaded(library)) {
-        processImport(library, temp, lazyImports);
-      }
+      // utils.dart contains very low-level utils and doesn't import anyone.
+      if (otherLibUri == 'dart:_utils') return true;

[Jennifer Messerly, 2016/01/07 20:18:05]

can we just add these cases to the existing corelibOrder list? Its job is to
enforce an import order for core libraries.

[ochafik, 2016/01/13 13:13:20]

Ack

+      // Types in types.dart extend rtti.LazyTagged in a way that isn't
+      // recognized by the loader: we help it out here.

[Jennifer Messerly, 2016/01/07 20:18:04]

can you explain the problem with this? I'm not following what is going on.

We can try and fix how rtti.LazyTagged works. We could throw all of our helpers
in one file if that was the blocker.

I get nervous when we have special logic for compiling certain libraries, it
means constraints from the input sources are leaking into the compiler, adding
compiler complexity and meaning we can hit bugs because things are not handled
uniformly.

[ochafik, 2016/01/13 13:13:20]

Dropped special logic

+      if (libUri == 'dart:_types' && otherLibUri == 'dart:_rtti') return true;
+      if (libUri == 'dart:_runtime') return otherLibUri != 'dart:_js_helper';
+      // runtime.dart needs to import everything but utils.dart lazily.
+      if (_isDartRuntime) return false;
+
+      return _loader.libraryIsLoaded(lib);
+    }
+
+    var importsEagerness = new Map<LibraryElement, bool>.fromIterable(
+        _imports.keys,
+        value: shouldImportEagerly);
+
+    _imports.forEach((LibraryElement lib, JS.TemporaryId temp) {
+      bool eager = importsEagerness[lib];
+      addImport(compiler.getModuleName(lib.source.uri), temp, eager: eager);
     });
 
+    params.addAll(lazyParams);
+
     var module =
         js.call("function(#) { 'use strict'; #; }", [params, moduleStatements]);
 
@@ -264,9 +298,12 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   void visitLibraryDirective(LibraryDirective node) {
     assert(_jsModuleValue == null);
 
-    var jsName = findAnnotation(node.element, isJSAnnotation);
-    _jsModuleValue =
-        getConstantField(jsName, 'name', types.stringType)?.toStringValue();
+    _jsModuleValue = _getJsName(node.element);
+  }
+
+  String _getJsName(Element e) {

[Jennifer Messerly, 2016/01/07 20:18:04]

I'm a little confused about this function. It could use a doc comment & more
clear name. Is it supposed to only accept LibraryElements? What does it do in
other cases, and under what conditions does it need to be called?

Mapping Dart names to JS names needs to be done *super* carefully given the join
goals of interop guarantees and generating valid JavaScript. Feels a bit like
walking a tightrope.

I think this is trying to support renaming of more kinds of top-level elements,
but it's not entirely clear which ones. If we need a new JS interop feature it
might be better to do as separate CL and also R=jacobr@google.com, since we will
want to ensure every place that can generate a reference to that name is
checking for the JsName.

[ochafik, 2016/01/13 13:13:20]

Spun this out in https://codereview.chromium.org/1580413002/, with no change to
handling of package:js annotations.

+    var jsName = findAnnotation(e, isJSAnnotation);
+    return getConstantField(jsName, 'name', types.stringType)?.toStringValue();
   }
 
   @override
@@ -294,8 +331,14 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       var hide = node.combinators.firstWhere((c) => c is HideCombinator,
           orElse: () => null) as HideCombinator;
       if (show != null) {
+        // If the import has a single shown name + a JsName(jsName) annotation,
+        // then we use that jsName as the exported name.
+        var singleJsName = _getJsName(node.element);
+        if (singleJsName != null && show.shownNames.length != 1) {
+          throw new StateError('Cannot set js name on more than one export');

[Jennifer Messerly, 2016/01/07 20:18:04]

Can this error be triggered by user code?

If so, we should definitely not throw! Compiler should never throw unless it has
a bug. If this is an error or warning we must report it to the user, with enough
information that they can fix it. 

[ochafik, 2016/01/13 13:13:20]

Dropped that (no need to re-export anymore with the parts)

+        }
         shownNames.addAll(show.shownNames
-            .map((i) => i.name)
+            .map((i) => singleJsName ?? i.name)
             .where((s) => !currentLibNames.containsKey(s))
             .map((s) => js.string(s, "'")));
       }
@@ -305,7 +348,9 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       args.add(new JS.ArrayInitializer(shownNames));
       args.add(new JS.ArrayInitializer(hiddenNames));
     }
-    _moduleItems.add(js.statement('dart.export_(#);', [args]));
+
+    // When we compile _runtime.js, we need to source export_ from _utils.js:
+    _moduleItems.add(js.statement('#(#);', [_dartExport, args]));
   }
 
   JS.Identifier _initSymbol(JS.Identifier id) {
@@ -579,10 +624,22 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     var genericName = '$name\$';
     var typeParams = type.typeParameters.map((p) => p.name);
     if (isPublic(name)) _exports.add(genericName);
-    return js.statement('const # = dart.generic(function(#) { #; return #; });',
-        [genericName, typeParams, body, name]);
+
+    return js.statement('const # = #(function(#) { #; return #; });',
+        [genericName, _dartGeneric, typeParams, body, name]);
   }
 
+  get _dartGeneric =>
+      js.call('#.generic', [_isDartRuntime ? _classesLibVar : _runtimeLibVar]);

[Jennifer Messerly, 2016/01/07 20:18:05]

I made this comment elsewhere, but I don't think we should switch on
`_isDartRuntime`.

Maybe it's worth a longer design discussion, but...

The way the libs were structured:

we had things like _rtti, _classes. They can import eachother, but they never
import dart:* SDK libs. They depend on loader.

dart:_runtime re-exports all of them.

All other Dart code we compiled implicitly depended on dart:_runtime.

That's a problem now that we're trying to compile _rtti, _classes, etc. How do
we bootstrap?

One answer here is we shouldn't keep separate libraries. That was just how we
structured the JavaScript. We could use "part" in Dart, and merge these all into
dart:_runtime. That would simplify a lot of code I suspect.

Another answer is keep them in separate libraries, but instead always refer to
those libraries.

We chatted a bit about this here and were leaning towards the "part" approach --
seems like it might really simplify the bootstrapping of dart_runtime.js

[ochafik, 2016/01/13 13:13:20]

Parts FTW!

+
+  get _dartExport =>
+      js.call('#.export', [_isDartRuntime ? _utilsLibVar : _runtimeLibVar]);
+
+  get _dartFn => js.call('#.fn', _isDartRuntime ? _rttiLibVar : _runtimeLibVar);
+
+  get _dartSetSignature => js.call(
+      '#.setSignature', [_isDartRuntime ? _classesLibVar : _runtimeLibVar]);
+
   final _hasDeferredSupertype = new HashSet<ClassElement>();
 
   bool _deferIfNeeded(DartType type, ClassElement current) {
@@ -832,7 +889,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       if (!sigFields.isEmpty || extensions.isNotEmpty) {
         var sig = new JS.ObjectInitializer(sigFields);
         var classExpr = new JS.Identifier(name);
-        body.add(js.statement('dart.setSignature(#, #);', [classExpr, sig]));
+        body.add(js.statement('#(#, #);', [_dartSetSignature, classExpr, sig]));
       }
     }
 
@@ -1305,15 +1362,15 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     var body = <JS.Statement>[];
     _flushLibraryProperties(body);
 
-    var name = node.name.name;
+    var name = _getJsName(node.element) ?? node.name.name;
 
     var fn = _visit(node.functionExpression);
-    bool needsTagging = true;
+    // Don't tag functions defined in the low-level runtime.
+    bool needsTagging = !_isDartRuntime;
 
     if (currentLibrary.source.isInSystemLibrary &&
         _isInlineJSFunction(node.functionExpression)) {
       fn = _simplifyPassThroughArrowFunCallBody(fn);
-      needsTagging = !_isDartUtils;
     }
 
     var id = new JS.Identifier(name);
@@ -1345,23 +1402,17 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   bool _isJSInvocation(Expression expr) =>
       expr is MethodInvocation && isInlineJS(expr.methodName.staticElement);
 
-  // Simplify `(args) => ((x, y) => { ... })(x, y)` to `(args) => { ... }`.
-  // Note: we don't check if the top-level args match the ones passed through
-  // the arrow function, which allows silently passing args through to the
-  // body (which only works if we don't do weird renamings of Dart params).
+  // Simplify `(args) => (() => { ... })()` to `(args) => { ... }`.
+  // Note: this allows silently passing args through to the body, which only

[Jennifer Messerly, 2016/01/07 20:18:05]

I don't think you need the "Note" anymore?

[ochafik, 2016/01/13 13:13:20]

Done.

+  // works if we don't do weird renamings of Dart params.
   JS.Fun _simplifyPassThroughArrowFunCallBody(JS.Fun fn) {
-    String getIdent(JS.Node node) => node is JS.Identifier ? node.name : null;
-    List<String> getIdents(List params) =>
-        params.map(getIdent).toList(growable: false);
-
     if (fn.body is JS.Block && fn.body.statements.length == 1) {
       var stat = fn.body.statements.single;
       if (stat is JS.Return && stat.value is JS.Call) {
         JS.Call call = stat.value;
-        if (call.target is JS.ArrowFun) {
-          var passedArgs = getIdents(call.arguments);
+        if (call.target is JS.ArrowFun && call.arguments.isEmpty) {
           JS.ArrowFun innerFun = call.target;
-          if (_listEquality.equals(getIdents(innerFun.params), passedArgs)) {
+          if (innerFun.params.isEmpty) {
             return new JS.Fun(fn.params, innerFun.body);
           }
         }
@@ -1409,12 +1460,13 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
           type.optionalParameterTypes.isEmpty &&
           type.namedParameterTypes.isEmpty &&
           type.normalParameterTypes.every((t) => t.isDynamic)) {
-        return js.call('dart.fn(#)', [clos]);
+        return js.call('#(#)', [_dartFn, clos]);
       }
       if (lazy) {
-        return js.call('dart.fn(#, () => #)', [clos, _emitFunctionRTTI(type)]);
+        return js.call(
+            '#(#, () => #)', [_dartFn, clos, _emitFunctionRTTI(type)]);
       }
-      return js.call('dart.fn(#, #)', [clos, _emitFunctionTypeParts(type)]);
+      return js.call('#(#, #)', [_dartFn, clos, _emitFunctionTypeParts(type)]);
     }
     throw 'Function has non function type: $type';
   }
@@ -1569,7 +1621,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
 
     _loader.declareBeforeUse(element);
 
-    var name = element.name;
+    var name = _getJsName(element) ?? element.name;

[Jennifer Messerly, 2016/01/07 20:18:04]

This is related to my other comment on _getJsName, but ... could we fold this
check into _maybeQualifiedName?

And maybe rename that function, _emitTopLevelName perhaps. (_maybeQualifiedName
is not a great name.)

I don't think we want to rename things in the other code paths. And that would
also cover _emitTypeName, which otherwise we would miss.

Generally speaking when we generate JS code for Dart code, the Dart names fall
into a few different categories:
* parameters and local variables: these can be freely renamed by the compiler
* class members, hence _emitMemberName
* top level names, which go through _maybeQualifiedName
* classes are special enough that they use _emitTypeName (which delegates to
_maybeQualifiedName)

Libraries (ES modules) also have names but those aren't relied on. For modules
their URL (the string path fragment) determines their identity. The name we
import that library as is just a temp that can be renamed.

(We also generate some JS names that are presumed to be from the JS global
scope, like "dart." and "dartx." -- but those didn't come from Dart Elements. At
least not prior to this CL)

[ochafik, 2016/01/13 13:13:20]

Moved the exported name resolution logic to down to _maybeQualifiedName (renamed
to _emitTopLevelName) in https://codereview.chromium.org/1580413002

 
     // type literal
     if (element is ClassElement ||
@@ -1581,7 +1633,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
 
     // library member
     if (element.enclosingElement is CompilationUnitElement) {
-      return _maybeQualifiedName(element);
+      return _maybeQualifiedName(element, name);
     }
 
     // Unqualified class member. This could mean implicit-this, or implicit
@@ -1884,11 +1936,32 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       new JS.Block(_visitList(node.statements) as List<JS.Statement>,
           isScope: true);
 
+  /// Return the type constructor `_foolib.Bar$` given `Bar` from lib `_foolib`.
+  ///
+  /// This implements / expands the `genericTypeConstructor` intrinsic defined
+  /// in `foreign_helper.dart`.
+  JS.Expression _emitGenericTypeConstructor(Expression typeExpression) {

[Jennifer Messerly, 2016/01/07 20:18:04]

I don't understand why this is needed. Given a type name, _emitTypeName will
emit it.

This may be related to my earlier comment about the _getJsName in
visitSimpleIdentifier path.

[ochafik, 2016/01/13 13:13:20]

This implements the new intrinsic used in the following places:
- generators.dart:
  - `const SyncIterable_E = ${genericTypeConstructor(SyncIterable)}($E);`
  - `return ${genericTypeConstructor(Future)}(T).new(function() {`
  - `this.controller = ${genericTypeConstructor(StreamController)}(T).new({`
- classes.dart:
  - `$setType($obj, ${genericTypeConstructor(JSArray)}($elementType))`

This intrinsic provides a reliable way to build generic types from JS
expressions in the runtime.

+    var ref = _visit(typeExpression);
+    if (ref is JS.PropertyAccess) {
+      var name = (ref.selector as JS.LiteralString).valueWithoutQuotes;
+      return new JS.PropertyAccess(
+          ref.receiver, new JS.LiteralString("'$name\$'"));
+    } else if (ref is JS.MaybeQualifiedId) {
+      var name = (ref.name as JS.Identifier).name;
+      return new JS.PropertyAccess(ref.qualifier, new JS.Identifier('$name\$'));
+    } else {
+      throw new ArgumentError('Invalid type ref: $ref (${ref?.runtimeType})');
+    }
+  }
+
   @override
   visitMethodInvocation(MethodInvocation node) {
     if (node.operator != null && node.operator.lexeme == '?.') {
       return _emitNullSafe(node);
     }
+    if (isGenericTypeConstructorInvocation(node)) {
+      return _emitGenericTypeConstructor(node.argumentList.arguments.single);

[Jennifer Messerly, 2016/01/07 20:18:04]

I don't understand why this case is needed. It seems totally broken to take a
method call, and generate only the first argument ...

[ochafik, 2016/01/13 13:13:20]

Sorry for the lacking doc, this is a new intrinsic that takes exacly 1 arg,
renamed predicate (+ added comments).

+    }
 
     var target = _getTarget(node);
     var result = _emitForeignJS(node);
@@ -2214,7 +2287,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     var isJSTopLevel = field.isFinal && _isFinalJSDecl(field);
     if (isJSTopLevel) eagerInit = true;
 
-    var fieldName = field.name.name;
+    var fieldName = _getJsName(element) ?? field.name.name;
+
     if ((field.isConst && eagerInit && element is TopLevelVariableElement) ||
         isJSTopLevel) {
       // constant fields don't change, so we can generate them as `let`
@@ -2850,7 +2924,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       } else if (_requiresStaticDispatch(target, memberId.name)) {
         var type = member.type;
         var clos = js.call('dart.#.bind(#)', [name, _visit(target)]);
-        return js.call('dart.fn(#, #)', [clos, _emitFunctionTypeParts(type)]);
+        return js.call(
+            '#(#, #)', [_dartFn, clos, _emitFunctionTypeParts(type)]);
       }
       code = 'dart.bind(#, #)';
     } else if (_requiresStaticDispatch(target, memberId.name)) {
@@ -3419,8 +3494,13 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   /// declaration) as it doesn't have any meaningful rules enforced.
   JS.Identifier _libraryName(LibraryElement library) {
     if (library == currentLibrary) return _exportsVar;
-    return _imports.putIfAbsent(
-        library, () => new JS.TemporaryId(jsLibraryName(library)));
+    return _imports.putIfAbsent(library, () {
+      var name = library.name;
+      if (name == 'dart._utils') return _utilsLibVar;

[Jennifer Messerly, 2016/01/07 20:18:04]

I don't think these special cases could be needed.

You could just ask for the _libraryName(_classesLib) where _classesLib is the
LibaryElement for dart:_classes.

[ochafik, 2016/01/13 13:13:20]

Done.

+      else if (name == 'dart._classes') return _classesLibVar;

[Jennifer Messerly, 2016/01/07 20:18:04]

also, relying on the library name is super risky. Anyone could impersonate it.
Only the URI is reliable way to determine if something is from the SDK.

[ochafik, 2016/01/13 13:13:20]

Acknowledged.

+      else if (name == 'dart._rtti') return _rttiLibVar;
+      else return new JS.TemporaryId(jsLibraryName(library));
+    });
   }
 
   DartType getStaticType(Expression e) => rules.getStaticType(e);