AK: Allow the Optional<T> move assignment operator to be trivial

This will change the behaviour for moved-from `Optional`s, since they
now no longer clear their value if it is trivial, but it should be
considered undefined behaviour to access a moved-from `Optional`.
Use `Optional<T>::clear` or `Optional<T>::release_value` instead.

AK/Optional.h

@@ -186,7 +186,7 @@ public:
     }
 
     AK_MAKE_CONDITIONALLY_COPYABLE(Optional, <T>);
-    AK_MAKE_CONDITIONALLY_NONMOVABLE(Optional, <T>);
+    AK_MAKE_CONDITIONALLY_MOVABLE(Optional, <T>);
     AK_MAKE_CONDITIONALLY_DESTRUCTIBLE(Optional, <T>);
 
     ALWAYS_INLINE constexpr Optional(Optional const& other)
@@ -250,7 +250,34 @@ public:
         return *this;
     }
 
+    Optional& operator=(Optional&& other)
+    requires(!IsMoveConstructible<T> || !IsDestructible<T>)
+    = delete;
+
+    // Note: This overload is optional. It exists purely to match the SerenityOS and `std::optional` behaviour.
+    // The only (observable) difference between this overload and the next one is that this one calls the move assignment operator when both `this` and `other` have a value.
+    // The other overload just unconditionally calls the move constructor.
     ALWAYS_INLINE constexpr Optional& operator=(Optional&& other)
+    requires(IsMoveAssignable<T> && IsMoveConstructible<T> && (!IsTriviallyMoveAssignable<T> || !IsTriviallyMoveConstructible<T> || !IsTriviallyDestructible<T>))
+    {
+        if (this != &other) {
+            if (has_value() && other.has_value()) {
+                value() = other.release_value();
+            } else if (has_value()) {
+                value().~T();
+                m_has_value = false;
+            } else if (other.has_value()) {
+                m_has_value = true;
+                construct_at<RemoveConst<T>>(&m_storage, other.release_value());
+            }
+        }
+        return *this;
+    }
+
+    // Allow for move constructible but non-move assignable types, such as those containing const or reference fields,
+    // Note: This overload can also handle move assignable types perfectly fine, but the behaviour would be slightly different.
+    ALWAYS_INLINE constexpr Optional& operator=(Optional&& other)
+    requires(!IsMoveAssignable<T> && IsMoveConstructible<T> && (!IsTriviallyMoveConstructible<T> || !IsTriviallyDestructible<T>))
     {
         if (this != &other) {
             clear();
@@ -261,6 +288,26 @@ public:
         return *this;
     }
 
+    template<class U = T>
+    requires(
+        !OneOf<RemoveCVReference<U>, Optional, OptionalNone>
+        && !(IsSame<U, T> && IsScalar<U>))
+    // Note: We restrict this to `IsScalar<U>` to prevent undesired overload resolution for `= {}`.
+    ALWAYS_INLINE constexpr Optional<T>& operator=(U&& value)
+    requires(IsConstructible<T, U &&>)
+    {
+        if constexpr (IsAssignable<AddLvalueReference<T>, AddRvalueReference<U>>) {
+            if (m_has_value)
+                m_storage = forward<U>(value);
+            else
+                construct_at<RemoveConst<T>>(&m_storage, forward<U>(value));
+            m_has_value = true;
+        } else {
+            emplace(forward<U>(value));
+        }
+        return *this;
+    }
+
     ALWAYS_INLINE constexpr ~Optional()
     requires(!IsTriviallyDestructible<T> && IsDestructible<T>)
     {

Tests/AK/TestOptional.cpp

@@ -67,7 +67,6 @@ TEST_CASE(move_optional)
     y = move(x);
     EXPECT_EQ(y.has_value(), true);
     EXPECT_EQ(y.value(), 3);
-    EXPECT_EQ(x.has_value(), false);
 }
 
 TEST_CASE(optional_rvalue_ref_qualified_getters)
@@ -194,17 +193,44 @@ TEST_CASE(test_constexpr)
     static_assert(!(Optional<int> { 1 } = {}).has_value(), "Assigning a `{}` should clear the Optional, even for scalar types^^");
 }
 
+TEST_CASE(non_trivial_destructor_is_called_on_move_assignment)
+{
+    static int foo_destruction_count = 0;
+
+    struct Foo {
+        Foo() { }
+        Foo(Foo&&) = default;
+        ~Foo()
+        {
+            ++foo_destruction_count;
+        }
+
+        Foo& operator=(Foo&&) = default;
+    };
+    static_assert(!IsTriviallyMoveAssignable<Optional<Foo>>);
+
+    Optional<Foo> foo = Foo {}; // 1. The immediate value needs to be destroyed
+    Optional<Foo> foo2;
+    foo = AK::move(foo2); // 2. The move releases the value, which destroys the moved-from stored value
+
+    EXPECT_EQ(foo_destruction_count, 2);
+
+    // As Optional<Foo> does not trivially move, moved-from values are empty
+    // Ignoring the fact that we are touching a moved from value here
+    EXPECT_EQ(foo.has_value(), false);
+}
+
 TEST_CASE(test_copy_ctor_and_dtor_called)
 {
-#ifdef AK_HAVE_CONDITIONALLY_TRIVIAL
     static_assert(IsTriviallyDestructible<Optional<u8>>);
     static_assert(IsTriviallyCopyable<Optional<u8>>);
     static_assert(IsTriviallyCopyConstructible<Optional<u8>>);
     static_assert(IsTriviallyCopyAssignable<Optional<u8>>);
-    // These can't be trivial as we have to clear the original object.
-    static_assert(!IsTriviallyMoveConstructible<Optional<u8>>);
-    static_assert(!IsTriviallyMoveAssignable<Optional<u8>>);
-#endif
+    static_assert(IsTriviallyMoveConstructible<Optional<u8>>);
+    static_assert(IsTriviallyMoveAssignable<Optional<u8>>);
+    static_assert(IsTriviallyCopyConstructible<Optional<int&>>);
+    static_assert(IsTriviallyCopyAssignable<Optional<int&>>);
+    static_assert(IsTriviallyDestructible<Optional<int&>>);
 
     struct DestructionChecker {
         explicit DestructionChecker(bool& was_destroyed)
@@ -278,12 +304,10 @@ TEST_CASE(test_copy_ctor_and_dtor_called)
     Optional<MoveChecker> move2 = move(move1);
     EXPECT(was_moved);
 
-#ifdef AK_HAVE_CONDITIONALLY_TRIVIAL
     struct NonDestructible {
         ~NonDestructible() = delete;
     };
     static_assert(!IsDestructible<Optional<NonDestructible>>);
-#endif
 }
 
 TEST_CASE(basic_optional_reference)