Decouple TypeMask from ClassElement

Decouple TypeMask from ClassElement

- by typing FlatTypeMask.base with Entity
- moving needsNoSuchMethodHandling to ClosedWorld
- and testing needsNoSuchMethodHandling

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/74b74a13f70dac3fab2bf1df164e94d9371f5f41

[Johnni Winther, 2016-10-14 12:08:58]

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

[Siggi Cherem (dart-lang), 2016-10-14 16:49:57]

lgtm!

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/flat_type_mask.dart (right):

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/flat_type_mask.dart:39: bool _validateBase(Element
element) => element.isDeclaration;
do we want to validate also that it is a ClassElement not just an Element?

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/flat_type_mask.dart:582: ClassElement enclosing =
result.enclosingClass.declaration;
what do you get in enclosingClass when it is not a declaration here?

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:332: }
we can also replace the 3 lines above with:

  var cachedMasks = _canonicalizedTypeMasks[flags] ??= <ClassElement,
TypeMask>{};

or combine with the one below:

  return (_canonicalizedTypeMasks[flags] ??= <ClassElement, TypeMask>{})
      .putIfAbsent(base, createMask);

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:334: base, () => new FlatTypeMask.internal(base,
flags));
use createMask here

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:697: current = current.superclass;
I assume we don't need to assert that superclass is a declaration, correct?

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:1144: enum ClassMask {
I'm not convinced about adding this enum. Since it is only used in one query,
what if we remove the enum and use a couple boolean named arguments there? For
example:

  bool needsNoSuchMethod(ClassElement cls, Selector selector,
     {includeSubclasses: false, includeSubtypes: false});

If we want to have the enum, I'd suggest to rename it to `ClassQuery` or
something else (the word Mask seems to be there only because it is coupled
somehow with TypeMask)

https://codereview.chromium.org/2420073002/diff/1/tests/compiler/dart2js/need...
File tests/compiler/dart2js/needs_no_such_method_test.dart (right):

https://codereview.chromium.org/2420073002/diff/1/tests/compiler/dart2js/need...
tests/compiler/dart2js/needs_no_such_method_test.dart:37: ClassElement
Superclass, Subclass, Subtype;
nit: I'm torn on this, I think I'm leaning towards renaming to use lower-case
variable names

[Johnni Winther, 2016-10-17 07:47:27]

Description was changed from

==========
Decouple TypeMask from ClassElement

- by typing FlatTypeMask.base with Entity
- moving needsNoSuchMethodHandling to ClosedWorld
- and testing needsNoSuchMethodHandling
==========

to

==========
Decouple TypeMask from ClassElement

- by typing FlatTypeMask.base with Entity
- moving needsNoSuchMethodHandling to ClosedWorld
- and testing needsNoSuchMethodHandling

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/74b74a13f70dac3fab2bf1df164e94d9371f5f41
==========

[Johnni Winther, 2016-10-17 07:47:28]

Committed patchset #2 (id:20001) manually as
74b74a13f70dac3fab2bf1df164e94d9371f5f41 (presubmit successful).

[Johnni Winther, 2016-10-17 07:57:40]

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/flat_type_mask.dart (right):

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/flat_type_mask.dart:39: bool _validateBase(Element
element) => element.isDeclaration;
On 2016/10/14 16:49:57, Siggi Cherem (dart-lang) wrote:
> do we want to validate also that it is a ClassElement not just an Element?

Done. I just wanted to remove all mentioning of ClassElement but function is
temporary anyway. The ClosedWorld should probably create the valid base entity.

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/flat_type_mask.dart:582: ClassElement enclosing =
result.enclosingClass.declaration;
On 2016/10/14 16:49:57, Siggi Cherem (dart-lang) wrote:
> what do you get in enclosingClass when it is not a declaration here? 

If target is an injected member (non-patch member in a patch class) enclosing
would be the implementation and _not_ the declaration element.

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:332: }
On 2016/10/14 16:49:57, Siggi Cherem (dart-lang) wrote:
> we can also replace the 3 lines above with:
> 
>   var cachedMasks = _canonicalizedTypeMasks[flags] ??= <ClassElement,
> TypeMask>{};
> 
> or combine with the one below:
> 
>   return (_canonicalizedTypeMasks[flags] ??= <ClassElement, TypeMask>{})
>       .putIfAbsent(base, createMask);

Done(-ish).

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:334: base, () => new FlatTypeMask.internal(base,
flags));
On 2016/10/14 16:49:57, Siggi Cherem (dart-lang) wrote:
> use createMask here

Done.

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:697: current = current.superclass;
On 2016/10/14 16:49:57, Siggi Cherem (dart-lang) wrote:
> I assume we don't need to assert that superclass is a declaration, correct?

Correct.

https://codereview.chromium.org/2420073002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:1144: enum ClassMask {
On 2016/10/14 16:49:57, Siggi Cherem (dart-lang) wrote:
> I'm not convinced about adding this enum. Since it is only used in one query,
> what if we remove the enum and use a couple boolean named arguments there? For
> example:
> 
>   bool needsNoSuchMethod(ClassElement cls, Selector selector,
>      {includeSubclasses: false, includeSubtypes: false});
> 
> If we want to have the enum, I'd suggest to rename it to `ClassQuery` or
> something else (the word Mask seems to be there only because it is coupled
> somehow with TypeMask)

Renamed to `ClassQuery`. We'll need it more function moved from xTypeMask to
ClosedWorld.

https://codereview.chromium.org/2420073002/diff/1/tests/compiler/dart2js/need...
File tests/compiler/dart2js/needs_no_such_method_test.dart (right):

https://codereview.chromium.org/2420073002/diff/1/tests/compiler/dart2js/need...
tests/compiler/dart2js/needs_no_such_method_test.dart:37: ClassElement
Superclass, Subclass, Subtype;
On 2016/10/14 16:49:57, Siggi Cherem (dart-lang) wrote:
> nit: I'm torn on this, I think I'm leaning towards renaming to use lower-case
> variable names

Done.