Partial fix for call methods #542

Partial fix for call methods #542

This fixes the generated code to support these. Subtype checks are not fixed yet.

R=vsm@google.com

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

[Jennifer Messerly, 2016-06-14 23:22:11]

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

[Jennifer Messerly, 2016-06-14 23:22:11]

Some progress towards https://github.com/dart-lang/dev_compiler/issues/542.

No promises on performance of these things :)

[Jennifer Messerly, 2016-06-15 00:25:10]

Found a bug -- it seems like "dart.bind" will incorrectly associate a function
type with what should look like an interface type. So we should use JS `bind`
directly. Working on a fix.

[Jennifer Messerly, 2016-06-15 00:31:14]

On 2016/06/15 00:25:10, John Messerly wrote:
> Found a bug -- it seems like "dart.bind" will incorrectly associate a function
> type with what should look like an interface type. So we should use JS `bind`
> directly. Working on a fix.

Above is now fixed.

[vsm, 2016-06-15 15:23:25]

lgtm

https://codereview.chromium.org/2069903002/diff/20001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2069903002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:714: !member.isSetter) {
Per spec / VM, 'call' could be a getter or field.  Are you thinking about this
as:
(a) TODO
(b) something we should explicitly not support
?

[Jennifer Messerly, 2016-06-15 16:50:32]

Thanks!

https://codereview.chromium.org/2069903002/diff/20001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2069903002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:714: !member.isSetter) {
On 2016/06/15 15:23:25, vsm wrote:
> Per spec / VM, 'call' could be a getter or field.  Are you thinking about this
> as:
> (a) TODO
> (b) something we should explicitly not support
> ?

Oh crazy... it felt like some of the tests seem to expect the opposite, which I
why I added this check (it makes at least one pass). Unclear what's going on
there. Yeah I'll leave it as a TODO because A) seems like something no one has
or will ever use, and B) it would require yet another special case in codegen
for these guys =(

[Jennifer Messerly, 2016-06-15 16:50:33]

Thanks!

[Jennifer Messerly, 2016-06-15 19:17:01]

PTAL. This should address most of those corner cases.

There are still some open issues, I will add a comment to 542.

[Jennifer Messerly, 2016-06-15 19:36:26]

On 2016/06/15 19:17:01, John Messerly wrote:
> PTAL. This should address most of those corner cases.
> 
> There are still some open issues, I will add a comment to 542.

Comment is here:
https://github.com/dart-lang/dev_compiler/issues/542#issuecomment-226293448

[vsm, 2016-06-15 19:52:26]

nice!

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

https://codereview.chromium.org/2069903002/diff/60001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:800: dynamicCall = callMethod.returnType
is! FunctionType;
Do we need a dcall here if one of the param types is dynamic?  _isDynamicCall in
the analyzer checker does this.

[Jennifer Messerly, 2016-06-15 20:16:51]

Yeah, good catch I will remove that unnecessary dcall

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

https://codereview.chromium.org/2069903002/diff/60001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:800: dynamicCall = callMethod.returnType
is! FunctionType;
On 2016/06/15 19:52:26, vsm wrote:
> Do we need a dcall here if one of the param types is dynamic?  _isDynamicCall
in
> the analyzer checker does this.

On 2016/06/15 19:52:26, vsm wrote:
> Do we need a dcall here if one of the param types is dynamic?  _isDynamicCall
in
> the analyzer checker does this.

I don't think so. Those dcalls will be handled by the caller:

    typedef F(x);
    F f = new C();
    f(42); // dcall here


Actually, now that I think about this, I'm not sure what case will use this ...
I was thinking about test cases like:

    class C {
       get call => ...;
    }
    Int2Int f = new C();

... but that will fail at runtime to cast to Int2Int. And if we replace with
Function (or dynamic):

    class C {
       Function get call => ...;
    }
    Function f = new C();
    f();

... then it should be a caller side dcall. Same with the noSuchMethod case. We
will not allow you to assign it anywhere a typed function is expected, so only
the typed path needs to be supported.

Yeah, I'll revert the "dynamicCall" support. And change "callMethod" back to
"isCallable"...

[Jennifer Messerly, 2016-06-16 22:58:44]

Fixed! Landing.

[Jennifer Messerly, 2016-06-16 23:38:29]

Description was changed from

==========
Partial fix for call methods #542

This fixes the generated code to support these. Subtype checks are not fixed
yet.
==========

to

==========
Partial fix for call methods #542

This fixes the generated code to support these. Subtype checks are not fixed
yet.

R=vsm@google.com

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

[Jennifer Messerly, 2016-06-16 23:38:32]

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