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

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 86 matching lines @@
   ConstFieldVisitor _constField;
 
   ModuleItemLoadOrder _loader;
 
   /// _interceptors.JSArray<E>, used for List literals.
   ClassElement _jsArray;
 
   /// The default value of the module object. See [visitLibraryDirective].
   String _jsModuleValue;
 
+  bool _isDartUtils;
+
   Map<String, DartType> _objectMembers;
 
   JSCodegenVisitor(AbstractCompiler compiler, this.rules, this.currentLibrary,
       this._extensionTypes, this._fieldsNeedingStorage)
       : compiler = compiler,
         options = compiler.options.codegenOptions,
         _types = compiler.context.typeProvider {
     _loader = new ModuleItemLoadOrder(_emitModuleItem);
 
     var context = compiler.context;
     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);
   }
 
   TypeProvider get types => rules.provider;
 
   JS.Program emitLibrary(LibraryUnit library) {
     // Modify the AST to make coercions explicit.
     new CoercionReifier(library, rules).reify();
 
@@ skipping 59 matching lines @@
     // 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 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);
       }
     });
 
     var lazyImports = <JS.Expression>[];
     _imports.forEach((library, temp) {
       if (!_loader.libraryIsLoaded(library)) {
         processImport(library, temp, lazyImports);
       }
     });
 
-    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, "'"),
       _jsModuleValue ?? new JS.LiteralNull(),
       js.commentExpression(
           "Imports", new JS.ArrayInitializer(imports, multiline: true)),
       js.commentExpression("Lazy imports",
           new JS.ArrayInitializer(lazyImports, multiline: true)),
       module
     ]);
@@ skipping 54 matching lines @@
             .map((i) => i.name)
             .where((s) => !currentLibNames.containsKey(s))
             .map((s) => js.string(s, "'")));
       }
       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]));
+    _moduleItems.add(js.statement('dart.export_(#);', [args]));
   }
 
   JS.Identifier _initSymbol(JS.Identifier id) {
     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.
@@ skipping 974 matching lines @@
       // Add these later so we can use getter/setter syntax.
       _properties.add(node);
       return null;
     }
 
     var body = <JS.Statement>[];
     _flushLibraryProperties(body);
 
     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(
         new JS.Method(_propertyName(name), _visit(node.functionExpression),
             isGetter: node.isGetter, isSetter: node.isSetter),
         node.element);
   }
 
   bool _executesAtTopLevel(AstNode node) {
     var ancestor = node.getAncestor((n) => n is FunctionBody ||
@@ skipping 2039 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);
 }