non_value_result API should not be defined in terms of exception_wrapper

Summary:
The prior iteration of `non_value_result` and `result<T>` copiously leaked the fact that we use `exception_wrapper` as the implementation of `non_value_result`.

This turned out to be very undesirable for my future RTTI-avoidance work.

This diff improves usability, since `non_value_result{YourError{}}` is clearer and shorter than `make_exception_wrapper<YourError>()`.

At the same time, it practically eliminates usage of bare `exception_wrapper` in the `result` API. Only the explicitly-for-compatibility shims of `non_value_result::from_exception_wrapper`, and `non_value_result::to_exception_wrapper()` remain.

This makes it easy to drop in support for immortal exception pointers down the line.

Reviewed By: ispeters

Differential Revision: D73410513

fbshipit-source-id: db619bd115230247a4d45fc6f6b9130a1a108278

Author: snarkmaster

third-party/folly/src/folly/coro/test/ReadyTest.cpp

@@ -25,11 +25,11 @@ namespace folly::coro {
 
 CO_TEST(ReadyTest, co_ready_of_error_result) {
   auto errorTaskFn = []() -> NowTask<> {
-    result<> res = make_exception_wrapper<std::runtime_error>("foo");
+    result<> res = non_value_result{std::runtime_error{"foo"}};
     try {
-      co_await co_ready(std::move(res));
+      co_await co_ready{std::move(res)}; // will not throw
     } catch (...) {
-    } // propagates exception WITHOUT rethrowing
+    }
     LOG(FATAL) << "not reached";
   };
   auto res = co_await co_await_result(errorTaskFn());
@@ -39,14 +39,14 @@ CO_TEST(ReadyTest, co_ready_of_error_result) {
 CO_TEST(ReadyTest, co_ready_of_value_result) {
   auto valueTaskFn = []() -> NowTask<int> {
     result res = 1300;
-    co_return 37 + co_await co_ready(std::move(res));
+    co_return 37 + co_await co_ready{std::move(res)};
   };
   EXPECT_EQ(1337, co_await valueTaskFn());
 
   // Use a move-only value to check we don't have extraneous copies
   auto moveValueTaskFn = []() -> NowTask<std::unique_ptr<int>> {
     result res = std::make_unique<int>(1300);
-    auto np = co_await co_ready(std::move(res));
+    auto np = co_await co_ready{std::move(res)};
     *np += 37;
     co_return std::move(np);
   };
@@ -56,7 +56,7 @@ CO_TEST(ReadyTest, co_ready_of_value_result) {
 CO_TEST(ReadyTest, co_ready_of_void_result) {
   auto taskFn = [&]() -> NowTask<int> {
     result<> res;
-    co_await co_ready(std::move(res));
+    co_await co_ready{std::move(res)};
     co_return 42;
   };
   EXPECT_EQ(42, co_await taskFn());
@@ -65,7 +65,7 @@ CO_TEST(ReadyTest, co_ready_of_void_result) {
 CO_TEST(ReadyTest, co_ready_stopped) {
   auto taskFn = [&]() -> NowTask<int> {
     result<> res = stopped_result;
-    co_await co_ready(std::move(res));
+    co_await co_ready{std::move(res)};
     co_return 42;
   };
   EXPECT_TRUE((co_await co_await_result(taskFn())).has_stopped());

third-party/folly/src/folly/result/README.md

@@ -28,7 +28,7 @@ more usable:
     [`folly::Expected`](
     https://github.com/facebook/folly/commit/0e8c7e1c97e27d96fddbdf552bc99faa22066d00).
 
-  - `result` uses `exception_wrapper`, `folly`'s enhanced `std::exception_ptr`
+  - `result` uses `std::exception_ptr` (with some folly-specific enhancements)
     to efficiently transport all exception types, which avoids the user-facing
     complexity of distinguishing `outcome::result` (`error_code` only) and
     `outcome::outcome` (code OR exception).
@@ -160,17 +160,17 @@ In bullets, `result<T>`:
 
   - Contains one of:
     * `T` -- which can be a value or reference, or
-    * `non_value_result` -- which either `has_stopped()`, or `has_error()`.
-      Error storage is `exception_wrapper`, an enhanced wrapper for
-      `std::exception_ptr`.
+    * `non_value_result` -- which either `has_stopped()`, or stores an error as
+      `std::exception_ptr`, with folly-specific optimizations.  Access the
+      latter via `folly::get_exception<Ex>(res)`.
 
     *NB*: Right now, `result` is "almost never empty, like `folly::Expected`",
     but when C++23 is widely available, it will be truly never empty.
 
   - Provides constructors optimized for usability. For example, unlike
     `folly::Try`, `result` is implicitly constructible from values & errors.
 
-    *NB*: Storing an empty `exception_wrapper` is a contract violation. It is
+    *NB*: Storing an empty `std::exception_ptr` is a contract violation.  It is
     fatal in debug builds, but "works" in opt unless you hit a later assertion,
     like the `std::terminate` in `exception_wrapper::throw_exception`.
 
@@ -196,15 +196,15 @@ In bullets, `result<T>`:
   - Supports migration from `Try` via `try_to_result` and `result_to_try`.
 
   - Uses automatic storage duration for `T`.  Allocation caveats:
-      * `exception_wrapper` uses the heap via `std::exception_ptr`.
+      * `std::exception_ptr` stores the exception on the heap.
       * While `result` coroutines are set up to be HALO-friendly, a compiler is
         not *obligated* to allocate `result` coroutines on the stack. Profile
         first, then read "how to avoid coro frame allocations" below.
 
 What to know about exceptions & `result`:
 
   - `result` coroutines (but **not** functions) are exception boundaries.
-    Any uncaught exception is returned as `res.non_value().error()`.
+    Any uncaught exception is captured in `res.non_value()` & returned.
 
   - The `result` API avoids throwing, aside from:
       * `value_or_throw()`, which you should avoid in favor of `co_await`,
@@ -277,8 +277,8 @@ Both are implicitly movable contexts, so the `std::move` is just visual
 noise, and can actually prevent NVRO for `return` (there's a linter against it).
 
 You can directly return any of these types: `result<V>`, `V`,
-convertible-to-`V`, convertible-to-`result<V>`, `exception_wrapper`,
-`non_value_result`, or `stopped_result`. None will incur unnecessary copies.
+convertible-to-`V`, convertible-to-`result<V>`, `non_value_result`, or
+`stopped_result`.  None will incur unnecessary copies.
 
 ## How to...
 
@@ -345,14 +345,14 @@ Most of the time, you will await a prvalue result, i.e. `co_await resultFunc()`.
 This moves the underlying value, or exposes the returned reference.
 
 However, if you have `result<V> r`, then `co_await r` will not compile -- that
-would have to copy `V` or `exception_wrapper`, and the `result` API tries to
+would have to copy `V` or `std::exception_ptr`, and the `result` API tries to
 make copies explicit.
 
 Instead, you have to choose from:
   - By-value: `co_await std::move(m)`: Returns a moved-out `V`. Error
     propagation is fast.
   - By-reference: `co_await std::cref(m)` / `std::ref(m)`: Returns `const V&` /
-    `V&`. Propagates `exception_wrapper` by copying (~25ns).
+    `V&`.  Propagates `std::exception_ptr` by copying (~25ns).
 
 For some `V`, `co_await`ing by-reference can speed up value access, at the
 expense of the error path.
@@ -400,8 +400,7 @@ difference, **as long as you follow the "mostly non-throwing" contract** of
 result<int> plantSeeds(int n) {
   return result_catch_all([&]() -> result<int> {
     if (n < 0) {
-      return make_exception_wrapper<std::logic_error>(
-          "cannot plant < 0 seeds");
+      return non_value_result{std::logic_error{"cannot plant < 0 seeds"}};
     }
     int seedsLeft = n;
     for (int i = 0; i < n; ++i) {}

third-party/folly/src/folly/result/gtest_helpers.h

@@ -21,16 +21,16 @@
 
 namespace folly {
 
-#define RESULT_CO_UNWRAP_BODY(body)                  \
-  {                                                  \
-    auto ret = body();                               \
-    if (!ret.has_value()) {                          \
-      if (ret.non_value().has_error()) {             \
-        FAIL() << ret.non_value().error();           \
-      } else {                                       \
-        FAIL() << "RESULT_CO_TEST got cancellation"; \
-      }                                              \
-    }                                                \
+#define RESULT_CO_UNWRAP_BODY(body)                                  \
+  {                                                                  \
+    auto ret = body();                                               \
+    if (!ret.has_value()) {                                          \
+      if (ret.non_value().has_stopped()) {                           \
+        FAIL() << "RESULT_CO_TEST got cancellation";                 \
+      } else {                                                       \
+        FAIL() << std::move(ret).non_value().to_exception_wrapper(); \
+      }                                                              \
+    }                                                                \
   }
 
 /*

third-party/folly/src/folly/result/result.cpp

@@ -24,28 +24,28 @@
 namespace folly::detail {
 
 const non_value_result& dfatal_get_empty_result_error() {
-  static const folly::Indestructible<non_value_result> r{
-      make_exception_wrapper<empty_result_error>()};
+  static const folly::Indestructible<non_value_result> r{empty_result_error{}};
   LOG(DFATAL) << "`folly::result` had an empty underlying `folly::Expected`";
   return *r;
 }
 
 const non_value_result& dfatal_get_bad_result_access_error() {
   static const folly::Indestructible<non_value_result> r{
-      make_exception_wrapper<bad_result_access_error>()};
-  LOG(DFATAL) << "Used `error()` accessor for `folly::result` in value state";
+      bad_result_access_error{}};
+  LOG(DFATAL)
+      << "Used `non_value()` accessor for `folly::result` in value state";
   return *r;
 }
 
 void fatal_if_exception_wrapper_invalid(const exception_wrapper& ew) {
   if (!ew.has_exception_ptr()) {
-    LOG(FATAL) << "`result` may not contain an empty `exception_wrapper`";
+    LOG(FATAL) << "`result` may not contain an empty `std::exception_ptr`";
   }
   if (folly::get_exception<folly::OperationCancelled>(ew)) {
     LOG(FATAL)
-        << "Do not put `OperationCancelled` in `result`, or get it via "
-        << "`error()`. Instead, store `stopped_result{}`, and check "
-        << "`has_error()` (or `!has_stopped()`) before calling `error()`.";
+        << "Do not store `OperationCancelled` in `result`. If you got this "
+        << "error while extracting an `exception_wrapper`, `exception_ptr`, "
+        << "or similar, you must check `has_stopped()` before doing that!";
   }
 }