fixes #25482, flatten Futures in strong mode so Future.then works

fixes #25482, flatten futures in strong mode so Future.then works
Also fixes https://github.com/dart-lang/dev_compiler/issues/228

A few tweaks were made to the propagatedType inference for Future.then:
* only record type if better than static type
* only go down this path when strong mode is off

also renames tests: test_pseudoGeneric to test_genericMethod.

R=brianwilkerson@google.com, vsm@google.com

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

[Jennifer Messerly, 2016-01-20 01:22:08]

Description was changed from

==========
fixes #25482, flatten futures in strong mode so Future.then works
Also fixes https://github.com/dart-lang/dev_compiler/issues/228

A few tweaks were made to the propagatedType inference for Future.then:
* only record type if better than static type
* only go down this path when strong mode is off
==========

to

==========
fixes #25482, flatten futures in strong mode so Future.then works
Also fixes https://github.com/dart-lang/dev_compiler/issues/228

A few tweaks were made to the propagatedType inference for Future.then:
* only record type if better than static type
* only go down this path when strong mode is off

also renames tests: test_pseudoGeneric to test_genericMethod.
==========

[Jennifer Messerly, 2016-01-20 01:22:08]

jmesserly@google.com changed reviewers:
+ brianwilkerson@google.com, vsm@google.com

[vsm, 2016-01-20 01:32:20]

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1703: // the return type so it is
Future< flatten(S) >, yielding Future<R>.
What happens if I do:

class Foo implements Future<String> { ... }

and I have:

Future<Foo>

?  Does that really get flattened as the comment suggests?

[Jennifer Messerly, 2016-01-20 01:40:48]

In strong mode, this defines: Future< T > to be Future< flatten(T) >

flatten(T) is from the Dart spec and was already used in a few places.

Also: we should probably skip those other places in strong mode now for
consistency, since they aren't needed. Let me know if I should do that.

BTW, I need to add a test for `class C<T> implements Future<C<C<T>>>`, and see
what happens after this change. We may need to reject it.

[Brian Wilkerson, 2016-01-20 16:55:15]

LGTM

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:20: // Or perhaps to TypeSystem?
It does seem strange for it to be where it is. Seems like it ought to be on
InterfaceType (and should return `this` if `this` is not `Future`, eliminating
the need for some tests on the client side). It could be in TypeSystem if the
definition can change, but it seems unnecessary.

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1708: library.name == 'dart.async')
{
LibraryElement defines `isDartCore`. Seems like it should also define
`isDartAsync` (rather than have that test inlined here).

[Jennifer Messerly, 2016-01-20 23:21:51]

PTAL. Think I've addressed comments. Looks nicer with Brian's fixes. Because of
how dart:async is tested now & removing the requirement to pass in Future's
ClassElement, I had to fix up TestTypeProvider to include a valid dart:async
library behind the Future type.

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:20: // Or perhaps to TypeSystem?
On 2016/01/20 16:55:15, Brian Wilkerson wrote:
> It does seem strange for it to be where it is. Seems like it ought to be on
> InterfaceType (and should return `this` if `this` is not `Future`, eliminating
> the need for some tests on the client side). It could be in TypeSystem if the
> definition can change, but it seems unnecessary.

Oooh, nice idea. Done!

In that spirit, and the spirit of your other comment, I added
"isDartAsyncFuture" to match the existing "isDartCoreFunction" API.

One thing I wasn't sure about is whether to put my 2 new APIs on DartType, or
hide them behind TypeImpl. I chose the latter to be conservative, but it does
mean a few downcasts. Let me know what you think -- happy to move them to
DartType too, with some encouragement. :-)

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1703: // the return type so it is
Future< flatten(S) >, yielding Future<R>.
On 2016/01/20 01:32:20, vsm wrote:
> What happens if I do:
> 
> class Foo implements Future<String> { ... }
> 
> and I have:
> 
> Future<Foo>
> 
> ?  Does that really get flattened as the comment suggests?

Yeah. The operation is defined in the spec & used for async/await, it will
flatten to Future<String>. If there are multiple types S where Foo <: Future<S>,
it will find the most specific type R, where R <: S for all S. (We do have an
issue of using Dart spec's more specific vs strong mode subtyping. I filed a bug
about it yesterday as it's a pretty widespread problem, but in the meantime I
can fix the definition of flatten(T) to use our subtype)

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1708: library.name == 'dart.async')
{
On 2016/01/20 16:55:15, Brian Wilkerson wrote:
> LibraryElement defines `isDartCore`. Seems like it should also define
> `isDartAsync` (rather than have that test inlined here).

Excellent idea, done! In the same spirit I added isDartAsyncFuture, similar to
isDartCoreFunction. IMO reads much nicer now!

[Jennifer Messerly, 2016-01-20 23:22:50]

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1609093003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:20: // Or perhaps to TypeSystem?
On 2016/01/20 23:21:51, John Messerly wrote:
> On 2016/01/20 16:55:15, Brian Wilkerson wrote:
> > It does seem strange for it to be where it is. Seems like it ought to be on
> > InterfaceType (and should return `this` if `this` is not `Future`,
eliminating
> > the need for some tests on the client side). It could be in TypeSystem if
the
> > definition can change, but it seems unnecessary.
> 
> Oooh, nice idea. Done!
> 
> In that spirit, and the spirit of your other comment, I added
> "isDartAsyncFuture" to match the existing "isDartCoreFunction" API.
> 
> One thing I wasn't sure about is whether to put my 2 new APIs on DartType, or
> hide them behind TypeImpl. I chose the latter to be conservative, but it does
> mean a few downcasts. Let me know what you think -- happy to move them to
> DartType too, with some encouragement. :-)

Update: I did end up moving them to DartType, as otherwise I needed a bunch of
casts a usage sites of flattenFutures.

[Brian Wilkerson, 2016-01-20 23:50:36]

LGTM. Much cleaner. We should add some tests of 'flatten' at some point, but
this is good for now.

[vsm, 2016-01-21 19:17:46]

lgtm

https://codereview.chromium.org/1609093003/diff/80001/pkg/analyzer/lib/dart/e...
File pkg/analyzer/lib/dart/element/type.dart (right):

https://codereview.chromium.org/1609093003/diff/80001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/type.dart:90: DartType flattenFutures(TypeSystem
typeSystem);
Might be cleaner to move this to TypeSystem.  It's a little weird that this
method takes a TypeSystem and the ones right below do not.

https://codereview.chromium.org/1609093003/diff/80001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1609093003/diff/80001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1944: static DartType
_findMostSpecificType(List<DartType> types, TypeSystem typeSystem) {
line len?

[Jennifer Messerly, 2016-01-21 19:31:38]

https://codereview.chromium.org/1609093003/diff/80001/pkg/analyzer/lib/dart/e...
File pkg/analyzer/lib/dart/element/type.dart (right):

https://codereview.chromium.org/1609093003/diff/80001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/type.dart:90: DartType flattenFutures(TypeSystem
typeSystem);
On 2016/01/21 19:17:46, vsm wrote:
> Might be cleaner to move this to TypeSystem.  It's a little weird that this
> method takes a TypeSystem and the ones right below do not.

Brian, what do you think?

I actually went back and forth. Did it both ways, so it's not hard to switch. :)

I agree that the fact that it uses isMoreSpecificThan internally, and is an
operation on types, suggests it would fit naturally into TypeSystem. That was my
slight preference too.

Here's the implication of moving it there, though: most of the code is shared
between the spec TypeSystem and strong TypeSystem ... Right now they don't have
a common base class, only a common interface. I wasn't sure if it was okay to
convert TypeSystem over to an abstract base class, or if I should introduce a
new thing (TypeSystemImpl?) to hold the common code, or not.

[Brian Wilkerson, 2016-01-21 20:20:57]

https://codereview.chromium.org/1609093003/diff/80001/pkg/analyzer/lib/dart/e...
File pkg/analyzer/lib/dart/element/type.dart (right):

https://codereview.chromium.org/1609093003/diff/80001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/type.dart:90: DartType flattenFutures(TypeSystem
typeSystem);
Personally, I'd say to leave it as is for now. There are other methods (such as
`isAssignableTo`) that either need to be removed or need to have the type system
be passed as an argument or something, so I'd just defer the question for now
and address the API as a whole later.

[Jennifer Messerly, 2016-01-22 21:30:09]

Description was changed from

==========
fixes #25482, flatten futures in strong mode so Future.then works
Also fixes https://github.com/dart-lang/dev_compiler/issues/228

A few tweaks were made to the propagatedType inference for Future.then:
* only record type if better than static type
* only go down this path when strong mode is off

also renames tests: test_pseudoGeneric to test_genericMethod.
==========

to

==========
fixes #25482, flatten futures in strong mode so Future.then works
Also fixes https://github.com/dart-lang/dev_compiler/issues/228

A few tweaks were made to the propagatedType inference for Future.then:
* only record type if better than static type
* only go down this path when strong mode is off

also renames tests: test_pseudoGeneric to test_genericMethod.

R=brianwilkerson@google.com, vsm@google.com

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

[Jennifer Messerly, 2016-01-22 21:30:10]

Committed patchset #5 (id:80001) manually as
9a4b1c36ce5f6b770b96c36847f63c3983cc662e (presubmit successful).