more function refactoring

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.
 
 import 'dart:collection' show HashSet, HashMap, SplayTreeSet;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/token.dart';
 import 'package:analyzer/src/generated/ast.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/constant.dart';
@@ skipping 1451 matching lines @@
     if (node.isGetter || node.isSetter) {
       // 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);
+    var fn = _emitFunction(node.functionExpression);
 
     if (currentLibrary.source.isInSystemLibrary &&
         _isInlineJSFunction(node.functionExpression)) {
       fn = _simplifyPassThroughArrowFunCallBody(fn);
     }
 
     var id = new JS.Identifier(name);
     body.add(annotate(new JS.FunctionDeclaration(id, fn), node, node.element));
     if (!_isDartRuntime) {
       body.add(_emitFunctionTagged(id, node.element.type, topLevel: true)
@@ skipping 40 matching lines @@
           }
         }
       }
     }
     return fn;
   }
 
   JS.Method _emitTopLevelProperty(FunctionDeclaration node) {
     var name = node.name.name;
     return annotate(
-        new JS.Method(_propertyName(name), _visit(node.functionExpression),
+        new JS.Method(
+            _propertyName(name), _emitFunction(node.functionExpression),
             isGetter: node.isGetter, isSetter: node.isSetter),
         node,
         node.element);
   }
 
   bool _executesAtTopLevel(AstNode node) {
     var ancestor = node.getAncestor((n) =>
         n is FunctionBody ||
         (n is FieldDeclaration && n.staticKeyword == null) ||
         (n is ConstructorDeclaration && n.constKeyword == null));
     return ancestor == null;
   }
 
   bool _typeIsLoaded(DartType type) {
     if (type is FunctionType && (type.name == '' || type.name == null)) {
       return (_typeIsLoaded(type.returnType) &&
           type.optionalParameterTypes.every(_typeIsLoaded) &&
           type.namedParameterTypes.values.every(_typeIsLoaded) &&
           type.normalParameterTypes.every(_typeIsLoaded));
     }
     if (type.isDynamic || type.isVoid || type.isBottom) return true;
     if (type is ParameterizedType && !type.typeArguments.every(_typeIsLoaded)) {
       return false;
     }
     return _loader.isLoaded(type.element);
   }
 
-  JS.Expression _emitFunctionTagged(JS.Expression clos, DartType type,
+  JS.Expression _emitFunctionTagged(JS.Expression fn, DartType type,
       {topLevel: false}) {
     var name = type.name;
     var lazy = topLevel && !_typeIsLoaded(type);
 
     if (type is FunctionType && (name == '' || name == null)) {
       if (type.returnType.isDynamic &&
           type.optionalParameterTypes.isEmpty &&
           type.namedParameterTypes.isEmpty &&
           type.normalParameterTypes.every((t) => t.isDynamic)) {
-        return js.call('dart.fn(#)', [clos]);
+        return js.call('dart.fn(#)', [fn]);
       }
       if (lazy) {
-        return js.call('dart.fn(#, () => #)', [clos, _emitFunctionRTTI(type)]);
+        return js.call('dart.fn(#, () => #)', [fn, _emitFunctionRTTI(type)]);
       }
-      return js.call('dart.fn(#, #)', [clos, _emitFunctionTypeParts(type)]);
+      return js.call('dart.fn(#, #)', [fn, _emitFunctionTypeParts(type)]);
     }
     throw 'Function has non function type: $type';
   }
 
+  /// Emits an arrow FunctionExpression node.
+  ///
+  /// This should be used for all places in Dart's AST where FunctionExpression
+  /// appears and the function is actually in an Expression context. These
+  /// correspond to arrow functions in Dart.
+  ///
+  /// Contrast with [_emitFunction].
   @override
   JS.Expression visitFunctionExpression(FunctionExpression node) {
-    var params = visitFormalParameterList(node.parameters);
+    assert(node.parent is! FunctionDeclaration &&
+        node.parent is! MethodDeclaration);
+    return _emitFunctionTagged(_emitArrowFunction(node), getStaticType(node),
+        topLevel: _executesAtTopLevel(node));
+  }
+
+  JS.ArrowFun _emitArrowFunction(FunctionExpression node) {
+    List<JS.Parameter> params;
 
     var body = node.body;
-    var parent = node.parent;
-    var inStmt = parent.parent is FunctionDeclarationStatement;
-    if (parent is FunctionDeclaration) {
-      return _emitFunctionBody(node.element, node.parameters, body);
-    }
-
     JS.Node jsBody;
     if (body.isGenerator || body.isAsynchronous) {
+      params = visitFormalParameterList(node.parameters, destructure: false);
       jsBody = _emitGeneratorFunctionBody(node.element, node.parameters, body);
-    } else if (body is ExpressionFunctionBody) {
-      jsBody = _visit(body.expression);
-    } else {
-      jsBody = _visit(body);
-    }
-
-    var type = node.element.type;
-    var typeFormals = _emitTypeFormals(type.typeFormals);
-    var returnType = emitTypeRef(type.returnType);
-
-    JS.FunctionExpression fn;
-    if (node.parameters.parameters
-        .every((p) => p.kind != ParameterKind.NAMED)) {
-      fn = new JS.ArrowFun(params, jsBody,
-          typeParams: typeFormals, returnType: returnType);
     } else {
       // Chrome Canary does not accept default values with destructuring in
       // arrow functions yet (e.g. `({a} = {}) => 1`) but happily accepts them
       // with regular functions (e.g. `function({a} = {}) { return 1 }`).
       // Note that Firefox accepts both syntaxes just fine.
       // TODO(ochafik): Simplify this code when Chrome Canary catches up.
+      bool hasNamedParams =
+          node.parameters.parameters.any((p) => p.kind == ParameterKind.NAMED);
 
-      if (jsBody is JS.Expression) {
-        jsBody = js.statement("{ return #; }", [jsBody]);
+      params = visitFormalParameterList(node.parameters,
+          destructure: !hasNamedParams);
+
+      if (body is ExpressionFunctionBody) {
+        // An arrow function can use the expression directly.
+        jsBody = _visit(body.expression);
+      } else {
+        jsBody = _visit(body);
       }
-      fn = new JS.Fun(params, jsBody,
-          typeParams: typeFormals, returnType: returnType);
     }
 
-    if (!inStmt) {
-      var type = getStaticType(node);
-      return _emitFunctionTagged(fn, type, topLevel: _executesAtTopLevel(node));
-    }
-    return fn;
+    var type = node.element.type;
+    var fn = new JS.ArrowFun(params, jsBody,
+        typeParams: _emitTypeFormals(type.typeFormals),
+        returnType: emitTypeRef(type.returnType));
+    return annotate(fn, node);
+  }
+
+  /// Emits a non-arrow FunctionExpression node.
+  ///
+  /// This should be used for all places in Dart's AST where FunctionExpression
+  /// appears but the function is not actually in an Expression context, such
+  /// as methods, properties, and top-level functions.
+  ///
+  /// Contrast with [visitFunctionExpression].
+  JS.Fun _emitFunction(FunctionExpression node) {
+    var fn = _emitFunctionBody(node.element, node.parameters, node.body);
+    return annotate(fn, node);
   }
 
   JS.Fun _emitFunctionBody(ExecutableElement element,
       FormalParameterList parameters, FunctionBody body) {
     var returnType = emitTypeRef(element.returnType);
 
     // sync*, async, async*
     if (element.isAsynchronous || element.isGenerator) {
       return new JS.Fun(
           visitFormalParameterList(parameters, destructure: false),
@@ skipping 70 matching lines @@
   }
 
   @override
   JS.Statement visitFunctionDeclarationStatement(
       FunctionDeclarationStatement node) {
     var func = node.functionDeclaration;
     if (func.isGetter || func.isSetter) {
       return js.comment('Unimplemented function get/set statement: $node');
     }
 
-    var fn = _visit(func.functionExpression);
+    var fn = _emitFunction(func.functionExpression);
 
     var name = new JS.Identifier(func.name.name);
     JS.Statement declareFn;
     if (JS.This.foundIn(fn)) {
       declareFn = js.statement('const # = #.bind(this);', [name, fn]);
     } else {
       declareFn = new JS.FunctionDeclaration(name, fn);
     }
     declareFn = annotate(declareFn, node, node.functionDeclaration.element);
 
@@ skipping 1760 matching lines @@
   }
 
   _visit(AstNode node) {
     if (node == null) return null;
     var result = node.accept(this);
     if (result is JS.Node) result = annotate(result, node);
     return result;
   }
 
   // TODO(jmesserly): this will need to be a generic method, if we ever want to
   // self-host strong mode.

[vsm, 2016/02/25 00:48:04]

Probably don't need the TODO anymore.  :-)

[Jennifer Messerly, 2016/02/25 00:53:45]

Yup! Have this fixed in a follow up CL I'm working on. :)

-  List/*<T>*/ _visitList/*<T>*/(Iterable<AstNode> nodes) {
+  List /*<T>*/ _visitList /*<T>*/ (Iterable<AstNode> nodes) {
     if (nodes == null) return null;
     var result = /*<T>*/ [];
     for (var node in nodes) result.add(_visit(node));
     return result;
   }
 
   /// Visits a list of expressions, creating a comma expression if needed in JS.
   JS.Expression _visitListToBinary(List<Expression> nodes, String operator) {
     if (nodes == null || nodes.isEmpty) return null;
     return new JS.Expression.binary(
@@ skipping 289 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);
 }