Fix issue 10216. If the type mask is a subtype, and its base mixes in class A, it should consider a…

Fix issue 10216. If the type mask is a subtype, and its base mixes in class A, it should consider an element of class A as potential runtime targets.

R=kasperl@google.com

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

[kasperl, 2013-04-29 09:46:46]

https://codereview.chromium.org/14268020/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/types/type_mask.dart (right):

https://codereview.chromium.org/14268020/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/type_mask.dart:448: if
(cls.isSubclassOf(other)
I'd like to structure this more like the other (exact and subclass) cases.
Something like:

   return hasElementIn(self, selector, element)
       || ...
       || ...;

It looks like an optimization to check if self is subclass -- and it seems
pretty complicated to get right. I think this is broken if you have a superclass
that mixes in a mixin application of other.

[ngeoffray, 2013-04-29 10:18:11]

PTAL. I added a test for the case where a class used as mixin also mixes in
another class.

https://codereview.chromium.org/14268020/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/types/type_mask.dart (right):

https://codereview.chromium.org/14268020/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/type_mask.dart:448: if
(cls.isSubclassOf(other)
On 2013/04/29 09:46:47, kasperl wrote:
> I'd like to structure this more like the other (exact and subclass) cases.
> Something like:
> 
>    return hasElementIn(self, selector, element)
>        || ...
>        || ...;
> 
> It looks like an optimization to check if self is subclass -- and it seems
> pretty complicated to get right. I think this is broken if you have a
superclass
> that mixes in a mixin application of other.

As discussed, I remove what looked like an optimization and now always check for
hasElementIn.

[kasperl, 2013-04-29 10:27:21]

LGTM.

https://codereview.chromium.org/14268020/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/14268020/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/world.dart:139: // Returns whether a
superclass of [cls] mixes in [other].
Get rid of this helper again?

https://codereview.chromium.org/14268020/diff/4001/tests/language/mixin_issue...
File tests/language/mixin_issue10216_2_test.dart (right):

https://codereview.chromium.org/14268020/diff/4001/tests/language/mixin_issue...
tests/language/mixin_issue10216_2_test.dart:19: makeM2().foo();
Check that it is 42?

[ngeoffray, 2013-04-29 10:37:48]

Thanks Kasper.

https://codereview.chromium.org/14268020/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/14268020/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/world.dart:139: // Returns whether a
superclass of [cls] mixes in [other].
On 2013/04/29 10:27:21, kasperl wrote:
> Get rid of this helper again?

Done.

https://codereview.chromium.org/14268020/diff/4001/tests/language/mixin_issue...
File tests/language/mixin_issue10216_2_test.dart (right):

https://codereview.chromium.org/14268020/diff/4001/tests/language/mixin_issue...
tests/language/mixin_issue10216_2_test.dart:19: makeM2().foo();
On 2013/04/29 10:27:21, kasperl wrote:
> Check that it is 42?

Done.

[ngeoffray, 2013-04-29 10:58:49]

Committed patchset #3 manually as r22111 (presubmit successful).