LibIPC+Everywhere: Change IPC decoders to construct values in-place

Currently, the generated IPC decoders will default-construct the type to
be decoded, then pass that value by reference to the concrete decoder.
This, of course, requires that the type is default-constructible. This
was an issue for decoding Variants, which had to require the first type
in the Variant list is Empty, to ensure it is default constructible.

Further, this made it possible for values to become uninitialized in
user-defined decoders.

This patch makes the decoder interface such that the concrete decoders
themselves contruct the decoded type upon return from the decoder. To do
so, the default decoders in IPC::Decoder had to be moved to the IPC
namespace scope, as these decoders are now specializations instead of
overloaded methods (C++ requires specializations to be in a namespace
scope).

Userland/Libraries/LibIPC/Concepts.h

@@ -0,0 +1,72 @@
+/*
+ * Copyright (c) 2022, Tim Flynn <trflynn89@serenityos.org>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#include <AK/HashMap.h>
+#include <AK/Optional.h>
+#include <AK/Variant.h>
+#include <AK/Vector.h>
+#include <LibCore/SharedCircularQueue.h>
+
+#pragma once
+
+// These concepts are used to help the compiler distinguish between specializations that would be
+// ambiguous otherwise. For example, if the specializations for int and Vector<T> were declared as
+// follows:
+//
+//     template<> ErrorOr<int> decode(Decoder& decoder);
+//     template<typename T> ErrorOr<Vector<T>> decode(Decoder& decoder);
+//
+// Then decode<int>() would be ambiguous because either declaration could work (the compiler would
+// not be able to distinguish if you wanted to decode an int or a Vector of int).
+namespace IPC::Concepts {
+
+namespace Detail {
+
+template<typename T>
+constexpr inline bool IsHashMap = false;
+template<typename K, typename V>
+constexpr inline bool IsHashMap<HashMap<K, V>> = true;
+template<typename K, typename V>
+constexpr inline bool IsHashMap<OrderedHashMap<K, V>> = true;
+
+template<typename T>
+constexpr inline bool IsOptional = false;
+template<typename T>
+constexpr inline bool IsOptional<Optional<T>> = true;
+
+template<typename T>
+constexpr inline bool IsSharedSingleProducerCircularQueue = false;
+template<typename T, size_t Size>
+constexpr inline bool IsSharedSingleProducerCircularQueue<Core::SharedSingleProducerCircularQueue<T, Size>> = true;
+
+template<typename T>
+constexpr inline bool IsVariant = false;
+template<typename... Ts>
+constexpr inline bool IsVariant<Variant<Ts...>> = true;
+
+template<typename T>
+constexpr inline bool IsVector = false;
+template<typename T>
+constexpr inline bool IsVector<Vector<T>> = true;
+
+}
+
+template<typename T>
+concept HashMap = Detail::IsHashMap<T>;
+
+template<typename T>
+concept Optional = Detail::IsOptional<T>;
+
+template<typename T>
+concept SharedSingleProducerCircularQueue = Detail::IsSharedSingleProducerCircularQueue<T>;
+
+template<typename T>
+concept Variant = Detail::IsVariant<T>;
+
+template<typename T>
+concept Vector = Detail::IsVector<T>;
+
+}

Userland/Libraries/LibIPC/Decoder.cpp

@@ -5,7 +5,6 @@
  */
 
 #include <AK/JsonValue.h>
-#include <AK/MemoryStream.h>
 #include <AK/URL.h>
 #include <LibCore/AnonymousBuffer.h>
 #include <LibCore/DateTime.h>
@@ -17,201 +16,110 @@
 
 namespace IPC {
 
-ErrorOr<void> Decoder::decode(bool& value)
+template<>
+ErrorOr<DeprecatedString> decode(Decoder& decoder)
 {
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
+    auto length = TRY(decoder.decode<i32>());
+    if (length < 0)
+        return DeprecatedString {};
+    if (length == 0)
+        return DeprecatedString::empty();
 
-ErrorOr<void> Decoder::decode(u8& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(u16& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(unsigned& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(unsigned long& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(unsigned long long& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(i8& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(i16& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(i32& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(i64& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(float& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(double& value)
-{
-    m_stream >> value;
-    return m_stream.try_handle_any_error();
-}
-
-ErrorOr<void> Decoder::decode(DeprecatedString& value)
-{
-    i32 length;
-    TRY(decode(length));
-
-    if (length < 0) {
-        value = {};
-        return {};
-    }
-    if (length == 0) {
-        value = DeprecatedString::empty();
-        return {};
-    }
     char* text_buffer = nullptr;
     auto text_impl = StringImpl::create_uninitialized(static_cast<size_t>(length), text_buffer);
-    m_stream >> Bytes { text_buffer, static_cast<size_t>(length) };
-    value = *text_impl;
-    return m_stream.try_handle_any_error();
+
+    Bytes bytes { text_buffer, static_cast<size_t>(length) };
+    TRY(decoder.decode_into(bytes));
+
+    return DeprecatedString { *text_impl };
 }
 
-ErrorOr<void> Decoder::decode(ByteBuffer& value)
+template<>
+ErrorOr<ByteBuffer> decode(Decoder& decoder)
 {
-    i32 length;
-    TRY(decode(length));
+    auto length = TRY(decoder.decode<i32>());
+    if (length <= 0)
+        return ByteBuffer {};
 
-    if (length < 0) {
-        value = {};
-        return {};
-    }
-    if (length == 0) {
-        value = {};
-        return {};
-    }
+    auto buffer = TRY(ByteBuffer::create_uninitialized(length));
+    auto bytes = buffer.bytes();
 
-    value = TRY(ByteBuffer::create_uninitialized(length));
-
-    m_stream >> value.bytes();
-    return m_stream.try_handle_any_error();
+    TRY(decoder.decode_into(bytes));
+    return buffer;
 }
 
-ErrorOr<void> Decoder::decode(JsonValue& value)
+template<>
+ErrorOr<JsonValue> decode(Decoder& decoder)
 {
-    DeprecatedString string;
-    TRY(decode(string));
-    value = TRY(JsonValue::from_string(string));
-    return {};
+    auto json = TRY(decoder.decode<DeprecatedString>());
+    return JsonValue::from_string(json);
 }
 
-ErrorOr<void> Decoder::decode(URL& value)
+template<>
+ErrorOr<URL> decode(Decoder& decoder)
 {
-    DeprecatedString string;
-    TRY(decode(string));
-    value = URL(string);
-    return {};
+    auto url = TRY(decoder.decode<DeprecatedString>());
+    return URL { url };
 }
 
-ErrorOr<void> Decoder::decode(Dictionary& dictionary)
+template<>
+ErrorOr<Dictionary> decode(Decoder& decoder)
 {
-    u64 size;
-    TRY(decode(size));
-    if (size >= (size_t)NumericLimits<i32>::max())
+    auto size = TRY(decoder.decode<u64>());
+    if (size >= NumericLimits<i32>::max())
         VERIFY_NOT_REACHED();
 
+    Dictionary dictionary {};
+
     for (size_t i = 0; i < size; ++i) {
-        DeprecatedString key;
-        TRY(decode(key));
-        DeprecatedString value;
-        TRY(decode(value));
+        auto key = TRY(decoder.decode<DeprecatedString>());
+        auto value = TRY(decoder.decode<DeprecatedString>());
         dictionary.add(move(key), move(value));
     }
 
-    return {};
-}
-
-ErrorOr<void> Decoder::decode([[maybe_unused]] File& file)
-{
-    int fd = TRY(m_socket.receive_fd(O_CLOEXEC));
-    file = File(fd, File::ConstructWithReceivedFileDescriptor);
-    return {};
+    return dictionary;
 }
 
 template<>
-ErrorOr<void> decode(Decoder& decoder, Core::AnonymousBuffer& buffer)
+ErrorOr<File> decode(Decoder& decoder)
 {
-    bool valid;
-    TRY(decoder.decode(valid));
-    if (!valid) {
-        buffer = {};
-        return {};
-    }
-    u32 size;
-    TRY(decoder.decode(size));
-    IPC::File anon_file;
-    TRY(decoder.decode(anon_file));
-
-    buffer = TRY(Core::AnonymousBuffer::create_from_anon_fd(anon_file.take_fd(), size));
-    return {};
+    int fd = TRY(decoder.socket().receive_fd(O_CLOEXEC));
+    return File { fd, File::ConstructWithReceivedFileDescriptor };
 }
 
 template<>
-ErrorOr<void> decode(Decoder& decoder, Core::DateTime& datetime)
+ErrorOr<Empty> decode(Decoder&)
 {
-    i64 timestamp;
-    TRY(decoder.decode(timestamp));
-    datetime = Core::DateTime::from_timestamp(static_cast<time_t>(timestamp));
-    return {};
+    return Empty {};
 }
 
 template<>
-ErrorOr<void> decode(Decoder& decoder, Core::ProxyData& data)
+ErrorOr<Core::AnonymousBuffer> decode(Decoder& decoder)
 {
-    UnderlyingType<decltype(data.type)> type;
-    TRY(decoder.decode(type));
-    data.type = static_cast<Core::ProxyData::Type>(type);
-    TRY(decoder.decode(data.host_ipv4));
-    TRY(decoder.decode(data.port));
-    return {};
+    if (auto valid = TRY(decoder.decode<bool>()); !valid)
+        return Core::AnonymousBuffer {};
+
+    auto size = TRY(decoder.decode<u32>());
+    auto anon_file = TRY(decoder.decode<IPC::File>());
+
+    return Core::AnonymousBuffer::create_from_anon_fd(anon_file.take_fd(), size);
 }
 
-// No-op.
-ErrorOr<void> Decoder::decode(AK::Empty&)
+template<>
+ErrorOr<Core::DateTime> decode(Decoder& decoder)
 {
-    return {};
+    auto timestamp = TRY(decoder.decode<i64>());
+    return Core::DateTime::from_timestamp(static_cast<time_t>(timestamp));
+}
+
+template<>
+ErrorOr<Core::ProxyData> decode(Decoder& decoder)
+{
+    auto type = TRY(decoder.decode<Core::ProxyData::Type>());
+    auto host_ipv4 = TRY(decoder.decode<u32>());
+    auto port = TRY(decoder.decode<int>());
+
+    return Core::ProxyData { type, host_ipv4, port };
 }
 
 }

Userland/Libraries/LibIPC/Decoder.h

@@ -9,6 +9,7 @@
 #include <AK/Concepts.h>
 #include <AK/DeprecatedString.h>
 #include <AK/Forward.h>
+#include <AK/MemoryStream.h>
 #include <AK/NumericLimits.h>
 #include <AK/StdLibExtras.h>
 #include <AK/Try.h>
@@ -16,6 +17,7 @@
 #include <AK/Variant.h>
 #include <LibCore/SharedCircularQueue.h>
 #include <LibCore/Stream.h>
+#include <LibIPC/Concepts.h>
 #include <LibIPC/File.h>
 #include <LibIPC/Forward.h>
 #include <LibIPC/Message.h>
@@ -23,7 +25,7 @@
 namespace IPC {
 
 template<typename T>
-inline ErrorOr<void> decode(Decoder&, T&)
+inline ErrorOr<T> decode(Decoder&)
 {
     static_assert(DependentFalse<T>, "Base IPC::decoder() instantiated");
     VERIFY_NOT_REACHED();
@@ -37,151 +39,144 @@ public:
     {
     }
 
-    ErrorOr<void> decode(bool&);
-    ErrorOr<void> decode(u8&);
-    ErrorOr<void> decode(u16&);
-    ErrorOr<void> decode(unsigned&);
-    ErrorOr<void> decode(unsigned long&);
-    ErrorOr<void> decode(unsigned long long&);
-    ErrorOr<void> decode(i8&);
-    ErrorOr<void> decode(i16&);
-    ErrorOr<void> decode(i32&);
-    ErrorOr<void> decode(i64&);
-    ErrorOr<void> decode(float&);
-    ErrorOr<void> decode(double&);
-    ErrorOr<void> decode(DeprecatedString&);
-    ErrorOr<void> decode(ByteBuffer&);
-    ErrorOr<void> decode(JsonValue&);
-    ErrorOr<void> decode(URL&);
-    ErrorOr<void> decode(Dictionary&);
-    ErrorOr<void> decode(File&);
-    ErrorOr<void> decode(AK::Empty&);
-    template<typename K, typename V>
-    ErrorOr<void> decode(HashMap<K, V>& hashmap)
-    {
-        u32 size;
-        TRY(decode(size));
-        if (size > NumericLimits<i32>::max())
-            return Error::from_string_literal("IPC: Invalid HashMap size");
-
-        for (size_t i = 0; i < size; ++i) {
-            K key;
-            TRY(decode(key));
-            V value;
-            TRY(decode(value));
-            TRY(hashmap.try_set(move(key), move(value)));
-        }
-        return {};
-    }
-
-    template<typename K, typename V>
-    ErrorOr<void> decode(OrderedHashMap<K, V>& hashmap)
-    {
-        u32 size;
-        TRY(decode(size));
-        if (size > NumericLimits<i32>::max())
-            return Error::from_string_literal("IPC: Invalid HashMap size");
-
-        for (size_t i = 0; i < size; ++i) {
-            K key;
-            TRY(decode(key));
-            V value;
-            TRY(decode(value));
-            TRY(hashmap.try_set(move(key), move(value)));
-        }
-        return {};
-    }
-
-    template<Enum T>
-    ErrorOr<void> decode(T& enum_value)
-    {
-        UnderlyingType<T> inner_value;
-        TRY(decode(inner_value));
-        enum_value = T(inner_value);
-        return {};
-    }
+    template<typename T>
+    ErrorOr<T> decode();
 
     template<typename T>
-    ErrorOr<void> decode(T& value)
+    ErrorOr<void> decode_into(T& value)
     {
-        return IPC::decode(*this, value);
-    }
-
-    template<typename T>
-    ErrorOr<void> decode(Vector<T>& vector)
-    {
-        u64 size;
-        TRY(decode(size));
-        if (size > NumericLimits<i32>::max())
-            return Error::from_string_literal("IPC: Invalid Vector size");
-        VERIFY(vector.is_empty());
-        TRY(vector.try_ensure_capacity(size));
-        for (size_t i = 0; i < size; ++i) {
-            T value;
-            TRY(decode(value));
-            vector.template unchecked_append(move(value));
-        }
+        m_stream >> value;
+        TRY(m_stream.try_handle_any_error());
         return {};
     }
 
-    template<typename T, size_t Size>
-    ErrorOr<void> decode(Core::SharedSingleProducerCircularQueue<T, Size>& queue)
-    {
-        // FIXME: We don't support decoding into valid queues.
-        VERIFY(!queue.is_valid());
-
-        IPC::File anon_file;
-        TRY(decode(anon_file));
-        queue = TRY((Core::SharedSingleProducerCircularQueue<T, Size>::try_create(anon_file.take_fd())));
-        return {};
-    }
-
-    template<typename... VariantTypes>
-    ErrorOr<void> decode(Variant<VariantTypes...>& variant)
-    {
-        typename AK::Variant<VariantTypes...>::IndexType type_index;
-        TRY(decode(type_index));
-        if (type_index >= sizeof...(VariantTypes))
-            return Error::from_string_literal("IPC: Invalid variant index");
-
-        TRY((decode_variant<0, sizeof...(VariantTypes), VariantTypes...>(type_index, variant)));
-        return {};
-    }
-
-    template<typename T>
-    ErrorOr<void> decode(Optional<T>& optional)
-    {
-        bool has_value;
-        TRY(decode(has_value));
-        if (!has_value) {
-            optional = {};
-            return {};
-        }
-        T value;
-        TRY(decode(value));
-        optional = move(value);
-        return {};
-    }
+    Core::Stream::LocalSocket& socket() { return m_socket; }
 
 private:
-    template<size_t CurrentIndex, size_t Max, typename... VariantTypes>
-    ErrorOr<void> decode_variant(size_t index, Variant<VariantTypes...>& variant)
-    {
-        if constexpr (CurrentIndex < Max) {
-            if (index == CurrentIndex) {
-                typename TypeList<VariantTypes...>::template Type<CurrentIndex> element;
-                TRY(decode(element));
-                variant.set(move(element));
-                return {};
-            }
-            return decode_variant<CurrentIndex + 1, Max, VariantTypes...>(index, variant);
-        } else {
-            VERIFY_NOT_REACHED();
-        }
-    }
-
     InputMemoryStream& m_stream;
     Core::Stream::LocalSocket& m_socket;
 };
 
+template<Arithmetic T>
+ErrorOr<T> decode(Decoder& decoder)
+{
+    T value { 0 };
+    TRY(decoder.decode_into(value));
+    return value;
+}
+
+template<Enum T>
+ErrorOr<T> decode(Decoder& decoder)
+{
+    auto value = TRY(decoder.decode<UnderlyingType<T>>());
+    return static_cast<T>(value);
+}
+
+template<>
+ErrorOr<DeprecatedString> decode(Decoder&);
+
+template<>
+ErrorOr<ByteBuffer> decode(Decoder&);
+
+template<>
+ErrorOr<JsonValue> decode(Decoder&);
+
+template<>
+ErrorOr<URL> decode(Decoder&);
+
+template<>
+ErrorOr<Dictionary> decode(Decoder&);
+
+template<>
+ErrorOr<File> decode(Decoder&);
+
+template<>
+ErrorOr<Empty> decode(Decoder&);
+
+template<Concepts::Vector T>
+ErrorOr<T> decode(Decoder& decoder)
+{
+    auto size = TRY(decoder.decode<u64>());
+    if (size > NumericLimits<i32>::max())
+        return Error::from_string_literal("IPC: Invalid Vector size");
+
+    T vector;
+    TRY(vector.try_ensure_capacity(size));
+
+    for (size_t i = 0; i < size; ++i) {
+        auto value = TRY(decoder.decode<typename T::ValueType>());
+        vector.template unchecked_append(move(value));
+    }
+
+    return vector;
+}
+
+template<Concepts::HashMap T>
+ErrorOr<T> decode(Decoder& decoder)
+{
+    auto size = TRY(decoder.decode<u32>());
+    if (size > NumericLimits<i32>::max())
+        return Error::from_string_literal("IPC: Invalid HashMap size");
+
+    T hashmap;
+
+    for (size_t i = 0; i < size; ++i) {
+        auto key = TRY(decoder.decode<typename T::KeyType>());
+        auto value = TRY(decoder.decode<typename T::ValueType>());
+        TRY(hashmap.try_set(move(key), move(value)));
+    }
+
+    return hashmap;
+}
+
+template<Concepts::SharedSingleProducerCircularQueue T>
+ErrorOr<T> decode(Decoder& decoder)
+{
+    auto anon_file = TRY(decoder.decode<IPC::File>());
+    return T::try_create(anon_file.take_fd());
+}
+
+template<Concepts::Optional T>
+ErrorOr<T> decode(Decoder& decoder)
+{
+    if (auto has_value = TRY(decoder.decode<bool>()); !has_value)
+        return T {};
+    return T { TRY(decoder.decode<typename T::ValueType>()) };
+}
+
+namespace Detail {
+
+template<Concepts::Variant T, size_t Index = 0>
+ErrorOr<T> decode_variant(Decoder& decoder, size_t index)
+{
+    using ElementList = TypeList<T>;
+
+    if constexpr (Index < ElementList::size) {
+        if (index == Index) {
+            using ElementType = typename ElementList::template Type<Index>;
+            return T { TRY(decoder.decode<ElementType>()) };
+        }
+
+        return decode_variant<T, Index + 1>(decoder, index);
+    } else {
+        VERIFY_NOT_REACHED();
+    }
+}
+
+}
+
+template<Concepts::Variant T>
+ErrorOr<T> decode(Decoder& decoder)
+{
+    auto index = TRY(decoder.decode<typename T::IndexType>());
+    return Detail::decode_variant<T>(decoder, index);
+}
+
+// This must be last so that it knows about the above specializations.
+template<typename T>
+ErrorOr<T> Decoder::decode()
+{
+    return IPC::decode<T>(*this);
+}
+
 }

Userland/Libraries/LibIPC/Forward.h

@@ -19,6 +19,6 @@ template<typename T>
 bool encode(Encoder&, T const&);
 
 template<typename T>
-ErrorOr<void> decode(Decoder&, T&);
+ErrorOr<T> decode(Decoder&);
 
 }