Make Try's copy/move constructability match inner type

Summary: Before this change Try always reports as copy constructible then fires a static_assert if you try to instantiate that constructor for a non-copy-constructible type, which breaks logic like that inside std::vector::resize.

Reviewed By: yfeldblum

Differential Revision: D32567774

fbshipit-source-id: 4e3e35a1f823ac9ccb5e2d648015fddccb14cfbf

folly/Try-inl.h

@@ -25,8 +25,10 @@
 
 namespace folly {
 
+namespace detail {
 template <class T>
-Try<T>::Try(Try<T>&& t) noexcept(std::is_nothrow_move_constructible<T>::value)
+TryBase<T>::TryBase(TryBase<T>&& t) noexcept(
+    std::is_nothrow_move_constructible<T>::value)
     : contains_(t.contains_) {
   if (contains_ == Contains::VALUE) {
     ::new (static_cast<void*>(std::addressof(value_))) T(std::move(t.value_));
@@ -36,28 +38,7 @@ Try<T>::Try(Try<T>&& t) noexcept(std::is_nothrow_move_constructible<T>::value)
 }
 
 template <class T>
-template <class T2>
-Try<T>::Try(typename std::enable_if<
-            std::is_same<Unit, T2>::value,
-            Try<void> const&>::type t) noexcept
-    : contains_(Contains::NOTHING) {
-  if (t.hasValue()) {
-    contains_ = Contains::VALUE;
-    ::new (static_cast<void*>(std::addressof(value_))) T();
-  } else if (t.hasException()) {
-    contains_ = Contains::EXCEPTION;
-    new (&e_) exception_wrapper(t.exception());
-  }
-}
-
-Try<void>::Try(const Try<Unit>& t) noexcept : hasValue_(!t.hasException()) {
-  if (t.hasException()) {
-    new (&e_) exception_wrapper(t.exception());
-  }
-}
-
-template <class T>
-Try<T>& Try<T>::operator=(Try<T>&& t) noexcept(
+TryBase<T>& TryBase<T>::operator=(TryBase<T>&& t) noexcept(
     std::is_nothrow_move_constructible<T>::value) {
   if (this == &t) {
     return *this;
@@ -77,11 +58,8 @@ Try<T>& Try<T>::operator=(Try<T>&& t) noexcept(
 }
 
 template <class T>
-Try<T>::Try(const Try<T>& t) noexcept(
+TryBase<T>::TryBase(const TryBase<T>& t) noexcept(
     std::is_nothrow_copy_constructible<T>::value) {
-  static_assert(
-      std::is_copy_constructible<T>::value,
-      "T must be copyable for Try<T> to be copyable");
   contains_ = t.contains_;
   if (contains_ == Contains::VALUE) {
     ::new (static_cast<void*>(std::addressof(value_))) T(t.value_);
@@ -91,12 +69,8 @@ Try<T>::Try(const Try<T>& t) noexcept(
 }
 
 template <class T>
-Try<T>& Try<T>::operator=(const Try<T>& t) noexcept(
+TryBase<T>& TryBase<T>::operator=(const TryBase<T>& t) noexcept(
     std::is_nothrow_copy_constructible<T>::value) {
-  static_assert(
-      std::is_copy_constructible<T>::value,
-      "T must be copyable for Try<T> to be copyable");
-
   if (this == &t) {
     return *this;
   }
@@ -115,7 +89,32 @@ Try<T>& Try<T>::operator=(const Try<T>& t) noexcept(
 }
 
 template <class T>
-Try<T>::~Try() {
+void TryBase<T>::destroy() noexcept {
+  auto oldContains = std::exchange(contains_, Contains::NOTHING);
+  if (LIKELY(oldContains == Contains::VALUE)) {
+    value_.~T();
+  } else if (UNLIKELY(oldContains == Contains::EXCEPTION)) {
+    e_.~exception_wrapper();
+  }
+}
+
+template <class T>
+template <class T2>
+TryBase<T>::TryBase(typename std::enable_if<
+                    std::is_same<Unit, T2>::value,
+                    Try<void> const&>::type t) noexcept
+    : contains_(Contains::NOTHING) {
+  if (t.hasValue()) {
+    contains_ = Contains::VALUE;
+    ::new (static_cast<void*>(std::addressof(value_))) T();
+  } else if (t.hasException()) {
+    contains_ = Contains::EXCEPTION;
+    new (&e_) exception_wrapper(t.exception());
+  }
+}
+
+template <class T>
+TryBase<T>::~TryBase() {
   if (LIKELY(contains_ == Contains::VALUE)) {
     value_.~T();
   } else if (UNLIKELY(contains_ == Contains::EXCEPTION)) {
@@ -123,15 +122,23 @@ Try<T>::~Try() {
   }
 }
 
+} // namespace detail
+
+Try<void>::Try(const Try<Unit>& t) noexcept : hasValue_(!t.hasException()) {
+  if (t.hasException()) {
+    new (&this->e_) exception_wrapper(t.exception());
+  }
+}
+
 template <typename T>
 template <typename... Args>
 T& Try<T>::emplace(Args&&... args) noexcept(
     std::is_nothrow_constructible<T, Args&&...>::value) {
   this->destroy();
-  ::new (static_cast<void*>(std::addressof(value_)))
+  ::new (static_cast<void*>(std::addressof(this->value_)))
       T(static_cast<Args&&>(args)...);
-  contains_ = Contains::VALUE;
-  return value_;
+  this->contains_ = Contains::VALUE;
+  return this->value_;
 }
 
 template <typename T>
@@ -139,33 +146,33 @@ template <typename... Args>
 exception_wrapper& Try<T>::emplaceException(Args&&... args) noexcept(
     std::is_nothrow_constructible<exception_wrapper, Args&&...>::value) {
   this->destroy();
-  new (&e_) exception_wrapper(static_cast<Args&&>(args)...);
-  contains_ = Contains::EXCEPTION;
-  return e_;
+  new (&this->e_) exception_wrapper(static_cast<Args&&>(args)...);
+  this->contains_ = Contains::EXCEPTION;
+  return this->e_;
 }
 
 template <class T>
 T& Try<T>::value() & {
   throwUnlessValue();
-  return value_;
+  return this->value_;
 }
 
 template <class T>
 T&& Try<T>::value() && {
   throwUnlessValue();
-  return std::move(value_);
+  return std::move(this->value_);
 }
 
 template <class T>
 const T& Try<T>::value() const& {
   throwUnlessValue();
-  return value_;
+  return this->value_;
 }
 
 template <class T>
 const T&& Try<T>::value() const&& {
   throwUnlessValue();
-  return std::move(value_);
+  return std::move(this->value_);
 }
 
 template <class T>
@@ -183,11 +190,11 @@ T Try<T>::value_or(U&& defaultValue) && {
 
 template <class T>
 void Try<T>::throwUnlessValue() const {
-  switch (contains_) {
+  switch (this->contains_) {
     case Contains::VALUE:
       return;
     case Contains::EXCEPTION:
-      e_.throw_exception();
+      this->e_.throw_exception();
     case Contains::NOTHING:
     default:
       throw_exception<UsingUninitializedTry>();
@@ -199,27 +206,17 @@ void Try<T>::throwIfFailed() const {
   throwUnlessValue();
 }
 
-template <class T>
-void Try<T>::destroy() noexcept {
-  auto oldContains = std::exchange(contains_, Contains::NOTHING);
-  if (LIKELY(oldContains == Contains::VALUE)) {
-    value_.~T();
-  } else if (UNLIKELY(oldContains == Contains::EXCEPTION)) {
-    e_.~exception_wrapper();
-  }
-}
-
 Try<void>& Try<void>::operator=(const Try<void>& t) noexcept {
   if (t.hasException()) {
     if (hasException()) {
-      e_ = t.e_;
+      this->e_ = t.e_;
     } else {
-      new (&e_) exception_wrapper(t.e_);
+      new (&this->e_) exception_wrapper(t.e_);
       hasValue_ = false;
     }
   } else {
     if (hasException()) {
-      e_.~exception_wrapper();
+      this->e_.~exception_wrapper();
       hasValue_ = true;
     }
   }
@@ -230,16 +227,16 @@ template <typename... Args>
 exception_wrapper& Try<void>::emplaceException(Args&&... args) noexcept(
     std::is_nothrow_constructible<exception_wrapper, Args&&...>::value) {
   if (hasException()) {
-    e_.~exception_wrapper();
+    this->e_.~exception_wrapper();
   }
-  new (&e_) exception_wrapper(static_cast<Args&&>(args)...);
+  new (&this->e_) exception_wrapper(static_cast<Args&&>(args)...);
   hasValue_ = false;
-  return e_;
+  return this->e_;
 }
 
 void Try<void>::throwIfFailed() const {
   if (hasException()) {
-    e_.throw_exception();
+    this->e_.throw_exception();
   }
 }
 

folly/Try.h

@@ -42,18 +42,13 @@ class FOLLY_EXPORT UsingUninitializedTry : public TryException {
   UsingUninitializedTry() : TryException("Using uninitialized try") {}
 };
 
-/*
- * Try<T> is a wrapper that contains either an instance of T, an exception, or
- * nothing. Exceptions are stored as exception_wrappers so that the user can
- * minimize rethrows if so desired.
- *
- * To represent success or a captured exception, use Try<Unit>.
- */
 template <class T>
-class Try {
-  static_assert(
-      !std::is_reference<T>::value, "Try may not be used with reference types");
+class Try;
 
+namespace detail {
+template <class T>
+class TryBase {
+ protected:
   enum class Contains {
     VALUE,
     EXCEPTION,
@@ -61,22 +56,17 @@ class Try {
   };
 
  public:
-  /*
-   * The value type for the Try
-   */
-  typedef T element_type;
-
   /*
    * Construct an empty Try
    */
-  Try() noexcept : contains_(Contains::NOTHING) {}
+  TryBase() noexcept : contains_(Contains::NOTHING) {}
 
   /*
    * Construct a Try with a value by copy
    *
    * @param v The value to copy in
    */
-  explicit Try(const T& v) noexcept(
+  explicit TryBase(const T& v) noexcept(
       std::is_nothrow_copy_constructible<T>::value)
       : contains_(Contains::VALUE), value_(v) {}
 
@@ -85,41 +75,77 @@ class Try {
    *
    * @param v The value to move in
    */
-  explicit Try(T&& v) noexcept(std::is_nothrow_move_constructible<T>::value)
+  explicit TryBase(T&& v) noexcept(std::is_nothrow_move_constructible<T>::value)
       : contains_(Contains::VALUE), value_(std::move(v)) {}
 
   template <typename... Args>
-  explicit Try(in_place_t, Args&&... args) noexcept(
+  explicit TryBase(in_place_t, Args&&... args) noexcept(
       std::is_nothrow_constructible<T, Args&&...>::value)
       : contains_(Contains::VALUE), value_(static_cast<Args&&>(args)...) {}
 
   /// Implicit conversion from Try<void> to Try<Unit>
   template <class T2 = T>
-  /* implicit */
-  Try(typename std::enable_if<std::is_same<Unit, T2>::value, Try<void> const&>::
-          type t) noexcept;
+  /* implicit */ TryBase(typename std::enable_if<
+                         std::is_same<Unit, T2>::value,
+                         Try<void> const&>::type t) noexcept;
 
   /*
    * Construct a Try with an exception_wrapper
    *
    * @param e The exception_wrapper
    */
-  explicit Try(exception_wrapper e) noexcept
+  explicit TryBase(exception_wrapper e) noexcept
       : contains_(Contains::EXCEPTION), e_(std::move(e)) {}
 
+  ~TryBase();
+
   // Move constructor
-  Try(Try<T>&& t) noexcept(std::is_nothrow_move_constructible<T>::value);
+  TryBase(TryBase&& t) noexcept(std::is_nothrow_move_constructible<T>::value);
   // Move assigner
-  Try& operator=(Try<T>&& t) noexcept(
+  TryBase& operator=(TryBase&& t) noexcept(
       std::is_nothrow_move_constructible<T>::value);
 
   // Copy constructor
-  Try(const Try& t) noexcept(std::is_nothrow_copy_constructible<T>::value);
+  TryBase(const TryBase& t) noexcept(
+      std::is_nothrow_copy_constructible<T>::value);
   // Copy assigner
-  Try& operator=(const Try& t) noexcept(
+  TryBase& operator=(const TryBase& t) noexcept(
       std::is_nothrow_copy_constructible<T>::value);
 
-  ~Try();
+ protected:
+  void destroy() noexcept;
+
+  Contains contains_;
+  union {
+    T value_;
+    exception_wrapper e_;
+  };
+};
+} // namespace detail
+
+/*
+ * Try<T> is a wrapper that contains either an instance of T, an exception, or
+ * nothing. Exceptions are stored as exception_wrappers so that the user can
+ * minimize rethrows if so desired.
+ *
+ * To represent success or a captured exception, use Try<Unit>.
+ */
+template <class T>
+class Try : detail::TryBase<T>,
+            moveonly_::EnableCopyMove<
+                std::is_copy_constructible<T>::value,
+                std::is_move_constructible<T>::value> {
+  static_assert(
+      !std::is_reference<T>::value, "Try may not be used with reference types");
+  using typename detail::TryBase<T>::Contains;
+
+ public:
+  using detail::TryBase<T>::TryBase;
+
+  /*
+   * The value type for the Try
+   */
+  using element_type = T;
 
   /*
    * In-place construct the value in the Try object.
@@ -241,46 +267,46 @@ class Try {
   /*
    * @returns True if the Try contains a value, false otherwise
    */
-  bool hasValue() const { return contains_ == Contains::VALUE; }
+  bool hasValue() const { return this->contains_ == Contains::VALUE; }
   /*
    * @returns True if the Try contains an exception, false otherwise
    */
-  bool hasException() const { return contains_ == Contains::EXCEPTION; }
+  bool hasException() const { return this->contains_ == Contains::EXCEPTION; }
 
   /*
    * @returns True if the Try contains an exception of type Ex, false otherwise
    */
   template <class Ex>
   bool hasException() const {
-    return hasException() && e_.is_compatible_with<Ex>();
+    return hasException() && this->e_.template is_compatible_with<Ex>();
   }
 
   exception_wrapper& exception() & {
     if (!hasException()) {
       throw_exception<TryException>("Try does not contain an exception");
     }
-    return e_;
+    return this->e_;
   }
 
   exception_wrapper&& exception() && {
     if (!hasException()) {
       throw_exception<TryException>("Try does not contain an exception");
     }
-    return std::move(e_);
+    return std::move(this->e_);
   }
 
   const exception_wrapper& exception() const& {
     if (!hasException()) {
       throw_exception<TryException>("Try does not contain an exception");
     }
-    return e_;
+    return this->e_;
   }
 
   const exception_wrapper&& exception() const&& {
     if (!hasException()) {
       throw_exception<TryException>("Try does not contain an exception");
     }
-    return std::move(e_);
+    return std::move(this->e_);
   }
 
   /*
@@ -288,10 +314,10 @@ class Try {
    *          otherwise, returns `nullptr`.
    */
   std::exception* tryGetExceptionObject() {
-    return hasException() ? e_.get_exception() : nullptr;
+    return hasException() ? this->e_.get_exception() : nullptr;
   }
   std::exception const* tryGetExceptionObject() const {
-    return hasException() ? e_.get_exception() : nullptr;
+    return hasException() ? this->e_.get_exception() : nullptr;
   }
 
   /*
@@ -301,11 +327,11 @@ class Try {
    */
   template <class Ex>
   Ex* tryGetExceptionObject() {
-    return hasException() ? e_.get_exception<Ex>() : nullptr;
+    return hasException() ? this->e_.template get_exception<Ex>() : nullptr;
   }
   template <class Ex>
   Ex const* tryGetExceptionObject() const {
-    return hasException() ? e_.get_exception<Ex>() : nullptr;
+    return hasException() ? this->e_.template get_exception<Ex>() : nullptr;
   }
 
   /*
@@ -320,14 +346,14 @@ class Try {
     if (!hasException()) {
       return false;
     }
-    return e_.with_exception<Ex>(std::move(func));
+    return this->e_.template with_exception<Ex>(std::move(func));
   }
   template <class Ex, class F>
   bool withException(F func) const {
     if (!hasException()) {
       return false;
     }
-    return e_.with_exception<Ex>(std::move(func));
+    return this->e_.template with_exception<Ex>(std::move(func));
   }
 
   /*
@@ -343,14 +369,14 @@ class Try {
     if (!hasException()) {
       return false;
     }
-    return e_.with_exception(std::move(func));
+    return this->e_.with_exception(std::move(func));
   }
   template <class F>
   bool withException(F func) const {
     if (!hasException()) {
       return false;
     }
-    return e_.with_exception(std::move(func));
+    return this->e_.with_exception(std::move(func));
   }
 
   template <bool isTry, typename R>
@@ -362,15 +388,6 @@ class Try {
   typename std::enable_if<!isTry, R>::type get() {
     return std::forward<R>(value());
   }
-
- private:
-  void destroy() noexcept;
-
-  Contains contains_;
-  union {
-    T value_;
-    exception_wrapper e_;
-  };
 };
 
 /*
@@ -697,6 +714,11 @@ auto unwrapTryTuple(Tuple&&);
 template <typename T>
 void tryAssign(Try<T>& t, Try<T>&& other) noexcept;
 
+#if FOLLY_CPLUSPLUS >= 201703L
+template <typename T>
+Try(T&&) -> Try<std::decay_t<T>>;
+#endif
+
 } // namespace folly
 
 #include <folly/Try-inl.h>

folly/Utility.h

@@ -271,25 +271,49 @@ FOLLY_INLINE_VARIABLE constexpr identity_fn identity;
 
 namespace moveonly_ { // Protection from unintended ADL.
 
+template <bool Copy, bool Move>
+class EnableCopyMove {
+ protected:
+  constexpr EnableCopyMove() noexcept = default;
+  ~EnableCopyMove() noexcept = default;
+
+  EnableCopyMove(EnableCopyMove&&) noexcept = default;
+  EnableCopyMove& operator=(EnableCopyMove&&) noexcept = default;
+  EnableCopyMove(const EnableCopyMove&) noexcept = default;
+  EnableCopyMove& operator=(const EnableCopyMove&) noexcept = default;
+};
+
 /**
  * Disallow copy but not move in derived types. This is essentially
  * boost::noncopyable (the implementation is almost identical) but it
  * doesn't delete move constructor and move assignment.
  */
-class MoveOnly {
+template <>
+class EnableCopyMove<false, true> {
  protected:
-  constexpr MoveOnly() = default;
-  ~MoveOnly() = default;
+  constexpr EnableCopyMove() noexcept = default;
+  ~EnableCopyMove() noexcept = default;
 
-  MoveOnly(MoveOnly&&) = default;
-  MoveOnly& operator=(MoveOnly&&) = default;
-  MoveOnly(const MoveOnly&) = delete;
-  MoveOnly& operator=(const MoveOnly&) = delete;
+  EnableCopyMove(EnableCopyMove&&) noexcept = default;
+  EnableCopyMove& operator=(EnableCopyMove&&) noexcept = default;
+  EnableCopyMove(const EnableCopyMove&) = delete;
+  EnableCopyMove& operator=(const EnableCopyMove&) = delete;
 };
 
+template <>
+class EnableCopyMove<false, false> {
+ protected:
+  constexpr EnableCopyMove() noexcept = default;
+  ~EnableCopyMove() noexcept = default;
+
+  EnableCopyMove(EnableCopyMove&&) = delete;
+  EnableCopyMove& operator=(EnableCopyMove&&) = delete;
+  EnableCopyMove(const EnableCopyMove&) = delete;
+  EnableCopyMove& operator=(const EnableCopyMove&) = delete;
+};
 } // namespace moveonly_
 
-using MoveOnly = moveonly_::MoveOnly;
+using MoveOnly = moveonly_::EnableCopyMove<false, true>;
 
 struct to_signed_fn {
   template <typename..., typename T>

folly/test/TryTest.cpp

@@ -774,3 +774,13 @@ TEST(Try, TestUnwrapForward) {
   auto unwrapped = unwrapTryTuple(std::move(original));
   EXPECT_EQ(*std::get<0>(unwrapped), 1);
 }
+
+TEST(Try, CopyConstructible) {
+  EXPECT_TRUE(std::is_copy_constructible<Try<int>>::value);
+  EXPECT_FALSE(std::is_copy_constructible<Try<MoveConstructOnly>>::value);
+}
+
+TEST(Try, CTAD) {
+  folly::Try t1(folly::unit);
+  folly::Try t2(42);
+}