|
|
Created:
4 years, 4 months ago by keertip Modified:
4 years, 4 months ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/async.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Descriptionfix 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
Patch Set 1 #
Total comments: 4
Patch Set 2 : add explicit /*<S>*/ to .then #Patch Set 3 : Revert "fix strong mode errors for 1.19.0-dev.4.0" #
Messages
Total messages: 36 (7 generated)
keertip@google.com changed reviewers: + nweiz@google.com
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...
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.
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@google.com changed reviewers: + jmesserly@google.com
vsm@google.com changed reviewers: + vsm@google.com
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?
jmesserly@google.com changed reviewers: + leafp@google.com
+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
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.
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
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
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.
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.
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.
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.
with the explicit /*<S>*/ added to _future.then it looks good to me
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.
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
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.
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.
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.
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... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as c230c8128dc6f178743a917136461041036b4157 (presubmit successful).
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
nweiz@ PTAL - revert the change.
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
LGTM
Message was sent while issue was closed.
On 2016/08/18 22:15:22, nweiz wrote: > LGTM I'm looking at subclass Future inference now.
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 7db0ec0b31b7d63c958dd3488ac99b46c2d3bead (presubmit successful). |