sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Change error handlers accepting `Function` to be typed.

Open lrhn opened this issue 1 year ago • 0 comments

The dart:async library has a number of error callback functions which are typed as Function, so that they can allow both a ... Function(Object) and Function(Object, StackTrace) as argument.

Accepting both was chosen in the Dart 1 days, where static typing wasn't a big deal. Today those Function types are a liability. Mainly because it hides the return type.

The Future<T>.catchError, which requires the error handler to return a FutureOr<T>, tries very hard to not enforce that too strictly, beacuse someone writing the callback as a function literal, (e, s) {...}, may get some other type inferred, even if their value will end up being a Future<T>, and because function literals cannot declare a return type, it's very hard to fix. (Which also means that a signficant number of existing functions have that problem, so changing the behavior is breaking.)

Other error handlers are more or less strict, because it's been handled ad-hoc.

I propose that we aim to changing the function signatures to require a binary callback:

class Future<T> { 
  // ...
  Future<R> then<R>(FutureOr<R> onValue(T value), {FutureOr<R> Function(Object, StackTrace)? onError});  
  Future<T> catchError(FutureOr<T> Function(Object, StackTrace) handleError, {bool Function(Object)? test});
}
class Stream<T> ... {
  // ...
  Stream<T> handleError(void Function(Object, StackTrace) onError, {bool Function(Object)? test});
  StreamSubscription<T> listen(void onData(T value)?,
      {void Function(Object, StackTrace)? onError, void onDone()?, bool? cancelOnError}) {
}
class StreamSubscription<T> {
  void onError(void Function(Object, StackTrace) onError);
}

Of these, only then and catchError have return types, so they are the more important ones. The rest will still get the advantage of not having to do is Function(Object) and is Function(Object, StackTrace) checks.

Migration

Changing the accepted type will break existing programs.

I propose to do a two-step, language-version based migration:

Upgrade signature types seen in new language-version libraries.

In language version 3.X we change the member signature types of the members to the above, so libraries with that language version or above will see that type, and prior language version libraries see the original type.

Since the original type, which is still the implementation's type, is more lenient, all existing code will keep working. Classes implementing the interfaces, and satisfying the older type, are still valid overrides and implementations of the new types.

Code which switchs to the 3.X language version will get errors where their arguments do not match the new type. There should then be a fix or migration which changes an argument expression e to (Object $error, StackTrace _) => e($error). That should preserve the behavior of the program.

If the return type is still not satisfactory, we can throw in a cast, (Object $error, StackTrace _) => e($error) as FutureOr<T> if the required type is FutureOr<T>. Or something even more complicated, if we want a Future<dynamic> to be accepted if its value is a T. (But I don't think we currently accept that.) That should only be needed for Future.then and Future.catchError, which are the ones with return type requirements.

After a while, all active code will have migrated to Dart 3.X, and nobody are using the old calling convention any more.

Remove old member signatures

Then, in Dart 4.0 (earlier if we dare a breaking change on a non-major-version increment), we change the implementation to the new API, and remove the ability to call with the old API.

This two step process gives us both a point to migrate at (someone upgrading to Dart 3.X language version) and a trigger for automatic migration, and a way to keep existing implementations valid.

Alternative: Adding new API instead of removing the old.

We can also add new functionality with better API instead of removing the old.

That does not appear to work. We added Future.onError as a better Future.catchError in Dart 2.12. I haven't seen much adoption. It is being used a little, but only sparsely.

Also, adding new API to Future is possibly harder than changing what's there to be more restrictive, because the latter keeps existing subclasses valid. We maed onError an extension member because we can't add members to widely implemented interfaces. (Here's another hope for "interface default methods", #30416.)

lrhn avatar Feb 16 '24 15:02 lrhn