Refactor qualified send sets.

Refactor qualified send sets.

Closes #23795

R=floitsch@google.com

Committed: https://github.com/dart-lang/sdk/commit/9ce4da5e1a3f9d06f617d40a1d642c0397f2ec11

[Johnni Winther, 2015-08-20 13:44:28]

johnniwinther@google.com changed reviewers:
+ floitsch@google.com

[Johnni Winther, 2015-08-20 13:44:28]

Main changes in members.dart; the rest are mainly side effects.

[floitsch, 2015-08-20 16:28:35]

LGTM.

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/resolu...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/resolu...
pkg/compiler/lib/src/resolution/members.dart:2440: if (text == 'this') {
That feels like a weird check.
Is it for captured 'this' variables?
Is there no cleaner way to do this?

[Johnni Winther, 2015-08-21 07:17:36]

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/resolu...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/resolu...
pkg/compiler/lib/src/resolution/members.dart:2440: if (text == 'this') {
On 2015/08/20 16:28:34, floitsch wrote:
> That feels like a weird check.
> Is it for captured 'this' variables?
> Is there no cleaner way to do this?

This is for cases like 'C.this' for which there is no easier way.

[Johnni Winther, 2015-08-21 07:34:55]

Committed patchset #1 (id:1) manually as
9ce4da5e1a3f9d06f617d40a1d642c0397f2ec11 (presubmit successful).

[floitsch, 2015-08-21 09:27:50]

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/resolu...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/resolu...
pkg/compiler/lib/src/resolution/members.dart:2440: if (text == 'this') {
On 2015/08/21 07:17:36, Johnni Winther wrote:
> On 2015/08/20 16:28:34, floitsch wrote:
> > That feels like a weird check.
> > Is it for captured 'this' variables?
> > Is there no cleaner way to do this?
> 
> This is for cases like 'C.this' for which there is no easier way.

Ah...
Didn't realize this was the selector and not the receiver.

[Siggi Cherem (dart-lang), 2015-08-24 21:25:17]

sigmund@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-08-24 21:25:17]

lgtm Thanks Johnni!


Just to confirm, I saw that you were about to delete computeSendStructure and
computeAccessSemantics in send_resolver, but had to revert. If the plan is still
to delete, then we should be good. If not, we might need to update the
isConditional code in computeAccessSemantics to match the new semantics (here
https://github.com/dart-lang/sdk/blob/73a49a5ddbf7bc989adfd07668db1d29c98f91f...).

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:524: (send.isConditional &&
!element.isStatic);
I think it should be fine to delete this whole line, if !element.isStatic, one
of the other two conditions will be true already.

[Johnni Winther, 2015-08-25 07:25:52]

On 2015/08/24 21:25:17, Siggi Cherem (dart-lang) wrote:
> lgtm Thanks Johnni!
> 
> 
> Just to confirm, I saw that you were about to delete computeSendStructure and
> computeAccessSemantics in send_resolver, but had to revert. If the plan is
still
> to delete, then we should be good. If not, we might need to update the
> isConditional code in computeAccessSemantics to match the new semantics (here
>
https://github.com/dart-lang/sdk/blob/73a49a5ddbf7bc989adfd07668db1d29c98f91f...).
> 
SendResolver.computeSendStructure _is_ going away!

[Johnni Winther, 2015-08-25 07:25:58]

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:524: (send.isConditional &&
!element.isStatic);
On 2015/08/24 21:25:17, Siggi Cherem (dart-lang) wrote:
> I think it should be fine to delete this whole line, if !element.isStatic, one
> of the other two conditions will be true already.

This makes C?.foo _not_ an instance-send.

[Siggi Cherem (dart-lang), 2015-08-25 15:48:07]

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:524: (send.isConditional &&
!element.isStatic);
On 2015/08/25 07:25:58, Johnni Winther wrote:
> On 2015/08/24 21:25:17, Siggi Cherem (dart-lang) wrote:
> > I think it should be fine to delete this whole line, if !element.isStatic,
one
> > of the other two conditions will be true already.
> 
> This makes C?.foo _not_ an instance-send.

I'm probably missing something - isInstanceMethod and isInstanceField are always
false when element.isStatic. So it seems to me that this condition is equivalent
to:

  return isInstanceMethod(element) || isInstnaceField(element);

(when I added `send.isConditional` it only added it because C?.foo used to be an
instance-send, now that this is no longer true, I thought we wouldn't need the
special case for send.isConditional altogether)

[Johnni Winther, 2015-08-26 07:18:36]

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1293953006/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:524: (send.isConditional &&
!element.isStatic);
On 2015/08/25 15:48:06, Siggi Cherem (dart-lang) wrote:
> On 2015/08/25 07:25:58, Johnni Winther wrote:
> > On 2015/08/24 21:25:17, Siggi Cherem (dart-lang) wrote:
> > > I think it should be fine to delete this whole line, if !element.isStatic,
> one
> > > of the other two conditions will be true already.
> > 
> > This makes C?.foo _not_ an instance-send.
> 
> I'm probably missing something - isInstanceMethod and isInstanceField are
always
> false when element.isStatic. So it seems to me that this condition is
equivalent
> to:
> 
>   return isInstanceMethod(element) || isInstnaceField(element);
> 
> (when I added `send.isConditional` it only added it because C?.foo used to be
an
> instance-send, now that this is no longer true, I thought we wouldn't need the
> special case for send.isConditional altogether)

Ahh. I didn't go into the cases of isInstanceMethod and isInstanceField. In any
case, methods like this should be removed and replaced by predicates on elements
if necessary. Not only is the reasoning often unclear, the implementation is
also based on a closed world of elements.