Fix null safe tests #37
Conversation
|
cc @kevmoo all tests pass now with strong null safety |
|
One possible area of improvement would be to improve the type checks here https://github.com/dart-lang/sdk/blob/master/sdk_nnbd/lib/async/future_impl.dart#L820 to actually check the return type? Something like this: Function _registerErrorHandler<R>(Function errorHandler, Zone zone) {
if (errorHandler is R Function(Object, StackTrace)) {
return zone
.registerBinaryCallback<dynamic, Object, StackTrace>(errorHandler);
}
if (errorHandler is R Function(Object)) {
return zone.registerUnaryCallback<dynamic, Object>(errorHandler);
}
throw new ArgumentError.value(
errorHandler,
"onError",
"Error handler must accept one Object or one Object and a StackTrace"
" as arguments, and return a a valid result");
}I think that should be sufficient to at least highlight the true error better. |
|
I don't have any good not-very-very-breaking way to fix Checking the function type is dangerous because existing code my pass in functions with return type My personal wish is that one day we drop the one-parameter error handler and require two parameters on all error handlers. Then we can give the parameter a proper type. Another option is to allow An intermediate approach we could take is to add an extension method like: extension FX<T> on Future<T> {
Future<T> onError<E>(T Function(E error, StackTrace stack), [bool test(E error)?]) { ... }
}and convince people to move from So, for now, use |
This is really really nasty and took a couple hours to trace down.
The
catchErrorcallback is now required to return a valid value for the future, or else you get a TypeError from deep within the bowels of the future implementation, and it is really really hard to trace it back to the root cause.cc @lrhn is there anything we can do about this? With no static enforcement on this callback I am worried how many other people will lose many hours or days trying to track this down...
Example stack trace, which given the known solution makes sense but if you aren't aware of this new requirement is quite cryptic: