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.

AK/Optional.h

@@ -109,7 +109,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>) ALWAYS_INLINE explicit Optional(Optional<U> const& other)
         : m_has_value(other.m_has_value)
     {
         if (other.has_value())
@@ -117,7 +117,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>) ALWAYS_INLINE explicit Optional(Optional<U>&& other)
         : m_has_value(other.m_has_value)
     {
         if (other.has_value())
@@ -475,12 +475,16 @@ public:
     ALWAYS_INLINE RawPtr<RemoveReference<T>> operator->() { return &value(); }
 
     // Conversion operators from Optional<T&> -> Optional<T>
-    ALWAYS_INLINE operator Optional<RemoveCVReference<T>>() const
+    ALWAYS_INLINE explicit operator Optional<RemoveCVReference<T>>() const
     {
         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

Kernel/Net/Routing.cpp

@@ -102,7 +102,7 @@ void ARPTableBlocker::will_unblock_immediately_without_blocking(UnblockImmediate
     SpinlockLocker lock(m_lock);
     if (!m_did_unblock) {
         m_did_unblock = true;
-        m_mac_address = move(addr);
+        m_mac_address = addr.copy();
     }
 }
 

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

@@ -117,7 +117,7 @@ HashMap<StringView, ValueID, AK::CaseInsensitiveASCIIStringViewTraits> g_stringv
 
 Optional<ValueID> value_id_from_string(StringView string)
 {
-    return g_stringview_to_value_id_map.get(string);
+    return g_stringview_to_value_id_map.get(string).copy();
 }
 
 StringView string_from_value_id(ValueID value_id) {

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

@@ -310,7 +310,7 @@ void add_@global_object_snake_name@_exposed_interfaces(JS::Object& global)
             add_namespace(gen, interface.name, interface.namespace_class);
         } else if (!interface.extended_attributes.contains("LegacyNamespace"sv)) {
             if (class_name == "Window") {
-                add_interface(gen, interface.namespaced_name, interface.prototype_class, lookup_legacy_constructor(interface), interface.extended_attributes.get("LegacyWindowAlias"sv));
+                add_interface(gen, interface.namespaced_name, interface.prototype_class, lookup_legacy_constructor(interface), interface.extended_attributes.get("LegacyWindowAlias"sv).copy());
             } else {
                 add_interface(gen, interface.namespaced_name, interface.prototype_class, lookup_legacy_constructor(interface), {});
             }

Tests/AK/TestOptional.cpp

@@ -248,6 +248,37 @@ TEST_CASE(move_optional_reference)
     EXPECT_EQ(x.has_value(), false);
 }
 
+TEST_CASE(optional_reference_to_optional)
+{
+    Optional<int&> x;
+    EXPECT_EQ(x.has_value(), false);
+    int c = 3;
+    x = c;
+    EXPECT_EQ(x.has_value(), true);
+    EXPECT_EQ(x.value(), 3);
+
+    auto y = x.copy();
+    EXPECT_EQ(y.has_value(), true);
+    EXPECT_EQ(y.value(), 3);
+
+    y = 4;
+    EXPECT_EQ(x.value(), 3);
+    EXPECT_EQ(y.value(), 4);
+    c = 5;
+    EXPECT_EQ(x.value(), 5);
+    EXPECT_EQ(y.value(), 4);
+
+    Optional<int> z = *x;
+    EXPECT_EQ(z.has_value(), true);
+    EXPECT_EQ(z.value(), 5);
+    z = 6;
+    EXPECT_EQ(x.value(), 5);
+    EXPECT_EQ(z.value(), 6);
+    c = 7;
+    EXPECT_EQ(x.value(), 7);
+    EXPECT_EQ(z.value(), 6);
+}
+
 TEST_CASE(short_notation_reference)
 {
     StringView test = "foo"sv;

Userland/Applications/SystemMonitor/ProcessModel.h

@@ -244,7 +244,7 @@ private:
 
         Optional<NonnullRefPtr<Thread>> main_thread() const
         {
-            return threads.first_matching([this](auto const thread) { return thread->current_state.tid == pid; });
+            return threads.first_matching([this](auto const thread) { return thread->current_state.tid == pid; }).copy();
         }
 
         // Return anything but the main thread; therefore, valid indices are anything up to threads.size()-1 exclusive.

Userland/Libraries/LibAccelGfx/GlyphAtlas.cpp

@@ -80,7 +80,7 @@ void GlyphAtlas::update(HashMap<Gfx::Font const*, HashTable<u32>> const& unique_
 Optional<Gfx::IntRect> GlyphAtlas::get_glyph_rect(Gfx::Font const* font, u32 code_point) const
 {
     auto atlas_key = GlyphsTextureKey { font, code_point };
-    return m_glyphs_texture_map.get(atlas_key);
+    return m_glyphs_texture_map.get(atlas_key).copy();
 }
 
 }

Userland/Libraries/LibCodeComprehension/Cpp/Tests.cpp

@@ -55,7 +55,7 @@ public:
         if (project_root().has_value() && filename.starts_with(*project_root())) {
             target_filename = LexicalPath::relative_path(filename, *project_root());
         }
-        return m_map.get(target_filename);
+        return m_map.get(target_filename).copy();
     }
 
 private:

Userland/Libraries/LibDebug/DebugSession.h

@@ -252,11 +252,11 @@ void DebugSession::run(DesiredInitialDebugeeState initial_debugee_state, Callbac
         Optional<BreakPoint> current_breakpoint;
 
         if (state == State::FreeRun || state == State::Syscall) {
-            current_breakpoint = m_breakpoints.get(current_instruction - 1);
+            current_breakpoint = m_breakpoints.get(current_instruction - 1).copy();
             if (current_breakpoint.has_value())
                 state = State::FreeRun;
         } else {
-            current_breakpoint = m_breakpoints.get(current_instruction);
+            current_breakpoint = m_breakpoints.get(current_instruction).copy();
         }
 
         if (current_breakpoint.has_value()) {

Userland/Libraries/LibDeviceTree/DeviceTree.h

@@ -104,7 +104,7 @@ public:
     bool has_child(StringView child) const { return m_children.contains(child); }
     bool child(StringView name) const { return has_property(name) || has_child(name); }
 
-    Optional<DeviceTreeProperty> get_property(StringView prop) const { return m_properties.get(prop); }
+    Optional<DeviceTreeProperty> get_property(StringView prop) const { return m_properties.get(prop).copy(); }
 
     // FIXME: The spec says that @address parts of the name should be ignored when looking up nodes
     //        when they do not appear in the queried name, and all nodes with the same name should be returned

Userland/Libraries/LibHTTP/HeaderMap.h

@@ -28,7 +28,7 @@ public:
 
     [[nodiscard]] Optional<ByteString> get(ByteString const& name) const
     {
-        return m_map.get(name);
+        return m_map.get(name).copy();
     }
 
     [[nodiscard]] Vector<Header> const& headers() const

Userland/Libraries/LibJS/Bytecode/Interpreter.h

@@ -76,7 +76,7 @@ public:
 
     Executable& current_executable() { return *m_current_executable; }
     Executable const& current_executable() const { return *m_current_executable; }
-    Optional<size_t> program_counter() const { return m_program_counter; }
+    Optional<size_t> program_counter() const { return m_program_counter.copy(); }
 
     ExecutionContext& running_execution_context() { return *m_running_execution_context; }
 

Userland/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)

Userland/Libraries/LibJS/Runtime/Shape.cpp

@@ -217,7 +217,7 @@ Optional<PropertyMetadata> Shape::lookup(StringOrSymbol const& property_key) con
     auto property = property_table().get(property_key);
     if (!property.has_value())
         return {};
-    return property;
+    return property.copy();
 }
 
 FLATTEN OrderedHashMap<StringOrSymbol, PropertyMetadata> const& Shape::property_table() const

Userland/Libraries/LibPDF/Document.cpp

@@ -492,7 +492,7 @@ PDFErrorOr<Destination> Document::create_destination_from_parameters(NonnullRefP
     if (page_ref.has<int>())
         page_number = page_ref.get<int>();
     else
-        page_number = page_number_by_index_ref.get(page_ref.as_ref_index());
+        page_number = page_number_by_index_ref.get(page_ref.as_ref_index()).copy();
 
     return Destination { type, page_number, parameters };
 }

Userland/Libraries/LibPDF/ObjectDerivatives.h

@@ -121,7 +121,7 @@ public:
     template<typename... Args>
     bool contains_any_of(Args&&... keys) const { return (m_map.contains(keys) || ...); }
 
-    ALWAYS_INLINE Optional<Value> get(DeprecatedFlyString const& key) const { return m_map.get(key); }
+    ALWAYS_INLINE Optional<Value> get(DeprecatedFlyString const& key) const { return m_map.get(key).copy(); }
 
     Value get_value(DeprecatedFlyString const& key) const
     {

Userland/Libraries/LibTLS/Certificate.h

@@ -201,17 +201,17 @@ public:
 
     Optional<String> get(StringView key) const
     {
-        return m_members.get(key);
+        return m_members.get(key).copy();
     }
 
     Optional<String> get(AttributeType key) const
     {
-        return m_members.get(enum_value(key));
+        return m_members.get(enum_value(key)).copy();
     }
 
     Optional<String> get(ObjectClass key) const
     {
-        return m_members.get(enum_value(key));
+        return m_members.get(enum_value(key)).copy();
     }
 
     String common_name() const

Userland/Libraries/LibUnicode/CurrencyCode.cpp

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

Userland/Libraries/LibWasm/Printer/Printer.cpp

@@ -28,7 +28,7 @@ Optional<OpCode> instruction_from_name(StringView name)
             Names::instructions_by_name.set(entry.value, entry.key);
     }
 
-    return Names::instructions_by_name.get(name);
+    return Names::instructions_by_name.get(name).copy();
 }
 
 void Printer::print_indent()

Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h

@@ -73,7 +73,7 @@ public:
 
     Vector<StyleProperty> const& properties() const { return m_properties; }
     HashMap<FlyString, StyleProperty> const& custom_properties() const { return m_custom_properties; }
-    Optional<StyleProperty> custom_property(FlyString const& custom_property_name) const { return m_custom_properties.get(custom_property_name); }
+    Optional<StyleProperty> custom_property(FlyString const& custom_property_name) const { return m_custom_properties.get(custom_property_name).copy(); }
     size_t custom_property_count() const { return m_custom_properties.size(); }
 
     virtual String serialized() const final override;

Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.cpp

@@ -134,7 +134,7 @@ ErrorOr<Optional<URL::URL>> Response::location_url(Optional<String> const& reque
         return Optional<URL::URL> {};
 
     // 3. If location is a header value, then set location to the result of parsing location with response’s URL.
-    auto location = DOMURL::parse(location_values.first(), url());
+    auto location = DOMURL::parse(location_values.first(), url().copy());
     if (!location.is_valid())
         return Error::from_string_view("Invalid 'Location' header URL"sv);
 

Userland/Libraries/LibWeb/Fetch/Request.cpp

@@ -468,7 +468,7 @@ WebIDL::ExceptionOr<JS::NonnullGCPtr<Request>> Request::construct_impl(JS::Realm
     // 38. Let inputOrInitBody be initBody if it is non-null; otherwise inputBody.
     Optional<Infrastructure::Request::BodyType> input_or_init_body = init_body
         ? Infrastructure::Request::BodyType { *init_body }
-        : input_body;
+        : input_body.copy();
 
     // 39. If inputOrInitBody is non-null and inputOrInitBody’s source is null, then:
     // FIXME: The spec doesn't check if inputOrInitBody is a body before accessing source.

Userland/Libraries/LibWeb/FileAPI/BlobURLStore.cpp

@@ -120,7 +120,7 @@ Optional<BlobURLEntry> resolve_a_blob_url(URL::URL const& url)
     auto url_string = MUST(String::from_byte_string(url.serialize(URL::ExcludeFragment::Yes)));
 
     // 4. If store[url string] exists, return store[url string]; otherwise return failure.
-    return store.get(url_string);
+    return store.get(url_string).copy();
 }
 
 }

Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.cpp

@@ -41,7 +41,7 @@ bool ModuleMap::is(URL::URL const& url, ByteString const& type, EntryType entry_
 
 Optional<ModuleMap::Entry> ModuleMap::get(URL::URL const& url, ByteString const& type) const
 {
-    return m_values.get({ url, type });
+    return m_values.get({ url, type }).copy();
 }
 
 AK::HashSetResult ModuleMap::set(URL::URL const& url, ByteString const& type, Entry entry)

Userland/Libraries/LibWeb/WebAssembly/Instance.cpp

@@ -49,7 +49,7 @@ void Instance::initialize(JS::Realm& realm)
     for (auto& export_ : m_module_instance->exports()) {
         export_.value().visit(
             [&](Wasm::FunctionAddress const& address) {
-                Optional<JS::GCPtr<JS::FunctionObject>> object = m_function_instances.get(address);
+                Optional<JS::GCPtr<JS::FunctionObject>> object = m_function_instances.get(address).copy();
                 if (!object.has_value()) {
                     object = Detail::create_native_function(vm, address, export_.name(), this);
                     m_function_instances.set(address, *object);
@@ -58,7 +58,7 @@ void Instance::initialize(JS::Realm& realm)
                 m_exports->define_direct_property(export_.name(), *object, JS::default_attributes);
             },
             [&](Wasm::MemoryAddress const& address) {
-                Optional<JS::GCPtr<Memory>> object = m_memory_instances.get(address);
+                Optional<JS::GCPtr<Memory>> object = m_memory_instances.get(address).copy();
                 if (!object.has_value()) {
                     object = heap().allocate<Memory>(realm, realm, address);
                     m_memory_instances.set(address, *object);
@@ -67,7 +67,7 @@ void Instance::initialize(JS::Realm& realm)
                 m_exports->define_direct_property(export_.name(), *object, JS::default_attributes);
             },
             [&](Wasm::TableAddress const& address) {
-                Optional<JS::GCPtr<Table>> object = m_table_instances.get(address);
+                Optional<JS::GCPtr<Table>> object = m_table_instances.get(address).copy();
                 if (!object.has_value()) {
                     object = heap().allocate<Table>(realm, realm, address);
                     m_table_instances.set(address, *object);

Userland/Libraries/LibWeb/WebAssembly/WebAssembly.h

@@ -43,7 +43,7 @@ public:
     void add_function_instance(Wasm::FunctionAddress address, JS::GCPtr<JS::NativeFunction> function) { m_function_instances.set(address, function); }
     void add_imported_object(JS::GCPtr<JS::Object> object) { m_imported_objects.set(object); }
 
-    Optional<JS::GCPtr<JS::NativeFunction>> get_function_instance(Wasm::FunctionAddress address) { return m_function_instances.get(address); }
+    Optional<JS::GCPtr<JS::NativeFunction>> get_function_instance(Wasm::FunctionAddress address) { return m_function_instances.get(address).copy(); }
 
     HashMap<Wasm::FunctionAddress, JS::GCPtr<JS::NativeFunction>> function_instances() const { return m_function_instances; }
     HashTable<JS::GCPtr<JS::Object>> imported_objects() const { return m_imported_objects; }

Userland/Libraries/LibWebView/CookieJar.cpp

@@ -559,7 +559,7 @@ void CookieJar::TransientStorage::set_cookie(CookieStorageKey key, Web::Cookie::
 
 Optional<Web::Cookie::Cookie> CookieJar::TransientStorage::get_cookie(CookieStorageKey const& key)
 {
-    return m_cookies.get(key);
+    return m_cookies.get(key).copy();
 }
 
 UnixDateTime CookieJar::TransientStorage::purge_expired_cookies()

Userland/Shell/Builtin.cpp

@@ -102,7 +102,7 @@ ErrorOr<int> Shell::builtin_where(Main::Arguments arguments)
     auto const look_up_alias = [do_only_path_search, &m_aliases = this->m_aliases](StringView alias) -> Optional<ByteString> {
         if (do_only_path_search)
             return {};
-        return m_aliases.get(alias);
+        return m_aliases.get(alias).copy();
     };
 
     auto const look_up_builtin = [do_only_path_search](StringView builtin) -> Optional<ByteString> {

Userland/Shell/Shell.cpp

@@ -586,7 +586,7 @@ Shell::Frame::~Frame()
 
 Optional<ByteString> Shell::resolve_alias(StringView name) const
 {
-    return m_aliases.get(name);
+    return m_aliases.get(name).copy();
 }
 
 Optional<Shell::RunnablePath> Shell::runnable_path_for(StringView name)

Userland/Utilities/tar.cpp

@@ -92,12 +92,12 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
         HashMap<ByteString, ByteString> local_overrides;
 
         auto get_override = [&](StringView key) -> Optional<ByteString> {
-            Optional<ByteString> maybe_local = local_overrides.get(key);
+            Optional<ByteString> maybe_local = local_overrides.get(key).copy();
 
             if (maybe_local.has_value())
                 return maybe_local;
 
-            Optional<ByteString> maybe_global = global_overrides.get(key);
+            Optional<ByteString> maybe_global = global_overrides.get(key).copy();
 
             if (maybe_global.has_value())
                 return maybe_global;