Better strong mode least upper bound for interface types.

Better strong mode least upper bound for interface types.  Adds some
checks to handle common cases of least upper-bound computations
on type parameters and interface types in a more reasonable way
than the spec lub.

BUG=
R=rnystrom@google.com

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

[Leaf, 2016-03-29 00:22:23]

Description was changed from

==========
Better strong mode least upper bound for interface types.

BUG=
==========

to

==========
Better strong mode least upper bound for interface types.  Adds some
checks to handle common cases of least upper-bound computations
on type parameters and interface types in a more reasonable way
than the spec lub.

BUG=
==========

[Leaf, 2016-03-29 00:22:23]

leafp@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2016-03-29 17:09:44]

A few little things, but LGTM!

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:47: // This currently does not
implement a very complete least upper bound
This is a great comment!

How about making it a doc comment for the method and using "1.", etc. for the
lists. That will make it valid Markdown syntax.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:61: //    might (or might-not)
be a well-formed type.
Nit: No hyphen needed in "might-not".

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:81: DartType t2 = tArgs2[i];
These locals don't really add much value. I think the next statement would still
fit on one line even if you inlined these.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:118: // and hence the
contra-positive.
Ditto about making this a doc comment and Markdown.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:142: type2 = type2
Do you need to cast type2 to TypeParameterType to keep this strong mode clean?

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:1070: // TODO(rnystrom): This
isn't correct. See: https://dartbug.com/26054.
If we don't plan to improve this for spec mode, can we remove the TODO?

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/type_system_test.dart (right):

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/type_system_test.dart:1368: // Check least upper
bound of the same class with different type parameters
Nits: "///" and end with ".". Below too.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/type_system_test.dart:1440:
_checkLeastUpperBound(typeParamT, typeParamS, typeParamS);
Is it worth testing symmetry too:

_checkLeastUpperBound(typeParamS, typeParamT, typeParamS);

?

I suppose if we wanted to test that, we should just make _checkLeastUpperBound()
check in both directions.

[Leaf, 2016-03-29 22:03:23]

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:47: // This currently does not
implement a very complete least upper bound
On 2016/03/29 17:09:43, Bob Nystrom wrote:
> This is a great comment!
> 
> How about making it a doc comment for the method and using "1.", etc. for the
> lists. That will make it valid Markdown syntax.

Done.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:61: //    might (or might-not)
be a well-formed type.
On 2016/03/29 17:09:43, Bob Nystrom wrote:
> Nit: No hyphen needed in "might-not".

Done.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:81: DartType t2 = tArgs2[i];
On 2016/03/29 17:09:43, Bob Nystrom wrote:
> These locals don't really add much value. I think the next statement would
still
> fit on one line even if you inlined these.

Done.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:118: // and hence the
contra-positive.
On 2016/03/29 17:09:43, Bob Nystrom wrote:
> Ditto about making this a doc comment and Markdown.

Done.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:142: type2 = type2
On 2016/03/29 17:09:43, Bob Nystrom wrote:
> Do you need to cast type2 to TypeParameterType to keep this strong mode clean?

I don't think so, unless I'm missing something.  type2 has a static type of
TypeParameterType, and resolveToBound and substitute2 are both defined on
DartType.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:1070: // TODO(rnystrom): This
isn't correct. See: https://dartbug.com/26054.
On 2016/03/29 17:09:43, Bob Nystrom wrote:
> If we don't plan to improve this for spec mode, can we remove the TODO?

Done.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/type_system_test.dart (right):

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/type_system_test.dart:1368: // Check least upper
bound of the same class with different type parameters
On 2016/03/29 17:09:43, Bob Nystrom wrote:
> Nits: "///" and end with ".". Below too.

Done.

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/type_system_test.dart:1440:
_checkLeastUpperBound(typeParamT, typeParamS, typeParamS);
On 2016/03/29 17:09:43, Bob Nystrom wrote:
> Is it worth testing symmetry too:
> 
> _checkLeastUpperBound(typeParamS, typeParamT, typeParamS);
> 
> ?
> 
> I suppose if we wanted to test that, we should just make
_checkLeastUpperBound()
> check in both directions.

Awesome idea!  I made _checkLeastUpperBound check symmetry, and also check that
it does in fact produce an upper bound.  I did the same for GLB as well.

To make this work, had to work around some problems with function type equality
in the analyzer, and had to change the way were testing GLB and LUB of void.

[Leaf, 2016-03-29 23:36:42]

Description was changed from

==========
Better strong mode least upper bound for interface types.  Adds some
checks to handle common cases of least upper-bound computations
on type parameters and interface types in a more reasonable way
than the spec lub.

BUG=
==========

to

==========
Better strong mode least upper bound for interface types.  Adds some
checks to handle common cases of least upper-bound computations
on type parameters and interface types in a more reasonable way
than the spec lub.

BUG=
R=rnystrom@google.com

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

[Leaf, 2016-03-29 23:36:44]

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

[Bob Nystrom, 2016-03-29 23:42:01]

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:142: type2 = type2
On 2016/03/29 22:03:22, Leaf wrote:
> On 2016/03/29 17:09:43, Bob Nystrom wrote:
> > Do you need to cast type2 to TypeParameterType to keep this strong mode
clean?
> 
> I don't think so, unless I'm missing something.  type2 has a static type of
> TypeParameterType,

Isn't it just DartType? I know in practice it will be a TypeParameterType
because of how the code path gets here, but we haven't done an actual type guard
like we did with type1, right?

> and resolveToBound and substitute2 are both defined on
> DartType.  

Ah, OK. That's fine then. :)

[Leaf, 2016-03-29 23:43:54]

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1843453002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:142: type2 = type2
On 2016/03/29 23:42:00, Bob Nystrom wrote:
> On 2016/03/29 22:03:22, Leaf wrote:
> > On 2016/03/29 17:09:43, Bob Nystrom wrote:
> > > Do you need to cast type2 to TypeParameterType to keep this strong mode
> clean?
> > 
> > I don't think so, unless I'm missing something.  type2 has a static type of
> > TypeParameterType,
> 
> Isn't it just DartType? I know in practice it will be a TypeParameterType
> because of how the code path gets here, but we haven't done an actual type
guard
> like we did with type1, right?
> 
> > and resolveToBound and substitute2 are both defined on
> > DartType.  
> 
> Ah, OK. That's fine then. :)

By "static type of TypeParameterType" I meant "static type of DartType" of
course... :)

[Jennifer Messerly, 2016-03-29 23:54:39]

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

[Jennifer Messerly, 2016-03-29 23:54:39]

LGTM!

https://codereview.chromium.org/1843453002/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1843453002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:137: InterfaceTypeImpl lub = new
InterfaceTypeImpl(type1.element);
drive by comment, is this equivalent to:

    type1.element.type.instantate(tArgs);

[Leaf, 2016-03-29 23:57:43]

https://codereview.chromium.org/1843453002/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1843453002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:137: InterfaceTypeImpl lub = new
InterfaceTypeImpl(type1.element);
On 2016/03/29 23:54:39, John Messerly wrote:
> drive by comment, is this equivalent to:
> 
>     type1.element.type.instantate(tArgs);

Yeah, I think that's right, and is nicer.