Make folly::get_mutable_exception separate from folly::get_exception

Summary:
`get_mutable_exception` is a separate verb because:

  - Mutable exception access is rare.  It may run into thread-safety bugs if a `std::current_exception()` pointer is accessed outside of the thread that threw it -- the standard permits reference semantics here!

  - Making mutable access explicit enables no-allocations, no-atomics optimizations for the `const`-access path.

I specifically ran into the *latter* issue, since I was implementing immortal exception pointers, and there's no 0-overhead way for those to provide mutable access.

On the other hand, with `get_mutable_exception`, it's completely fine for the accessor to modify the state of the potentially-immortal exception pointer to contain a just-copied `std::exception_ptr`.

Reviewed By: ispeters

Differential Revision: D73390138

fbshipit-source-id: ebfa60d54462cd372340b76b4dad08b24096e81a

Author: snarkmaster

third-party/folly/src/folly/ExceptionWrapper.h

@@ -338,13 +338,13 @@ class exception_wrapper final {
   template <class... CatchFns>
   void handle(CatchFns... fns) const;
 
-  // Implementation of `folly::get_exception<Ex>(ew)`
+  // Implement the `folly::get_exception<Ex>(ew)` protocol
   template <typename Ex>
-  Ex* get_exception(get_exception_tag_t) noexcept {
+  Ex const* get_exception(get_exception_tag_t) const noexcept {
     return get_exception<Ex>();
   }
   template <typename Ex>
-  Ex const* get_exception(get_exception_tag_t) const noexcept {
+  Ex* get_mutable_exception(get_exception_tag_t) noexcept {
     return get_exception<Ex>();
   }
 };

third-party/folly/src/folly/lang/Exception.h

@@ -589,29 +589,44 @@ T* exception_ptr_get_object_hint(std::exception_ptr const& ptr) noexcept {
 
 /// get_exception_tag_t
 ///
-/// Passkey: To extend `folly::get_exception<Ex>()`, a type must have the member
-/// `get_exception<Ex>(get_exception_tag_t)`.
+/// A type that may contain an exception may take this passkey in the following
+/// member functions:
+///   - `get_exception<Ex>(get_exception_tag_t) const` when implementing the
+///     `folly::get_exception<Ex>()` protocol.
+///   - `get_mutable_exception<Ex>(get_exception_tag_t)` when implementing the
+///     `folly::get_mutable_exception<Ex>()` protocol.
 struct get_exception_tag_t {};
 
 /// get_exception_fn
 /// get_exception
+/// get_mutable_exception_fn
+/// get_mutable_exception
 ///
-/// `get_exception<Ex>(v)` is meant to become a uniform way for accessing
-/// exception-containers in `folly`.  It returns:
+/// `get_exception<Ex>(v)` is meant to become the default way for accessing
+/// exception-containers in `folly`.
+///
+/// For the less-common scenario where you need mutable access to an error, use
+/// `get_mutable_exception<Ex>(v)`.  This is a separate verb because:
+///    - Mutable exception access is rare.  It may run into thread-safety bugs
+///      if a `std::current_exception()` pointer is accessed outside of the
+///      thread that threw it -- the standard permits reference semantics here!
+///    - Making mutable access explicit enables no-alloctions, no-atomics
+///      optimizations for the `const`-access path.
+///
+/// Both verbs return:
 ///   - `nullptr` if `v` is of a variant type, but is not in an "error" state,
 ///   - A pointer to the `Ex` held by `v`, if it holds an error whose type
 ///     `From` permits `std::is_convertible<From*, Ex*>`,
 ///   - `nullptr` for errors incompatible with `Ex*`.
 ///
 /// In addition to the `std::exception_ptr` support above, a type can support
-/// this verb by providing a `get_exception<Ex>(get_exception_tag_t)` member.
-/// For an example, see `ExceptionWrapper.h`. Requirements:
+/// this verb by providing member functions taking `get_exception_tag_t`.  For
+/// an example, see `ExceptionWrapper.h`.  Requirements:
 ///   - `noexcept`
-///   - `const`-correct
-///   - returns `Ex*` or `const Ex*` depending on the overload.
+///   - returns `Ex*` or `const Ex*` depending on the verb.
 ///
 /// This is most efficient when `Ex` matches the exact stored type, or when the
-/// type alias `Ex::folly_get_exception_hint_types` has a good hint.
+/// type alias `Ex::folly_get_exception_hint_types` provides a correct hint.
 ///
 /// NB: `result<T>` supports `get_exception<Ex>(res)`, but `Try<T>` currently
 /// omits `get_exception(get_exception_tag_t)`, because that might encourage
@@ -624,41 +639,66 @@ struct get_exception_tag_t {};
 ///   }
 template <typename Ex>
 class get_exception_fn {
- private:
-  template <typename CEx, typename SrcRef>
-  CEx* impl(SrcRef src) const noexcept {
-    using Src = remove_cvref_t<SrcRef>;
+ public:
+  template <typename Src>
+  const Ex* operator()(const Src& src) const noexcept {
     if constexpr (std::is_same_v<Src, std::exception_ptr>) {
-      return exception_ptr_get_object_hint<CEx>(src);
+      return exception_ptr_get_object_hint<const Ex>(src);
     } else {
       constexpr get_exception_tag_t passkey;
       static_assert( // Return type & `noexcept`ness must match
           std::is_same_v<
-              CEx*,
+              const Ex*,
               decltype(src.template get_exception<Ex>(passkey))> &&
           noexcept(noexcept(src.template get_exception<Ex>(passkey))));
       return src.template get_exception<Ex>(passkey);
     }
   }
-
- public:
+  // For a mutable ptr, use `folly::get_mutable_exception<Ex>(v)` instead.
   template <typename Src>
-  const Ex* operator()(const Src& src) const noexcept {
-    return impl<const Ex, const Src&>(src);
+  const Ex* operator()(Src& s) const noexcept {
+    return operator()(std::as_const(s));
   }
+
+  // It is unsafe to use `get_exception()` to get a pointer into an rvalue.
+  // If you know what you're doing, add a `static_cast`.
+  template <typename Src>
+  void operator()(Src&&) const noexcept = delete;
+  template <typename Src>
+  void operator()(const Src&&) const noexcept = delete;
+};
+template <typename Ex>
+class get_mutable_exception_fn {
+ public:
   template <typename Src>
   Ex* operator()(Src& src) const noexcept {
-    return impl<Ex, Src&>(src);
+    if constexpr (std::is_same_v<Src, std::exception_ptr>) {
+      return exception_ptr_get_object_hint<Ex>(src);
+    } else {
+      constexpr get_exception_tag_t passkey;
+      static_assert( // Return type & `noexcept`ness must match
+          std::is_same_v<
+              Ex*,
+              decltype(src.template get_mutable_exception<Ex>(passkey))> &&
+          noexcept(noexcept(src.template get_mutable_exception<Ex>(passkey))));
+      return src.template get_mutable_exception<Ex>(passkey);
+    }
   }
-  // It is unsafe to use `get_exception()` to get a pointer into an rvalue.
-  // If you know what you're doing, add a `static_cast`.
+  // You want `folly::get_exception<Ex>(v)` instead.
+  template <typename Src>
+  void operator()(const Src&) const noexcept = delete;
+
+  // It is unsafe to use `get_mutable_exception()` to get a pointer into an
+  // rvalue.  If you know what you're doing, add a `static_cast`.
   template <typename Src>
   void operator()(Src&&) const noexcept = delete;
-  template <typename Src> // NB: Actually, redundant with `Src&&` overload.
+  template <typename Src>
   void operator()(const Src&&) const noexcept = delete;
 };
 template <typename Ex = std::exception>
 inline constexpr get_exception_fn<Ex> get_exception{};
+template <typename Ex = std::exception>
+inline constexpr get_mutable_exception_fn<Ex> get_mutable_exception{};
 
 namespace detail {
 

third-party/folly/src/folly/lang/test/ExceptionTest.cpp

@@ -423,15 +423,21 @@ TEST_F(ExceptionTest, make_exception_ptr_with_in_place) {
 
 TEST_F(ExceptionTest, get_exception_from_std_exception_ptr) {
   using folly::get_exception;
+  using folly::get_mutable_exception;
 
   static_assert(
-      std::is_invocable_v< //
+      std::is_invocable_v<
           folly::get_exception_fn<std::exception>,
-          std::exception_ptr&>);
+          const std::exception_ptr&>);
   static_assert(
       std::is_invocable_v<
-          folly::get_exception_fn<std::exception>,
+          folly::get_mutable_exception_fn<std::exception>,
+          std::exception_ptr&>);
+  static_assert(
+      !std::is_invocable_v<
+          folly::get_mutable_exception_fn<std::exception>,
           const std::exception_ptr&>);
+
   // Unsafe to extract a pointer out of rvalues
   static_assert(
       !std::is_invocable_v<
@@ -441,6 +447,14 @@ TEST_F(ExceptionTest, get_exception_from_std_exception_ptr) {
       !std::is_invocable_v<
           folly::get_exception_fn<std::exception>,
           const std::exception_ptr&&>);
+  static_assert(
+      !std::is_invocable_v<
+          folly::get_mutable_exception_fn<std::exception>,
+          std::exception_ptr&&>);
+  static_assert(
+      !std::is_invocable_v<
+          folly::get_mutable_exception_fn<std::exception>,
+          const std::exception_ptr&&>);
 
   auto eptr = folly::make_exception_ptr_with([]() {
     return std::runtime_error{"foo"};
@@ -449,17 +463,33 @@ TEST_F(ExceptionTest, get_exception_from_std_exception_ptr) {
   EXPECT_EQ(nullptr, get_exception<std::system_error>(eptr));
 
   EXPECT_STREQ("foo", get_exception<std::exception>(eptr)->what());
+  EXPECT_STREQ(
+      "foo", get_exception<std::exception>(std::as_const(eptr))->what());
+  EXPECT_STREQ("foo", get_mutable_exception<std::exception>(eptr)->what());
+
   EXPECT_STREQ("foo", get_exception<const std::exception>(eptr)->what());
+  EXPECT_STREQ(
+      "foo", get_exception<const std::exception>(std::as_const(eptr))->what());
+  // While this is a very silly kind of usage, it does work.
+  EXPECT_STREQ(
+      "foo", get_mutable_exception<const std::exception>(eptr)->what());
+
   EXPECT_STREQ("foo", get_exception<>(eptr)->what());
+  EXPECT_STREQ("foo", get_exception<>(std::as_const(eptr))->what());
+  EXPECT_STREQ("foo", get_mutable_exception<>(eptr)->what());
 
   EXPECT_STREQ("foo", get_exception<std::runtime_error>(eptr)->what());
-  EXPECT_EQ(
-      folly::exception_ptr_get_object<std::runtime_error>(eptr),
-      get_exception<std::runtime_error>(eptr));
+  EXPECT_STREQ(
+      "foo", get_exception<std::runtime_error>(std::as_const(eptr))->what());
+  EXPECT_STREQ("foo", get_mutable_exception<std::runtime_error>(eptr)->what());
+
+  auto* expected_p = folly::exception_ptr_get_object<std::runtime_error>(eptr);
+  EXPECT_EQ(expected_p, get_exception<std::runtime_error>(eptr));
+  EXPECT_EQ(expected_p, get_exception<std::runtime_error>(std::as_const(eptr)));
 
   static_assert(
       std::is_same_v<
-          std::runtime_error*,
+          const std::runtime_error*,
           decltype(get_exception<std::runtime_error>(eptr))>);
   static_assert(
       std::is_same_v<
@@ -469,6 +499,10 @@ TEST_F(ExceptionTest, get_exception_from_std_exception_ptr) {
       std::is_same_v<
           const std::runtime_error*,
           decltype(get_exception<std::runtime_error>(std::as_const(eptr)))>);
+  static_assert(
+      std::is_same_v<
+          std::runtime_error*,
+          decltype(get_mutable_exception<std::runtime_error>(eptr))>);
 }
 
 TEST_F(ExceptionTest, exception_shared_string) {

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

@@ -144,10 +144,6 @@ class non_value_result {
 
   template <typename Ex, typename EW>
   static Ex* get_exception_impl(EW& ew) {
-    static_assert(
-        // This "cheap" test is fine since `OperationCancelled` is final.
-        !std::is_same_v<const OperationCancelled, const Ex>,
-        "Test results for cancellation via `has_stopped()`");
     return folly::get_exception<Ex>(ew);
   }
 
@@ -195,12 +191,18 @@ class non_value_result {
 
   // Implement the `folly::get_exception<Ex>(res)` protocol
   template <typename Ex>
-  Ex* get_exception(get_exception_tag_t) noexcept {
-    return get_exception_impl<Ex>(ew_);
+  const Ex* get_exception(get_exception_tag_t) const noexcept {
+    static_assert( // Note: `OperationCancelled` is final
+        !std::is_same_v<const OperationCancelled, const Ex>,
+        "Test results for cancellation via `has_stopped()`");
+    return folly::get_exception<Ex>(ew_);
   }
   template <typename Ex>
-  const Ex* get_exception(get_exception_tag_t) const noexcept {
-    return get_exception_impl<const Ex>(ew_);
+  Ex* get_mutable_exception(get_exception_tag_t) noexcept {
+    static_assert( // Note: `OperationCancelled` is final
+        !std::is_same_v<const OperationCancelled, const Ex>,
+        "Test results for cancellation via `has_stopped()`");
+    return folly::get_mutable_exception<Ex>(ew_);
   }
 
   // DO NOT USE these "legacy" functions outside of `folly` internals.  See the
@@ -462,11 +464,11 @@ class result_crtp {
 
   // Implement the `folly::get_exception<Ex>(res)` protocol
   template <typename Ex>
-  Ex* get_exception(get_exception_tag_t) noexcept {
+  Ex* get_mutable_exception(get_exception_tag_t) noexcept {
     if (!exp_.hasError()) {
       return nullptr;
     }
-    return folly::get_exception<Ex>(exp_.error());
+    return folly::get_mutable_exception<Ex>(exp_.error());
   }
   template <typename Ex>
   const Ex* get_exception(get_exception_tag_t) const noexcept {

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

@@ -740,7 +740,7 @@ TEST(Result, accessError) {
 
   // We can mutate the exception in-place.
   auto newErr = MyError{"buh-bye"};
-  *get_exception<MyError>(r) = std::move(newErr);
+  *get_mutable_exception<MyError>(r) = std::move(newErr);
   std::string msg2{"buh-bye"};
   EXPECT_EQ(msg2, get_exception<MyError>(r)->what());
 

third-party/folly/src/folly/test/ExceptionWrapperTest.cpp

@@ -391,39 +391,49 @@ TEST(ExceptionWrapper, withExceptionPtrAnyNilTest) {
 
 TEST(ExceptionWrapper, get_exception_from_exception_wrapper) {
   static_assert(
-      std::is_invocable_v< //
+      std::is_invocable_v<
           get_exception_fn<std::exception>,
-          exception_wrapper&>);
+          const exception_wrapper&>);
   static_assert(
       std::is_invocable_v<
-          get_exception_fn<std::exception>,
+          get_mutable_exception_fn<std::exception>,
+          exception_wrapper&>);
+  static_assert(
+      !std::is_invocable_v<
+          get_mutable_exception_fn<std::exception>,
           const exception_wrapper&>);
+
   // Unsafe to extract a pointer out of rvalues
   static_assert(
       !std::is_invocable_v<
           get_exception_fn<std::exception>,
-          exception_wrapper&&>);
+          const exception_wrapper&&>);
   static_assert(
       !std::is_invocable_v<
-          get_exception_fn<std::exception>,
-          const exception_wrapper&&>);
+          get_mutable_exception_fn<std::exception>,
+          exception_wrapper&&>);
 
   auto ew = make_exception_wrapper<std::runtime_error>("foo");
 
   EXPECT_EQ(nullptr, get_exception<std::system_error>(ew));
+  EXPECT_EQ(nullptr, get_mutable_exception<std::system_error>(ew));
 
   EXPECT_STREQ("foo", get_exception<std::exception>(ew)->what());
   EXPECT_STREQ("foo", get_exception<const std::exception>(ew)->what());
   EXPECT_STREQ("foo", get_exception<>(ew)->what());
+  EXPECT_STREQ("foo", get_exception<>(std::as_const(ew))->what());
+  EXPECT_STREQ("foo", get_mutable_exception<>(ew)->what());
 
   EXPECT_STREQ("foo", get_exception<std::runtime_error>(ew)->what());
-  EXPECT_EQ(
-      ew.get_exception<std::runtime_error>(),
-      get_exception<std::runtime_error>(ew));
+  EXPECT_STREQ("foo", get_mutable_exception<std::runtime_error>(ew)->what());
+
+  auto expected_p = ew.get_exception<std::runtime_error>();
+  EXPECT_EQ(expected_p, get_exception<std::runtime_error>(ew));
+  EXPECT_EQ(expected_p, get_mutable_exception<std::runtime_error>(ew));
 
   static_assert(
       std::is_same_v<
-          std::runtime_error*,
+          const std::runtime_error*,
           decltype(get_exception<std::runtime_error>(ew))>);
   static_assert(
       std::is_same_v<
@@ -433,6 +443,10 @@ TEST(ExceptionWrapper, get_exception_from_exception_wrapper) {
       std::is_same_v<
           const std::runtime_error*,
           decltype(get_exception<std::runtime_error>(std::as_const(ew)))>);
+  static_assert(
+      std::is_same_v<
+          std::runtime_error*,
+          decltype(get_mutable_exception<std::runtime_error>(ew))>);
 }
 
 TEST(ExceptionWrapper, withExceptionDeduction) {