Convert dart_utils.js to input_sdk/lib/_internal/utils.dart (#310)

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 0dd62fd6d66a560036728d90a15671685a87c050..367e895ea4c2e1a4d38574f28f9130b4a82f28ab 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -104,6 +104,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   /// The default value of the module object. See [visitLibraryDirective].
   String _jsModuleValue;
 
+  bool _isDartUtils;
+
   Map<String, DartType> _objectMembers;
 
   JSCodegenVisitor(AbstractCompiler compiler, this.rules, this.currentLibrary,
@@ -117,6 +119,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     var src = context.sourceFactory.forUri('dart:_interceptors');
     var interceptors = context.computeLibraryElement(src);
     _jsArray = interceptors.getType('JSArray');
+    _isDartUtils = currentLibrary.isInSdk &&

[Jennifer Messerly, 2015/11/30 18:54:11]

you shouldn't need the isInSdk check, it's just checking if the library name
starting with "dart." ... IMO, checking the URI should be sufficient.

+        currentLibrary.source.uri.toString() == 'dart:_utils';
 
     _objectMembers = getObjectMemberMap(types);
   }
@@ -196,7 +200,19 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       list.add(js.string(compiler.getModuleName(library.source.uri), "'"));
     };
 
-    var imports = <JS.Expression>[js.string('dart/_runtime')];
+    var needsDartRuntime = !_isDartUtils;
+
+    var imports = <JS.Expression>[];
+    var moduleStatements = <JS.Statement>[];
+    if (needsDartRuntime) {
+      imports.add(js.string('dart/_runtime'));
+
+      var dartxImport =
+          js.statement("let # = #.dartx;", [_dartxVar, _runtimeLibVar]);

[Jennifer Messerly, 2015/11/30 18:54:11]

const?

+      moduleStatements.add(dartxImport);
+    }
+    moduleStatements.addAll(_moduleItems);
+
     _imports.forEach((library, temp) {
       if (_loader.libraryIsLoaded(library)) {
         processImport(library, temp, imports);
@@ -210,11 +226,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       }
     });
 
-    var dartxImport =
-        js.statement("let # = #.dartx;", [_dartxVar, _runtimeLibVar]);
-
-    var module = js.call("function(#) { 'use strict'; #; #; }",
-        [params, dartxImport, _moduleItems]);
+    var module =
+        js.call("function(#) { 'use strict'; #; }", [params, moduleStatements]);
 
     var moduleDef = js.statement("dart_library.library(#, #, #, #, #)", [
       js.string(jsPath, "'"),
@@ -289,7 +302,7 @@ 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]));
+    _moduleItems.add(js.statement('dart.export_(#);', [args]));
   }
 
   JS.Identifier _initSymbol(JS.Identifier id) {
@@ -1284,17 +1297,64 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
 
     var name = node.name.name;
 
+    var fn = _visit(node.functionExpression);
+    bool needsTagging = true;
+    if (currentLibrary.isInSdk &&

[Jennifer Messerly, 2015/11/30 18:54:11]

I try and avoid LibraryElement.isInSdk, because any library whose name starts
with "dart." can pretend to be part of the SDK.

(Aside, we should try and get this fixed in Analyzer... it's a bit of a trap
right now)

So far we've been checking if the source.uri.scheme == 'dart', but going
forward, source.isInSystemLibrary might be better. It's equivalent but reads
nicer IMO.

[ochafik, 2015/12/01 14:41:03]

Done.

+        _isInlineJSBody(node.functionExpression.body)) {
+      fn = _simplifyFun(fn);
+      needsTagging = !_isDartUtils;
+    }
     var id = new JS.Identifier(name);
     body.add(annotate(
-        new JS.FunctionDeclaration(id, _visit(node.functionExpression)),
+        new JS.FunctionDeclaration(id, fn),
         node.element));
-    body.add(_emitFunctionTagged(id, node.element.type, topLevel: true)
-        .toStatement());
+    if (needsTagging) {
+      body.add(_emitFunctionTagged(id, node.element.type, topLevel: true)
+          .toStatement());
+    }
 
     if (isPublic(name)) _addExport(name);
     return _statement(body);
   }
 
+  /// Simplify `f(x) => (function() { ... })()` to `f(x) => { ... }`.
+  ///
+  /// This is useful for runtime utils written with JS intrinsics, e.g.
+  /// `foo(x) => JS('', '(function() { let x = #; ... })()', x);`.
+  JS.Fun _simplifyFun(JS.Fun fn) {

[Jennifer Messerly, 2015/11/30 18:54:11]

love this!

Regarding:
    JS('', '(function() { let x = #; ... })()', x);

it might be nicer to write this as:

    JS('', '((x) => { ... })(#)', x);

In other words, use arrow functions & parameters.

Not a big deal though, as they should be equivalent. Don't worry about it unless
you're feeling inspired :)

[ochafik, 2015/12/01 14:41:03]

I was equally bothered by `let x = x` and the silent pass-through, but your
suggestion is much cleaner, thanks, Done!

+    if (fn.body is JS.Block && fn.body.statements.length == 1) {
+      var stat = fn.body.statements.single;
+      if (stat is JS.Return) {
+        if (stat.value is JS.Call) {
+          JS.Call call = stat.value;
+          if (call.target is JS.Fun && call.arguments.isEmpty) {
+            JS.Fun innerFun = call.target;
+            return new JS.Fun(fn.params, innerFun.body);
+          }
+        }
+      }
+    }
+    return fn;
+  }
+
+  bool _isInlineJSBody(FunctionBody body) {

[Jennifer Messerly, 2015/11/30 18:54:11]

It might be nice to combine this with the simplification step, so the two
operations are done "atomically":

JS.Fun _emitSdkJsFunction(body);

And then it does checks & simplification internally. That might make the
pre/post conditions more clear and guarantee we only simplify if _isInlineJSBody
is true.

[ochafik, 2015/12/01 14:41:03]

I can't find a very clean way to combine them (have to do the checks outside the
call, or do it inside and return null, etc).

I've switched the pattern to only using arrow functions, which means the rewrite
should always be correct (I think? even for `this` bindings): I don't think that
method (renamed to _simplifyPassThroughArrowFunCallBody) needs a precondition
anymore.

+    bool isJsInvocation(Expression expr) =>
+        expr is MethodInvocation &&
+        isInlineJS(expr.methodName.staticElement);
+
+    if (body is ExpressionFunctionBody) {
+      return isJsInvocation(body.expression);
+    } else if (body is BlockFunctionBody) {
+      if (body.block.statements.length == 1) {
+        var stat = body.block.statements.single;
+        if (stat is ReturnStatement) {
+          return isJsInvocation(stat.expression);
+        }
+      }
+    }
+    return false;
+  }
+
   JS.Method _emitTopLevelProperty(FunctionDeclaration node) {
     var name = node.name.name;
     return annotate(