fix strong mode errors for 1.19.0-dev.4.0

fix strong mode errors for 1.19.0-dev.4.0

BUG=
R=jmesserly@google.com, leafp@google.com, nweiz@google.com

Committed: https://github.com/dart-lang/async/commit/c230c8128dc6f178743a917136461041036b4157
Committed: https://github.com/dart-lang/async/commit/7db0ec0b31b7d63c958dd3488ac99b46c2d3bead

[keertip, 2016-08-16 20:58:39]

keertip@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2016-08-16 21:27:11]

I don't understand this change. It looks like the current strong-mode definition
of Future.then() has the annotation you're removing:
https://github.com/dart-lang/dev_compiler/blob/master/tool/input_sdk/lib/asyn...

[vsm, 2016-08-16 21:49:28]

On 2016/08/16 21:27:11, nweiz wrote:
> I don't understand this change. It looks like the current strong-mode
definition
> of Future.then() has the annotation you're removing:
>
https://github.com/dart-lang/dev_compiler/blob/master/tool/input_sdk/lib/asyn...

I think this is right.  The closure should be able to return S or Future<S> now.
 We have no way to write that union, so it has to be left unspecified and
inferred.

[vsm, 2016-08-16 21:49:39]

Description was changed from

==========
fix strong mode errors for 1.19.0-dev.4.0

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

to

==========
fix strong mode errors for 1.19.0-dev.4.0

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

[vsm, 2016-08-16 21:49:39]

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

[vsm, 2016-08-16 21:49:46]

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

[nweiz, 2016-08-16 22:00:33]

Does that mean that my link is no longer the canonical strong-mode library
definition?

Won't this change cause the DDC to fail at runtime if downward inference chooses
a return value for then() that's different than the value that actually ends up
getting wrapped?

[Jennifer Messerly, 2016-08-16 22:29:33]

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

[Jennifer Messerly, 2016-08-16 22:29:33]

+Leaf too. Hey just got looped into this

SDK did indeed recently change signature:

    Future<T>.then:  <S>(T -> *) -> Future<S>

here's a bug for that: https://github.com/dart-lang/dev_compiler/issues/627

@Leaf -- do we need to support custom Futures? I don't think the special
inference will currently kick in for them. That might be an important missing
feature.

@Keerti -- yeah I'm already working on
https://github.com/dart-lang/sdk/issues/27088

[Leaf, 2016-08-16 22:35:30]

On 2016/08/16 22:29:33, John Messerly wrote:
> +Leaf too. Hey just got looped into this
> 
> SDK did indeed recently change signature:
> 
>     Future<T>.then:  <S>(T -> *) -> Future<S>
> 
> here's a bug for that: https://github.com/dart-lang/dev_compiler/issues/627
> 
> @Leaf -- do we need to support custom Futures? I don't think the special
> inference will currently kick in for them. That might be an important missing
> feature.
> 
> @Keerti -- yeah I'm already working on
> https://github.com/dart-lang/sdk/issues/27088

I would prefer if we could avoid adding support for custom futures - it's hard
to know when to stop here.  So long as you subsume the DelegatingFuture<T> into
a Future<T>, then inference should just work.

[Jennifer Messerly, 2016-08-16 22:36:24]

Another thought here (somewhat orthogonal) for you Leaf. We could actually do
some magic inside Future.then at runtime, to make sure we always return a
Future<S> with some S where S is not dynamic. Kind of hacky, but it would help
prevent runtime errors.

https://codereview.chromium.org/2250513003/diff/1/lib/src/delegate/future.dart
File lib/src/delegate/future.dart (right):

https://codereview.chromium.org/2250513003/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:31: _future.then(onValue, onError: onError);
fyi -- probably want to pass /*<S>*/ explicitly here.

and we'll need to implement the custom Future inference, so this method gets as
good of inference when people call it as Future<T> would. With those changes,
this should work as well as built in Future.then

[Jennifer Messerly, 2016-08-16 22:37:26]

https://codereview.chromium.org/2250513003/diff/1/lib/src/typed/future.dart
File lib/src/typed/future.dart (right):

https://codereview.chromium.org/2250513003/diff/1/lib/src/typed/future.dart#n...
lib/src/typed/future.dart:18: _future.then((value) => onValue(value as T),
onError: onError);
/*<S>*/ here too

[nweiz, 2016-08-16 22:37:28]

On 2016/08/16 22:35:30, Leaf wrote:
> On 2016/08/16 22:29:33, John Messerly wrote:
> > +Leaf too. Hey just got looped into this
> > 
> > SDK did indeed recently change signature:
> > 
> >     Future<T>.then:  <S>(T -> *) -> Future<S>
> > 
> > here's a bug for that: https://github.com/dart-lang/dev_compiler/issues/627
> > 
> > @Leaf -- do we need to support custom Futures? I don't think the special
> > inference will currently kick in for them. That might be an important
missing
> > feature.
> > 
> > @Keerti -- yeah I'm already working on
> > https://github.com/dart-lang/sdk/issues/27088
> 
> I would prefer if we could avoid adding support for custom futures - it's hard
> to know when to stop here.  So long as you subsume the DelegatingFuture<T>
into
> a Future<T>, then inference should just work.

That might work for DelegatingFuture, but there are subclasses of it that never
get subsumed.

[Leaf, 2016-08-16 23:24:10]

On 2016/08/16 22:37:28, nweiz wrote:
> On 2016/08/16 22:35:30, Leaf wrote:
> > On 2016/08/16 22:29:33, John Messerly wrote:
> > > +Leaf too. Hey just got looped into this
> > > 
> > > SDK did indeed recently change signature:
> > > 
> > >     Future<T>.then:  <S>(T -> *) -> Future<S>
> > > 
> > > here's a bug for that:
https://github.com/dart-lang/dev_compiler/issues/627
> > > 
> > > @Leaf -- do we need to support custom Futures? I don't think the special
> > > inference will currently kick in for them. That might be an important
> missing
> > > feature.
> > > 
> > > @Keerti -- yeah I'm already working on
> > > https://github.com/dart-lang/sdk/issues/27088
> > 
> > I would prefer if we could avoid adding support for custom futures - it's
hard
> > to know when to stop here.  So long as you subsume the DelegatingFuture<T>
> into
> > a Future<T>, then inference should just work.
> 
> That might work for DelegatingFuture, but there are subclasses of it that
never
> get subsumed.

Hrrm.  That's annoying.  I'd like to draw the line on this ad hoc future
inference as tightly as possible.  I guess the .then signature is effectively
invariant, so hopefully we wouldn't have to worry about the signature changing
in the subclass (modulo interactions with covariant overrides).  I guess it
might not be too hard just to check for subclass of Future instead of actual
Future, but this still feels a bit hinky.

[nweiz, 2016-08-16 23:36:24]

On 2016/08/16 23:24:10, Leaf wrote:
> On 2016/08/16 22:37:28, nweiz wrote:
> > On 2016/08/16 22:35:30, Leaf wrote:
> > > On 2016/08/16 22:29:33, John Messerly wrote:
> > > > +Leaf too. Hey just got looped into this
> > > > 
> > > > SDK did indeed recently change signature:
> > > > 
> > > >     Future<T>.then:  <S>(T -> *) -> Future<S>
> > > > 
> > > > here's a bug for that:
> https://github.com/dart-lang/dev_compiler/issues/627
> > > > 
> > > > @Leaf -- do we need to support custom Futures? I don't think the special
> > > > inference will currently kick in for them. That might be an important
> > missing
> > > > feature.
> > > > 
> > > > @Keerti -- yeah I'm already working on
> > > > https://github.com/dart-lang/sdk/issues/27088
> > > 
> > > I would prefer if we could avoid adding support for custom futures - it's
> hard
> > > to know when to stop here.  So long as you subsume the DelegatingFuture<T>
> > into
> > > a Future<T>, then inference should just work.
> > 
> > That might work for DelegatingFuture, but there are subclasses of it that
> never
> > get subsumed.
> 
> Hrrm.  That's annoying.  I'd like to draw the line on this ad hoc future
> inference as tightly as possible.  I guess the .then signature is effectively
> invariant, so hopefully we wouldn't have to worry about the signature changing
> in the subclass (modulo interactions with covariant overrides).  I guess it
> might not be too hard just to check for subclass of Future instead of actual
> Future, but this still feels a bit hinky.

It's more complicated than that, though. We use then-like patterns in a number
of utility functions that return futures. Right now we get sort-of-mostly the
right behavior for those by relying on Future<Future<T>> being flattened, and
switching them to be completely un-inferred would be a major downgrade.

[keertip, 2016-08-17 17:22:47]

On 2016/08/16 23:36:24, nweiz wrote:
> On 2016/08/16 23:24:10, Leaf wrote:
> > On 2016/08/16 22:37:28, nweiz wrote:
> > > On 2016/08/16 22:35:30, Leaf wrote:
> > > > On 2016/08/16 22:29:33, John Messerly wrote:
> > > > > +Leaf too. Hey just got looped into this
> > > > > 
> > > > > SDK did indeed recently change signature:
> > > > > 
> > > > >     Future<T>.then:  <S>(T -> *) -> Future<S>
> > > > > 
> > > > > here's a bug for that:
> > https://github.com/dart-lang/dev_compiler/issues/627
> > > > > 
> > > > > @Leaf -- do we need to support custom Futures? I don't think the
special
> > > > > inference will currently kick in for them. That might be an important
> > > missing
> > > > > feature.
> > > > > 
> > > > > @Keerti -- yeah I'm already working on
> > > > > https://github.com/dart-lang/sdk/issues/27088
> > > > 
> > > > I would prefer if we could avoid adding support for custom futures -
it's
> > hard
> > > > to know when to stop here.  So long as you subsume the
DelegatingFuture<T>
> > > into
> > > > a Future<T>, then inference should just work.
> > > 
> > > That might work for DelegatingFuture, but there are subclasses of it that
> > never
> > > get subsumed.
> > 
> > Hrrm.  That's annoying.  I'd like to draw the line on this ad hoc future
> > inference as tightly as possible.  I guess the .then signature is
effectively
> > invariant, so hopefully we wouldn't have to worry about the signature
changing
> > in the subclass (modulo interactions with covariant overrides).  I guess it
> > might not be too hard just to check for subclass of Future instead of actual
> > Future, but this still feels a bit hinky.
> 
> It's more complicated than that, though. We use then-like patterns in a number
> of utility functions that return futures. Right now we get sort-of-mostly the
> right behavior for those by relying on Future<Future<T>> being flattened, and
> switching them to be completely un-inferred would be a major downgrade.

So how do we proceed here? These errors need to be fixed.

[Jennifer Messerly, 2016-08-17 18:21:01]

with the explicit /*<S>*/ added to _future.then it looks good to me

[nweiz, 2016-08-17 19:12:50]

Until strong mode supports whatever special sauce they're using for
Future.then() for all subclasses, this is a breaking change. And regardless,
it'll break in 1.18. I don't want to release a version that's broken against the
current stable.

We could potentially land this on a branch, but I'd like to have a concrete plan
for not breaking our users first.

[Jennifer Messerly, 2016-08-17 20:47:59]

On 2016/08/17 19:12:50, nweiz wrote:
> Until strong mode supports whatever special sauce they're using for
> Future.then() for all subclasses, this is a breaking change. And regardless,
> it'll break in 1.18. I don't want to release a version that's broken against
the
> current stable.
> 
> We could potentially land this on a branch, but I'd like to have a concrete
plan
> for not breaking our users first.

Hey everyone, I've filed https://github.com/dart-lang/sdk/issues/27101 to track
possible improvements here.

In general, I'm not sure we can solve this perfectly without union types or
overloads. For better or worse our type inference can't be handle all cases,
given current Dart, as much as I wish it could

[Leaf, 2016-08-17 20:53:11]

On 2016/08/17 20:47:59, John Messerly wrote:
> On 2016/08/17 19:12:50, nweiz wrote:
> > Until strong mode supports whatever special sauce they're using for
> > Future.then() for all subclasses, this is a breaking change. And regardless,
> > it'll break in 1.18. I don't want to release a version that's broken against
> the
> > current stable.
> > 
> > We could potentially land this on a branch, but I'd like to have a concrete
> plan
> > for not breaking our users first.
> 
> Hey everyone, I've filed https://github.com/dart-lang/sdk/issues/27101 to
track
> possible improvements here.
> 
> In general, I'm not sure we can solve this perfectly without union types or
> overloads. For better or worse our type inference can't be handle all cases,
> given current Dart, as much as I wish it could

For Future.then, I think this is moving us in a much better direction.  Let's
continue discussion of related issues on the bug that John filed.

[keertip, 2016-08-18 17:51:06]

PTAL

https://codereview.chromium.org/2250513003/diff/1/lib/src/delegate/future.dart
File lib/src/delegate/future.dart (right):

https://codereview.chromium.org/2250513003/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:31: _future.then(onValue, onError: onError);
On 2016/08/16 22:36:24, John Messerly wrote:
> fyi -- probably want to pass /*<S>*/ explicitly here.
> 
> and we'll need to implement the custom Future inference, so this method gets
as
> good of inference when people call it as Future<T> would. With those changes,
> this should work as well as built in Future.then

Done.

https://codereview.chromium.org/2250513003/diff/1/lib/src/typed/future.dart
File lib/src/typed/future.dart (right):

https://codereview.chromium.org/2250513003/diff/1/lib/src/typed/future.dart#n...
lib/src/typed/future.dart:18: _future.then((value) => onValue(value as T),
onError: onError);
On 2016/08/16 22:37:26, John Messerly wrote:
> /*<S>*/ here too

Done.

[Jennifer Messerly, 2016-08-18 17:58:47]

LGTM. We should work on getting the inference fixed so these guys get the nice
inference Future.then. But this code is right for once that bug is fixed, so
it's nice preparation.

[keertip, 2016-08-18 18:03:09]

Description was changed from

==========
fix strong mode errors for 1.19.0-dev.4.0

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

to

==========
fix strong mode errors for 1.19.0-dev.4.0

BUG=
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/async/commit/c230c8128dc6f178743a917136461041036...
==========

[keertip, 2016-08-18 18:03:10]

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

[Leaf, 2016-08-18 18:25:45]

lgtm

[nweiz, 2016-08-18 20:11:16]

Please roll this back. All packages need to be compatible with the current
*stable* analyzer, which this is not. Even if this is the right change for a
post-1.19 world, it's not safe to release yet.

In general please don't commit a change without an LGTM from an owner of the
code in question.

[keertip, 2016-08-18 20:28:47]

nweiz@ PTAL - revert the change.

[Jennifer Messerly, 2016-08-18 20:29:38]

On 2016/08/18 20:11:16, nweiz wrote:
> Please roll this back. All packages need to be compatible with the current
> *stable* analyzer, which this is not. Even if this is the right change for a
> post-1.19 world, it's not safe to release yet.
> 
> In general please don't commit a change without an LGTM from an owner of the
> code in question.

Hi Natalie,

Could we have the package "master" be tracking SDK "master"? It seems like no
one on 1.18 is broken until we release it. Maybe the issue here is the SDK
constraint should be bumped to 1.19?

Where do we want to put the 1.19 compat version for 1.19 users? The signature of
Future.then is different in 1.19, so we've got to have that version somewhere...


BTW in terms of override errors, this isn't a breaking change, because it's
widening the signature:

    <S>(T -> *) -> Future<S>  should be a subtype of   <S>(T -> S) -> Future<S>

So the code here should work on 1.18 analyzer, I think. There is definitely the
inference issue for anyone who calls it, though, you're totally right about
that.

[keertip, 2016-08-18 20:42:29]

On 2016/08/18 20:29:38, John Messerly wrote:
> On 2016/08/18 20:11:16, nweiz wrote:
> > Please roll this back. All packages need to be compatible with the current
> > *stable* analyzer, which this is not. Even if this is the right change for a
> > post-1.19 world, it's not safe to release yet.
> > 
> > In general please don't commit a change without an LGTM from an owner of the
> > code in question.
> 
> Hi Natalie,
> 
> Could we have the package "master" be tracking SDK "master"? It seems like no
> one on 1.18 is broken until we release it. Maybe the issue here is the SDK
> constraint should be bumped to 1.19?
> 
> Where do we want to put the 1.19 compat version for 1.19 users? The signature
of
> Future.then is different in 1.19, so we've got to have that version
somewhere...
> 
> 
> BTW in terms of override errors, this isn't a breaking change, because it's
> widening the signature:
> 
>     <S>(T -> *) -> Future<S>  should be a subtype of   <S>(T -> S) ->
Future<S>
> 
> So the code here should work on 1.18 analyzer, I think. There is definitely
the
> inference issue for anyone who calls it, though, you're totally right about
> that.

The code works with the 1.18 analyzer, there are no strong mode errors.

[nweiz, 2016-08-18 22:14:59]

On 2016/08/18 20:29:38, John Messerly wrote:
> On 2016/08/18 20:11:16, nweiz wrote:
> > Please roll this back. All packages need to be compatible with the current
> > *stable* analyzer, which this is not. Even if this is the right change for a
> > post-1.19 world, it's not safe to release yet.
> > 
> > In general please don't commit a change without an LGTM from an owner of the
> > code in question.
> 
> Hi Natalie,
> 
> Could we have the package "master" be tracking SDK "master"? It seems like no
> one on 1.18 is broken until we release it. Maybe the issue here is the SDK
> constraint should be bumped to 1.19?

The master branch is generally code that could be released as-is. In other
words, we want it to be the case that if we need to make a bug fix or add a new
single-CL feature, we can safely make the change to master and release it
without worrying about compatibility issues.

> Where do we want to put the 1.19 compat version for 1.19 users? The signature
of
> Future.then is different in 1.19, so we've got to have that version
somewhere...

Historically we've created a separate branch (e.g. strong-1.19) that contains
future-compatible strong-mode changes.

> BTW in terms of override errors, this isn't a breaking change, because it's
> widening the signature:
> 
>     <S>(T -> *) -> Future<S>  should be a subtype of   <S>(T -> S) ->
Future<S>
> 
> So the code here should work on 1.18 analyzer, I think. There is definitely
the
> inference issue for anyone who calls it, though, you're totally right about
> that.

That's a good point, but I am worried about the inference regression. I'm
pushing for landing Future subclass checking in the same stable SDK release as
this signature change, so we can safely pin the signature change to that release
and avoid any regressions.

[nweiz, 2016-08-18 22:15:22]

LGTM

[Leaf, 2016-08-18 22:18:27]

On 2016/08/18 22:15:22, nweiz wrote:
> LGTM

I'm looking at subclass Future inference now.

[keertip, 2016-08-18 23:12:01]

Description was changed from

==========
fix strong mode errors for 1.19.0-dev.4.0

BUG=
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/async/commit/c230c8128dc6f178743a917136461041036...
==========

to

==========
fix strong mode errors for 1.19.0-dev.4.0

BUG=
R=jmesserly@google.com, leafp@google.com, nweiz@google.com

Committed:
https://github.com/dart-lang/async/commit/c230c8128dc6f178743a917136461041036...
Committed:
https://github.com/dart-lang/async/commit/7db0ec0b31b7d63c958dd3488ac99b46c2d...
==========

[keertip, 2016-08-18 23:12:02]

Committed patchset #3 (id:40001) manually as
7db0ec0b31b7d63c958dd3488ac99b46c2d3bead (presubmit successful).