Normalize type masks to use the least upper instantiated subclass/type.

Normalize type masks to use the least upper instantiated subclass/type.

BUG=
R=sigurdm@google.com

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

Reverted: https://github.com/dart-lang/sdk/commit/c1b102a94dbc6a7a0ffe022d4bef26f9390a90f2

Committed: https://github.com/dart-lang/sdk/commit/7b458eb1d62fe2ef896ed60e0a463f95bf0b0890

[Johnni Winther, 2015-11-03 12:20:25]

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

[Johnni Winther, 2015-11-03 12:20:25]

Main changes in universe/class_set.dart, world.dart, and types/type_mask.dart

[sigurdm, 2015-11-03 13:06:50]

LGTM

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:3250: new
TypeMask.nonNullSubtype(closureClassElement, compiler.world);
?

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:5081: return new
TypeMask.nonNullExact(cls.thisType.element, compiler.world);
long line

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

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:109: return new
TypeMask.exactOrEmpty(base, classWorld);
Not empty

https://codereview.chromium.org/1413613010/diff/1/tests/compiler/dart2js/type...
File tests/compiler/dart2js/type_mask2_test.dart (right):

https://codereview.chromium.org/1413613010/diff/1/tests/compiler/dart2js/type...
tests/compiler/dart2js/type_mask2_test.dart:47: // behaviour in unchecked more
is not defined and thus cannot be
more => mode

[Johnni Winther, 2015-11-03 13:20:01]

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

[Johnni Winther, 2015-11-03 13:20:02]

@herhut: PTAL

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/enqueu...
File pkg/compiler/lib/src/enqueue.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/enqueu...
pkg/compiler/lib/src/enqueue.dart:194: if (!cls.isAbstract || isNative ||
mirrorUsage) {
Make a variable and use for universe.register...

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:3250: new
TypeMask.nonNullSubtype(closureClassElement, compiler.world);
On 2015/11/03 13:06:50, sigurdm wrote:
> ?

Why not notNullExact?

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

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:153: return new
TypeMask.nonNullExactOrEmpty(base, classWorld);
also exact here.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/function_set.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/function_set.dart:303: } else {
Add TODO to avoid this.

[Siggi Cherem (dart-lang), 2015-11-03 21:01:57]

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

[Siggi Cherem (dart-lang), 2015-11-03 21:01:58]

dbc

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:649: returnType =
types.nonNullSubclass(cls);
same here - why not nonNullExact?

[herhut, 2015-11-04 10:42:20]

Very nice! 

I am not sure about the asserts that disallow creating type masks for
uninstantiated types. Maybe we could have an optional flag to allow empty
typemasks, like

x = TypeMasks.nonNullSubclass(cls, allowEmpty: true)

That would make it possible to use the strictest constraints when using the API
without all those instantiated checks. Typically, one knows that one injects a
type without proof of the classes existence at runtime, so it is easy to
identify when the allowEmpty is needed.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:3250: new
TypeMask.nonNullSubtype(closureClassElement, compiler.world);
On 2015/11/03 13:20:02, Johnni Winther wrote:
> On 2015/11/03 13:06:50, sigurdm wrote:
> > ?
> 
> Why not notNullExact?

Hmm, not sure this works. I know that we introduce classes for closures and that
technically they are all subtypes of Function. However, all of this happens
after resolution and during type inference IIRC. Do we update the required
information?

if so, notNullExact would be more precise. We know here that it is an instance
of exactly that class.

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

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:105: base =
classWorld.getLeastUpperInstantiatedSubclass(base);
Name this topmost, as well?

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:109: return new
TypeMask.exactOrEmpty(base, classWorld);
On 2015/11/03 13:06:50, sigurdm wrote:
> Not empty

I would go for the empty one, as the fact that it is instantiated is only
checked by an assert. Otherwise, we'll get illegal masks in production mode.
Empty at least fails somewhat reliably.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:153: return new
TypeMask.nonNullExactOrEmpty(base, classWorld);
On 2015/11/03 13:20:02, Johnni Winther wrote:
> also exact here.

I'd prefer to keep the empty.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/class_set.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/class_set.dart:108: ClassElement
getLeastUpperInstantiatedSubclass() {
This name is very misleading. I know it is long already, but how about

getLeastUpperBoundOfInstantiatedSubclasses

or, to make it shorter

getLubOfInstantiatedSubclasses

Because I don't really know what a least upper instantiated subclass is, and it
may return something that is not directly instantiated but just a upper bound.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/class_set.dart:363: ClassElement
getLeastUpperInstantiatedSubtype() {
Same name comment here, maybe

getLubOfInstantiatedSubtypes

[Johnni Winther, 2015-11-05 09:54:29]

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

[Johnni Winther, 2015-11-06 13:43:23]

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/enqueu...
File pkg/compiler/lib/src/enqueue.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/enqueu...
pkg/compiler/lib/src/enqueue.dart:194: if (!cls.isAbstract || isNative ||
mirrorUsage) {
On 2015/11/03 13:20:02, Johnni Winther wrote:
> Make a variable and use for universe.register...

Done.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:649: returnType =
types.nonNullSubclass(cls);
On 2015/11/03 21:01:57, Siggi Cherem (dart-lang) wrote:
> same here - why not nonNullExact?

Updated to use nonNullExact if [cls] is directly instantiated.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:3250: new
TypeMask.nonNullSubtype(closureClassElement, compiler.world);
On 2015/11/04 10:42:20, herhut wrote:
> On 2015/11/03 13:20:02, Johnni Winther wrote:
> > On 2015/11/03 13:06:50, sigurdm wrote:
> > > ?
> > 
> > Why not notNullExact?
> 
> Hmm, not sure this works. I know that we introduce classes for closures and
that
> technically they are all subtypes of Function. However, all of this happens
> after resolution and during type inference IIRC. Do we update the required
> information?
> 
> if so, notNullExact would be more precise. We know here that it is an instance
> of exactly that class.

Update the closed world for the latter.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:5081: return new
TypeMask.nonNullExact(cls.thisType.element, compiler.world);
On 2015/11/03 13:06:50, sigurdm wrote:
> long line

Done.

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

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:105: base =
classWorld.getLeastUpperInstantiatedSubclass(base);
On 2015/11/04 10:42:20, herhut wrote:
> Name this topmost, as well?

Done.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:109: return new
TypeMask.exactOrEmpty(base, classWorld);
On 2015/11/04 10:42:20, herhut wrote:
> On 2015/11/03 13:06:50, sigurdm wrote:
> > Not empty
> 
> I would go for the empty one, as the fact that it is instantiated is only
> checked by an assert. Otherwise, we'll get illegal masks in production mode.
> Empty at least fails somewhat reliably.

Empty if topmost is null.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:153: return new
TypeMask.nonNullExactOrEmpty(base, classWorld);
On 2015/11/04 10:42:20, herhut wrote:
> On 2015/11/03 13:20:02, Johnni Winther wrote:
> > also exact here.
> 
> I'd prefer to keep the empty.

Again empty if topmost is null, exact otherwise.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/class_set.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/class_set.dart:108: ClassElement
getLeastUpperInstantiatedSubclass() {
On 2015/11/04 10:42:20, herhut wrote:
> This name is very misleading. I know it is long already, but how about
> 
> getLeastUpperBoundOfInstantiatedSubclasses
> 
> or, to make it shorter
> 
> getLubOfInstantiatedSubclasses
> 
> Because I don't really know what a least upper instantiated subclass is, and
it
> may return something that is not directly instantiated but just a upper bound.

getLubOfInstantiatedSubclasses it is.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/class_set.dart:363: ClassElement
getLeastUpperInstantiatedSubtype() {
On 2015/11/04 10:42:20, herhut wrote:
> Same name comment here, maybe
> 
> getLubOfInstantiatedSubtypes

Done.

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/function_set.dart (right):

https://codereview.chromium.org/1413613010/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/function_set.dart:303: } else {
On 2015/11/03 13:20:02, Johnni Winther wrote:
> Add TODO to avoid this.

Done.

https://codereview.chromium.org/1413613010/diff/1/tests/compiler/dart2js/type...
File tests/compiler/dart2js/type_mask2_test.dart (right):

https://codereview.chromium.org/1413613010/diff/1/tests/compiler/dart2js/type...
tests/compiler/dart2js/type_mask2_test.dart:47: // behaviour in unchecked more
is not defined and thus cannot be
On 2015/11/03 13:06:50, sigurdm wrote:
> more => mode

Done.

[Johnni Winther, 2015-11-06 13:43:40]

Description was changed from

==========
Normalize type masks to use the least upper instantiated subclass/type.

BUG=
R=sigurdm@google.com

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

to

==========
Normalize type masks to use the least upper instantiated subclass/type.

BUG=
R=sigurdm@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/c1b102a94dbc6a7a0ffe022d4bef26f9390a90f2
==========

[Johnni Winther, 2015-11-06 14:37:23]

Committed patchset #4 (id:60001) manually as
7b458eb1d62fe2ef896ed60e0a463f95bf0b0890 (presubmit successful).