Fix bug when ensuring union

Summary:
# Problem

A long-standing bug affected Thrift unions used as optional fields. When ensuring different fields in a union, the patch logic would incorrectly remove the entire union from its parent struct. For example:

```
union Bar {
  1: i32 field_1;
  2: i32 field_2;
}
struct Foo {
  1: optional Bar bar;
}
```

If you performed:

```
Foo.patch<ident::bar>().ensure<ident::field_1>(10);
Foo.patch<ident::bar>().ensure<ident::field_2>(20);
```

The patch would incorrectly translate to:

```
Foo.remove<ident::bar>();
```

# Why?

The union's ensure semantic clears all fields before ensuring a new one. E.g.,

```
MyUnion u;
u.field_1().ensure();
u.field_2().ensure();

EXPECT_FALSE(u.field_1().has_value());
```

In this case we ensured `field_1` first. When ensuring `field_2`, the existing code saw previously we ensured `field_1`, thus it will first clear all fields, then ensure field_2. In another word,

```
Foo.patch<ident::bar>().ensure<ident::field_2>(20);
```

this will be translated to

```
auto& patch = Foo.patch<ident::bar>();
patch.clear();
patch.ensure<ident::field_2>(20);
```

The problem is that `Foo.patch<ident::bar>().clear()` has a different meaning when `bar` is an optional field: it means removing the field from the parent struct. Thus the patch would be equivalent to

```
Foo.remove<ident::bar>();
```

which is not the intended behavior.

# Fix

The patch changes the behavior so that ensuring a different field in a union now uses assign instead of clear. e.g., if `field_1` is ensured previously, the following operation

```
Foo.patch<ident::bar>().ensure<ident::field_2>(20);
```

is now translated to:

```
Foo.patch<ident::bar>().assign<ident::field_2>(20);
```

# Another problem

To avoid similar problems, we should never translate other patch operations into `clear`, since `clear` will remove the data from the parent struct, which is not what we want.

I found that we also translate `assign` operation in union to `clear`, e.g.,

```
MyUnionPatch patch;
patch.assign(foo);
patch.patchIfSet<Id>();
```

when we saw `patchIfSet`, we will call `ensurePatchable` which translate `patch.assign(foo)` into

```
patch.clear();
patch.ensure(foo);
```

This is also problematic (see unit-test for concrete example). This diff changed so that it will be translated to the following code instead

```
patch.patch<Id>().assign(active field in foo);
```

Which is basically an ensure + assign.

Reviewed By: praihan

Differential Revision: D88342407

fbshipit-source-id: 082e4022ec192c0982c25905ead34043de9700df

Author: Mizuchi

third-party/thrift/src/thrift/lib/cpp2/op/detail/StructPatch.h

@@ -345,6 +345,21 @@ class BaseEnsurePatch : public BaseClearPatch<Patch, Derived> {
   /// Ensures the given field is set.
   template <typename Id>
   void ensure() {
+    if constexpr (is_thrift_union_v<T>) {
+      ensurePatchable();
+      if (data_.assign() ||
+          (!ensures<Id>() && !apache::thrift::empty(*data_.ensure()))) {
+        // If we are ensuring a different field than we previously ensured,
+        // we should assign this field.
+        constexpr bool kIsIOBufPtr =
+            std::is_same_v<FieldType<Id>, std::unique_ptr<folly::IOBuf>>;
+        if constexpr (kIsIOBufPtr) {
+          return Base::derived().template assign<Id>(folly::IOBuf{});
+        } else {
+          return Base::derived().template assign<Id>(FieldType<Id>{});
+        }
+      }
+    }
     // Ensuring non-optional field to intrinsic default is allowed since we
     // might want to ensure field in case the field doesn't exist in dynamic
     // value. (e.g., Terse field with default value. Without ensuring it first,
@@ -355,6 +370,16 @@ class BaseEnsurePatch : public BaseClearPatch<Patch, Derived> {
   template <typename Id, typename U = FieldType<Id>>
   std::enable_if_t<type::is_optional_or_union_field_v<T, Id>> ensure(
       U&& defaultVal) {
+    // TODO: simplify the logic by combining with the `ensure()` method above.
+    if constexpr (is_thrift_union_v<T>) {
+      ensurePatchable();
+      if (data_.assign() ||
+          (!ensures<Id>() && !apache::thrift::empty(*data_.ensure()))) {
+        // If we are ensuring a different field than we previously ensured,
+        // we should assign this field.
+        return Base::derived().template assign<Id>(std::forward<U>(defaultVal));
+      }
+    }
     if (maybeEnsure<Id>()) {
       if (patchPrior<Id>().toThrift().clear().value() &&
           !is_thrift_union_v<T>) {
@@ -591,12 +616,31 @@ class BaseEnsurePatch : public BaseClearPatch<Patch, Derived> {
   // operation so that we can have sub-patches.
   template <typename U = T>
   if_union_patch<U> ensurePatchable() {
-    if (data_.assign().has_value()) {
-      data_.clear() = true;
-      *data_.ensure() = std::move(*data_.assign());
-      // Unset assign.
-      data_.assign().reset();
+    if (!data_.assign().has_value()) {
+      return;
     }
+
+    if (data_.assign()->getType() == T::Type::__EMPTY__) {
+      // If we assigned patch to an empty union, we can't turn it into ensure +
+      // patch. In this case we can just keep it as it is since all field
+      // patches would be no-op unless we ensure them later.
+      return;
+    }
+
+    auto tmp = std::move(*data_.assign());
+    op::visit_union_with_tag(
+        tmp,
+        [&](auto tag, auto& field) {
+          using Id = folly::type_list_element_t<0, decltype(tag)>;
+          using Tag = op::get_type_tag<T, Id>;
+          Base::reset();
+          FieldPatchAssigner<Tag>{}(patch<Id>(), std::move(field));
+        },
+        [] {
+          throw std::logic_error(
+              "This should never happen. "
+              "We just handled the __EMPTY__ case.");
+        });
   }
 
   // For Thrift Struct, we always ensure the patch is patchable.