From 4c2edfd5b9fe33139aad7e9ca021bd8530f47986 Mon Sep 17 00:00:00 2001 From: Nicholas Shahan Date: Wed, 9 Dec 2020 01:44:38 +0000 Subject: [PATCH] [html] Make cancel() always return synchronously Before Null Safety, `_EventStreamSubscription.cancel()` used a trick to run with synchronous timing even though it was typed to return a `Future`. During the migration of the SDK to support Null Safety it kept the synchronous timing in weak mode, but was changed to asynchronous in sound mode so that the behavior matched the method signature. In hindsight, changing the timing when opting into Null Safety is problematic: * A shared package has no control over what mode it runs in. Libraries may be opted in and run their tests with sound null safety but the apps they are used in could still be running in weak mode. This results in library unit tests that behave differently than the production app that deploys the code. * This codepath can be triggered by EventTarget.dispatchEvent() from dart:html which should have synchronous timings for the event listeners before returning to the calling code. The asynchronous timing when running with sound null safety is inconsistent with the browser API. This change reverses that migration decision and keeps the synchronous timing in both modes. To support this in sound mode it returns a special future value that is internal to the SDK and known to be used for synchronous timing. This change also removes the workaround introduced in DDC to avoid warning/failing when `_EventStreamSubscription.cancel()` returned null and the extra warnings/errors features were enabled in weak null safety mode. Change-Id: I6b08a2ada5b10120bea787ad59d1d58e6e181de5 Fixes: https://github.com/dart-lang/sdk/issues/44157 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175323 Commit-Queue: Nicholas Shahan Reviewed-by: Srujan Gaddam Reviewed-by: Sigmund Cherem --- CHANGELOG.md | 8 +++++++ pkg/dev_compiler/lib/src/kernel/compiler.dart | 21 ------------------- sdk/lib/html/dart2js/html_dart2js.dart | 9 ++------ tools/dom/src/EventStreamProvider.dart | 9 ++------ 4 files changed, 12 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0d08774093..a0982c4fa0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,14 @@ opted out of null safety by adding `// @dart=2.9` to the beginning of the file. * `LinkedList` made it explicit that elements are compared by identity, and updated `contains` to take advantage of this. +#### `dart:html` + +* `EventStreamSubscription.cancel` has been updated to retain its synchronous + timing when running in both sound and unsound null safety modes. See issue + [#44157][] for more details. + +[#44157]: https://github.com/dart-lang/sdk/issues/44157 + ### Dart VM * **Breaking Change** [#42312][]: `Dart_WeakPersistentHandle`s will no longer diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index 8800d4c562e..87fed8fc870 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -5530,27 +5530,6 @@ class ProgramCompiler extends ComputeOnceConstantVisitor js_ast.Expression visitAsExpression(AsExpression node) { var fromExpr = node.operand; var jsFrom = _visitExpression(fromExpr); - - // The `_EventStreamSubscription.cancel()` method dart:html returns null in - // weak mode. This causes unwanted warnings/failures when you turn on the - // weak mode warnings/errors so we remove these specific runtime casts. - // TODO(44157) Remove this workaround once it returns a consistent type. - if (_isWebLibrary(currentLibraryUri) && node.parent is ReturnStatement) { - var parent = node.parent; - while (parent != null && parent is! FunctionNode) { - parent = parent?.parent; - } - parent = parent?.parent; - if (parent is Procedure) { - if (parent.enclosingClass != null && - parent.enclosingClass.name == '_EventStreamSubscription' && - parent.name.name == 'cancel') { - // Ignore these casts and just emit the expression. - return jsFrom; - } - } - } - var to = node.type; var from = fromExpr.getStaticType(_staticTypeContext); diff --git a/sdk/lib/html/dart2js/html_dart2js.dart b/sdk/lib/html/dart2js/html_dart2js.dart index a79aff6f7a6..bc7bdfbc053 100644 --- a/sdk/lib/html/dart2js/html_dart2js.dart +++ b/sdk/lib/html/dart2js/html_dart2js.dart @@ -37285,18 +37285,13 @@ class _EventStreamSubscription extends StreamSubscription { } Future cancel() { - // Check for strong mode. This function can no longer return null in strong - // mode, so only return null in weak mode to preserve synchronous timing. - // See issue 41653 for more details. - dynamic emptyFuture = - typeAcceptsNull() ? null : Future.value(); - if (_canceled) return emptyFuture as Future; + if (_canceled) return nullFuture; _unlisten(); // Clear out the target to indicate this is complete. _target = null; _onData = null; - return emptyFuture as Future; + return nullFuture; } bool get _canceled => _target == null; diff --git a/tools/dom/src/EventStreamProvider.dart b/tools/dom/src/EventStreamProvider.dart index fe01be9be79..feadc26d26f 100644 --- a/tools/dom/src/EventStreamProvider.dart +++ b/tools/dom/src/EventStreamProvider.dart @@ -248,18 +248,13 @@ class _EventStreamSubscription extends StreamSubscription { } Future cancel() { - // Check for strong mode. This function can no longer return null in strong - // mode, so only return null in weak mode to preserve synchronous timing. - // See issue 41653 for more details. - dynamic emptyFuture = - typeAcceptsNull() ? null : Future.value(); - if (_canceled) return emptyFuture as Future; + if (_canceled) return nullFuture; _unlisten(); // Clear out the target to indicate this is complete. _target = null; _onData = null; - return emptyFuture as Future; + return nullFuture; } bool get _canceled => _target == null;