Flesh out dynamic invocation code

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 cadeb9090c0f1e429e6bb906ea58913be5bbd7f2..a09350c6529ee25448ee18862a5afddd905b7e55 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -18,6 +18,8 @@ import 'package:source_maps/source_maps.dart' show SourceMapSpan;
 import 'package:source_span/source_span.dart' show SourceLocation;
 import 'package:path/path.dart' as path;
 
+import 'package:dev_compiler/src/codegen/ast_builder.dart' show AstBuilder;
+
 // TODO(jmesserly): import from its own package
 import 'package:dev_compiler/src/js/js_ast.dart' as JS;
 import 'package:dev_compiler/src/js/js_ast.dart' show js;
@@ -698,10 +700,27 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     return result;
   }
 
+  JS.Node _emitDPutIfDynamic(
+      Expression target, SimpleIdentifier id, Expression rhs) {
+    if (rules.isDynamicTarget(target)) {
+      return js.call('dart.dput(#, #, #)', [
+        _visit(target),
+        js.string(id.name, "'"),
+        _visit(rhs)
+      ]);
+    } else {
+      return null;
+    }
+  }
+
   @override
   JS.Node visitAssignmentExpression(AssignmentExpression node) {
     var lhs = node.leftHandSide;
     var rhs = node.rightHandSide;
+    return _emitAssignment(lhs, rhs, node.parent);
+  }
+
+  JS.Node _emitAssignment(Expression lhs, Expression rhs, [AstNode parent]) {
     if (lhs is IndexExpression) {
       String code;
       var target = _getTarget(lhs);
@@ -714,17 +733,15 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     }
 
     if (lhs is PropertyAccess) {
-      var target = _getTarget(lhs);
-      if (rules.isDynamicTarget(target)) {
-        return js.call('dart.dput(#, #, #)', [
-          _visit(target),
-          js.string(lhs.propertyName.name, "'"),
-          _visit(rhs)
-        ]);
-      }
+      var result = _emitDPutIfDynamic(_getTarget(lhs), lhs.propertyName, rhs);
+      if (result != null) return result;
+    } else if (lhs is PrefixedIdentifier) {
+      // TODO(vsm): Is this the right code if the prefix is a library?

[Jennifer Messerly, 2015/03/04 16:03:51]

ah, I think the PrefixedIdentifier branch makes sense now, but IMO you shouldn't
hit dynamic path if prefix is a library.

maybe replace TODO with an assert inside isTargetDynamic?

  bool isDynamicTarget(Expression expr) {
    var result = isDynamic(getStaticType(expr));
    assert(!result || expr.staticElement is! LibraryElement);
    return result;
  }

+      var result = _emitDPutIfDynamic(lhs.prefix, lhs.identifier, rhs);
+      if (result != null) return result;
     }
 
-    if (node.parent is ExpressionStatement &&
+    if (parent is ExpressionStatement &&
         rhs is CascadeExpression &&
         _isStateless(lhs, rhs)) {
       // Special case: cascade assignment to a variable in a statement.
@@ -1169,6 +1186,38 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
 
   bool _isNull(Expression expr) => expr is NullLiteral;
 
+  JS.Expression _emitIncrement(Token op, Expression expr, {bool prefix}) {
+    assert(prefix != null);
+
+    // TODO(vsm): Avoid collisions.
+    var tmp1 = '_t1';

[Jennifer Messerly, 2015/03/04 16:03:51]

so far we've been using `$` for compiler generated things (I think Justin
started this with opt$), as it's less likely to collide than `_` ... that said
we do need to fix all of temps to be better. (I'm working on it now)

[vsm, 2015/03/04 17:55:30]

Done.

+    var tmp2 = '_t2';
+
+    // Read
+    var read = _visit(expr);
+
+    // Increment
+    var one = AstBuilder.integerLiteral(1);
+    var binary = AstBuilder.binaryExpression(expr, op.lexeme[0], one);

[Jennifer Messerly, 2015/03/04 16:03:51]

hmmm. Should this be `id` instead of `expr` as the LHS? I think the expression
is getting expanded twice:

_t1 = EXPR;
_t2 = EXPR + 1;
EXPR = _t1;
return _t2; // or _t1 if postfix

[vsm, 2015/03/04 17:55:30]

good catch!  fixed.

+    var increment = _visit(binary);

[Jennifer Messerly, 2015/03/04 16:03:51]

this could be directly `visitBinaryExpression` since we don't need to attach
source info (it has none)

preface: this pattern is totally okay. But, it's sometimes nice to avoid extra
allocations if possible. In this case, it's probably not too bad to factor out a
`_visitBinary` or `_emitBinary` method that lets you supply these as arguments
(like `_emitAssignment`). Not sure I would change it now, but something to
consider. Definitely will be nice to have tree-builder powers in some cases :)

[vsm, 2015/03/04 17:55:30]

This ends up being a little complicated.  I'm feeding increment (now) directly
into _emitAssignment (following your suggestion below), so I need an analyzer
Expression. 

+
+    // Write
+    var id =
+        new SimpleIdentifier(new StringToken(TokenType.IDENTIFIER, tmp1, 0));

[Jennifer Messerly, 2015/03/04 16:03:51]

maybe a TODO: I think we had to do something similar in a few other places
(exception parameter, cascade temp?) might be worth a helper. Anyway I'll take a
look at this in my temps change

[vsm, 2015/03/04 17:55:30]

I factored this out a bit.

+    id.staticElement = new LocalVariableElementImpl.forNode(id);
+    id.staticType = expr.staticType;
+    var write = _emitAssignment(expr, id);
+
+    return js.call('''
+(function () {

[Jennifer Messerly, 2015/03/04 16:03:51]

I think you want an `=>` function here. Otherwise this bindings will be broken.
(and for now, need the $bindThis workaround).

    var bindThis = _maybeBindThis(expr);

    return js.call('''
(() => {
...
})$bindThis();
''', ...

Unlike Dart, ES6 arrow functions can have statements. They're supposed to
replace basically all inline `function` expressions. The few places I still use
`function` in the code generator is either top level guys (which need
`function`), or method/getter/setter bodies (which don't actually pretty print
with `function`, it's just a js_ast artifact)

+  var $tmp1 = #;

[Jennifer Messerly, 2015/03/04 16:03:51]

one warning I got from Stephen was to make sure to only Dart-string-interpolate
constants (else it breaks caching). But you're doing that here, so all good :).
Just wanted to pass that on.

+  var $tmp2 = #;
+  #;
+  return ${prefix ? tmp2 : tmp1};

[Jennifer Messerly, 2015/03/04 16:03:51]

hmmm, I wonder if we can do better for prefix and postfix by special casing
them?


Couldn't prefix just be:

(expr = expr + 1)

Not sure we need temps or the function for prefix?


Postfix needs temps in the general case (I think just one):

{
  var t = expr;
  expr = t + 1;
  return t;
}

Since we're generating a lambda anyway, we can just use that for the temp:

    ((t) => (expr = t + 1, t))(expr)

I did a trick like that for cascades.

I think that's about as small as it can get.

[vsm, 2015/03/04 17:55:30]

Nice idea!  I will try this.  In the postfix, dynamic case, this would be:

((t) => (dput(expr, dbinary(t, '+', 1), t)(dget(expr))

+})()
+''', [read, increment, write]);
+  }
+
   @override
   JS.Expression visitPostfixExpression(PostfixExpression node) {
     var op = node.operator;
@@ -1179,8 +1228,9 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
       // TODO(vsm): When do Dart ops not map to JS?
       return js.call('#$op', notNull(expr));
     } else {
-      // TODO(vsm): Figure out operator calling convention / dispatch.
-      return visitExpression(node);
+      assert(op.lexeme == '++' || op.lexeme == '--');
+      var result = _emitIncrement(op, expr, prefix: false);
+      return (result != null) ? result : visitExpression(node);
     }
   }
 
@@ -1194,8 +1244,24 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
       // TODO(vsm): When do Dart ops not map to JS?
       return js.call('$op#', notNull(expr));
     } else {
-      // TODO(vsm): Figure out operator calling convention / dispatch.
-      return visitExpression(node);
+      // Increment or decrement must be numerical
+      if (op.lexeme == '++' || op.lexeme == '--') {
+        var result = _emitIncrement(op, expr, prefix: true);
+        return (result != null) ? result : visitExpression(node);
+      }
+    }
+    // TODO(vsm): Statically invoke the operator if the type is known.
+    // Fall back to dynamic dispatch.
+    var opString = _jsMemberName(op.lexeme, unary: true);
+    if (rules.isDynamicTarget(expr)) {
+      // dynamic dispatch
+      return js.call('dart.dunary(#, #)', [opString, _visit(expr)]);
+    } else if (_isJSBuiltinType(dispatchType)) {
+      return js.call(
+          '#.#(#)', [_emitTypeName(dispatchType), opString, _visit(expr)]);
+    } else {
+      // Generic static-dispatch, user-defined operator code path.
+      return js.call('#.#()', [_visit(expr), opString]);
     }
   }