Make Zone functions not call register*, but properly catch errors.

Make `Zone.createTimer`, `Zone.createPeriodicTimer` and `Zone.scheduleMicrotask`
not call register on the same zone.
These are low-level functions that should not build on top
of other low-level functions.

Instead the function is registered in the `Timer` constructors and
top-level `scheduleMicrotask` code, and the root zone's `createTimer`
and `scheduleMicrotask` just make sure that the callback will run
in its original zone, and that uncaught errors are handled in the
correct zone.
Avoids a double-registration that timers did.

Significant clean-up.
- The private `_Zone`, `_ZoneSpecification` and `_ZoneDelegate`
  subclasses are not necessary now that their superclasses are `final`.
- Stopped using type aliases instead of function types
  and old-style function arguments.
- Renamed some parameters to not be, fx, `f`.

Avoided some closure allocations for microtasks by putting the zone
into the queue entry, instead of wrapping the callback in a closure calling `zone.run`.

This is a potentially visible change if any code uses the zone functions
directly and expect them to invoke `Zone.register*`.
(Suspiciously no test failed with this change. May need to add some.)

Fixes #59913.

CoreLibraryReviewExempt: Have had all +1s, just not at the same time.
Bug: https://dartbug.com/59913
Change-Id: Ie3c87f5f22aedcbe30ab04718df98e1c0f0659c3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/405020
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Lasse Nielsen <lrn@google.com>

Author: lrhn

CHANGELOG.md

@@ -4,6 +4,27 @@
 
 ### Libraries
 
+#### `dart:async`
+
+- The `Timer` and `Timer.periodic` constructors and the top-level
+  `scheduleMicrotask` function no longer *bind* and *guard* their callback
+  in the current zone, using `Zone.bindCallbackGuarded` and
+  `Zone.bindUnaryCallbackGuarded`. They only registers the callback
+  before calling the zone's `Zone.createTimer`, `Zone.createPeriodicTimer`
+  or `Zone.scheduleMicrotask` with the registered function.
+- The default `Zone.createTimer` and `Zone.createPeriodicTimer`,
+  no longer registers the callback in the current zone, which would
+  be a second registration if called through `Timer` or `Timer.periodic`.
+- The default (root) zone's `Zone.createTimer`, `Zone.createPeriodicTimer`
+  and `Zone.scheduleMicrotask` now always run the callback using
+  `Zone.run`/`Zone.runUnary`, and catch and reports errors from that
+  to the same zone, rather than relying on the callback
+  having been bound (and guarded) in the zone.
+- Together these changes ensure that if a custom zone's
+  `Zone.registerCallback` (or similar for a unary or binary function)
+  returns a new function which can throw, that thrown error is reported
+  in the correct error zone.
+
 #### `dart:core`
 
 - Added `Iterable.withIterator` constructor.

pkg/front_end/testcases/nnbd/return_async.dart.strong.expect

@@ -21,7 +21,7 @@ static method allYield3() → asy::Future<void> async /* emittedValueType= void
   self::throwSync();
 }
 static method customErrorZone() → asy::Future<void> async /* emittedValueType= void */ {
-  final asy::Completer<void> completer = asy::Completer::•<void>();
+  final asy::Completer<void> completer = new asy::_AsyncCompleter::•<void>();
   asy::runZonedGuarded<asy::Future<Null>>(() → asy::Future<Null> async /* emittedValueType= Null */ {
     await self::allYield();
     completer.{asy::Completer::complete}(null){([FutureOr<void>?]) → void};

pkg/front_end/testcases/nnbd/return_async.dart.strong.modular.expect

@@ -21,7 +21,7 @@ static method allYield3() → asy::Future<void> async /* emittedValueType= void
   self::throwSync();
 }
 static method customErrorZone() → asy::Future<void> async /* emittedValueType= void */ {
-  final asy::Completer<void> completer = asy::Completer::•<void>();
+  final asy::Completer<void> completer = new asy::_AsyncCompleter::•<void>();
   asy::runZonedGuarded<asy::Future<Null>>(() → asy::Future<Null> async /* emittedValueType= Null */ {
     await self::allYield();
     completer.{asy::Completer::complete}(null){([FutureOr<void>?]) → void};

pkg/front_end/testcases/nnbd/return_async.dart.strong.transformed.expect

@@ -21,7 +21,7 @@ static method allYield3() → asy::Future<void> async /* emittedValueType= void
   self::throwSync();
 }
 static method customErrorZone() → asy::Future<void> async /* emittedValueType= void */ {
-  final asy::Completer<void> completer = asy::Completer::•<void>();
+  final asy::Completer<void> completer = new asy::_AsyncCompleter::•<void>();
   asy::runZonedGuarded<asy::Future<Null>>(() → asy::Future<Null> async /* emittedValueType= Null */ {
     await self::allYield();
     completer.{asy::Completer::complete}(null){([FutureOr<void>?]) → void};

sdk/lib/async/future.dart

@@ -235,7 +235,7 @@ abstract interface class Future<T> {
   static final _Future<void> _nullFuture = nullFuture as _Future<void>;
 
   /// A `Future<bool>` completed with `false`.
-  static final _Future<bool> _falseFuture = new _Future<bool>.zoneValue(
+  static final _Future<bool> _falseFuture = _Future<bool>.zoneValue(
     false,
     _rootZone,
   );
@@ -253,7 +253,7 @@ abstract interface class Future<T> {
   /// If a non-future value is returned, the returned future is completed
   /// with that value.
   factory Future(FutureOr<T> computation()) {
-    _Future<T> result = new _Future<T>();
+    _Future<T> result = _Future<T>();
     Timer.run(() {
       FutureOr<T> computationResult;
       try {
@@ -280,7 +280,7 @@ abstract interface class Future<T> {
   /// If calling [computation] returns a non-future value,
   /// the returned future is completed with that value.
   factory Future.microtask(FutureOr<T> computation()) {
-    _Future<T> result = new _Future<T>();
+    _Future<T> result = _Future<T>();
     scheduleMicrotask(() {
       FutureOr<T> computationResult;
       try {
@@ -314,7 +314,7 @@ abstract interface class Future<T> {
     try {
       result = computation();
     } catch (error, stackTrace) {
-      var future = new _Future<T>();
+      var future = _Future<T>();
       _asyncCompleteWithErrorCallback(future, error, stackTrace);
       return future;
     }
@@ -349,7 +349,7 @@ abstract interface class Future<T> {
   @pragma("vm:entry-point")
   @pragma("vm:prefer-inline")
   factory Future.value([FutureOr<T>? value]) {
-    return new _Future<T>.immediate(value == null ? value as T : value);
+    return _Future<T>.immediate(value == null ? value as T : value);
   }
 
   /// Creates a future that completes with an error.
@@ -371,7 +371,7 @@ abstract interface class Future<T> {
   /// ```
   factory Future.error(Object error, [StackTrace? stackTrace]) {
     AsyncError(:error, :stackTrace) = _interceptUserError(error, stackTrace);
-    return new _Future<T>.immediateError(error, stackTrace);
+    return _Future<T>.immediateError(error, stackTrace);
   }
 
   /// Creates a future that runs its computation after a delay.
@@ -390,7 +390,7 @@ abstract interface class Future<T> {
   /// If [computation] is omitted,
   /// it will be treated as if [computation] was `() => null`,
   /// and the future will eventually complete with the `null` value.
-  /// In that case, [T] must be nullable.
+  /// In that case, [T] **must** be nullable.
   ///
   /// If calling [computation] throws, the created future will complete with the
   /// error.
@@ -405,28 +405,32 @@ abstract interface class Future<T> {
   /// });
   /// ```
   factory Future.delayed(Duration duration, [FutureOr<T> computation()?]) {
-    if (computation == null && !typeAcceptsNull<T>()) {
-      throw ArgumentError.value(
-        null,
-        "computation",
-        "The type parameter is not nullable",
-      );
-    }
     _Future<T> result = _Future<T>();
-    new Timer(duration, () {
-      if (computation == null) {
-        result._complete(null as T);
+    if (computation == null) {
+      const T? defaultResult = null;
+      if (defaultResult is T) {
+        result._zone.createTimer(duration, () {
+          result._complete(defaultResult);
+        });
       } else {
+        throw ArgumentError(
+          "Required when the type parameter is not nullable",
+          "computation",
+        );
+      }
+    } else {
+      var registeredComputation = result._zone.registerCallback(computation);
+      result._zone.createTimer(duration, () {
         FutureOr<T> computationResult;
         try {
-          computationResult = computation();
+          computationResult = result._zone.run(registeredComputation);
         } catch (e, s) {
           _completeWithErrorCallback(result, e, s);
           return;
         }
         result._complete(computationResult);
-      }
-    });
+      });
+    }
     return result;
   }
 
@@ -704,31 +708,63 @@ abstract interface class Future<T> {
   /// // Outputs: 'Finished with 3'
   /// ```
   static Future<void> doWhile(FutureOr<bool> action()) {
-    _Future<void> doneSignal = new _Future<void>();
-    late void Function(bool) nextIteration;
-    // Bind this callback explicitly so that each iteration isn't bound in the
-    // context of all the previous iterations' callbacks.
-    // This avoids, e.g., deeply nested stack traces from the stack trace
+    var zone = Zone.current;
+    _Future<void> doneSignal = _Future<void>.zone(zone);
+
+    /// As a function instead of a tear-off because some compilers
+    /// prefer that. (Same overhead for creating, but tear-offs have
+    /// more complicated equality, which isn't used anyway.)
+    void completeError(Object e, StackTrace s) {
+      doneSignal._completeError(e, s);
+    }
+
+    // Bind/register the callbacks only once, and reuse them on every iteration.
+    // This avoids, e.g., deeply nested stack traces from the `stack_trace`
     // package.
-    nextIteration = Zone.current.bindUnaryCallbackGuarded((bool keepGoing) {
+    void Function(bool)? registeredNextIteration;
+    void Function(Object, StackTrace)? registeredCompleteError;
+
+    /// True until the first asynchronous gap.
+    bool sync = true;
+    void nextIteration(bool keepGoing) {
       while (keepGoing) {
         FutureOr<bool> result;
         try {
           result = action();
         } catch (error, stackTrace) {
-          // Cannot use _completeWithErrorCallback because it completes
-          // the future synchronously.
-          _asyncCompleteWithErrorCallback(doneSignal, error, stackTrace);
+          if (sync) {
+            // Complete asynchronously if still running synchronously.
+            _asyncCompleteWithErrorCallback(doneSignal, error, stackTrace);
+          } else {
+            _completeWithErrorCallback(doneSignal, error, stackTrace);
+          }
           return;
         }
         if (result is Future<bool>) {
-          result.then(nextIteration, onError: doneSignal._completeError);
+          if (result is _Future<bool>) {
+            var onValue =
+                registeredNextIteration ??= zone.registerUnaryCallback(
+                  nextIteration,
+                );
+            var onError =
+                registeredCompleteError ??= zone.registerBinaryCallback(
+                  completeError,
+                );
+            // Reuse registered callbacks when it's a `_Future`.
+            result._addThenListener(zone, onValue, onError);
+          } else {
+            // Let `then` do callback registration for non-native futures.
+            // It will do that anyway.
+            result.then(nextIteration, onError: completeError);
+          }
+          sync = false;
           return;
         }
         keepGoing = result;
       }
       doneSignal._complete(null);
-    });
+    }
+
     nextIteration(true);
     return doneSignal;
   }
@@ -898,7 +934,7 @@ abstract interface class Future<T> {
 
   /// Stop waiting for this future after [timeLimit] has passed.
   ///
-  /// Creates a new _timeout future_ that completes
+  /// Creates a _timeout future_ that completes
   /// with the same result as this future, the _source future_,
   /// *if* the source future completes in time.
   ///
@@ -1159,7 +1195,7 @@ class TimeoutException implements Exception {
 /// one — you can use a Completer as follows:
 /// ```dart
 /// class AsyncOperation {
-///   final Completer _completer = new Completer();
+///   final Completer _completer = Completer();
 ///
 ///   Future<T> doOperation() {
 ///     _startOperation();
@@ -1198,7 +1234,7 @@ abstract interface class Completer<T> {
   ///   completer.complete('completion value');
   /// }
   /// ```
-  factory Completer() => new _AsyncCompleter<T>();
+  factory Completer() = _AsyncCompleter<T>;
 
   /// Completes the future synchronously.
   ///
@@ -1249,7 +1285,7 @@ abstract interface class Completer<T> {
   ///   foo();  // In this case, foo() runs after bar().
   /// });
   /// ```
-  factory Completer.sync() => new _SyncCompleter<T>();
+  factory Completer.sync() = _SyncCompleter<T>;
 
   /// The future that is completed by this completer.
   ///
@@ -1331,7 +1367,7 @@ abstract interface class Completer<T> {
   bool get isCompleted;
 }
 
-// Helper function completing a _Future with error, but checking the zone
+// Helper function completing a `_Future` with error, but checking the zone
 // for error replacement and missing stack trace first.
 // Only used for errors that are *caught*.
 // A user provided error object should use `_interceptUserError` which