Fix move assignment for TypeErasedValue

Summary:
The current implementation of move assignment is completely broken if the two `TypeErasedValue` instances are of different types. the `move_assign` in the vtable is called with the wrong type!

This diff implements move assign in terms of destroy + move (similar to move constructor) and removes `move_assign` from the vtable (which already has very limited usefulness)

Reviewed By: evanjzou

Differential Revision: D70298897

fbshipit-source-id: 39f2001d1f3338b35068c437c478360d7dbcb7aa

thrift/lib/cpp2/util/TypeErasedValue.h

@@ -16,7 +16,6 @@
 
 #pragma once
 
-#include <string>
 #include <typeinfo>
 #include <utility>
 
@@ -33,7 +32,6 @@ class VTable {
       : typeInfo_(typeInfo) {}
   virtual void destroy(std::byte*) const noexcept = 0;
   virtual void move(std::byte* dst, std::byte* src) const noexcept = 0;
-  virtual void move_assign(std::byte* dst, std::byte* src) const noexcept = 0;
 
   const std::type_info& type() const noexcept { return typeInfo_; }
 
@@ -52,10 +50,6 @@ class VTableImpl final : public VTable {
   void move(std::byte* origin, std::byte* destination) const noexcept final {
     new (destination) T(std::move(value(origin)));
   }
-  void move_assign(
-      std::byte* origin, std::byte* destination) const noexcept final {
-    value(destination) = std::move(value(origin));
-  }
 
  private:
   static T& value(std::byte* storage) noexcept {
@@ -192,9 +186,10 @@ class TypeErasedValue final {
   }
 
   TypeErasedValue& operator=(TypeErasedValue&& other) noexcept {
+    reset();
     vtable_ = other.vtable_;
     if (has_value()) {
-      vtable_->move_assign(other.storage_, storage_);
+      vtable_->move(other.storage_, storage_);
       other.reset();
     }
     return *this;

thrift/lib/cpp2/util/test/TypeErasedValueTest.cpp

@@ -254,7 +254,7 @@ TEST_F(TypeErasedValueTest, moveTest) {
       LifetimeOperationsTracker::Ops::DESTRUCT,
       LifetimeOperationsTracker::Ops::DESTRUCT,
       LifetimeOperationsTracker::Ops::CONSTRUCT,
-      LifetimeOperationsTracker::Ops::MOVE_ASSIGN,
+      LifetimeOperationsTracker::Ops::MOVE,
       LifetimeOperationsTracker::Ops::DESTRUCT,
       LifetimeOperationsTracker::Ops::DESTRUCT,
   };
@@ -365,4 +365,15 @@ TEST_F(TypeErasedValueTest, swapTest) {
   ASSERT_EQ(storageTwo.value<CounterStorage>().getCount(), 2);
 }
 
+TEST_F(TypeErasedValueTest, moveAssignNonTrivial) {
+  using StringStorage =
+      TypeErasedValue<sizeof(std::string), alignof(std::string)>;
+  StringStorage storage1;
+  storage1.emplace<std::string>("hello");
+
+  StringStorage storage2;
+  storage2 = std::move(storage1);
+  ASSERT_EQ(storage2.value<std::string>(), "hello");
+}
+
 } // namespace apache::thrift::util::test