Support intercepted getters, setters and index operations.

Support intercepted getters, setters and index operations.

R=asgerf@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43471

[asgerf, 2015-01-21 10:21:06]

https://codereview.chromium.org/861093002/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/codegen/glue.dart (right):

https://codereview.chromium.org/861093002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/glue.dart:32: }
It seems to me the JsCallSelector is never created.

https://codereview.chromium.org/861093002/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart (right):

https://codereview.chromium.org/861093002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart:46: Selector
selector = new Selector.call(name, null, 1);
Was this supposed to be the JsCallSelector?

[asgerf, 2015-01-21 14:03:39]

LGTM

https://codereview.chromium.org/861093002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/unsugar.dart (right):

https://codereview.chromium.org/861093002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:52: // TODO(karlklose): 
should we rewrite all selectors?
I'm a bit torn on this, but I lean toward yes.

JsCallSelector could be another one of those things that require special
treatment in some cases, but you constantly forget about them unless there is a
test case to remind you of it.

If the selector is everywhere, it reveals itself right away if you forgot to
treat it properly.

[karlklose, 2015-01-22 07:25:23]

Thanks for the review, Asger!

After merging with your changes to unsugar, I saw that the assertion in the
constructor of InvokeMethod is no longer an invariant, if we modify the
arguments of the call after construction.

I have uploaded a different version, which introduces a calling convention
abstraction instead of a new selector. The idea is to have calling conventions
also control the actual argument assignment for the generated code.

Going forward, they will also support leaving out arguments to a call (for
example, the type arguments to a factory, if they are all dynamic).

[asgerf, 2015-01-22 11:29:54]

So we don't update the parameter list to ir.FunctionDefinition, but instead use
a calling convention that handles the mapping?

It would be nicer if we could just update the parameter list for intercepted
function so they stay in sync. So we are doing this because we can't mutate the
element accordingly?

Overall I'm a bit confused about the motivation behind calling conventions. So
far it's just a way to satisfy an assertion, but I think there's more to it than
that.

I'd like Kevin's input on this.

[karlklose, 2015-01-22 12:08:08]

On 2015/01/22 11:29:54, asgerf wrote:
> So we don't update the parameter list to ir.FunctionDefinition, but instead
use
> a calling convention that handles the mapping?
> 
> It would be nicer if we could just update the parameter list for intercepted
> function so they stay in sync. So we are doing this because we can't mutate
the
> element accordingly?
> 
> Overall I'm a bit confused about the motivation behind calling conventions. So
> far it's just a way to satisfy an assertion, but I think there's more to it
than
> that.
> 
> I'd like Kevin's input on this.

I am not sure what this has to do with ir.FunctionDefinitions.

We cannot change the element model, because it represents the semantic model of
the program. What we could do is decouple the whole IR from the element model
and move it to the emitter model, where we could change the signatures of
functions accordingly.

[asgerf, 2015-01-22 16:39:08]

On 2015/01/22 12:08:08, karlklose wrote:
> On 2015/01/22 11:29:54, asgerf wrote:
> > So we don't update the parameter list to ir.FunctionDefinition, but instead
> use
> > a calling convention that handles the mapping?
> > 
> > It would be nicer if we could just update the parameter list for intercepted
> > function so they stay in sync. So we are doing this because we can't mutate
> the
> > element accordingly?
> > 
> > Overall I'm a bit confused about the motivation behind calling conventions.
So
> > far it's just a way to satisfy an assertion, but I think there's more to it
> than
> > that.
> > 
> > I'd like Kevin's input on this.
> 
> I am not sure what this has to do with ir.FunctionDefinitions.
> 
> We cannot change the element model, because it represents the semantic model
of
> the program. What we could do is decouple the whole IR from the element model
> and move it to the emitter model, where we could change the signatures of
> functions accordingly.

The way I see it, we rewrite call and functions like this:

Before:

    function foo(x, y) {}
    foo(x, y);

After:

    function foo(x, y) {}
    foo(self, x, y);

The function `foo` still expects two arguments, but the call now provides three.

Calling conventions are there to bridge the discrepancy, yes?

Or is it just a bug that we don't add extra parameters to ir.FunctionDefinitions
for intercepted methods?

I just want to know if my intuition is correct so far.

[karlklose, 2015-02-03 09:57:14]

On 2015/01/22 16:39:08, asgerf wrote:
> On 2015/01/22 12:08:08, karlklose wrote:
> > On 2015/01/22 11:29:54, asgerf wrote:
> > > So we don't update the parameter list to ir.FunctionDefinition, but
instead
> > use
> > > a calling convention that handles the mapping?
> > > 
> > > It would be nicer if we could just update the parameter list for
intercepted
> > > function so they stay in sync. So we are doing this because we can't
mutate
> > the
> > > element accordingly?
> > > 
> > > Overall I'm a bit confused about the motivation behind calling
conventions.
> So
> > > far it's just a way to satisfy an assertion, but I think there's more to
it
> > than
> > > that.
> > > 
> > > I'd like Kevin's input on this.
> > 
> > I am not sure what this has to do with ir.FunctionDefinitions.
> > 
> > We cannot change the element model, because it represents the semantic model
> of
> > the program. What we could do is decouple the whole IR from the element
model
> > and move it to the emitter model, where we could change the signatures of
> > functions accordingly.
> 
> The way I see it, we rewrite call and functions like this:
> 
> Before:
> 
>     function foo(x, y) {}
>     foo(x, y);
> 
> After:
> 
>     function foo(x, y) {}
>     foo(self, x, y);
> 
> The function `foo` still expects two arguments, but the call now provides
three.
> 
> Calling conventions are there to bridge the discrepancy, yes?
> 
> Or is it just a bug that we don't add extra parameters to
ir.FunctionDefinitions
> for intercepted methods?
> 
> I just want to know if my intuition is correct so far.

It is just a bug, I'll have a look.

[karlklose, 2015-02-03 11:57:02]

This CL does not support compiling functions that need interceptor support,
since it was not necessary before you landed the support for creating objects.

I added a bailout and would like to add the special compilation for these
methods in a different CL. What do you think?

[asgerf, 2015-02-03 12:56:32]

OK, LGTM then.

[karlklose, 2015-02-04 13:18:40]

Small update: one of the tests overwrites an intercepted operator, so I had to
disable that test for the moment.

Also, we do not have to check for intercepted names for static functions.

[karlklose, 2015-02-04 15:31:37]

Committed patchset #6 (id:100001) manually as 43471 (presubmit successful).