NowTask can only be awaited immediately

Summary:
This is the first of two changes.
 - This one just introduces a new, immovable `NowTask` coroutine ... and tests that it works similarly to `Task`.
 - The next one teaches various `folly::coro` APIs to take `NowTask` by-value instead of by-forwarding-reference, thus enforcing the following goal ---

**Goal:** The intent of a non-copyable, non-movable `NowTask` is to ensure that it can only be awaited on the line that instantiates it. This avoids a couple of common classes of bugs:

(1) Ephemeral lambda destroyed before its task is awaited:
```
auto failsauce = [&]() -> Task<void> { ... }();
co_await std::move(failsauce);
```

(2) Dangling ref parameters:
```
auto fail = [](){
    int a = 5;
    return [](int& badref) -> Task<void> {...}(a);
  }();
co_await std::move(fail);
```

The reason this helps with `folly/coro/safe` is that `async_closure()` can now automatically turn closures with unsafe args into `NowTask`s. So, the common "immediately awaited" usage of references will still work, but the unsafe stuff will not compile.

Reviewed By: yfeldblum, ispeters

Differential Revision: D64650413

fbshipit-source-id: 6f75ac71a77cc10bf3df181f3c9f1689d9ef2e3c

folly/Portability.h

@@ -621,6 +621,7 @@ constexpr auto kCpplibVer = 0;
 
 #if defined(FOLLY_CFG_NO_COROUTINES)
 #define FOLLY_HAS_COROUTINES 0
+#define FOLLY_HAS_IMMOVABLE_COROUTINES 0
 #else
 // folly::coro requires C++17 support
 #if defined(__NVCC__)
@@ -657,6 +658,23 @@ constexpr auto kCpplibVer = 0;
 #else
 #define FOLLY_HAS_COROUTINES 0
 #endif
+// This logic is written as "good until proven broken" because it's possible
+// that there's a good compiler older than the oldest good version I checked.
+#if defined(__clang_major__) && __clang_major__ <= 14
+//  - 12.0.1 is bad: https://godbolt.org/z/6s489xE8P
+//  - 14 is still bad: https://godbolt.org/z/nW1W8cWvb
+//  - 15.0.0 is good: https://godbolt.org/z/Tco4c9hbq and sEaKKTf8r
+#define FOLLY_HAS_IMMOVABLE_COROUTINES 0
+// BEWARE: Older versions of Clang pretend to be MSVC and define
+// `_MSC_FULL_VER`, but fortunately none of clang 15, 16, 17, 18, 19 do this,
+// so this branch should not result in a false-negative.
+#elif defined(_MSC_FULL_VER) && _MSC_FULL_VER <= 192930040
+//  - 192930040 is bad: https://godbolt.org/z/E797W8xTT
+//  - 192930153 is good: https://godbolt.org/z/cM4nW5rTK
+#define FOLLY_HAS_IMMOVABLE_COROUTINES 0
+#else
+#define FOLLY_HAS_IMMOVABLE_COROUTINES 1 // good until proven broken
+#endif
 #endif // FOLLY_CFG_NO_COROUTINES
 
 // C++20 consteval

folly/coro/Task.h

@@ -785,6 +785,8 @@ class FOLLY_CORO_TASK_ATTRS Task {
         invoke(static_cast<F&&>(f), static_cast<A&&>(a)...)));
   }
 
+  using PrivateAwaiterTypeForTests = Awaiter;
+
  private:
   friend class detail::TaskPromiseBase;
   friend class detail::TaskPromiseCrtpBase<detail::TaskPromise<T>, T>;

folly/coro/ViaIfAsync.h

@@ -543,7 +543,7 @@ constexpr bool is_semi_awaitable_v = is_semi_awaitable<T>::value;
 
 template <typename T>
 using semi_await_result_t = await_result_t<decltype(folly::coro::co_viaIfAsync(
-    std::declval<folly::Executor::KeepAlive<>>(), std::declval<T>()))>;
+    std::declval<folly::Executor::KeepAlive<>>(), FOLLY_DECLVAL(T)))>;
 
 namespace detail {
 

folly/coro/safe/BUCK

@@ -0,0 +1,9 @@
+load("@fbcode_macros//build_defs:cpp_library.bzl", "cpp_library")
+
+oncall("fbcode_entropy_wardens_folly")
+
+cpp_library(
+    name = "now_task",
+    headers = ["NowTask.h"],
+    exported_deps = ["//folly/coro:task_wrapper"],
+)

folly/coro/safe/NowTask.h

@@ -0,0 +1,126 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <folly/coro/TaskWrapper.h>
+
+#if FOLLY_HAS_IMMOVABLE_COROUTINES
+
+namespace folly {
+enum class safe_alias;
+}
+namespace folly::coro {
+
+template <typename>
+class NowTask;
+
+namespace detail {
+
+template <typename T>
+class NowTaskPromise final
+    : public TaskPromiseWrapper<T, NowTask<T>, TaskPromise<T>> {};
+
+template <auto>
+auto bind_captures_to_closure(auto&&, auto);
+
+} // namespace detail
+
+template <safe_alias, typename>
+class BackgroundTask;
+
+// IMPORTANT: This omits `start()` because that would destroy reference
+// lifetime guarantees expected of `NowTask`. Use `BackgroundTask.h`.
+template <typename T>
+class FOLLY_NODISCARD NowTaskWithExecutor final
+    : private NonCopyableNonMovable {
+ protected:
+  template <safe_alias, typename>
+  friend class BackgroundTask; // see `CanUnwrapNowTask` below
+  TaskWithExecutor<T> unwrapTaskWithExecutor() && { return std::move(inner_); }
+
+  template <typename>
+  friend class NowTask; // can construct
+
+  explicit NowTaskWithExecutor(TaskWithExecutor<T> t) : inner_(std::move(t)) {}
+
+ private:
+  TaskWithExecutor<T> inner_;
+};
+
+/// `NowTask<T>` quacks like `Task<T>` but is nonmovable, and therefore
+/// must be `co_await`ed in the same expression that created it.
+///
+/// Defaulting to `NowTask` brings considerable safety benefits.  With
+/// `Task`, the following would be anti-patterns that cause dangling
+/// reference bugs, but with `NowTask`, C++ lifetime extension rules ensure
+/// that they simply work.
+///   - Pass-by-reference into coroutines.
+///   - Ephemeral coro lambdas with captures.
+///   - Coro lambdas with capture-by-reference.
+///
+/// Notes:
+///   - (subject to change) Unlike `SafeTask`, `NowTask` does NOT check
+///     `safe_alias_of_v` for the return type `T`.  The rationale is that
+///     `NowTask` is essentially an immediate async function, i.e. it
+///     satisfies the structured concurrency maxim of "lexical scope drives
+///     both control flow & lifetime".  That shrinks the odds that returned
+///     pointers/references are unexpectedly invalid.  The one failure mode
+///     I can think of is that the pointed-to-data gets invalidated by a
+///     concurrent thread of execution, but in that case the program almost
+///     certainly has a data race -- regardless of the lifetime bug -- and
+///     that requires runtime instrumentation (like TSAN) to detect in
+///     present-day C++.
+template <typename T>
+class FOLLY_CORO_TASK_ATTRS NowTask final
+    : public TaskWrapperCrtp<NowTask<T>, T, Task<T>>,
+      private NonCopyableNonMovable {
+ public:
+  using promise_type = detail::NowTaskPromise<T>;
+
+  // If `makeNowTask().scheduleOn()` is movable, it defeats our purpose.
+  FOLLY_NODISCARD
+  NowTaskWithExecutor<T> scheduleOn(Executor::KeepAlive<> exec) && noexcept {
+    return NowTaskWithExecutor<T>{
+        std::move(*this).unwrap().scheduleOn(std::move(exec))};
+  }
+
+  explicit NowTask(Task<T> t)
+      : TaskWrapperCrtp<NowTask<T>, T, Task<T>>(std::move(t)) {}
+
+ protected:
+  // These 3 `friend`s (+ 1 above) are for `unwrap()`.  If this list grows,
+  // introduce a `CanUnwrapNowTask` passkey type.
+  template <typename U>
+  friend auto toNowTask(NowTask<U> t);
+  // `async_now_closure` wraps `NowTask`s into `NowTask`s
+  template <auto>
+  friend auto detail::bind_captures_to_closure(auto&&, auto);
+};
+
+// NB: `toNowTask(SafeTask)` is in `SafeTask.h` to avoid circular deps.
+template <typename T>
+auto toNowTask(Task<T> t) {
+  return NowTask<T>{std::move(t)};
+}
+template <typename T>
+auto toNowTask(NowTask<T> t) {
+  return NowTask<T>{std::move(t).unwrap()};
+}
+
+} // namespace folly::coro
+
+#endif

folly/coro/safe/test/BUCK

@@ -0,0 +1,13 @@
+load("@fbcode_macros//build_defs:cpp_unittest.bzl", "cpp_unittest")
+
+oncall("fbcode_entropy_wardens_folly")
+
+cpp_unittest(
+    name = "now_task_test",
+    srcs = ["NowTaskTest.cpp"],
+    deps = [
+        "//folly/coro:blocking_wait",
+        "//folly/coro:gtest_helpers",
+        "//folly/coro/safe:now_task",
+    ],
+)

folly/coro/safe/test/NowTaskTest.cpp

@@ -0,0 +1,144 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <folly/coro/BlockingWait.h>
+#include <folly/coro/GtestHelpers.h>
+#include <folly/coro/safe/NowTask.h>
+
+#if FOLLY_HAS_IMMOVABLE_COROUTINES
+
+namespace folly::coro {
+
+template <typename T>
+constexpr T decl_prvalue();
+
+Task<int> demoTask(int x) {
+  co_return 1300 + x;
+}
+NowTask<int> demoNowTask(int x) {
+  co_return 1300 + x;
+}
+
+template <typename T, typename Res = int>
+inline constexpr bool test_semi_await_result_v =
+    std::is_same_v<detected_t<semi_await_result_t, T>, Res>;
+
+static_assert(test_semi_await_result_v<Task<int>>);
+static_assert(test_semi_await_result_v<Task<int>&&>);
+static_assert(test_semi_await_result_v<NowTask<int>>);
+// FIXME: See next diff
+static_assert(test_semi_await_result_v<NowTask<int>&&>);
+
+using DemoTryTask = decltype(co_awaitTry(demoTask(37)));
+using DemoTryNowTask = decltype(co_awaitTry(demoNowTask(37)));
+static_assert(test_semi_await_result_v<DemoTryTask, Try<int>>);
+static_assert(test_semi_await_result_v<DemoTryTask&&, Try<int>>);
+static_assert(test_semi_await_result_v<DemoTryNowTask, Try<int>>);
+// FIXME: See next diff
+static_assert(test_semi_await_result_v<DemoTryNowTask&&, Try<int>>);
+
+// Note: This `test_` predicate, and similar ones below, may look somewhat
+// redundant with `test_semi_await_result_v` above.  Both aim to test this:
+//   co_await demoNowTask(37); // works
+//   auto t = demoNowTask(37);
+//   co_await std::move(t); // does not compile
+// We check against test bugs by ensuring that BOTH forms work for `Task`.
+//
+// The rationale for the supposed redundancy is that here, we spell out the
+// expected call-path-to-awaiter.  This is pretty robust, so long as I check
+// both `Task` (good) and `NowTask` (bad), whereas with the fancy
+// metaprogramming of `semi_await_result_t`, there's more risk that
+// `co_await std::move(t)` compiles for `NowTask`, even when the type
+// function fails to substitute.
+template <typename T>
+using await_transform_result_t =
+    decltype(std::declval<detail::TaskPromise<void>>().await_transform(
+        FOLLY_DECLVAL(T)));
+template <typename T>
+inline constexpr bool test_transform_moved_v = std::is_same_v<
+    detected_t<await_transform_result_t, T>,
+    typename Task<int>::PrivateAwaiterTypeForTests>;
+
+CO_TEST(NowTaskTest, simple) {
+  EXPECT_EQ(1337, co_await demoNowTask(37));
+
+  using T = Task<int>;
+  static_assert(
+      std::is_same<
+          decltype(std::declval<detail::TaskPromise<void>>().await_transform(
+              decl_prvalue<T>())),
+          typename Task<int>::PrivateAwaiterTypeForTests>::value);
+
+  static_assert(test_transform_moved_v<Task<int>>);
+  static_assert(test_transform_moved_v<Task<int>&&>);
+  static_assert(test_transform_moved_v<NowTask<int>>);
+  // FIXME: See next diff
+  static_assert(test_transform_moved_v<NowTask<int>&&>);
+#if 1 // The above asserts are a proxy for this manual test
+  auto t = demoNowTask(37);
+  co_await std::move(t);
+#endif
+}
+
+// `std::invoke_result_t` cannot pass prvalues -- it invokes a move ctor.
+template <typename T>
+using blockingWait_result_t = decltype(blockingWait(FOLLY_DECLVAL(T)));
+template <typename T, typename Res>
+inline constexpr bool test_blocking_wait_moved_v =
+    std::is_same_v<detected_t<blockingWait_result_t, T>, Res>;
+
+TEST(NowTaskTest, blockingWait) {
+  EXPECT_EQ(1337, blockingWait(demoNowTask(37)));
+
+  static_assert(test_blocking_wait_moved_v<Task<int>, int>);
+  static_assert(test_blocking_wait_moved_v<Task<int>&&, int>);
+  static_assert(test_blocking_wait_moved_v<NowTask<int>, int>);
+  // FIXME: See next diff
+  static_assert(test_blocking_wait_moved_v<NowTask<int>&&, int>);
+#if 1 // The above asserts are a proxy for this manual test
+  auto t = demoNowTask(37);
+  blockingWait(std::move(t));
+#endif
+}
+
+// Both of these are antipatterns with `Task` because if you awaited either
+// of these coros outside of the statement that created them, it would have
+// dangling refs.
+//
+// Since `NowTask` tries to ensure it can ONLY be awaited in the statement
+// that created it, C++ lifetime extension should save our bacon.
+CO_TEST(NowTaskTest, passByRef) {
+  auto res = co_await [](int&& x) -> NowTask<int> { co_return 1300 + x; }(37);
+  EXPECT_EQ(1337, res);
+}
+CO_TEST(NowTaskTest, lambdaWithCaptures) {
+  int a = 1300, b = 37;
+  auto res = co_await [&a, b]() -> NowTask<int> { co_return a + b; }();
+  EXPECT_EQ(1337, res);
+}
+
+CO_TEST(NowTaskTest, toNowTask) {
+  static_assert(
+      std::is_same_v<NowTask<int>, decltype(toNowTask(demoNowTask(5)))>);
+  auto t = []() -> Task<int> { co_return 5; }();
+  static_assert(
+      std::is_same_v<NowTask<int>, decltype(toNowTask(std::move(t)))>);
+  EXPECT_EQ(5, co_await toNowTask(std::move(t)));
+}
+
+} // namespace folly::coro
+
+#endif