From 75b482bbb91ed07b2d4680c86885913f178df6ae Mon Sep 17 00:00:00 2001
From: Jonne Ransijn <jonne@yyny.dev>
Date: Mon, 25 Nov 2024 00:25:57 +0100
Subject: [PATCH] AK: Fix "assignment from temporary" check of
 `Optional::operator=`

There was an existing check to ensure that `U` was an lvalue reference,
but when this check fails, overload resolution will just move right on
to the copy asignment operator, which will cause the temporary to be
assigned anyway.

Disallowing `Optional<T&>`s to be created from temporaries entirely
would be undesired, since existing code has valid reasons for creating
`Optional<T&>`s from temporaries, such as for function call arguments.

This fix explicitly deletes the `Optional::operator=(U&&)` operator,
so overload resolution stops.
---
 AK/Optional.h             | 12 +++++++++---
 Tests/AK/TestOptional.cpp | 28 ++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/AK/Optional.h b/AK/Optional.h
index 1bb6441bcb..7519072a8a 100644
--- a/AK/Optional.h
+++ b/AK/Optional.h
@@ -446,16 +446,22 @@ public:
         return *this;
     }
 
-    // Note: Disallows assignment from a temporary as this does not do any lifetime extension.
     template<typename U>
     requires(!IsSame<OptionalNone, RemoveCVReference<U>>)
-    ALWAYS_INLINE Optional& operator=(U&& value)
-    requires(CanBePlacedInOptional<U> && IsLvalueReference<U>)
+    ALWAYS_INLINE Optional& operator=(U& value)
+    requires(CanBePlacedInOptional<U>)
     {
         m_pointer = &value;
         return *this;
     }
 
+    // Note: Disallows assignment from a temporary as this does not do any lifetime extension.
+    template<typename U>
+    requires(!IsSame<OptionalNone, RemoveCVReference<U>>)
+    ALWAYS_INLINE consteval Optional& operator=(RemoveReference<U> const&& value)
+    requires(CanBePlacedInOptional<U>)
+    = delete;
+
     ALWAYS_INLINE void clear()
     {
         m_pointer = nullptr;
diff --git a/Tests/AK/TestOptional.cpp b/Tests/AK/TestOptional.cpp
index cb17d62deb..2c0f3f49a5 100644
--- a/Tests/AK/TestOptional.cpp
+++ b/Tests/AK/TestOptional.cpp
@@ -272,6 +272,34 @@ TEST_CASE(comparison_reference)
     EXPECT_NE(opt1, opt3);
 }
 
+template<typename To, typename From>
+struct CheckAssignments;
+
+template<typename To, typename From>
+requires(requires { declval<To>() = declval<From>(); })
+struct CheckAssignments<To, From> {
+    static constexpr bool allowed = true;
+};
+
+template<typename To, typename From>
+requires(!requires { declval<To>() = declval<From>(); })
+struct CheckAssignments<To, From> {
+    static constexpr bool allowed = false;
+};
+
+static_assert(CheckAssignments<Optional<int>, int>::allowed);
+static_assert(!CheckAssignments<Optional<int*>, double*>::allowed);
+
+static_assert(CheckAssignments<Optional<int&>, int&>::allowed);
+static_assert(!CheckAssignments<Optional<int&>, int const&>::allowed);
+static_assert(!CheckAssignments<Optional<int&>, int&&>::allowed);
+static_assert(!CheckAssignments<Optional<int&>, int const&&>::allowed);
+
+static_assert(CheckAssignments<Optional<int const&>, int&>::allowed);
+static_assert(CheckAssignments<Optional<int const&>, int const&>::allowed);
+static_assert(CheckAssignments<Optional<int const&>, int&&>::allowed);       // Lifetime extension
+static_assert(CheckAssignments<Optional<int const&>, int const&&>::allowed); // Lifetime extension
+
 TEST_CASE(string_specialization)
 {
     EXPECT_EQ(sizeof(Optional<String>), sizeof(String));