Flesh out dynamic invocation code

Flesh out dynamic invocation code

This gets sunflower working with ignore-types.

R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/cf0117eb4b5fc6c2154316b15c538cec61a2a2f3

[vsm, 2015-02-26 00:33:55]

vsm@google.com changed reviewers:
+ jmesserly@google.com

[vsm, 2015-02-26 00:34:00]

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:79: case '+':
Not sure which way we ought to go with these.  Are you thinking of something
like core.num['+']?

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.dart
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:1163: return visitExpression(node);
Should this just be:
   x['-']()
?  Or similar?  This is one case where Dart kind of allows overloading ...
operator- can be defined in both no and one arg forms.

[Jennifer Messerly, 2015-02-26 20:53:17]

Nice! some initial thoughts

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:25: // ES 6 methods have a different syntax.
fyi, this is spec'd in Function.prototype.toString():
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-function.prototype....

relevant spec text:
"The string representation must have the syntax of a FunctionDeclaration
FunctionExpression, GeneratorDeclaration, GeneratorExpression, ClassDeclaration,
ClassExpression, ArrowFunction, MethodDefinition, or GeneratorMethod depending
upon the actual characteristics of the object."

we'll likely need support for arrow functions, generators too

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:40: // TODO(vsm): How do we differentiate?  For now,
just reject native methods.
Assuming we still have the target object, "obj instanceof dart.Object" should do
the trick.
Definitely harder if we only have the unbound function. I added a note to
https://github.com/dart-lang/dev_compiler/issues/44

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:79: case '+':
On 2015/02/26 00:34:00, vsm wrote:
> Not sure which way we ought to go with these.  Are you thinking of something
> like core.num['+']?

probably not anymore, given the generalize type proposal ... that would be an
instance method on the type. :)
but yeah, ideally we call the core library method here somehow, rather than
having it all in a dynamic dispatch helper. I think that's a bit cleaner design.
Then tghe num/string/bool methods are just things you link in from core.js,
rather than baked into dynamic dispatch.

reminds me of https://github.com/dart-lang/dev_compiler/issues/46 :)

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:107: // Check for primitive types.
to build on previous comments ... Ideally, this worked more like:


var primitiveTypeMethods = {
  'number': core.num$methods,
  'boolean': core.bool$methods,
  'string': core.String$methods,
};

var methods = primitiveTypeMethods[typeof(obj)];
if (methods !== void 0) {
  f = methods[name];
} else {
 ...
}

note that this code likely lives down in "dinvoke" ... because by checkAndCall,
we already have an "f". The bug, I think, is down on this line: `return
checkAndCall(obj[method], obj, args, method);`

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:128: if (!isDartFunction(f)) {
we could also just see if it has our type signature. If not, just call.

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.dart
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:700: JS.Node _generateAssignment(Expression lhs,
Expression rhs,
I've been using the "_emit" prefix. For better or worse. I like "generate"
better but it's too many characters :). I'd be happy with "_gen" if you prefer,
but then we should probably rename everything to be consistent

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:721: } else if (lhs is PrefixedIdentifier) {
how does a prefixed identifier end up as a dynamic set? we should always be able
to resolve this statically, right?

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:1114: // TODO(vsm): Should we generate a runtime
helper for this?
yeah, for now i think that's the pattern. this looks big enough I'd consider
putting it there.

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:1144: return _generateIncrement(op, expr,
false);
nit: use named arg `prefix: false`

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:1163: return visitExpression(node);
On 2015/02/26 00:34:00, vsm wrote:
> Should this just be:
>    x['-']()
> ?  Or similar?  This is one case where Dart kind of allows overloading ...
> operator- can be defined in both no and one arg forms.  

IMO, name should be "unary-" unless we find a good reason not to ... from
language spec:

"The name of the unary operator - is unary-.
This device allows the two methods to be distinguished for purposes of method
lookup, override and reflection."

[vsm, 2015-03-03 21:06:12]

PTAL

I've stripped out the runtime changes from this CL.

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:25: // ES 6 methods have a different syntax.
On 2015/02/26 20:53:16, John Messerly wrote:
> fyi, this is spec'd in Function.prototype.toString():
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-function.prototype....
> 
> relevant spec text:
> "The string representation must have the syntax of a FunctionDeclaration
> FunctionExpression, GeneratorDeclaration, GeneratorExpression,
ClassDeclaration,
> ClassExpression, ArrowFunction, MethodDefinition, or GeneratorMethod depending
> upon the actual characteristics of the object."
> 
> we'll likely need support for arrow functions, generators too

Thanks!  I made pointer to the spec and filed an issue.

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:40: // TODO(vsm): How do we differentiate?  For now,
just reject native methods.
On 2015/02/26 20:53:16, John Messerly wrote:
> Assuming we still have the target object, "obj instanceof dart.Object" should
do
> the trick.
> Definitely harder if we only have the unbound function. I added a note to
> https://github.com/dart-lang/dev_compiler/issues/44

Thanks, added a pointer to the issue.

https://codereview.chromium.org/961513002/diff/1/lib/runtime/dart_runtime.js#...
lib/runtime/dart_runtime.js:128: if (!isDartFunction(f)) {
On 2015/02/26 20:53:16, John Messerly wrote:
> we could also just see if it has our type signature. If not, just call.

Right, thinking isDartFunction would do something along those lines.

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.dart
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:700: JS.Node _generateAssignment(Expression lhs,
Expression rhs,
On 2015/02/26 20:53:16, John Messerly wrote:
> I've been using the "_emit" prefix. For better or worse. I like "generate"
> better but it's too many characters :). I'd be happy with "_gen" if you
prefer,
> but then we should probably rename everything to be consistent

Done.

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:721: } else if (lhs is PrefixedIdentifier) {
On 2015/02/26 20:53:16, John Messerly wrote:
> how does a prefixed identifier end up as a dynamic set? we should always be
able
> to resolve this statically, right?

In sunflower, notes.textContent shows up as a PrefixedIdentifier.  Without this,
I generate:

dart.dload(exports.notes, 'textContent') = `${exports.seeds} seeds`;

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:1114: // TODO(vsm): Should we generate a runtime
helper for this?
On 2015/02/26 20:53:16, John Messerly wrote:
> yeah, for now i think that's the pattern. this looks big enough I'd consider
> putting it there.

Note - there is a free expression in the closure, so it's not trivial to
helperize.  I was also missing a side effect on the expression.  It appears
twice below, so the semantics will be wrong.  Unimplemented for now.  Ideas?

https://codereview.chromium.org/961513002/diff/1/lib/src/codegen/js_codegen.d...
lib/src/codegen/js_codegen.dart:1144: return _generateIncrement(op, expr,
false);
On 2015/02/26 20:53:17, John Messerly wrote:
> nit: use named arg `prefix: false`

Done.

[vsm, 2015-03-04 00:54:21]

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1191: if (!_isStateless(expr) &&
Actually, I think this is unnecessary...

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1199: var applied = prefix ? '$tmp$op' :
'$op$tmp';
This might not be a number.  I'll change this to call ['+'] or ['-']
appropriately.

[Jennifer Messerly, 2015-03-04 01:09:31]

yeah this is coming along nicely

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:737: // TODO(vsm): Is this the right code if the
prefix is a library?
how can this happen? I don't think PrefixedIdentifier should ever be dynamic?

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1190: // TODO(vsm): If the expression has a side
effect, we need something different.
long line

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1191: if (!_isStateless(expr) &&
On 2015/03/04 00:54:21, vsm wrote:
> Actually, I think this is unnecessary...

right, your temps are handling this?

consider `foo += 1` where foo is defined as:

    get foo() { print('get foo'); return 42; }
    set foo(x) { print('set foo $x'); }

the getter and setter should be called once each.

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1199: var applied = prefix ? '$tmp$op' :
'$op$tmp';
On 2015/03/04 00:54:21, vsm wrote:
> This might not be a number.  I'll change this to call ['+'] or ['-']
> appropriately.

yeah. ideally it could "desugar" to `expr += 1`, as per spec.

[Siggi Cherem (dart-lang), 2015-03-04 03:56:06]

sigmund@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-03-04 03:56:07]

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:737: // TODO(vsm): Is this the right code if the
prefix is a library?
On 2015/03/04 01:09:31, John Messerly wrote:
> how can this happen? I don't think PrefixedIdentifier should ever be dynamic?

My understanding is that PrefixedIdentifier is used also when parsing property
accesses if it is ambiguous. The resolver will attach a different element in
each case, but they'll use the same AST node type for both during parsing.

For example:
  - because `a.b` is ambiguous, the analyzer will use a PrefixedIdentifier node
  - because `a.b.c` has more than 2 dots, the analyzer knows .c is a property
access and creates a PropertyAccess right away.

[vsm, 2015-03-04 14:59:27]

PTAL

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1190: // TODO(vsm): If the expression has a side
effect, we need something different.
On 2015/03/04 01:09:31, John Messerly wrote:
> long line

removed

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1191: if (!_isStateless(expr) &&
On 2015/03/04 01:09:31, John Messerly wrote:
> On 2015/03/04 00:54:21, vsm wrote:
> > Actually, I think this is unnecessary...
> 
> right, your temps are handling this?
> 
> consider `foo += 1` where foo is defined as:
> 
>     get foo() { print('get foo'); return 42; }
>     set foo(x) { print('set foo $x'); }
> 
> the getter and setter should be called once each.

right

https://codereview.chromium.org/961513002/diff/80001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1199: var applied = prefix ? '$tmp$op' :
'$op$tmp';
On 2015/03/04 01:09:31, John Messerly wrote:
> On 2015/03/04 00:54:21, vsm wrote:
> > This might not be a number.  I'll change this to call ['+'] or ['-']
> > appropriately.
> 
> yeah. ideally it could "desugar" to `expr += 1`, as per spec.

mapping to x = x + 1 for now, but yeah.

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

This is coming along really nicely!

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:739: // TODO(vsm): Is this the right code if the
prefix is a library?
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;
  }

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1193: var tmp1 = '_t1';
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)

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1201: var binary =
AstBuilder.binaryExpression(expr, op.lexeme[0], one);
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

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1202: var increment = _visit(binary);
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 :)

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1206: new SimpleIdentifier(new
StringToken(TokenType.IDENTIFIER, tmp1, 0));
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

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1212: (function () {
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)

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1213: var $tmp1 = #;
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.

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1216: return ${prefix ? tmp2 : tmp1};
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]

PTAL

Really like your idea to streamline / specialize pre and post.

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1193: var tmp1 = '_t1';
On 2015/03/04 16:03:51, John Messerly wrote:
> 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)

Done.

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1201: var binary =
AstBuilder.binaryExpression(expr, op.lexeme[0], one);
On 2015/03/04 16:03:51, John Messerly wrote:
> 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

good catch!  fixed.

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1202: var increment = _visit(binary);
On 2015/03/04 16:03:51, John Messerly wrote:
> 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 :)

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.

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1206: new SimpleIdentifier(new
StringToken(TokenType.IDENTIFIER, tmp1, 0));
On 2015/03/04 16:03:51, John Messerly wrote:
> 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

I factored this out a bit.

https://codereview.chromium.org/961513002/diff/100001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1216: return ${prefix ? tmp2 : tmp1};
On 2015/03/04 16:03:51, John Messerly wrote:
> 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.

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

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

[Jennifer Messerly, 2015-03-04 17:59:45]

LGTM!

[vsm, 2015-03-04 18:13:34]

Committed patchset #7 (id:120001) manually as
cf0117eb4b5fc6c2154316b15c538cec61a2a2f3 (presubmit successful).