AK: Don't implicitly convert Optional<T&> to Optional<T>

C++ will jovially select the implicit conversion operator, even if it's
complete bogus, such as for unknown-size types or non-destructible
types. Therefore, all such conversions (which incur a copy) must
(unfortunately) be explicit so that non-copyable types continue to work.

NOTE: We make an exception for trivially copyable types, since they
are, well, trivially copyable.

Co-authored-by: kleines Filmröllchen <filmroellchen@serenityos.org>

AK/Optional.h

@@ -210,7 +210,7 @@ public:
     }
 
     template<typename U>
-    requires(IsConstructible<T, U const&> && !IsSpecializationOf<T, Optional> && !IsSpecializationOf<U, Optional>) ALWAYS_INLINE explicit Optional(Optional<U> const& other)
+    requires(IsConstructible<T, U const&> && !IsSpecializationOf<T, Optional> && !IsSpecializationOf<U, Optional> && (!IsLvalueReference<U> || IsTriviallyCopyConstructible<U>)) ALWAYS_INLINE explicit Optional(Optional<U> const& other)
         : m_has_value(other.has_value())
     {
         if (other.has_value())
@@ -218,7 +218,7 @@ public:
     }
 
     template<typename U>
-    requires(IsConstructible<T, U &&> && !IsSpecializationOf<T, Optional> && !IsSpecializationOf<U, Optional>) ALWAYS_INLINE explicit Optional(Optional<U>&& other)
+    requires(IsConstructible<T, U &&> && !IsSpecializationOf<T, Optional> && !IsSpecializationOf<U, Optional> && (!IsLvalueReference<U> || IsTriviallyMoveConstructible<U>)) ALWAYS_INLINE explicit Optional(Optional<U>&& other)
         : m_has_value(other.has_value())
     {
         if (other.has_value())
@@ -503,14 +503,29 @@ public:
     ALWAYS_INLINE RawPtr<AddConst<RemoveReference<T>>> operator->() const { return &value(); }
     ALWAYS_INLINE RawPtr<RemoveReference<T>> operator->() { return &value(); }
 
-    // Conversion operators from Optional<T&> -> Optional<T>
+    // Conversion operators from Optional<T&> -> Optional<T>, implicit when T is trivially copyable.
     ALWAYS_INLINE operator Optional<RemoveCVReference<T>>() const
+    requires(IsTriviallyCopyable<RemoveCVReference<T>>)
     {
         if (has_value())
             return Optional<RemoveCVReference<T>>(value());
         return {};
     }
 
+    // Conversion operators from Optional<T&> -> Optional<T>, explicit when T is not trivially copyable, since this is usually a mistake.
+    ALWAYS_INLINE explicit operator Optional<RemoveCVReference<T>>() const
+    requires(!IsTriviallyCopyable<RemoveCVReference<T>>)
+    {
+        if (has_value())
+            return Optional<RemoveCVReference<T>>(value());
+        return {};
+    }
+
+    ALWAYS_INLINE constexpr Optional<RemoveCVReference<T>> copy() const
+    {
+        return static_cast<Optional<RemoveCVReference<T>>>(*this);
+    }
+
     template<typename Callback>
     [[nodiscard]] ALWAYS_INLINE T value_or_lazy_evaluated(Callback callback) const
     {

Libraries/LibCrypto/Certificate/Certificate.h

@@ -50,17 +50,17 @@ public:
         return m_members.try_set(move(key), move(value));
     }
 
-    Optional<String> get(StringView key) const
+    Optional<String const&> get(StringView key) const
     {
         return m_members.get(key);
     }
 
-    Optional<String> get(ASN1::AttributeType key) const
+    Optional<String const&> get(ASN1::AttributeType key) const
     {
         return m_members.get(enum_value(key));
     }
 
-    Optional<String> get(ASN1::ObjectClass key) const
+    Optional<String const&> get(ASN1::ObjectClass key) const
     {
         return m_members.get(enum_value(key));
     }

Libraries/LibHTTP/HeaderMap.h

@@ -39,7 +39,7 @@ public:
         return m_map.contains(name);
     }
 
-    [[nodiscard]] Optional<ByteString> get(ByteString const& name) const
+    [[nodiscard]] Optional<ByteString const&> get(ByteString const& name) const
     {
         return m_map.get(name);
     }

Libraries/LibJS/Runtime/IndexedProperties.cpp

@@ -101,7 +101,7 @@ Optional<ValueAndAttributes> GenericIndexedPropertyStorage::get(u32 index) const
 {
     if (index >= m_array_size)
         return {};
-    return m_sparse_elements.get(index);
+    return m_sparse_elements.get(index).copy();
 }
 
 void GenericIndexedPropertyStorage::put(u32 index, Value value, PropertyAttributes attributes)

Libraries/LibURL/Parser.cpp

@@ -699,7 +699,7 @@ String Parser::percent_encode_after_encoding(TextCodec::Encoder& encoder, String
 }
 
 // https://url.spec.whatwg.org/#concept-basic-url-parser
-URL Parser::basic_parse(StringView raw_input, Optional<URL> const& base_url, URL* url, Optional<State> state_override, Optional<StringView> encoding)
+URL Parser::basic_parse(StringView raw_input, Optional<URL const&> base_url, URL* url, Optional<State> state_override, Optional<StringView> encoding)
 {
     dbgln_if(URL_PARSER_DEBUG, "URL::Parser::basic_parse: Parsing '{}'", raw_input);
 

Libraries/LibURL/Parser.h

@@ -58,7 +58,7 @@ public:
     }
 
     // https://url.spec.whatwg.org/#concept-basic-url-parser
-    static URL basic_parse(StringView input, Optional<URL> const& base_url = {}, URL* url = nullptr, Optional<State> state_override = {}, Optional<StringView> encoding = {});
+    static URL basic_parse(StringView input, Optional<URL const&> base_url = {}, URL* url = nullptr, Optional<State> state_override = {}, Optional<StringView> encoding = {});
 
     // https://url.spec.whatwg.org/#string-percent-encode-after-encoding
     static String percent_encode_after_encoding(TextCodec::Encoder&, StringView input, PercentEncodeSet percent_encode_set, bool space_as_plus = false);

Libraries/LibUnicode/CurrencyCode.cpp

@@ -197,7 +197,7 @@ static auto const& ensure_currency_codes()
     return currency_codes;
 }
 
-Optional<CurrencyCode> get_currency_code(StringView currency)
+Optional<CurrencyCode const&> get_currency_code(StringView currency)
 {
     static auto const& currency_codes = ensure_currency_codes();
     return currency_codes.get(currency);

Libraries/LibUnicode/CurrencyCode.h

@@ -17,6 +17,6 @@ struct CurrencyCode {
     Optional<int> minor_unit {};
 };
 
-Optional<CurrencyCode> get_currency_code(StringView currency);
+Optional<CurrencyCode const&> get_currency_code(StringView currency);
 
 }

Libraries/LibWeb/CSS/CSSStyleDeclaration.h

@@ -29,7 +29,7 @@ public:
     virtual String item(size_t index) const = 0;
 
     virtual Optional<StyleProperty> property(PropertyID) const = 0;
-    virtual Optional<StyleProperty> custom_property(FlyString const& custom_property_name) const = 0;
+    virtual Optional<StyleProperty const&> custom_property(FlyString const& custom_property_name) const = 0;
 
     virtual WebIDL::ExceptionOr<void> set_property(PropertyID, StringView css_text, StringView priority = ""sv);
     virtual WebIDL::ExceptionOr<String> remove_property(PropertyID);
@@ -76,7 +76,7 @@ public:
     virtual String item(size_t index) const override;
 
     virtual Optional<StyleProperty> property(PropertyID) const override;
-    virtual Optional<StyleProperty> custom_property(FlyString const& custom_property_name) const override { return m_custom_properties.get(custom_property_name); }
+    virtual Optional<StyleProperty const&> custom_property(FlyString const& custom_property_name) const override { return m_custom_properties.get(custom_property_name); }
 
     virtual WebIDL::ExceptionOr<void> set_property(StringView property_name, StringView css_text, StringView priority) override;
     virtual WebIDL::ExceptionOr<String> remove_property(StringView property_name) override;

Libraries/LibWeb/CSS/ResolvedCSSStyleDeclaration.cpp

@@ -601,7 +601,7 @@ Optional<StyleProperty> ResolvedCSSStyleDeclaration::property(PropertyID propert
     };
 }
 
-Optional<StyleProperty> ResolvedCSSStyleDeclaration::custom_property(FlyString const& name) const
+Optional<StyleProperty const&> ResolvedCSSStyleDeclaration::custom_property(FlyString const& name) const
 {
     const_cast<DOM::Document&>(m_element->document()).update_style();
 

Libraries/LibWeb/CSS/ResolvedCSSStyleDeclaration.h

@@ -24,7 +24,7 @@ public:
     virtual String item(size_t index) const override;
 
     virtual Optional<StyleProperty> property(PropertyID) const override;
-    virtual Optional<StyleProperty> custom_property(FlyString const& custom_property_name) const override;
+    virtual Optional<StyleProperty const&> custom_property(FlyString const& custom_property_name) const override;
     virtual WebIDL::ExceptionOr<void> set_property(PropertyID, StringView css_text, StringView priority) override;
     virtual WebIDL::ExceptionOr<void> set_property(StringView property_name, StringView css_text, StringView priority) override;
     virtual WebIDL::ExceptionOr<String> remove_property(PropertyID) override;

Libraries/LibWeb/DOMURL/DOMURL.cpp

@@ -506,7 +506,7 @@ void strip_trailing_spaces_from_an_opaque_path(DOMURL& url)
 }
 
 // https://url.spec.whatwg.org/#concept-url-parser
-URL::URL parse(StringView input, Optional<URL::URL> const& base_url, Optional<StringView> encoding)
+URL::URL parse(StringView input, Optional<URL::URL const&> base_url, Optional<StringView> encoding)
 {
     // FIXME: We should probably have an extended version of URL::URL for LibWeb instead of standalone functions like this.
 

Libraries/LibWeb/DOMURL/DOMURL.h

@@ -98,6 +98,6 @@ bool host_is_domain(URL::Host const&);
 void strip_trailing_spaces_from_an_opaque_path(DOMURL& url);
 
 // https://url.spec.whatwg.org/#concept-url-parser
-URL::URL parse(StringView input, Optional<URL::URL> const& base_url = {}, Optional<StringView> encoding = {});
+URL::URL parse(StringView input, Optional<URL::URL const&> base_url = {}, Optional<StringView> encoding = {});
 
 }

Libraries/LibWeb/Fetch/Request.cpp

@@ -467,7 +467,7 @@ WebIDL::ExceptionOr<GC::Ref<Request>> Request::construct_impl(JS::Realm& realm,
     }
 
     // 38. Let inputOrInitBody be initBody if it is non-null; otherwise inputBody.
-    Optional<Infrastructure::Request::BodyType> input_or_init_body = init_body
+    Optional<Infrastructure::Request::BodyType const&> input_or_init_body = init_body
         ? Infrastructure::Request::BodyType { *init_body }
         : input_body;
 

Libraries/LibWeb/FileAPI/BlobURLStore.cpp

@@ -108,7 +108,7 @@ void run_unloading_cleanup_steps(GC::Ref<DOM::Document> document)
 }
 
 // https://w3c.github.io/FileAPI/#blob-url-resolve
-Optional<BlobURLEntry> resolve_a_blob_url(URL::URL const& url)
+Optional<BlobURLEntry const&> resolve_a_blob_url(URL::URL const& url)
 {
     // 1. Assert: url’s scheme is "blob".
     VERIFY(url.scheme() == "blob"sv);

Libraries/LibWeb/FileAPI/BlobURLStore.h

@@ -28,7 +28,7 @@ BlobURLStore& blob_url_store();
 ErrorOr<String> generate_new_blob_url();
 ErrorOr<String> add_entry_to_blob_url_store(GC::Ref<Blob> object);
 ErrorOr<void> remove_entry_from_blob_url_store(StringView url);
-Optional<BlobURLEntry> resolve_a_blob_url(URL::URL const&);
+Optional<BlobURLEntry const&> resolve_a_blob_url(URL::URL const&);
 
 void run_unloading_cleanup_steps(GC::Ref<DOM::Document>);
 

Libraries/LibWeb/IndexedDB/Internal/Database.cpp

@@ -38,7 +38,7 @@ ConnectionQueue& ConnectionQueueHandler::for_key_and_name(StorageAPI::StorageKey
         });
 }
 
-Optional<GC::Root<Database>> Database::for_key_and_name(StorageAPI::StorageKey& key, String& name)
+Optional<GC::Root<Database> const&> Database::for_key_and_name(StorageAPI::StorageKey& key, String& name)
 {
     return m_databases.ensure(key, [] {
                           return HashMap<String, GC::Root<Database>>();

Libraries/LibWeb/IndexedDB/Internal/Database.h

@@ -39,7 +39,7 @@ public:
         return connections;
     }
 
-    [[nodiscard]] static Optional<GC::Root<Database>> for_key_and_name(StorageAPI::StorageKey&, String&);
+    [[nodiscard]] static Optional<GC::Root<Database> const&> for_key_and_name(StorageAPI::StorageKey&, String&);
     [[nodiscard]] static ErrorOr<GC::Root<Database>> create_for_key_and_name(JS::Realm&, StorageAPI::StorageKey&, String&);
     [[nodiscard]] static ErrorOr<void> delete_for_key_and_name(StorageAPI::StorageKey&, String&);
 

Libraries/LibWebView/CookieJar.cpp

@@ -618,7 +618,7 @@ void CookieJar::TransientStorage::set_cookie(CookieStorageKey key, Web::Cookie::
     m_dirty_cookies.set(move(key), move(cookie));
 }
 
-Optional<Web::Cookie::Cookie> CookieJar::TransientStorage::get_cookie(CookieStorageKey const& key)
+Optional<Web::Cookie::Cookie const&> CookieJar::TransientStorage::get_cookie(CookieStorageKey const& key)
 {
     return m_cookies.get(key);
 }

Libraries/LibWebView/CookieJar.h

@@ -43,7 +43,7 @@ class CookieJar {
 
         void set_cookies(Cookies);
         void set_cookie(CookieStorageKey, Web::Cookie::Cookie);
-        Optional<Web::Cookie::Cookie> get_cookie(CookieStorageKey const&);
+        Optional<Web::Cookie::Cookie const&> get_cookie(CookieStorageKey const&);
 
         size_t size() const { return m_cookies.size(); }
 

Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateWindowOrWorkerInterfaces.cpp

@@ -265,7 +265,7 @@ void add_@global_object_snake_name@_exposed_interfaces(JS::Object& global)
     static constexpr u8 attr = JS::Attribute::Writable | JS::Attribute::Configurable;
 )~~~");
 
-    auto add_interface = [](SourceGenerator& gen, StringView name, StringView prototype_class, Optional<LegacyConstructor> const& legacy_constructor, Optional<ByteString> const& legacy_alias_name) {
+    auto add_interface = [](SourceGenerator& gen, StringView name, StringView prototype_class, Optional<LegacyConstructor> const& legacy_constructor, Optional<ByteString const&> legacy_alias_name) {
         gen.set("interface_name", name);
         gen.set("prototype_class", prototype_class);
 

Tests/AK/TestOptional.cpp

@@ -13,6 +13,39 @@
 #include <AK/String.h>
 #include <AK/Vector.h>
 
+class NonCopyable {
+    AK_MAKE_NONCOPYABLE(NonCopyable);
+    AK_MAKE_DEFAULT_MOVABLE(NonCopyable);
+
+public:
+    NonCopyable() { }
+    ~NonCopyable() = default;
+
+    int x { 13 };
+};
+
+class NonTriviallyCopyable {
+    AK_MAKE_DEFAULT_COPYABLE(NonTriviallyCopyable);
+    AK_MAKE_DEFAULT_MOVABLE(NonTriviallyCopyable);
+
+public:
+    NonTriviallyCopyable() = default;
+    ~NonTriviallyCopyable() = default;
+
+    ByteString x { "13" };
+};
+
+class TriviallyCopyable {
+    AK_MAKE_DEFAULT_COPYABLE(TriviallyCopyable);
+    AK_MAKE_DEFAULT_MOVABLE(TriviallyCopyable);
+
+public:
+    TriviallyCopyable() = default;
+    ~TriviallyCopyable() = default;
+
+    int x { 13 };
+};
+
 TEST_CASE(basic_optional)
 {
     Optional<int> x;
@@ -39,23 +72,12 @@ TEST_CASE(move_optional)
 
 TEST_CASE(optional_rvalue_ref_qualified_getters)
 {
-    struct DontCopyMe {
-        DontCopyMe() { }
-        ~DontCopyMe() = default;
-        DontCopyMe(DontCopyMe&&) = default;
-        DontCopyMe& operator=(DontCopyMe&&) = default;
-        DontCopyMe(DontCopyMe const&) = delete;
-        DontCopyMe& operator=(DontCopyMe const&) = delete;
-
-        int x { 13 };
-    };
-
-    auto make_an_optional = []() -> Optional<DontCopyMe> {
-        return DontCopyMe {};
+    auto make_an_optional = []() -> Optional<NonCopyable> {
+        return NonCopyable {};
     };
 
     EXPECT_EQ(make_an_optional().value().x, 13);
-    EXPECT_EQ(make_an_optional().value_or(DontCopyMe {}).x, 13);
+    EXPECT_EQ(make_an_optional().value_or(NonCopyable {}).x, 13);
 }
 
 TEST_CASE(optional_leak_1)
@@ -299,6 +321,47 @@ 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
 
+static_assert(CheckAssignments<Optional<NonTriviallyCopyable const&>, NonTriviallyCopyable&>::allowed);
+static_assert(CheckAssignments<Optional<NonTriviallyCopyable const&>, NonTriviallyCopyable const&>::allowed);
+static_assert(CheckAssignments<Optional<NonTriviallyCopyable const&>, NonTriviallyCopyable&&>::allowed);       // Lifetime extension
+static_assert(CheckAssignments<Optional<NonTriviallyCopyable const&>, NonTriviallyCopyable const&&>::allowed); // Lifetime extension
+
+static_assert(CheckAssignments<Optional<TriviallyCopyable const&>, TriviallyCopyable>::allowed);
+static_assert(CheckAssignments<Optional<TriviallyCopyable const&>, TriviallyCopyable const&>::allowed);
+static_assert(CheckAssignments<Optional<TriviallyCopyable const&>, Optional<TriviallyCopyable>>::allowed);
+static_assert(CheckAssignments<Optional<TriviallyCopyable const&>, Optional<TriviallyCopyable const&>>::allowed);
+static_assert(CheckAssignments<Optional<TriviallyCopyable>, Optional<TriviallyCopyable const&>>::allowed);
+
+static_assert(CheckAssignments<Optional<NonTriviallyCopyable const&>, NonTriviallyCopyable>::allowed);
+static_assert(CheckAssignments<Optional<NonTriviallyCopyable const&>, NonTriviallyCopyable const&>::allowed);
+static_assert(CheckAssignments<Optional<NonTriviallyCopyable const&>, Optional<NonTriviallyCopyable>>::allowed);
+static_assert(CheckAssignments<Optional<NonTriviallyCopyable const&>, Optional<NonTriviallyCopyable const&>>::allowed);
+static_assert(!CheckAssignments<Optional<NonTriviallyCopyable>, Optional<NonTriviallyCopyable const&>>::allowed);
+
+TEST_CASE(nontrivially_copyable_assignment)
+{
+    {
+        TriviallyCopyable x {};
+        Optional<TriviallyCopyable const&> y = x;
+        Optional<TriviallyCopyable> z = y; // Can copy implicitly
+        EXPECT_EQ(z->x, 13);
+    }
+
+    {
+        NonTriviallyCopyable x {};
+        Optional<NonTriviallyCopyable const&> y = x;
+        Optional<NonTriviallyCopyable> z = y.copy(); // Have to copy explicitly
+        EXPECT_EQ(z->x, "13");
+    }
+
+    {
+        NonTriviallyCopyable x {};
+        Optional<NonTriviallyCopyable const&> y = x;
+        Optional<NonTriviallyCopyable> z = Optional<NonTriviallyCopyable>(y); // Explicit copy constructor is still defined
+        EXPECT_EQ(z->x, "13");
+    }
+}
+
 TEST_CASE(string_specialization)
 {
     EXPECT_EQ(sizeof(Optional<String>), sizeof(String));