Make instance methods whose names collide with intercepted methods have the interceptor calling con…

Make instance methods whose names collide with intercepted methods have the interceptor calling convention (that is take the receiver as the first argument).
Committed: https://code.google.com/p/dart/source/detail?r=19310

[sra1, 2013-02-27 05:13:28]

It would be cool if the unused receiver parameters were renamed to '_'.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:788: * [name[.
Returns [:null:] if there is no class.
[name]

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1112: //
Getters and setters with suffixes will be generated dynamically.
How does the dynamic generator know whether to add a receiver.
e.g.  many Dart classes have a 'length' field.
Calling it via x.get$length(x) when it takes zero arguments will cause and arity
mismatch.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1160:
js.fun(args, js['this'][fieldName].assign('v')));
Don't you need to choose between 
   this.field = v
and
   receiver.field = v
?

If setter is added to an interceptor class, you need receiver.field.

Ditto for generateGetter.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:47: *
intercepted.
That could be several thousand names with the DOM.
How are the names used?
How is the variable initialized?

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1329: return
isInterceptorCall && inputs[0] != inputs[1];
Consider a.sb.add(123)
The optimization you mention does

t1 = a.sb
t2 = getInterceptor(t1);
t3 = t2.add$1(t1, 123);
-->
t1 = a.sb;
t3 = t1.add$1(t1, 123);

There is a further optimization of replacing argument t1 with junk if we know it
is not used:
t1 = a.sb;
t3 = t1.add$1(0, 123);
-->
t3 = a.sb.add$1(0, 123);

We can substitute junk for the explicit receiver if the function set for add$1
at that point uniformly ignores the explicit receiver parameter*.

However, the predicate you wrote will be confused by this (a.sb != 0).
I suggest inspecting inputs[0] rather than rely on this fragile invariant. 

* If we make mixins work by copying compiled methods into the appropriate
prototypes, some methods will use their explicit receiver, even though they are
methods on a plain class, because they are also copied onto an interceptor class
- I expect there will be a lot of this eventually for the DOM classes.

[ngeoffray, 2013-02-28 10:39:42]

Thanks Stephen. PTAL.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:788: * [name[.
Returns [:null:] if there is no class.
On 2013/02/27 05:13:28, sra1 wrote:
> [name]

Done.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1112: //
Getters and setters with suffixes will be generated dynamically.
On 2013/02/27 05:13:28, sra1 wrote:
> How does the dynamic generator know whether to add a receiver.
> e.g.  many Dart classes have a 'length' field.
> Calling it via x.get$length(x) when it takes zero arguments will cause and
arity
> mismatch.

Yes, nice catch. The dynamic generator does not deal with that. We could make it
know it (but that requires to emit the set of intercepted names), or explicitly
generate the getter, or expect the argument mismatch will not have big
performance impact. I chose the last one.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1160:
js.fun(args, js['this'][fieldName].assign('v')));
On 2013/02/27 05:13:28, sra1 wrote:
> Don't you need to choose between 
>    this.field = v
> and
>    receiver.field = v
> ?
> 
> If setter is added to an interceptor class, you need receiver.field.
> 
> Ditto for generateGetter.

Good catch again. We never emit implicit getters and setters for interceptor
classes (they don't have fields). I added an assertion.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:47: *
intercepted.
On 2013/02/27 05:13:28, sra1 wrote:
> That could be several thousand names with the DOM.
> How are the names used?
> How is the variable initialized?

They're used in InvocationMirror.invokeOn, where we check if the name to invoke
is in that set. We need it to know whether we should pass the receiver as the
first parameter.

invokeOn being a feature we don't encourage when deploying, I would not worry
about it.

It's initialized in the emitter, with the set of names collected by the backend.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:15: : super(const
SourceString('receiver'),
Changed the name to '_' in case of a non-interceptor class.

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12330135/diff/2002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1329: return
isInterceptorCall && inputs[0] != inputs[1];
On 2013/02/27 05:13:28, sra1 wrote:
> Consider a.sb.add(123)
> The optimization you mention does
> 
> t1 = a.sb
> t2 = getInterceptor(t1);
> t3 = t2.add$1(t1, 123);
> -->
> t1 = a.sb;
> t3 = t1.add$1(t1, 123);
> 
> There is a further optimization of replacing argument t1 with junk if we know
it
> is not used:
> t1 = a.sb;
> t3 = t1.add$1(0, 123);
> -->
> t3 = a.sb.add$1(0, 123);
> 
> We can substitute junk for the explicit receiver if the function set for add$1
> at that point uniformly ignores the explicit receiver parameter*.
> 
> However, the predicate you wrote will be confused by this (a.sb != 0).
> I suggest inspecting inputs[0] rather than rely on this fragile invariant. 
> 
> * If we make mixins work by copying compiled methods into the appropriate
> prototypes, some methods will use their explicit receiver, even though they
are
> methods on a plain class, because they are also copied onto an interceptor
class
> - I expect there will be a lot of this eventually for the DOM classes. 
> 

inputs[0] can either be a call to getInterceptor or a constant. So decoding it a
bit annoying. I added a TODO to stop relying on this fragile invariant. I will
do the optimization of replacing with 0 in a following CL.

[sra1, 2013-02-28 23:37:57]

lgtm

I'd like to make progress on top of this CL

[kasperl, 2013-03-01 09:06:47]

Given Stephen's review, I didn't review this too carefully but the general idea
and implementation LGTM.

https://codereview.chromium.org/12330135/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12330135/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:802:
interceptedClassesCache[name] = result;
So if we're not using the trick of storing null in the interceptedClassesCache
anymore, we could simplify the cache check in lines 793-795. It seems better to
fetch result from interceptedClassesCache and if it isn't null, then we return
it. No need to use containsKey.

https://codereview.chromium.org/12330135/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12330135/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:107: if (JS('int',
'#.indexOf(#)', interceptedNames, name) == -1) {
So we emit the interceptedNames as a JS array with strings? Wouldn't it be
better to build an object with the interceptedNames as property names?

[ngeoffray, 2013-03-01 10:46:56]

Thanks Kasper and Stephen.

https://codereview.chromium.org/12330135/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12330135/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:802:
interceptedClassesCache[name] = result;
On 2013/03/01 09:06:47, kasperl wrote:
> So if we're not using the trick of storing null in the interceptedClassesCache
> anymore, we could simplify the cache check in lines 793-795. It seems better
to
> fetch result from interceptedClassesCache and if it isn't null, then we return
> it. No need to use containsKey.

Done.

https://codereview.chromium.org/12330135/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12330135/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:107: if (JS('int',
'#.indexOf(#)', interceptedNames, name) == -1) {
On 2013/03/01 09:06:47, kasperl wrote:
> So we emit the interceptedNames as a JS array with strings? Wouldn't it be
> better to build an object with the interceptedNames as property names?

As discussed, added a TODO.

[ngeoffray, 2013-03-01 13:28:26]

Committed patchset #7 manually as r19310 (presubmit successful).