From 61a4fb4f9ee2a8a0c35fa788462ebd581aade05d Mon Sep 17 00:00:00 2001 From: "Lasse R.H. Nielsen" Date: Sat, 18 Feb 2023 12:01:45 +0000 Subject: [PATCH] Update `Future.onError` extension method. Now works better with futures that have been up-cast. CoreLibraryReviewExempt: Rewrite of existing pure Dart function. Change-Id: Iefd05b9cfd8ff0cc3e27fc1122a670030e0901ff Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/283680 Reviewed-by: Nate Bosch Commit-Queue: Lasse Nielsen --- sdk/lib/async/future.dart | 25 +- sdk/lib/async/future_impl.dart | 17 ++ tests/lib/async/future_onerror_test.dart | 293 +++++++++++++++-------- 3 files changed, 231 insertions(+), 104 deletions(-) diff --git a/sdk/lib/async/future.dart b/sdk/lib/async/future.dart index 65155ef96f9..dc002ccc1f6 100644 --- a/sdk/lib/async/future.dart +++ b/sdk/lib/async/future.dart @@ -748,8 +748,11 @@ abstract class Future { /// and a stack trace for the error. /// In the case of `onError`, /// if the exception thrown is `identical` to the error argument to `onError`, + /// and it is thrown *synchronously* /// the throw is considered a rethrow, /// and the original stack trace is used instead. + /// To rethrow with the same stack trace in an asynchronous callback, + /// use [Error.throwWithStackTrace]. /// /// If the callback returns a [Future], /// the future returned by `then` will be completed with @@ -1054,12 +1057,20 @@ extension FutureExtensions on Future { Future onError( FutureOr handleError(E error, StackTrace stackTrace), {bool test(E error)?}) { - // There are various ways to optimize this to avoid the double is E/as E - // type check, but for now we are not optimizing the error path. - return this.catchError( - (Object error, StackTrace stackTrace) => - handleError(error as E, stackTrace), - test: (Object error) => error is E && (test == null || test(error))); + FutureOr onError(Object error, StackTrace stackTrace) { + if (error is! E || test != null && !test(error)) { + // Counts as rethrow, preserves stack trace. + throw error; + } + return handleError(error, stackTrace); + } + if (this is _Future) { + // Internal method working like `catchError`, + // but allows specifying a different result future type. + return unsafeCast<_Future>(this)._safeOnError(onError); + } + + return this.then((T value) => value, onError: onError); } /// Completely ignores this future and its result. @@ -1110,7 +1121,7 @@ class TimeoutException implements Exception { /// A way to produce Future objects and to complete them later /// with a value or error. /// -/// Most of the time, the simplest way to create a future is to just use +/// Most of the time, the simples t way to create a future is to just use /// one of the [Future] constructors to capture the result of a single /// asynchronous computation: /// ```dart diff --git a/sdk/lib/async/future_impl.dart b/sdk/lib/async/future_impl.dart index f8fb75052df..812fff45c0a 100644 --- a/sdk/lib/async/future_impl.dart +++ b/sdk/lib/async/future_impl.dart @@ -349,6 +349,23 @@ class _Future implements Future { return result; } + // Used by extension method `onError` to up-cast the result to `R`. + // + // Not public because we cannot statically ensure that [R] is a supertype + // of [T]. + // + // Avoids needing to allocate an extra value handler to do the up cast, + // which is needed if using `.then`. + Future _safeOnError(FutureOr Function(Object, StackTrace) onError) { + assert(this is _Future); // Is up-cast. + _Future result = _Future(); + if (!identical(result._zone, _rootZone)) { + onError = result._zone.registerBinaryCallback(onError); + } + _addListener(_FutureListener.catchError(result, onError, null)); + return result; + } + Future whenComplete(dynamic action()) { _Future result = new _Future(); if (!identical(result._zone, _rootZone)) { diff --git a/tests/lib/async/future_onerror_test.dart b/tests/lib/async/future_onerror_test.dart index ea5ffab9acc..307f76e9dac 100644 --- a/tests/lib/async/future_onerror_test.dart +++ b/tests/lib/async/future_onerror_test.dart @@ -2,114 +2,213 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -import 'package:async_helper/async_helper.dart'; +import "dart:async"; + +import "package:async_helper/async_helper.dart"; import "package:expect/expect.dart"; main() { + // Values used. + var error = StateError("test"); var stack = StackTrace.fromString("for testing"); - var error = Object(); - var stateError = StateError("test"); + var expectedError = AsyncError(error, stack); + + var otherError = ArgumentError("Other error"); + var expectedOtherError = AsyncError(otherError, StackTrace.empty); + + var errorFuture = Future.error(error, stack)..ignore(); + var nonNativeErrorFuture = NonNativeFuture._(errorFuture); asyncStart(); - { - // Match any error. + /// Perform `onError` on [future] with arguments, and expect [result]. + /// + /// If [result] is [AsyncError], expecte an error result with + /// that error and stack trace. + /// If [result.stackTrace] is [StackTrace.empty], + /// check that the stack trace is *not* the original [stack]. + /// + /// Otherwise expect the future to complete with value equal to [result]. + void test( + String testName, + Future future, + FutureOr Function(E, StackTrace) onError, { + bool Function(E)? test, + Object? result, + }) { asyncStart(); - var future = Future.error(error, stack); - future.onError((e, s) { - Expect.identical(error, e); - Expect.identical(stack, s); - asyncEnd(); - }); + var resultFuture = future.onError(onError, test: test); + + if (result is AsyncError) { + resultFuture.then((value) { + Expect.fail("$testName: Did not throw, value: $value"); + }, onError: (Object error, StackTrace stackTrace) { + Expect.identical(result.error, error, testName); + if (result.stackTrace != StackTrace.empty) { + Expect.identical(result.stackTrace, stackTrace, testName); + } else { + Expect.notEquals(stack, stackTrace, testName); + } + asyncEnd(); + }); + } else { + resultFuture.then((value) { + Expect.equals(result, value); + asyncEnd(); + }, onError: (error, stackTrace) { + Expect.fail("$testName: Threw $error"); + }); + } } - { - // With matching test. - asyncStart(); - var future = Future.error(error, stack); - future.onError((e, s) { - Expect.identical(error, e); - Expect.identical(stack, s); - asyncEnd(); - }, test: (o) => true); + // Go through all the valid permutations of the following options: + // + // * Native future or non-native future. + // * The error type parameter matches the error or not. + // * If type matches, test function is provided or not. + // * If test function provided, it returns either true or false. + // * If type and test both succeeds, + // * Error handler is synchronous or asynchronous + // * Error handler returns a value, throws new error, + // or rethrows same error. + // * The future is up-cast to a supertype or not when `onError` is called. + // (Since `onError` is an extension method, it affects the method.) + // + // The expected result is computed for each combination, + // and checked against the behavior: + // * If type or test fails to match, + // the original error and stack is the error result + // * If type matches and test doesn't reject, + // the error handler is invoked. + // * If it returns a value, that becomes the result. + // * If upcast, a return value of the supertype is accepted. + // * If it throws a new error, that is the error result. + // * If it throws the original error *synchronously*, + // the original stack trace is retained. + // * If it throws the original error *asynchronously*, + // the original stack trace is not expected to be retained. + for (var native in [true, false]) { + var nativity = native ? "native" : "non-native"; + var future = native ? errorFuture : nonNativeErrorFuture; + for (var upcast in [true, false]) { + var upcastText = upcast ? " as Future" : ""; + // "fails" means not to match type parameter. + // Rest means match type parameter, a bool is what `test` returns, + // `null` means no test function. + for (var testKind in ["fails", true, false, null]) { + var matches = testKind == true || testKind == null; + // If `matches` is false, the error handler should not be invoked. + // Don't bother with creating sync/async versions of it then, + // just one which fails if called. + for (var async in [if (matches) true, false]) { + var asyncText = async ? "async" : "sync"; + // Action of the handler body. If test doesn't match, it will fail. + for (var action + in matches ? ["return", "throw", "rethrow"] : ["fail"]) { + // Called to provide the necessary types as type parameters. + void doTest() { + String testName = + "$nativity Future.error(StateError(..))$upcastText "; + + bool Function(E)? testFunction; + switch (testKind) { + case "fails": + testFunction = (E value) { + Expect.fail("$testName: Matched error"); + throw "unreachable"; // Expect.fail throws. + }; + break; + case true: + case false: + var testResult = testKind == true; + testFunction = (E value) => testResult; + testName += + "matches type $E ${testResult ? "and test" : "but not test"}"; + break; + default: + testName += "matches type with no test"; + } + Object? expectation = expectedError; + FutureOr Function(E, StackTrace) onError; + switch (action) { + case "return": + expectation = upcast ? 3.14 : 42; + testName += " and returns $expectation"; + onError = (E e, StackTrace s) { + return expectation as T; + }; + break; + case "throw": + expectation = expectedOtherError; + onError = (E e, StackTrace s) { + throw otherError; + }; + testName += " and throws new error"; + break; + case "rethrow": + onError = (E e, StackTrace s) { + throw e; + }; + testName += " and rethrows"; + // Asynchronous callbacks rethrowing the error + // doesn't count as a rethrow, only synchronous throws. + if (async) expectation = AsyncError(error, StackTrace.empty); + break; + case "fail": + onError = (E e, StackTrace s) { + Expect.fail("$testName: Matched error"); + throw "unreachable"; // Expect.fail throws. + }; + testName += " and rethrows original error"; + break; + default: + throw "unreachable"; + } + if (async) { + testName += " asynchronously"; + var originalOnError = onError; + onError = (E e, StackTrace s) async => originalOnError(e, s); + } + test(testName, future as Future, onError, + test: testFunction, result: expectation); + } + + // Find the types to use. + if (testKind == "fails") { + if (upcast) { + doTest(); + } else { + doTest(); + } + } else { + if (upcast) { + doTest(); + } else { + doTest(); + } + } + } + } + } + } } - { - // With failing test. - asyncStart(); - var future = Future.error(error, stack); - var onErrorFuture = future.onError((e, s) { - Expect.fail('unreachable'); - }, test: (o) => false); - onErrorFuture.catchError((e, s) { - Expect.identical(error, e); - Expect.identical(stack, s); - asyncEnd(); - }); - } - - { - // With matching type. - asyncStart(); - var future = Future.error(stateError, stack); - future.onError((e, s) { - Expect.identical(stateError, e); - Expect.identical(stack, s); - asyncEnd(); - }); - } - - { - // With non-matching type. - asyncStart(); - var future = Future.error(stateError, stack); - var onErrorFuture = future.onError((e, s) { - Expect.fail('unreachable'); - }); - onErrorFuture.catchError((e, s) { - Expect.identical(stateError, e); - Expect.identical(stack, s); - asyncEnd(); - }); - } - - { - // With non-matching type and matching test. - asyncStart(); - var future = Future.error(stateError, stack); - var onErrorFuture = future.onError((e, s) { - Expect.fail('unreachable'); - }, test: (ArgumentError e) => true); - onErrorFuture.catchError((e, s) { - Expect.identical(stateError, e); - Expect.identical(stack, s); - asyncEnd(); - }); - } - - { - // With matching type and matching test. - asyncStart(); - var future = Future.error(stateError, stack); - future.onError((e, s) { - Expect.identical(stateError, e); - Expect.identical(stack, s); - asyncEnd(); - }, test: (StateError e) => true); - } - - { - // With matching type and non-matching test. - asyncStart(); - var future = Future.error(stateError, stack); - var onErrorFuture = future.onError((e, s) { - Expect.fail('unreachable'); - }, test: (StateError e) => false); - onErrorFuture.catchError((e, s) { - Expect.identical(stateError, e); - Expect.identical(stack, s); - asyncEnd(); - }); - } asyncEnd(); } + +class NonNativeFuture implements Future { + Future _original; + NonNativeFuture.value(T value) : _original = Future.value(value); + NonNativeFuture.error(Object error, [StackTrace? stack]) + : _original = Future.error(error, stack)..ignore(); + NonNativeFuture._(this._original); + Future then(FutureOr Function(T) handleValue, {Function? onError}) => + NonNativeFuture._(_original.then(handleValue, onError: onError)); + Future catchError(Function handleError, {bool Function(Object)? test}) => + NonNativeFuture._(_original.catchError(handleError, test: test)); + Future whenComplete(FutureOr Function() onDone) => + NonNativeFuture._(_original.whenComplete(onDone)); + Future timeout(Duration timeLimit, {FutureOr Function()? onTimeout}) => + NonNativeFuture._(_original.timeout(timeLimit, onTimeout: onTimeout)); + Stream asStream() => _original.asStream(); +}