fix #33330, sync async functions should complete their future async

This was difficult to observe in practice, as it required a combination
of two unlikely things:

- an `async` function with no awaits
- a custom Zone with side-effecting scheduleMicrotask callback

The new implementation matches dart2js and the Dart VM's behavior.

Change-Id: I4ff275f37aa93d23643481f00c33aeacb7138b01
Reviewed-on: https://dart-review.googlesource.com/58980
Commit-Queue: Jenny Messerly <jmesserly@google.com>
Reviewed-by: Vijay Menon <vsm@google.com>

pkg/dev_compiler/tool/input_sdk/patch/async_patch.dart

@@ -67,54 +67,68 @@ _async<T>(Function() initGenerator) {
   };
 
   var zone = Zone.current;
-  if (zone != Zone.root) {
+  if (!identical(zone, _rootZone)) {
     onValue = zone.registerUnaryCallback(onValue);
     onError = zone.registerUnaryCallback(onError);
   }
+
   var asyncFuture = _Future<T>();
-  var body = () {
+
+  // This will be set to true once we've yielded to the event loop.
+  //
+  // Before we've done that, we need to complete the future asynchronously to
+  // match dart2js/VM. See https://github.com/dart-lang/sdk/issues/33330
+  //
+  // Once we've yielded to the event loop we can complete synchronously.
+  // Other implementations call this `isSync` to indicate that.
+  bool isRunningAsEvent = false;
+  runBody() {
     try {
       iter = JS('', '#[Symbol.iterator]()', initGenerator());
       var iteratorValue = JS('', '#.next(null)', iter);
       var value = JS('', '#.value', iteratorValue);
       if (JS('bool', '#.done', iteratorValue)) {
-        // TODO(jmesserly): this is needed to work around unsoundness in our
-        // allowed cast failures. We have async methods that return a raw Future
-        // where a Future<T> is expected. If we call:
+        // TODO(jmesserly): this is a workaround for ignored cast failures.
+        // Remove it once we've fixed those. We should be able to call:
         //
-        //     asyncFuture._complete(value);
+        //     if (isRunningAsEvent) {
+        //       asyncFuture._complete(value);
+        //     } else {
+        //       asyncFuture._asyncComplete(value);
+        //     }
         //
-        // Then it ends up interpreting these invalid Future<dynamic> as values
-        // rather than as futures (because complete checks `is Future<T>`).
-        //
-        // For now we inline `_Future._complete` and handle the unsoundness by
-        // checking against raw future types instead of the Fuutre<T> types.
+        // But if the user code returns `Future<dynamic>` instead of
+        // `Future<T>`, that function won't recognize it as a future and will
+        // instead treat it as a completed value.
         if (value is Future) {
           if (value is _Future) {
             _Future._chainCoreFuture(value, asyncFuture);
           } else {
             _Future._chainForeignFuture(value, asyncFuture);
           }
-        } else {
+        } else if (isRunningAsEvent) {
           asyncFuture._completeWithValue(JS('', '#', value));
+        } else {
+          asyncFuture._asyncComplete(JS('', '#', value));
         }
       } else {
         _Future._chainCoreFuture(onAwait(value), asyncFuture);
       }
     } catch (e, s) {
-      if (dart.startAsyncSynchronously) {
-        scheduleMicrotask(() {
-          _completeWithErrorCallback(asyncFuture, e, s);
-        });
-      } else {
+      if (isRunningAsEvent) {
         _completeWithErrorCallback(asyncFuture, e, s);
+      } else {
+        _asyncCompleteWithErrorCallback(asyncFuture, e, s);
       }
     }
-  };
+  }
+
   if (dart.startAsyncSynchronously) {
-    body();
+    runBody();
+    isRunningAsEvent = true;
   } else {
-    scheduleMicrotask(body);
+    isRunningAsEvent = true;
+    scheduleMicrotask(runBody);
   }
   return asyncFuture;
 }

tests/lib_2/async/async_no_await_zones_test.dart

@@ -0,0 +1,32 @@
+// Copyright (c) 2018, 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.
+
+// Regression test for https://github.com/dart-lang/sdk/issues/33330
+import 'dart:async';
+import 'package:expect/expect.dart';
+import 'package:async_helper/async_helper.dart';
+
+var log = [];
+
+main() {
+  asyncStart();
+  runZoned(() {
+    dynamic d = new AsyncDoEvent();
+    return d.doEvent();
+  }, zoneSpecification: new ZoneSpecification(
+    scheduleMicrotask: (self, parent, zone, fn) {
+      log.add('scheduleMicrotask()');
+      return parent.scheduleMicrotask(zone, fn);
+    },
+  )).then((_) {
+    Expect.listEquals(log, ['doEvent()', 'scheduleMicrotask()']);
+    asyncEnd();
+  });
+}
+
+class AsyncDoEvent {
+  Future doEvent() async {
+    log.add('doEvent()');
+  }
+}

tests/lib_2/lib_2_vm.status

@@ -93,6 +93,9 @@ mirrors/library_uri_package_test: RuntimeError
 [ $runtime == vm && !$checked && !$strong ]
 mirrors/regress_16321_test/01: MissingCompileTimeError
 
+[ $runtime == vm && $no_preview_dart_2 ]
+async/async_no_await_zones_test: RuntimeError # not supported in Dart 1 mode.
+
 [ $runtime == vm && ($arch == simarm || $arch == simarmv5te || $arch == simarmv6) ]
 convert/utf85_test: Skip # Pass, Slow Issue 12644.