Make expected<T, E> support movable and non-movable types + add tests

Summary: The code already pretty much worked except for `reinit_impl` which had a bug that was never exercised previously.

Reviewed By: vitaut

Differential Revision: D62713190

fbshipit-source-id: ff7e5fd038c44c601c3cde60cad34f51947f2afa

thrift/compiler/whisker/expected.h

@@ -215,12 +215,8 @@ template <typename T, typename E>
 class expected {
   static_assert(detail::check_valid_value_type<T>());
   static_assert(!std::is_void_v<T>, "Use std::monostate instead.");
-  static_assert(std::is_nothrow_move_constructible_v<T>);
-  static_assert(std::is_nothrow_move_assignable_v<T>);
 
   static_assert(detail::check_valid_error_type<E>());
-  static_assert(std::is_nothrow_move_constructible_v<E>);
-  static_assert(std::is_nothrow_move_assignable_v<E>);
 
   // Restrictions on constructor (6):
   //   https://en.cppreference.com/w/cpp/utility/expected/expected#Version_6
@@ -296,12 +292,22 @@ class expected {
   expected(const expected& other) noexcept(
       std::is_nothrow_copy_constructible_v<T> &&
       std::is_nothrow_copy_constructible_v<E>)
-      : storage_(from_other(other.storage_)) {}
+      : storage_(from_other(other.storage_)) {
+    // To properly SFINAE this, we need to move the copy ctor to a base class or
+    // use C++20 concepts
+    static_assert(
+        std::is_copy_constructible_v<T> && std::is_copy_constructible_v<E>);
+  }
   // https://en.cppreference.com/w/cpp/utility/expected/expected#Version_3
   expected(expected&& other) noexcept(
       std::is_nothrow_move_constructible_v<T> &&
       std::is_nothrow_move_constructible_v<E>)
-      : storage_(from_other(std::move(other.storage_))) {}
+      : storage_(from_other(std::move(other.storage_))) {
+    // To properly SFINAE this, we need to move the move ctor to a base class or
+    // use C++20 concepts
+    static_assert(
+        std::is_move_constructible_v<T> && std::is_move_constructible_v<E>);
+  }
 
   // https://en.cppreference.com/w/cpp/utility/expected/expected#Version_4
   // (implicit)
@@ -735,7 +741,7 @@ class expected {
       storage_.template emplace<New>(std::move(tmp));
     } else {
       static_assert(std::is_nothrow_move_constructible_v<Current>);
-      Current tmp(*std::move(*this));
+      Current tmp(std::move(std::get<Current>(storage_)));
       try {
         storage_.template emplace<New>(std::forward<Args>(args)...);
       } catch (...) {

thrift/compiler/whisker/test/expected_test.cc

@@ -85,6 +85,30 @@ struct move_only {
   move_only& operator=(move_only&&) noexcept = default;
 };
 
+struct non_movable {
+  non_movable() = default;
+  non_movable(non_movable&&) = delete;
+  non_movable& operator=(non_movable&&) = delete;
+};
+
+struct maythrow_movable {
+  maythrow_movable(bool should_throw = false) noexcept
+      : should_throw(should_throw) {}
+  maythrow_movable(maythrow_movable&&) noexcept(false) {
+    if (should_throw) {
+      throw std::runtime_error("maythrow_movable(maythrow_movable&&)");
+    }
+  }
+  maythrow_movable& operator=(maythrow_movable&&) noexcept(false) {
+    if (should_throw) {
+      throw std::runtime_error("maythrow_movable::operator=");
+    }
+    return *this;
+  }
+
+  bool should_throw;
+};
+
 } // namespace
 
 TEST(ExpectedTest, construct_default) {
@@ -134,6 +158,28 @@ TEST(ExpectedTest, construct_move_only) {
   EXPECT_TRUE(e2.has_value());
 }
 
+TEST(ExpectedTest, construct_value_with_move_only_error) {
+  expected<int, move_only> e1;
+  expected<int, move_only> e2 = std::move(e1);
+  EXPECT_EQ(std::move(e2).value(), 0);
+}
+
+TEST(ExpectedTest, construct_non_movable) {
+  expected<non_movable, int> e1;
+  EXPECT_TRUE(e1.has_value());
+  e1 = unexpected(1);
+  EXPECT_FALSE(e1.has_value());
+  EXPECT_EQ(e1.error(), 1);
+}
+
+TEST(ExpectedTest, construct_value_with_non_movable_error) {
+  expected<int, non_movable> e1{unexpect};
+  EXPECT_FALSE(e1.has_value());
+  e1 = 1;
+  EXPECT_TRUE(e1.has_value());
+  EXPECT_EQ(*e1, 1);
+}
+
 TEST(ExpectedTest, construct_error) {
   expected<int, int> e = unexpected(1);
   EXPECT_FALSE(e.has_value());
@@ -215,6 +261,12 @@ TEST(ExpectedTest, emplace_move_only) {
   EXPECT_TRUE(e.has_value());
 }
 
+TEST(ExpectedTest, emplace_non_movable) {
+  expected<non_movable, int> e = unexpected(1);
+  e.emplace();
+  EXPECT_TRUE(e.has_value());
+}
+
 TEST(ExpectedTest, emplace_never_empty) {
   class throw_on_construct {
    public:
@@ -340,6 +392,39 @@ TEST(ExpectedTest, assign_move_only) {
   EXPECT_TRUE(e1.has_value());
 }
 
+TEST(ExpectedTest, assign_move_only_error) {
+  expected<int, move_only> e1 = unexpected(move_only());
+  expected<int, move_only> e2;
+  e2 = std::move(e1);
+  EXPECT_FALSE(e2.has_value());
+}
+
+TEST(ExpectedTest, assign_maythrow_movable) {
+  expected<maythrow_movable, int> e1 = unexpected(1);
+  expected<maythrow_movable, int> e2;
+  e2 = std::move(e1);
+  EXPECT_EQ(e2.error(), 1);
+
+  expected<maythrow_movable, int> e3{std::in_place, true};
+  expected<maythrow_movable, int> e4 = unexpected(1);
+  EXPECT_THROW((e4 = std::move(e3)), std::runtime_error);
+  EXPECT_FALSE(e4.has_value());
+  EXPECT_EQ(e4.error(), 1);
+}
+
+TEST(ExpectedTest, assign_maythrow_movable_error) {
+  expected<int, maythrow_movable> e1 = unexpected(1);
+  expected<int, maythrow_movable> e2;
+  e2 = std::move(e1);
+  EXPECT_FALSE(e2.has_value());
+
+  expected<int, maythrow_movable> e3{unexpect, true};
+  expected<int, maythrow_movable> e4 = 1;
+  EXPECT_THROW((e4 = std::move(e3)), std::runtime_error);
+  EXPECT_TRUE(e4.has_value());
+  EXPECT_EQ(*e4, 1);
+}
+
 TEST(ExpectedTest, comparison) {
   expected<int, int> e1 = 42;
   expected<int, int> e2 = 42;