Fail synchronously if null is passed as an error to async APIs.

The first patchset is Lasse's original changes.

Change-Id: Ic5f24bcfc0ef4e82edee68d61e015b095cb5916e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138605
Reviewed-by: Leaf Petersen <leafp@google.com>

sdk/lib/_http/websocket_impl.dart

@@ -115,6 +115,7 @@ class _WebSocketProtocolTransformer extends StreamTransformerBase<List<int>,
   }
 
   void addError(Object error, [StackTrace stackTrace]) {
+    ArgumentError.checkNotNull(error, "error");
     _eventSink.addError(error, stackTrace);
   }
 
@@ -723,6 +724,7 @@ class _WebSocketOutgoingTransformer
   }
 
   void addError(Object error, [StackTrace stackTrace]) {
+    ArgumentError.checkNotNull(error, "error");
     _eventSink.addError(error, stackTrace);
   }
 

sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart

@@ -419,6 +419,7 @@ class _AsyncStarImpl<T> {
   }
 
   void addError(Object error, StackTrace stackTrace) {
+    ArgumentError.checkNotNull(error, "error");
     if (cancellationCompleter != null && !cancellationCompleter.isCompleted) {
       // If the stream has been cancelled, complete the cancellation future
       // with the error.

sdk/lib/_internal/vm/lib/async_patch.dart

@@ -209,6 +209,7 @@ class _AsyncStarStreamController<T> {
   }
 
   void addError(Object error, StackTrace stackTrace) {
+    ArgumentError.checkNotNull(error, "error");
     if ((cancellationFuture != null) && cancellationFuture._mayComplete) {
       // If the stream has been cancelled, complete the cancellation future
       // with the error.

sdk/lib/async/broadcast_stream_controller.dart

@@ -253,7 +253,7 @@ abstract class _BroadcastStreamController<T>
   }
 
   void addError(Object error, [StackTrace stackTrace]) {
-    error = _nonNullError(error);
+    ArgumentError.checkNotNull(error, "error");
     if (!_mayAddEvent) throw _addEventError();
     AsyncError replacement = Zone.current.errorCallback(error, stackTrace);
     if (replacement != null) {
@@ -480,6 +480,7 @@ class _AsBroadcastStreamController<T> extends _SyncBroadcastStreamController<T>
   }
 
   void addError(Object error, [StackTrace stackTrace]) {
+    ArgumentError.checkNotNull(error, "error");
     if (!isClosed && _isFiring) {
       _addPendingEvent(new _DelayedError(error, stackTrace));
       return;

sdk/lib/async/future.dart

@@ -266,12 +266,12 @@ abstract class Future<T> {
    * If an error handler isn't added before the future completes, the error
    * will be considered unhandled.
    *
-   * If [error] is `null`, it is replaced by a [NullThrownError].
+   * The [error] must not be `null`.
    *
    * Use [Completer] to create a future and complete it later.
    */
   factory Future.error(Object error, [StackTrace stackTrace]) {
-    error = _nonNullError(error);
+    ArgumentError.checkNotNull(error, "error");
     if (!identical(Zone.current, _rootZone)) {
       AsyncError replacement = Zone.current.errorCallback(error, stackTrace);
       if (replacement != null) {
@@ -874,7 +874,7 @@ abstract class Completer<T> {
    * Completing a future with an error indicates that an exception was thrown
    * while trying to produce a value.
    *
-   * If [error] is `null`, it is replaced by a [NullThrownError].
+   * The [error] must not be `null`.
    *
    * If `error` is a `Future`, the future itself is used as the error value.
    * If you want to complete with the result of the future, you can use:

sdk/lib/async/future_impl.dart

@@ -19,7 +19,7 @@ abstract class _Completer<T> implements Completer<T> {
   void complete([FutureOr<T> value]);
 
   void completeError(Object error, [StackTrace stackTrace]) {
-    error = _nonNullError(error);
+    ArgumentError.checkNotNull(error, "error");
     if (!future._mayComplete) throw new StateError("Future already completed");
     AsyncError replacement = Zone.current.errorCallback(error, stackTrace);
     if (replacement != null) {
@@ -94,8 +94,8 @@ class _FutureListener<S, T> {
       this.result, _FutureOnValue<S, T> onValue, Function errorCallback)
       : callback = onValue,
         errorCallback = errorCallback,
-        state = ((errorCallback == null) ? stateThen : stateThenOnerror)
-              | stateIsAwait ;
+        state = ((errorCallback == null) ? stateThen : stateThenOnerror) |
+            stateIsAwait;
 
   _FutureListener.catchError(this.result, this.errorCallback, this.callback)
       : state = (callback == null) ? stateCatcherror : stateCatcherrorTest;
@@ -293,8 +293,7 @@ class _Future<T> implements Future<T> {
   /// The system created liseners are not registered in the zone,
   /// and the listener is marked as being from an `await`.
   /// This marker is used in [_continuationFunctions].
-  Future<E> _thenAwait<E>(
-      FutureOr<E> f(T value), Function onError) {
+  Future<E> _thenAwait<E>(FutureOr<E> f(T value), Function onError) {
     _Future<E> result = new _Future<E>();
     _addListener(new _FutureListener<T, E>.thenAwait(result, f, onError));
     return result;
@@ -812,5 +811,5 @@ Function _registerErrorHandler(Function errorHandler, Zone zone) {
       errorHandler,
       "onError",
       "Error handler must accept one Object or one Object and a StackTrace"
-      " as arguments, and return a a valid result");
+          " as arguments, and return a a valid result");
 }

sdk/lib/async/stream.dart

@@ -132,6 +132,7 @@ abstract class Stream<T> {
    *
    * This stream emits a single error event of [error] and [stackTrace]
    * and then completes with a done event.
+   * The [error] must not be `null`.
    *
    * Example:
    * ```dart
@@ -152,11 +153,13 @@ abstract class Stream<T> {
    * stack trace as well.
    */
   @Since("2.5")
-  factory Stream.error(Object error, [StackTrace stackTrace]) =>
-      (_AsyncStreamController<T>(null, null, null, null)
-            .._addError(error, stackTrace)
-            .._closeUnchecked())
-          .stream;
+  factory Stream.error(Object error, [StackTrace stackTrace]) {
+    ArgumentError.checkNotNull(error, "error");
+    return (_AsyncStreamController<T>(null, null, null, null)
+          .._addError(error, stackTrace)
+          .._closeUnchecked())
+        .stream;
+  }
 
   /**
    * Creates a new single-subscription stream from the future.
@@ -1811,6 +1814,8 @@ abstract class EventSink<T> implements Sink<T> {
   /**
    * Adds an [error] to the sink.
    *
+   * The [error] must not be `null`.
+   *
    * Must not be called on a closed sink.
    */
   void addError(Object error, [StackTrace stackTrace]);

sdk/lib/async/stream_controller.dart

@@ -237,7 +237,7 @@ abstract class StreamController<T> implements StreamSink<T> {
   /**
    * Sends or enqueues an error event.
    *
-   * If [error] is `null`, it is replaced by a [NullThrownError].
+   * The [error] must not be `null`.
    *
    * Listeners receive this event at a later microtask. This behavior can be
    * overridden by using `sync` controllers. Note, however, that sync
@@ -361,6 +361,8 @@ abstract class SynchronousStreamController<T> implements StreamController<T> {
   /**
    * Adds error to the controller's stream.
    *
+   * The [error] must not be `null`.
+   *
    * As [StreamController.addError], but must not be called while an event is
    * being added by [add], [addError] or [close].
    */
@@ -597,8 +599,11 @@ abstract class _StreamController<T> implements _StreamControllerBase<T> {
 
   /**
    * Send or enqueue an error event.
+   *
+   * The [error] must not be `null`.
    */
   void addError(Object error, [StackTrace stackTrace]) {
+    ArgumentError.checkNotNull(error, "error");
     if (!_mayAddEvent) throw _badEventState();
     error = _nonNullError(error);
     AsyncError replacement = Zone.current.errorCallback(error, stackTrace);

sdk/lib/async/stream_transformers.dart

@@ -234,6 +234,7 @@ class _HandlerEventSink<S, T> implements EventSink<S> {
   }
 
   void addError(Object error, [StackTrace stackTrace]) {
+    ArgumentError.checkNotNull(error, "error");
     if (_isClosed) {
       throw StateError("Sink is closed");
     }

sdk/lib/async/zone.dart

@@ -43,7 +43,9 @@ class AsyncError implements Error {
   final Object error;
   final StackTrace stackTrace;
 
-  AsyncError(this.error, this.stackTrace);
+  AsyncError(this.error, this.stackTrace) {
+    ArgumentError.checkNotNull(error, "error");
+  }
 
   String toString() => '$error';
 }
@@ -750,6 +752,7 @@ class _ZoneDelegate implements ZoneDelegate {
   }
 
   AsyncError errorCallback(Zone zone, Object error, StackTrace stackTrace) {
+    ArgumentError.checkNotNull(error, "error");
     var implementation = _delegationTarget._errorCallback;
     _Zone implZone = implementation.zone;
     if (identical(implZone, _rootZone)) return null;
@@ -1065,6 +1068,7 @@ class _CustomZone extends _Zone {
   }
 
   AsyncError errorCallback(Object error, StackTrace stackTrace) {
+    ArgumentError.checkNotNull(error, "error");
     var implementation = this._errorCallback;
     assert(implementation != null);
     final Zone implementationZone = implementation.zone;
@@ -1109,8 +1113,11 @@ class _CustomZone extends _Zone {
 
 void _rootHandleUncaughtError(
     Zone self, ZoneDelegate parent, Zone zone, error, StackTrace stackTrace) {
+  if (error == null) {
+    error = ArgumentError.notNull("error");
+    stackTrace = StackTrace.current;
+  }
   _schedulePriorityAsyncCallback(() {
-    error ??= new NullThrownError();
     if (stackTrace == null) throw error;
     _rethrow(error, stackTrace);
   });

sdk/lib/convert/chunked_conversion.dart

@@ -75,6 +75,7 @@ class _ConverterStreamEventSink<S, T> implements EventSink<S> {
   }
 
   void addError(Object error, [StackTrace stackTrace]) {
+    ArgumentError.checkNotNull(error, "error");
     _eventSink.addError(error, stackTrace);
   }
 

sdk_nnbd/lib/_http/websocket_impl.dart

@@ -113,6 +113,8 @@ class _WebSocketProtocolTransformer extends StreamTransformerBase<List<int>,
   }
 
   void addError(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     _eventSink!.addError(error, stackTrace);
   }
 
@@ -722,6 +724,8 @@ class _WebSocketOutgoingTransformer
   }
 
   void addError(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     _eventSink!.addError(error, stackTrace);
   }
 
@@ -1216,6 +1220,7 @@ class _WebSocketImpl extends Stream with _ServiceObject implements WebSocket {
   }
 
   void addUtf8Text(List<int> bytes) {
+    // TODO(40614): Remove once non-nullability is sound.
     ArgumentError.checkNotNull(bytes, "bytes");
     _sink.add(new _EncodedString(bytes));
   }

sdk_nnbd/lib/_internal/js_dev_runtime/patch/async_patch.dart

@@ -417,6 +417,7 @@ class _AsyncStarImpl<T> {
   }
 
   void addError(Object error, StackTrace stackTrace) {
+    ArgumentError.checkNotNull(error, "error");
     var completer = cancellationCompleter;
     if (completer != null && !completer.isCompleted) {
       // If the stream has been cancelled, complete the cancellation future

sdk_nnbd/lib/_internal/vm/lib/async_patch.dart

@@ -214,6 +214,8 @@ class _AsyncStarStreamController<T> {
   }
 
   void addError(Object error, StackTrace stackTrace) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     final future = cancellationFuture;
     if ((future != null) && future._mayComplete) {
       // If the stream has been cancelled, complete the cancellation future

sdk_nnbd/lib/async/broadcast_stream_controller.dart

@@ -248,6 +248,8 @@ abstract class _BroadcastStreamController<T>
   }
 
   void addError(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     if (!_mayAddEvent) throw _addEventError();
     AsyncError? replacement = Zone.current.errorCallback(error, stackTrace);
     if (replacement != null) {
@@ -476,6 +478,8 @@ class _AsBroadcastStreamController<T> extends _SyncBroadcastStreamController<T>
   }
 
   void addError(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     if (!isClosed && _isFiring) {
       _addPendingEvent(new _DelayedError(error, stackTrace));
       return;

sdk_nnbd/lib/async/future.dart

@@ -269,11 +269,11 @@ abstract class Future<T> {
    * If an error handler isn't added before the future completes, the error
    * will be considered unhandled.
    *
-   * If [error] is `null`, it is replaced by a [NullThrownError].
-   *
    * Use [Completer] to create a future and complete it later.
    */
   factory Future.error(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     if (!identical(Zone.current, _rootZone)) {
       AsyncError? replacement = Zone.current.errorCallback(error, stackTrace);
       if (replacement != null) {

sdk_nnbd/lib/async/future_impl.dart

@@ -17,6 +17,8 @@ abstract class _Completer<T> implements Completer<T> {
   void complete([FutureOr<T>? value]);
 
   void completeError(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     if (!future._mayComplete) throw new StateError("Future already completed");
     AsyncError? replacement = Zone.current.errorCallback(error, stackTrace);
     if (replacement != null) {

sdk_nnbd/lib/async/stream.dart

@@ -150,11 +150,14 @@ abstract class Stream<T> {
    * stack trace as well.
    */
   @Since("2.5")
-  factory Stream.error(Object error, [StackTrace? stackTrace]) =>
-      (_AsyncStreamController<T>(null, null, null, null)
-            .._addError(error, stackTrace)
-            .._closeUnchecked())
-          .stream;
+  factory Stream.error(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
+    return (_AsyncStreamController<T>(null, null, null, null)
+          .._addError(error, stackTrace)
+          .._closeUnchecked())
+        .stream;
+  }
 
   /**
    * Creates a new single-subscription stream from the future.

sdk_nnbd/lib/async/stream_controller.dart

@@ -600,6 +600,8 @@ abstract class _StreamController<T> implements _StreamControllerBase<T> {
    * Send or enqueue an error event.
    */
   void addError(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     if (!_mayAddEvent) throw _badEventState();
     AsyncError? replacement = Zone.current.errorCallback(error, stackTrace);
     if (replacement != null) {

sdk_nnbd/lib/async/stream_transformers.dart

@@ -229,6 +229,8 @@ class _HandlerEventSink<S, T> implements EventSink<S> {
   }
 
   void addError(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     var sink = _sink;
     if (sink == null) {
       throw StateError("Sink is closed");

sdk_nnbd/lib/async/zone.dart

@@ -41,7 +41,10 @@ class AsyncError implements Error {
   final Object error;
   final StackTrace? stackTrace;
 
-  AsyncError(this.error, this.stackTrace);
+  AsyncError(this.error, this.stackTrace) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
+  }
 
   String toString() => '$error';
 }
@@ -742,6 +745,8 @@ class _ZoneDelegate implements ZoneDelegate {
   }
 
   AsyncError? errorCallback(Zone zone, Object error, StackTrace? stackTrace) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     var implementation = _delegationTarget._errorCallback;
     _Zone implZone = implementation.zone;
     if (identical(implZone, _rootZone)) return null;
@@ -1075,6 +1080,8 @@ class _CustomZone extends _Zone {
   }
 
   AsyncError? errorCallback(Object error, StackTrace? stackTrace) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     var implementation = this._errorCallback;
     final _Zone implementationZone = implementation.zone;
     if (identical(implementationZone, _rootZone)) return null;

sdk_nnbd/lib/convert/chunked_conversion.dart

@@ -73,6 +73,8 @@ class _ConverterStreamEventSink<S, T> implements EventSink<S> {
   }
 
   void addError(Object error, [StackTrace? stackTrace]) {
+    // TODO(40614): Remove once non-nullability is sound.
+    ArgumentError.checkNotNull(error, "error");
     _eventSink.addError(error, stackTrace);
   }
 

tests/lib_2/async/future_error_test.dart

@@ -0,0 +1,13 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// 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 'dart:async';
+
+import 'package:expect/expect.dart';
+
+main() {
+  // The error cannot be null.
+  Expect.throwsArgumentError(() {
+    Future.error(null);
+  });
+}

tests/lib_2/async/future_test.dart

@@ -697,13 +697,10 @@ void testCompleteErrorWithCustomFuture() {
 }
 
 void testCompleteErrorWithNull() {
-  asyncStart();
   final completer = new Completer<int>();
-  completer.future.catchError((e) {
-    Expect.isTrue(e is NullThrownError);
-    asyncEnd();
+  Expect.throwsArgumentError(() {
+    completer.completeError(null);
   });
-  completer.completeError(null);
 }
 
 void testChainedFutureValue() {

tests/lib_2/async/stream_controller_add_error_test.dart

@@ -0,0 +1,48 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// 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 'dart:async';
+
+import 'package:expect/expect.dart';
+
+main() {
+  // Single-cast async.
+  var controller = StreamController();
+  Expect.throwsArgumentError(() {
+    controller.addError(null);
+  });
+
+  Expect.throwsArgumentError(() {
+    controller.sink.addError(null);
+  });
+
+  // Single-cast sync.
+  controller = StreamController(sync: true);
+  Expect.throwsArgumentError(() {
+    controller.addError(null);
+  });
+
+  Expect.throwsArgumentError(() {
+    controller.sink.addError(null);
+  });
+
+  // Broadcast async.
+  controller = StreamController.broadcast();
+  Expect.throwsArgumentError(() {
+    controller.addError(null);
+  });
+
+  Expect.throwsArgumentError(() {
+    controller.sink.addError(null);
+  });
+
+  // Broadcast sync.
+  controller = StreamController.broadcast(sync: true);
+  Expect.throwsArgumentError(() {
+    controller.addError(null);
+  });
+
+  Expect.throwsArgumentError(() {
+    controller.sink.addError(null);
+  });
+}

tests/lib_2/async/stream_error_test.dart

@@ -72,6 +72,11 @@ void main() async {
     await onDone.future;
   }
 
+  // A null error argument is a synchronous error.
+  Expect.throwsArgumentError(() {
+    Stream.error(null);
+  });
+
   asyncEnd();
 }
 

tests/lib_2/async/zone_async_error_test.dart

@@ -0,0 +1,13 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// 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 'dart:async';
+
+import 'package:expect/expect.dart';
+
+main() {
+  // The error cannot be null.
+  Expect.throwsArgumentError(() {
+    AsyncError(null, StackTrace.current);
+  });
+}