Implement complex super assignment.

Implement complex super assignment.
Committed: https://code.google.com/p/dart/source/detail?r=19452

[kasperl, 2013-03-05 07:32:36]

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:762: Element
lookupSelector(Selector selector, {bool superLookup});
Maybe it's time to refactor this a bit instead? I think we should go for
something like this:

   lookupMember(Selector selector);
   lookupMemberByName(SourceString name);
   
   lookupSuperMember(Selector selector);
   lookupSuperMemberByName(SourceString name);   

and try to get rid of lookupSuperMemberInLibrary and lookupSuperInterfaceMember.
Ideally, we could also get rid of some of the ...ByName variants.

If you want to take a small step in the right direction, I would leave
lookupSelector as it is and add a lookupSuperSelector. Inside the
implementation, it would be fine to have a single method that does the lookup
and give it a mandatory boolean argument to indicate if you want to start in the
superclass.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1537:
(superLookup || !identical(this, current))) {
Hmm. It sounds like superLookup means something different than I expected in
this context. That's a bad sign I guess. It's basically a skip-statics flag?

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2008: target =
currentClass.superclass.lookupSelector(
I think this would be easier to read if you did something like:

   target = currentClass.lookupSuperMember(selector);

Manually going to the superclass everywhere seems a bit fishy.

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

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3061: if
(selector.isGetter()) {
Could this logic be moved to the enqueuer? If we had something like
registerSelectorUse(selector) wouldn't that be enough? Maybe that would be
useful in other places too.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3599: if
(node.isSuperCall) {
Think about if some of this could be shared with the non-super variant of the
same thing.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3600: HInstruction
toPush;
I'd call this the value/result or something like that. That we happen to want to
push that at the end seems like a detail.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3604: HInstruction
target = new HStatic(element);
Is any of this refactorable? The resolved target/getterTarget handling seems to
share some structure.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3627: Link<Node>
arguments = node.arguments;
Add a comment that explains what you're dealing with here.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3635: HInstruction
getterInstruction;
Could this (the computation of the getterInstruction) be put in a separate
method?

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3646: HInstruction
rhs;
Do we have code like this elsewhere in the builder?

[ngeoffray, 2013-03-05 08:46:25]

Thanks Kasper. PTAL.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:762: Element
lookupSelector(Selector selector, {bool superLookup});
On 2013/03/05 07:32:36, kasperl wrote:
> Maybe it's time to refactor this a bit instead? I think we should go for
> something like this:
> 
>    lookupMember(Selector selector);
>    lookupMemberByName(SourceString name);
>    
>    lookupSuperMember(Selector selector);
>    lookupSuperMemberByName(SourceString name);   
> 
> and try to get rid of lookupSuperMemberInLibrary and
lookupSuperInterfaceMember.
> Ideally, we could also get rid of some of the ...ByName variants.
> 
> If you want to take a small step in the right direction, I would leave
> lookupSelector as it is and add a lookupSuperSelector. Inside the
> implementation, it would be fine to have a single method that does the lookup
> and give it a mandatory boolean argument to indicate if you want to start in
the
> superclass.

I've added a lookupSuperSelector, and an internalLookupSelector that takes an
arbitrary argument.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1537:
(superLookup || !identical(this, current))) {
On 2013/03/05 07:32:36, kasperl wrote:
> Hmm. It sounds like superLookup means something different than I expected in
> this context. That's a bad sign I guess. It's basically a skip-statics flag?

The lookup rules are different when you're looking up for a super method. The
static members should not be considered. When it's a lookup on a class, only
static members of that class are considered (as it is currently implemented).

I could call it 'skipStatic', but that sounds a bit general, and does not
express what it is implementing (that is lookup rules for super call).

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2008: target =
currentClass.superclass.lookupSelector(
On 2013/03/05 07:32:36, kasperl wrote:
> I think this would be easier to read if you did something like:
> 
>    target = currentClass.lookupSuperMember(selector);
> 
> Manually going to the superclass everywhere seems a bit fishy.

Done.

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

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3061: if
(selector.isGetter()) {
On 2013/03/05 07:32:36, kasperl wrote:
> Could this logic be moved to the enqueuer? If we had something like
> registerSelectorUse(selector) wouldn't that be enough? Maybe that would be
> useful in other places too.

Done.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3600: HInstruction
toPush;
On 2013/03/05 07:32:36, kasperl wrote:
> I'd call this the value/result or something like that. That we happen to want
to
> push that at the end seems like a detail.

Done.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3604: HInstruction
target = new HStatic(element);
On 2013/03/05 07:32:36, kasperl wrote:
> Is any of this refactorable? The resolved target/getterTarget handling seems
to
> share some structure.

Done.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3627: Link<Node>
arguments = node.arguments;
On 2013/03/05 07:32:36, kasperl wrote:
> Add a comment that explains what you're dealing with here.

Done.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3635: HInstruction
getterInstruction;
On 2013/03/05 07:32:36, kasperl wrote:
> Could this (the computation of the getterInstruction) be put in a separate
> method?

Done.

https://codereview.chromium.org/12374094/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3646: HInstruction
rhs;
On 2013/03/05 07:32:36, kasperl wrote:
> Do we have code like this elsewhere in the builder?

Yes, refactored.

[kasperl, 2013-03-05 08:57:42]

LGTM.

https://codereview.chromium.org/12374094/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/12374094/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1536: for
(ClassElement current = isSuperLookup ? getEnclosingClass() : this;
When is the enclosing class of a class element not the class itself?

https://codereview.chromium.org/12374094/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1544: if
(member.modifiers.isStatic() && !identical(this, current)) {
Fits on one line?

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

https://codereview.chromium.org/12374094/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3606: 
Too many newlines.

https://codereview.chromium.org/12374094/diff/12001/tests/language/language.s...
File tests/language/language.status (right):

https://codereview.chromium.org/12374094/diff/12001/tests/language/language.s...
tests/language/language.status:69: super_getter_setter_test: Fail
File bugs?

[ahe, 2013-03-05 09:08:41]

I like how this ended up.

[ngeoffray, 2013-03-05 09:09:40]

Thanks Kasper

https://codereview.chromium.org/12374094/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/12374094/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1536: for
(ClassElement current = isSuperLookup ? getEnclosingClass() : this;
On 2013/03/05 08:57:42, kasperl wrote:
> When is the enclosing class of a class element not the class itself?

That was a braino caught by tests: should have been 'superclass'.

https://codereview.chromium.org/12374094/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1544: if
(member.modifiers.isStatic() && !identical(this, current)) {
On 2013/03/05 08:57:42, kasperl wrote:
> Fits on one line?

Done.

https://codereview.chromium.org/12374094/diff/12001/tests/language/language.s...
File tests/language/language.status (right):

https://codereview.chromium.org/12374094/diff/12001/tests/language/language.s...
tests/language/language.status:69: super_getter_setter_test: Fail
On 2013/03/05 08:57:42, kasperl wrote:
> File bugs?

Was waiting for an LGTM. Done: 
https://code.google.com/p/dart/issues/detail?id=8917
https://code.google.com/p/dart/issues/detail?id=8918

[ngeoffray, 2013-03-05 09:29:16]

Committed patchset #5 manually as r19452 (presubmit successful).