fix future onError handler wrapping to avoid cast error

fix future onError handler wrapping to avoid cast error

the return type was invalid, which caused a runtime cast failure. Many of the onError handlers return `void` which is not assignable to T.

This was discovered in internal testing as we attempted to run DDC against the unforked SDK sources.

R=floitsch@google.com

Committed: https://github.com/dart-lang/sdk/commit/18c4f5cf08e43366635f1be97532a9033247f5dc

[Jennifer Messerly, 2017-02-24 20:15:45]

Description was changed from

==========
fix future onError handler wrapping to avoid cast error

the return type was invalid, which caused a runtime cast failure. Many of the
onError handlers return `void` which is not assignable to T.
==========

to

==========
fix future onError handler wrapping to avoid cast error

the return type was invalid, which caused a runtime cast failure. Many of the
onError handlers return `void` which is not assignable to T.

This was discovered in internal testing as we attempted to run DDC against the
unforked SDK sources.
==========

[Jennifer Messerly, 2017-02-24 20:16:58]

jmesserly@google.com changed reviewers:
+ floitsch@google.com, lrn@google.com

[Jennifer Messerly, 2017-02-24 20:16:59]

Hi Lasse, could you take a look?

Thanks to Vijay for helping me track this down via internal tests.

[Jennifer Messerly, 2017-02-24 20:18:40]

this is related to this bug: https://github.com/dart-lang/sdk/issues/27314,
about unforking DDC's SDK.

[floitsch, 2017-02-27 13:19:04]

With the exception of the mistake in future_impl.dart:240 the types here are
correct.
The result of the `catchError` (or `onError`) functions is used to complete the
"next" future. Unless that future is a `Future<void>` or `Future<dynamic>` we
have to get a value from the function.

For example:

```
var f = new Future<int>.sync(() => int.parse("bad"));
var f2 = f.catchError(dealWithError);
f2.then(useAsInt);
```

The `dealWithError` function *must* return an `int`, as `f2` is typed as
`Future<int>`.

https://codereview.chromium.org/2718643003/diff/1/sdk/lib/async/future_impl.dart
File sdk/lib/async/future_impl.dart (left):

https://codereview.chromium.org/2718643003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:240: onError = _registerErrorHandler<T>(onError,
currentZone);
This was a mistake. It should have been `_registerErrorHandler<E>`.

[Jennifer Messerly, 2017-02-27 18:41:02]

thanks!

https://codereview.chromium.org/2718643003/diff/1/sdk/lib/async/future_impl.dart
File sdk/lib/async/future_impl.dart (left):

https://codereview.chromium.org/2718643003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:240: onError = _registerErrorHandler<T>(onError,
currentZone);
On 2017/02/27 13:19:04, floitsch wrote:
> This was a mistake. It should have been `_registerErrorHandler<E>`.

okay. I can fix it that way, however: at least a couple of places in the SDK use
"void" as the return type of the error handler.

Are those locations correct? Should they return a value of some sort? I can
change the return type and return null.

[floitsch, 2017-02-27 19:02:39]

https://codereview.chromium.org/2718643003/diff/1/sdk/lib/async/future_impl.dart
File sdk/lib/async/future_impl.dart (left):

https://codereview.chromium.org/2718643003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:240: onError = _registerErrorHandler<T>(onError,
currentZone);
On 2017/02/27 18:41:02, Jennifer Messerly wrote:
> On 2017/02/27 13:19:04, floitsch wrote:
> > This was a mistake. It should have been `_registerErrorHandler<E>`.
> 
> okay. I can fix it that way, however: at least a couple of places in the SDK
use
> "void" as the return type of the error handler.
> 
> Are those locations correct? Should they return a value of some sort? I can
> change the return type and return null.

I will have a look tomorrow.
If you have these places handy, please send them to me. Otherwise I will grep
for them tomorrow myself.

[Jennifer Messerly, 2017-02-28 18:50:00]

here's the alternate fix.

[floitsch, 2017-02-28 19:04:35]

Thanks!
LGTM.

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

https://codereview.chromium.org/2718643003/diff/20001/sdk/lib/async/future.da...
sdk/lib/async/future.dart:307: handleError(theError, theStackTrace) {
This looks like a temporary problem.

The 'then' in line 335, gets a 'void' function as argument. I'm not sure what
strong mode infers for the function literal, but it must be either `T->Null` or
`T->dynamic`. It should be `T->void`, in which case the `handleError` function
would have the correct type.

I think we should keep a TODO here, to add the `void` back, once `void` is
correctly inferred.

[Jennifer Messerly, 2017-02-28 19:23:31]

thank you!

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

https://codereview.chromium.org/2718643003/diff/20001/sdk/lib/async/future.da...
sdk/lib/async/future.dart:307: handleError(theError, theStackTrace) {
On 2017/02/28 19:04:35, floitsch wrote:
> This looks like a temporary problem.
> 
> The 'then' in line 335, gets a 'void' function as argument. I'm not sure what
> strong mode infers for the function literal, but it must be either `T->Null`
or
> `T->dynamic`. It should be `T->void`, in which case the `handleError` function
> would have the correct type.
> 
> I think we should keep a TODO here, to add the `void` back, once `void` is
> correctly inferred.

sounds great! I'll add that TODO, does this look good?

TODO(jmesserly): use `void` return type once it can be inferred for the `then`
call below.

[Jennifer Messerly, 2017-02-28 22:59:55]

Description was changed from

==========
fix future onError handler wrapping to avoid cast error

the return type was invalid, which caused a runtime cast failure. Many of the
onError handlers return `void` which is not assignable to T.

This was discovered in internal testing as we attempted to run DDC against the
unforked SDK sources.
==========

to

==========
fix future onError handler wrapping to avoid cast error

the return type was invalid, which caused a runtime cast failure. Many of the
onError handlers return `void` which is not assignable to T.

This was discovered in internal testing as we attempted to run DDC against the
unforked SDK sources.

R=floitsch@google.com

Committed:
https://github.com/dart-lang/sdk/commit/18c4f5cf08e43366635f1be97532a9033247f5dc
==========

[Jennifer Messerly, 2017-02-28 22:59:57]

Committed patchset #3 (id:40001) manually as
18c4f5cf08e43366635f1be97532a9033247f5dc (presubmit successful).

[floitsch, 2017-03-01 08:52:16]

TODO LGTM.
thanks again.