Fix covariant overrides in SynchronousFuture. #5262
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There were two things going on here. In timeout(), the callback's return type was needlessly tightened to only allow callbacks that return futures. This makes SynchronousFuture not substitutable with Future, whose timeout() allows callbacks that return immediate values. Since SynchronousFuture.timeout() never calls the callback anyway, I just loosened it to match Future.timeout(). SynchronousFuture.whenComplete() is just wrong. The type error, again, is that the callback's return type is too tight. Future.whenComplete() allows synchronous callbacks. But the actual implementation is wrong as well. whenComplete() should return a future that completes to the *original value*, not whatever the callback returns. So I just fixed the method to work correctly, including handling callbacks with synchronous results.
Contributor
|
/cc @Hixie |
| Future<T> whenComplete(dynamic action()) { | ||
| try { | ||
| dynamic result = action(); | ||
| if (result is Future) |
Contributor
There was a problem hiding this comment.
I'm surprised the analyzer didn't complain about the missing type here, but I don't see value in being explicit about the <dynamic> so that's fine I guess!
Contributor
Author
There was a problem hiding this comment.
Yeah, I think they tried to warn on this at one point, but it causes too much noise. Especially with Future, it's really common to use the raw type for things like an asynchronous method that just does a side effect and doesn't return a useful value.
Contributor
|
|
aam
added a commit
to aam/flutter
that referenced
this pull request
May 17, 2018
Changes since last roll: ``` 4374ccc Roll Dart to 43635d3372253262cbf51e55b2ccfceae4f94682. (flutter#5282) 8862465 [fuchsia] Update FIDL include paths. (flutter#5279) a7b44d4 Only send a11y events if a11y is turned on (flutter#5281) f218099 Roll src/third_party/skia/ 02faa2b99..94f585ed0 (8 commits) (flutter#5280) 30c649d [fuchsia] Teach engine how to set up an isolate from a list of kernel files. (flutter#5210) c95d432 Roll src/third_party/skia/ 682c58da4..02faa2b99 (10 commits) (flutter#5277) bbfe593 Enable flutter service protocol rpcs to run on UI isolate. (flutter#5263) 4dc0183 Roll src/third_party/skia/ 4c2a34e48..682c58da4 (1 commit) (flutter#5276) ff884db Roll src/third_party/skia/ 5b8b472b3..4c2a34e48 (1 commit) (flutter#5274) d6f0f2f Roll src/third_party/skia/ c8799aa92..5b8b472b3 (7 commits) (flutter#5273) d1d5497 Drain any pending work on the IO thread before shutting down the platform view (flutter#5272) e32e390 Roll Dart to a5c11d7d0329432ca37e35bb249b20f60aa0aa31. (flutter#5269) db34bb6 Roll buildroot to 78cf6d8 for newer GN for Windows. (flutter#5271) 645c15f Roll src/third_party/skia/ 6e9f34f0e..c8799aa92 (13 commits) (flutter#5270) 99b3262 Mark the linux group testonly (flutter#5268) 17a71f6 Build the flutter tester on Linux in the default group. (flutter#5267) 63fdebf Revert "Roll Dart to a5c11d7d0329432ca37e35bb249b20f60aa0aa31. (flutter#5259)" (flutter#5266) 755dbee Roll Dart to a5c11d7d0329432ca37e35bb249b20f60aa0aa31. (flutter#5259) 73a0014 Create an empty group that the Fuchsia bots use to determine the root_out_dir. (flutter#5265) c7ab033 Support a model where the application creates a FlutterNativeView that is never destroyed (flutter#5256) e463e80 Roll src/third_party/skia/ 81f60ecd9..6e9f34f0e (7 commits) (flutter#5264) be6c517 Fix documentation mistake in painting.dart (flutter#5236) fb303b9 Roll src/third_party/skia/ 6bbd386a0..81f60ecd9 (1 commit) (flutter#5262) 765b7d4 Roll src/third_party/skia/ 3b9effcb1..6bbd386a0 (2 commits) (flutter#5260) c3c6c36 Create a session presentation backed Vsync waiter on Fuchsia. (flutter#5255) f943345 Roll src/third_party/skia/ 8803ebb47..3b9effcb1 (8 commits) (flutter#5257) f12b5a5 Roll src/third_party/skia/ 16ffdd4ed..8803ebb47 (8 commits) (flutter#5254) cde5014 Roll src/third_party/skia/ 5140f9a8e..16ffdd4ed (4 commits) (flutter#5252) 4a5d8bc Roll src/third_party/skia/ b06a1eb4e..5140f9a8e (1 commit) (flutter#5251) 966ef68 Roll src/third_party/skia/ ec48812c5..b06a1eb4e (1 commit) (flutter#5250) fb89534 Roll src/third_party/skia/ 96b0b46f2..ec48812c5 (1 commit) (flutter#5249) 5f57c59 Roll src/third_party/skia/ 3202ac4d2..96b0b46f2 (1 commit) (flutter#5248) ```
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were two things going on here. In timeout(), the callback's return
type was needlessly tightened to only allow callbacks that return
futures. This makes SynchronousFuture not substitutable with Future,
whose timeout() allows callbacks that return immediate values.
Since SynchronousFuture.timeout() never calls the callback anyway, I
just loosened it to match Future.timeout().
SynchronousFuture.whenComplete() is just wrong. The type error, again,
is that the callback's return type is too tight. Future.whenComplete()
allows synchronous callbacks.
But the actual implementation is wrong as well. whenComplete() should
return a future that completes to the original value, not whatever the
callback returns.
So I just fixed the method to work correctly, including handling
callbacks with synchronous results.