fix #25944, improve Future.then inference

fix #25944, improve Future.then inference
also fixes #25322, return values of async functions are T | F<T>

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

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

[Jennifer Messerly, 2016-08-04 00:37:54]

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

[Jennifer Messerly, 2016-08-04 00:37:55]

Hi Leaf,

Here's my attempt to fix the various issues around implicit Future unions. Note
the change to the signature of Future.then's onValue parameter.

The basic idea is to represent the Future<T> | T union during downwards
inference. It affects a lot of locations because we need to decide what to do if
we are in that context. Usually we ignore it and use "T" but sometimes we pick
one or the other.

[Leaf, 2016-08-05 18:27:34]

On 2016/08/04 00:37:55, John Messerly wrote:
> Hi Leaf,
> 
> Here's my attempt to fix the various issues around implicit Future unions.
Note
> the change to the signature of Future.then's onValue parameter.
> 
> The basic idea is to represent the Future<T> | T union during downwards
> inference. It affects a lot of locations because we need to decide what to do
if
> we are in that context. Usually we ignore it and use "T" but sometimes we pick
> one or the other.

Sorry to be slow on this, I'll get to it today.

[Leaf, 2016-08-05 22:32:36]

This is so awesome to see, I can't tell you!  Really lgtm - I'm super happy with
how this worked out.

Once this is rolled out, we should look at 
 1) Finding all the places that the .then<Future<T>> hack is used and
eliminating them (this might be a good test case for skybrian's refactoring
tools)
 2) getting rid of Future flattening.

:)

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

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/type.dart:35: abstract class DartType implements
TypeContext {
It bothers me a bit that this leaks into the public API, even though it's just a
marker.  It seems like it would work to make FutureUnion type a subclass of
TypeImpl instead.  That's probably a little annoying since there's a bunch of
API surface from DartType that doesn't really apply to TypeContext.  But at
least it keeps it behind the API boundary.

Maybe an alternative would be to provide for this in a more abstract way by
replacing TypeContext here with something more abstract like "PreType" or
"TypeSchema" or "Classifier" or "TypeData" or something.  Then TypeContext could
be a private implementation of this, and if we want to use this trick again we
don't have to change this again?

WDYT?

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/type.dart:294: * <i>(T<sub>1</sub>, &hellip;,
T<sub>n</sub>, `[]`) &rarr; T <: (T<sub>1</sub>,
line length.

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:4912: static DartType
getNonFutureType(AstNode node) {
I don't love this name.  It seems to be that it would be nicer to keep the names
matching what gets returned.  So this would be `getType` (it returns a type),
and what's now called `getType below would maybe become `getContext`?  This
matches better with returnContext which yields a TypeContext.  `setType` should
probably become `setContext` if you make this change, I guess.

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

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:680: // Union types can also
arise in some cases relating to the Future type
Is this vestigial?  I don't see any new code to handle Future unions, and I
would kind of expect them not to show up here.

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1524: ''');
sweeeet.

https://codereview.chromium.org/2208953002/diff/40001/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/2208953002/diff/40001/sdk/lib/async/future.da...
sdk/lib/async/future.dart:436: Future/*<S>*/ then/*<S>*/(onValue(T value), {
Function onError });
Presumably theres are corresponding patch to be coordinated in the main repo at
some point?

[Jennifer Messerly, 2016-08-08 21:59:26]

Thanks! Addressed all of these if you want to take another look :D

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

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/type.dart:35: abstract class DartType implements
TypeContext {
On 2016/08/05 22:32:35, Leaf wrote:
> It bothers me a bit that this leaks into the public API, even though it's just
a
> marker.  It seems like it would work to make FutureUnion type a subclass of
> TypeImpl instead.  That's probably a little annoying since there's a bunch of
> API surface from DartType that doesn't really apply to TypeContext.  But at
> least it keeps it behind the API boundary.
> 
> Maybe an alternative would be to provide for this in a more abstract way by
> replacing TypeContext here with something more abstract like "PreType" or
> "TypeSchema" or "Classifier" or "TypeData" or something.  Then TypeContext
could
> be a private implementation of this, and if we want to use this trick again we
> don't have to change this again?
> 
> WDYT?

Yeah, making it a subtype of TypeImpl works for me. Let me give that a try.

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/type.dart:294: * <i>(T<sub>1</sub>, &hellip;,
T<sub>n</sub>, `[]`) &rarr; T <: (T<sub>1</sub>,
On 2016/08/05 22:32:35, Leaf wrote:
> line length.

Done.

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:4912: static DartType
getNonFutureType(AstNode node) {
On 2016/08/05 22:32:35, Leaf wrote:
> I don't love this name.  It seems to be that it would be nicer to keep the
names
> matching what gets returned.  So this would be `getType` (it returns a type),
> and what's now called `getType below would maybe become `getContext`?  This
> matches better with returnContext which yields a TypeContext.  `setType`
should
> probably become `setContext` if you make this change, I guess.

Great idea! Yeah I didn't like those names either... Done!

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

https://codereview.chromium.org/2208953002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:680: // Union types can also
arise in some cases relating to the Future type
On 2016/08/05 22:32:35, Leaf wrote:
> Is this vestigial?  I don't see any new code to handle Future unions, and I
> would kind of expect them not to show up here.

Eeeek, good catch, done!

(yes for a while I had them not as well contained, they were indeed spreading to
LUB and subtype and stuff like that, at which point I backed off and started
over)

https://codereview.chromium.org/2208953002/diff/40001/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/2208953002/diff/40001/sdk/lib/async/future.da...
sdk/lib/async/future.dart:436: Future/*<S>*/ then/*<S>*/(onValue(T value), {
Function onError });
On 2016/08/05 22:32:36, Leaf wrote:
> Presumably theres are corresponding patch to be coordinated in the main repo
at
> some point?

This is the main repo so it changes it here, but yeah, we'll need to merge it
into DDC's SDK & coordinate an upgrade of DDC's Analyzer dependency and make
sure DDC's DEPS gets rolled in main repo.

[Jennifer Messerly, 2016-08-09 12:36:52]

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

[Jennifer Messerly, 2016-08-09 12:36:53]

+vsm as well if you want to sanity check my latest changes. but basically just
took all of Leaf's awesome refactor suggestions :)

[vsm, 2016-08-09 20:29:33]

lgtm

nice!

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

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:4721: if (context == null ||
context is DartType && context.isDynamic) {
Why "context is DartType"?  That's the static type.

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:4723: inferred =
_typeSystem.getLeastUpperBound(_typeProvider, inferred, type);
Does the order actually matter?

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:7132: // IF it's async* we expect
Stream<T>
IF -> If

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

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:1337: /// (due to covariant
generic types) as would `() -> A <: () -> int`. In
nit: () -> num not () -> int

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:1609: return new
FutureUnionType._([
maybe wrap the list in an UnmodifiableListView?

[Jennifer Messerly, 2016-08-09 20:56:57]

Thanks!!!

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

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:4721: if (context == null ||
context is DartType && context.isDynamic) {
On 2016/08/09 20:29:33, vsm wrote:
> Why "context is DartType"?  That's the static type.

Good catch. Holdover from the earlier design when I had FutureUnion/TypeContext
not inheriting from DartType.

Fortunately this goes away when merging with one of my other changes (the return
context fix)

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:4723: inferred =
_typeSystem.getLeastUpperBound(_typeProvider, inferred, type);
On 2016/08/09 20:29:33, vsm wrote:
> Does the order actually matter?

no. it got swapped because I had removed and reconstructed this line. anyway
it's reverted now because of the "return context" fix from another recent CL

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:7132: // IF it's async* we expect
Stream<T>
On 2016/08/09 20:29:33, vsm wrote:
> IF -> If

Done.

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

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:1337: /// (due to covariant
generic types) as would `() -> A <: () -> int`. In
On 2016/08/09 20:29:33, vsm wrote:
> nit: () -> num not () -> int

You have keen eyes my friend ;). Done!

https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:1609: return new
FutureUnionType._([
On 2016/08/09 20:29:33, vsm wrote:
> maybe wrap the list in an UnmodifiableListView?

yeah I tend to agree and I think we should use immutable collections a lot more
than we do... Analyzer tends to not use them though for anything. There's a lot
of "conceptually immutable" arrays floating around (probably most of the
arrays), nd this type is internal. Also in Dart our immutable collects are only
checked at runtime, we don't have nice interfaces for the immutable API
(Iterable is close but it also typically implies lazy). So it's kind of a tough
one to know what to do.

Anyway, I went ahead and changed the public getter to expose Iterable and the
private List field, so that provides a bit more options for the future...

[Leaf, 2016-08-09 21:17:54]

On 2016/08/09 20:56:57, John Messerly wrote:
> Thanks!!!
> 
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> File pkg/analyzer/lib/src/generated/resolver.dart (right):
> 
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> pkg/analyzer/lib/src/generated/resolver.dart:4721: if (context == null ||
> context is DartType && context.isDynamic) {
> On 2016/08/09 20:29:33, vsm wrote:
> > Why "context is DartType"?  That's the static type.
> 
> Good catch. Holdover from the earlier design when I had
FutureUnion/TypeContext
> not inheriting from DartType.
> 
> Fortunately this goes away when merging with one of my other changes (the
return
> context fix)
> 
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> pkg/analyzer/lib/src/generated/resolver.dart:4723: inferred =
> _typeSystem.getLeastUpperBound(_typeProvider, inferred, type);
> On 2016/08/09 20:29:33, vsm wrote:
> > Does the order actually matter?
> 
> no. it got swapped because I had removed and reconstructed this line. anyway
> it's reverted now because of the "return context" fix from another recent CL
> 
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> pkg/analyzer/lib/src/generated/resolver.dart:7132: // IF it's async* we expect
> Stream<T>
> On 2016/08/09 20:29:33, vsm wrote:
> > IF -> If
> 
> Done.
> 
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> File pkg/analyzer/lib/src/generated/type_system.dart (right):
> 
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> pkg/analyzer/lib/src/generated/type_system.dart:1337: /// (due to covariant
> generic types) as would `() -> A <: () -> int`. In
> On 2016/08/09 20:29:33, vsm wrote:
> > nit: () -> num not () -> int
> 
> You have keen eyes my friend ;). Done!
> 
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> pkg/analyzer/lib/src/generated/type_system.dart:1609: return new
> FutureUnionType._([
> On 2016/08/09 20:29:33, vsm wrote:
> > maybe wrap the list in an UnmodifiableListView?
> 
> yeah I tend to agree and I think we should use immutable collections a lot
more
> than we do... Analyzer tends to not use them though for anything. There's a
lot
> of "conceptually immutable" arrays floating around (probably most of the
> arrays), nd this type is internal. Also in Dart our immutable collects are
only
> checked at runtime, we don't have nice interfaces for the immutable API
> (Iterable is close but it also typically implies lazy). So it's kind of a
tough
> one to know what to do.
> 
> Anyway, I went ahead and changed the public getter to expose Iterable and the
> private List field, so that provides a bit more options for the future...

I won't get to this until tomorrow (DK time) at best.  I was pretty happy with
it to start, so feel free to land this and I can follow up with comments if I
have any.

[Jennifer Messerly, 2016-08-09 22:20:31]

On 2016/08/09 21:17:54, Leaf wrote:
> On 2016/08/09 20:56:57, John Messerly wrote:
> > Thanks!!!
> > 
> >
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> > File pkg/analyzer/lib/src/generated/resolver.dart (right):
> > 
> >
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> > pkg/analyzer/lib/src/generated/resolver.dart:4721: if (context == null ||
> > context is DartType && context.isDynamic) {
> > On 2016/08/09 20:29:33, vsm wrote:
> > > Why "context is DartType"?  That's the static type.
> > 
> > Good catch. Holdover from the earlier design when I had
> FutureUnion/TypeContext
> > not inheriting from DartType.
> > 
> > Fortunately this goes away when merging with one of my other changes (the
> return
> > context fix)
> > 
> >
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> > pkg/analyzer/lib/src/generated/resolver.dart:4723: inferred =
> > _typeSystem.getLeastUpperBound(_typeProvider, inferred, type);
> > On 2016/08/09 20:29:33, vsm wrote:
> > > Does the order actually matter?
> > 
> > no. it got swapped because I had removed and reconstructed this line. anyway
> > it's reverted now because of the "return context" fix from another recent CL
> > 
> >
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> > pkg/analyzer/lib/src/generated/resolver.dart:7132: // IF it's async* we
expect
> > Stream<T>
> > On 2016/08/09 20:29:33, vsm wrote:
> > > IF -> If
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> > File pkg/analyzer/lib/src/generated/type_system.dart (right):
> > 
> >
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> > pkg/analyzer/lib/src/generated/type_system.dart:1337: /// (due to covariant
> > generic types) as would `() -> A <: () -> int`. In
> > On 2016/08/09 20:29:33, vsm wrote:
> > > nit: () -> num not () -> int
> > 
> > You have keen eyes my friend ;). Done!
> > 
> >
>
https://codereview.chromium.org/2208953002/diff/60001/pkg/analyzer/lib/src/ge...
> > pkg/analyzer/lib/src/generated/type_system.dart:1609: return new
> > FutureUnionType._([
> > On 2016/08/09 20:29:33, vsm wrote:
> > > maybe wrap the list in an UnmodifiableListView?
> > 
> > yeah I tend to agree and I think we should use immutable collections a lot
> more
> > than we do... Analyzer tends to not use them though for anything. There's a
> lot
> > of "conceptually immutable" arrays floating around (probably most of the
> > arrays), nd this type is internal. Also in Dart our immutable collects are
> only
> > checked at runtime, we don't have nice interfaces for the immutable API
> > (Iterable is close but it also typically implies lazy). So it's kind of a
> tough
> > one to know what to do.
> > 
> > Anyway, I went ahead and changed the public getter to expose Iterable and
the
> > private List field, so that provides a bit more options for the future...
> 
> I won't get to this until tomorrow (DK time) at best.  I was pretty happy with
> it to start, so feel free to land this and I can follow up with comments if I
> have any.

Sounds good. Yeah happy to tweak it here. Moar code quality changes are always
good. The renames/class structure change you suggested made a huge difference.

[Jennifer Messerly, 2016-08-09 23:04:29]

Description was changed from

==========
fix #25944, improve Future.then inference
also fixes #25322, return values of async functions are T | F<T>
==========

to

==========
fix #25944, improve Future.then inference
also fixes #25322, return values of async functions are T | F<T>

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

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

[Jennifer Messerly, 2016-08-09 23:04:31]

Committed patchset #5 (id:80001) manually as
1c7aca0d6ddc4f03da5ed2c32f208d587c17b078 (presubmit successful).