allow 'super' in async and finally blocks

allow 'super' in async and finally blocks

Fixes #499
Fixes #528

R=jmesserly@google.com

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

[Harry Terkelsen, 2016-05-03 17:44:38]

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

[Jennifer Messerly, 2016-05-03 18:56:18]

Really nice! I like how it turned out. Only thing to fix is using symbols for
the method names.

I've rambled a bit below brainstorming some various ideas, but that is an FYI.
Take & run with any if you feel inspired, I mostly wanted to share the full
spectrum of solutions, but I'm totally happy with this approach too.

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2348: JS.Expression
_emitMethodCall(Expression target, MethodInvocation node) {
do we need the same fix in _emitAccess?

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2380: if
(DynamicInvoke.get(node.methodName)) {
Is there a way to restructure this code so it's reused? it looks like
_emitSuperForwarder wants to use the rest of this code.

(It's always nice to try and keep the code reused, that way if someone tweaks
this code here later on, they won't regress how "super" is handled)

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2402: return new JS.Call(newTarget,
helperArgs);
FYI: you could build these like:

return js.call('this.#(#)', helperMethod, helperArgs);

and avoid the need for "newTarget" ... sra@ designed the `js.call` and
`js.statement` system to be pretty fast, it only parses once, and caches a
factory function that quickly creates the AST.

That said, it's always a bit of taste thing, whether to use raw JS AST nodes or
js.call. I tend to use raw nodes if it feels more natural (especially if I'm
just creating 1-2 nodes) or if we don't have JS AST parser support yet (e.g.
there's no `js.method(...)` helper for making a JS.Method) and use
js.call/js.statement for more complex things.

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2405: JS.Expression _getSuperHelperFor(
Probably worth a doc comment about what this method is for & how it works.

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2408: List<JS.Expression> typeArgs,
Hmmm, this is an interesting approach. It's one super forwarder per call site?

I probably would've tried to get the super method signature (e.g. the super
MethodElement), and just emit a forwarder that forwards everything. Not sure
which approach would end up simpler, but that way every call site to "super.foo"
could use the same helper. That's unlikely to be a big deal though.

Anyway, since this is a simple+clean implementation as you have it, I don't
think it's worth changing now :). It's so well factored. Just throwing out some
other ideas so you know your options.


Another trick is to use runtime helpers if it's easier:

class MyDerived extends MyBaseClass {
  someMethod() {
    try { ... } finally { dart.superCall(MyDerived, 'foo', this, arg, to, foo);
}
  }
}


function superCall(cls, name, self, ...args) {
  let baseClass = cls.__proto__;
  let superMethod = baseClass.prototype[name];
  superMethod.call(self, args);
}


In fact it's possible to avoid all of this by just desugaring ES6 super in
place:


class MyDerived extends MyBaseClass {
  someMethod() {
    try { ... } finally { MyBaseClass.prototype.foo.apply(this, arg, to, foo); }
  }
}

(if MyBaseClass was a mixin application, just save it to a temp first)


... but anyway since you've got the forwarding methods already, and they'll
likely perform better, it seems alright to keep, if the Symbol fix below is
done.

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2413: new JS.Identifier('a${counter++}');
this should be symbolized so it doesn't collide with methods:


let super$foo = Symbol('super.foo');

class D extends B {
  [super$foo](... args ... ) {
    ... impl ...
  }
}


we have a couple of places we generate Symbols, for example, how private method
names are handled, this could likely work in a similar way.

Also since "super$foo" will be a JS.TemporaryId, you won't need a counter --
renaming will be handled automagically.

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2435:
js.string('super\$${node.methodName.name}${_superHelpers.length}', "'");
based on comment above, "helperMethodName" will just be the JS.TemporaryId we
create to hold the method symbol.

[Harry Terkelsen, 2016-05-05 23:37:35]

Thanks John! PTAL

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2348: JS.Expression
_emitMethodCall(Expression target, MethodInvocation node) {
On 2016/05/03 18:56:18, John Messerly wrote:
> do we need the same fix in _emitAccess?

Done.

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2380: if
(DynamicInvoke.get(node.methodName)) {
On 2016/05/03 18:56:18, John Messerly wrote:
> Is there a way to restructure this code so it's reused? it looks like
> _emitSuperForwarder wants to use the rest of this code.
> 
> (It's always nice to try and keep the code reused, that way if someone tweaks
> this code here later on, they won't regress how "super" is handled)

There is a way ;)

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2402: return new JS.Call(newTarget,
helperArgs);
On 2016/05/03 18:56:18, John Messerly wrote:
> FYI: you could build these like:
> 
> return js.call('this.#(#)', helperMethod, helperArgs);
> 
> and avoid the need for "newTarget" ... sra@ designed the `js.call` and
> `js.statement` system to be pretty fast, it only parses once, and caches a
> factory function that quickly creates the AST.
> 
> That said, it's always a bit of taste thing, whether to use raw JS AST nodes
or
> js.call. I tend to use raw nodes if it feels more natural (especially if I'm
> just creating 1-2 nodes) or if we don't have JS AST parser support yet (e.g.
> there's no `js.method(...)` helper for making a JS.Method) and use
> js.call/js.statement for more complex things.

Done.

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2413: new JS.Identifier('a${counter++}');
On 2016/05/03 18:56:18, John Messerly wrote:
> this should be symbolized so it doesn't collide with methods:
> 
> 
> let super$foo = Symbol('super.foo');
> 
> class D extends B {
>   [super$foo](... args ... ) {
>     ... impl ...
>   }
> }
> 
> 
> we have a couple of places we generate Symbols, for example, how private
method
> names are handled, this could likely work in a similar way.
> 
> Also since "super$foo" will be a JS.TemporaryId, you won't need a counter --
> renaming will be handled automagically.

Done.

https://codereview.chromium.org/1948563002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:2435:
js.string('super\$${node.methodName.name}${_superHelpers.length}', "'");
On 2016/05/03 18:56:18, John Messerly wrote:
> based on comment above, "helperMethodName" will just be the JS.TemporaryId we
> create to hold the method symbol.

Done.

[Jennifer Messerly, 2016-05-05 23:50:48]

LGTM

https://codereview.chromium.org/1948563002/diff/60001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (left):

https://codereview.chromium.org/1948563002/diff/60001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:3540: /// Shared code for
[PrefixedIdentifier] and [PropertyAccess].
just curious, what happened to this comment?

We were confused early on by the distinction between the two nodes (it's only a
parse tree difference. For example, the same semantic operation can be in either
node just by adding parenthesis: `obj.name` vs `(obj).name`)

IMO we could improve/expand on the comment... seems bad to just remove doc
comments though.

(https://www.dartlang.org/effective-dart/documentation/#consider-writing-doc-c...)

https://codereview.chromium.org/1948563002/diff/60001/test/codegen/language/s...
File test/codegen/language/super_in_async2_test.dart (right):

https://codereview.chromium.org/1948563002/diff/60001/test/codegen/language/s...
test/codegen/language/super_in_async2_test.dart:22: Expect.equals(84, await new
B().foo());
fyi ... it's okay to combine tests too, especially more recent Dart lang tests
(like async_await_test) tend to do a lot more in a single test. Makes test runs
a good bit faster. That said this is good too, probably not worth changing now
:)

[Harry Terkelsen, 2016-05-05 23:58:40]

https://codereview.chromium.org/1948563002/diff/60001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (left):

https://codereview.chromium.org/1948563002/diff/60001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:3540: /// Shared code for
[PrefixedIdentifier] and [PropertyAccess].
On 2016/05/05 23:50:48, John Messerly wrote:
> just curious, what happened to this comment?
> 
> We were confused early on by the distinction between the two nodes (it's only
a
> parse tree difference. For example, the same semantic operation can be in
either
> node just by adding parenthesis: `obj.name` vs `(obj).name`)
> 
> IMO we could improve/expand on the comment... seems bad to just remove doc
> comments though.
> 
>
(https://www.dartlang.org/effective-dart/documentation/#consider-writing-doc-c...)

Oops, it got dropped when I was moving things around

https://codereview.chromium.org/1948563002/diff/60001/test/codegen/language/s...
File test/codegen/language/super_in_async2_test.dart (right):

https://codereview.chromium.org/1948563002/diff/60001/test/codegen/language/s...
test/codegen/language/super_in_async2_test.dart:22: Expect.equals(84, await new
B().foo());
On 2016/05/05 23:50:48, John Messerly wrote:
> fyi ... it's okay to combine tests too, especially more recent Dart lang tests
> (like async_await_test) tend to do a lot more in a single test. Makes test
runs
> a good bit faster. That said this is good too, probably not worth changing now
> :)

Yeah, agreed. Filed this: https://github.com/dart-lang/dev_compiler/issues/554

[Harry Terkelsen, 2016-05-06 00:03:14]

Description was changed from

==========
allow 'super' in async and finally blocks

Fixes #499
Fixes #528
==========

to

==========
allow 'super' in async and finally blocks

Fixes #499
Fixes #528

R=jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/63d3774000875b7b3aa7553c65c7...
==========

[Harry Terkelsen, 2016-05-06 00:03:16]

Committed patchset #5 (id:80001) manually as
63d3774000875b7b3aa7553c65c779cdcdc6271d (presubmit successful).