proxy-types: Remove requirement for return types to be default-constructible

include/mp/proxy.h

@@ -219,8 +219,8 @@ template <class Fn>
 struct FunctionTraits;
 
 //! Specialization of above extracting result and params types assuming the
-//! template argument is a pointer-to-method type,
-//! decltype(&ClassName::methodName)
+//! template argument is a pointer-to-method type. Handles all four PMF
+//! cv-qualifier combinations by forwarding to this canonical form.
 template <class _Class, class _Result, class... _Params>
 struct FunctionTraits<_Result (_Class::*const)(_Params...)>
 {
@@ -241,6 +241,17 @@ struct FunctionTraits<_Result (_Class::*const)(_Params...)>
     template <size_t N>
     static decltype(auto) Fwd(Param<N>& arg) { return static_cast<Param<N>&&>(arg); }
 };
+// non-const pointer to non-const method
+template <class _Class, class _Result, class... _Params>
+struct FunctionTraits<_Result (_Class::*)(_Params...)>
+    : FunctionTraits<_Result (_Class::*const)(_Params...)> {};
+// (either pointer kind) to const method — collapses const-method distinction
+template <class _Class, class _Result, class... _Params>
+struct FunctionTraits<_Result (_Class::*const)(_Params...) const>
+    : FunctionTraits<_Result (_Class::*const)(_Params...)> {};
+template <class _Class, class _Result, class... _Params>
+struct FunctionTraits<_Result (_Class::*)(_Params...) const>
+    : FunctionTraits<_Result (_Class::*const)(_Params...)> {};
 
 //! Traits class for a proxy method, providing the same
 //! Params/Result/Param/Fields described in the FunctionTraits class above, plus

include/mp/util.h

@@ -247,35 +247,70 @@ void Unlock(Lock& lock, Callback&& callback)
     callback();
 }
 
-//! Invoke a function and run a follow-up action before returning the original
-//! result.
+//! Uninitialized aligned storage for a single T value. Provides a ptr()
+//! accessor to avoid repeated reinterpret_cast/std::launder boilerplate.
+template <typename T>
+struct AlignedStorage {
+    alignas(T) std::byte data[sizeof(T)];
+    T* ptr() { return std::launder(reinterpret_cast<T*>(data)); }
+    const T* ptr() const { return std::launder(reinterpret_cast<const T*>(data)); }
+};
+
+//! Invoke fn() then run an after() callback. Like KJ_DEFER but works better
+//! when after() can throw: avoids clang bug
+//! https://github.com/llvm/llvm-project/issues/12658 which skips destructors,
+//! and when both functions throw lets one exception take precedence.
 //!
-//! This can be used similarly to KJ_DEFER to run cleanup code, but works better
-//! if the cleanup function can throw because it avoids clang bug
-//! https://github.com/llvm/llvm-project/issues/12658 which skips calling
-//! destructors in that case and can lead to memory leaks. Also, if both
-//! functions throw, this lets one exception take precedence instead of
-//! terminating due to having two active exceptions.
+//! after() always receives a pointer whose nullness indicates fn()'s outcome:
+//! - Non-null if fn() returned normally. Pointer type and value depend on R:
+//!   - void: std::true_type* (points to AlignedStorage<std::true_type>)
+//!   - lvalue reference T&: T* pointing to the referenced object
+//!   - value type T: T* pointing to result in AlignedStorage<T>; C++17
+//!     guaranteed copy elision constructs it in place from the prvalue so no
+//!     move or copy constructor is required (handles non-movable types)
+//! - Null if fn() threw; after() is still called so it can do cleanup.
+//! If after() throws, its exception takes precedence over any fn() exception.
 template <typename Fn, typename After>
-decltype(auto) TryFinally(Fn&& fn, After&& after)
+void TryFinally(Fn&& fn, After&& after)
 {
     bool success{false};
     using R = std::invoke_result_t<Fn>;
-    try {
-        if constexpr (std::is_void_v<R>) {
-            std::forward<Fn>(fn)();
+    if constexpr (std::is_lvalue_reference_v<R>) {
+        // Lvalue reference return: pass pointer to the referenced object; no
+        // storage needed since the object's lifetime is managed by the caller.
+        using StorageT = std::remove_reference_t<R>;
+        try {
+            StorageT* ptr = &std::forward<Fn>(fn)();
             success = true;
-            std::forward<After>(after)();
-            return;
-        } else {
-            decltype(auto) result = std::forward<Fn>(fn)();
+            std::forward<After>(after)(ptr);
+        } catch (...) {
+            if (!success) std::forward<After>(after)(static_cast<StorageT*>(nullptr));
+            throw;
+        }
+    } else {
+        // Value (or rvalue-reference or void) return. void is mapped to
+        // std::true_type so the same AlignedStorage pattern covers all cases.
+        // remove_reference_t handles the rvalue-reference sub-case (move-constructed).
+        using StorageT = std::conditional_t<std::is_void_v<R>, std::true_type, std::remove_reference_t<R>>;
+        AlignedStorage<StorageT> storage;
+        bool constructed{false};
+        try {
+            if constexpr (std::is_void_v<R>) {
+                std::forward<Fn>(fn)();
+                new (storage.ptr()) StorageT{};
+            } else {
+                new (storage.ptr()) StorageT(std::forward<Fn>(fn)());  // C++17 guaranteed copy elision
+            }
+            constructed = true;
             success = true;
-            std::forward<After>(after)();
-            return result;
+            std::forward<After>(after)(storage.ptr());
+            storage.ptr()->~StorageT();
+            constructed = false;
+        } catch (...) {
+            if (constructed) storage.ptr()->~StorageT();
+            if (!success) std::forward<After>(after)(static_cast<StorageT*>(nullptr));
+            throw;
         }
-    } catch (...) {
-        if (!success) std::forward<After>(after)();
-        throw;
     }
 }
 

src/mp/gen.cpp

@@ -552,8 +552,10 @@ static void Generate(kj::StringPtr src_prefix,
 
                 std::ostringstream client_args;
                 std::ostringstream client_invoke;
+                std::string client_invoke_return;
                 std::ostringstream server_invoke_start;
                 std::ostringstream server_invoke_end;
+                std::string server_ret_accessor;
                 int argc = 0;
                 for (const auto& field : fields.fields) {
                     if (field.skip) continue;
@@ -578,32 +580,28 @@ static void Generate(kj::StringPtr src_prefix,
 
                         ++argc;
                     }
-                    client_invoke << ", ";
-
                     if (field.exception.size()) {
-                        client_invoke << "ClientException<" << field.exception << ", ";
-                    } else {
-                        client_invoke << "MakeClientParam<";
-                    }
-
-                    client_invoke << AccessorType(base_name, field) << ">(";
-
-                    if (field.retval) {
-                        client_invoke << field_name;
+                        client_invoke << ", ClientException<" << field.exception << ", "
+                                      << AccessorType(base_name, field) << ">()";
+                    } else if (field.retval) {
+                        client_invoke_return = "<typename M" + std::to_string(method_ordinal) + "::Result, " +
+                                               AccessorType(base_name, field) + ">";
                     } else {
-                        client_invoke << fwd_args.str();
+                        client_invoke << ", MakeClientParam<" << AccessorType(base_name, field) << ">("
+                                      << fwd_args.str() << ")";
                     }
-                    client_invoke << ")";
 
                     if (field.exception.size()) {
-                        server_invoke_start << "Make<ServerExcept, " << field.exception;
+                        server_invoke_start << "Make<ServerExcept, " << field.exception
+                                            << ", " << AccessorType(base_name, field) << ">(";
+                        server_invoke_end << ")";
                     } else if (field.retval) {
-                        server_invoke_start << "Make<ServerRet";
+                        server_ret_accessor = AccessorType(base_name, field);
                     } else {
-                        server_invoke_start << "MakeServerField<" << field.args;
+                        server_invoke_start << "MakeServerField<" << field.args
+                                            << ", " << AccessorType(base_name, field) << ">(";
+                        server_invoke_end << ")";
                     }
-                    server_invoke_start << ", " << AccessorType(base_name, field) << ">(";
-                    server_invoke_end << ")";
                 }
 
                 const std::string static_str{is_construct || is_destroy ? "static " : ""};
@@ -618,12 +616,11 @@ static void Generate(kj::StringPtr src_prefix,
                 def_client << "ProxyClient<" << message_namespace << "::" << node_name << ">::M" << method_ordinal
                            << "::Result ProxyClient<" << message_namespace << "::" << node_name << ">::" << method_name
                            << "(" << super_str << client_args.str() << ") {\n";
-                if (fields.has_result) {
-                    def_client << "    typename M" << method_ordinal << "::Result result;\n";
-                }
-                def_client << "    clientInvoke(" << self_str << ", &" << message_namespace << "::" << node_name
-                           << "::Client::" << method_name << "Request" << client_invoke.str() << ");\n";
-                if (fields.has_result) def_client << "    return result;\n";
+                if (fields.has_result) def_client << "    return ";
+                else def_client << "    ";
+                def_client << "clientInvoke" << client_invoke_return << "(" << self_str << ", &" << message_namespace
+                           << "::" << node_name << "::Client::" << method_name << "Request"
+                           << client_invoke.str() << ");\n";
                 def_client << "}\n";
 
                 server << "    kj::Promise<void> " << method_name << "(" << Cap(method_name)
@@ -636,8 +633,10 @@ static void Generate(kj::StringPtr src_prefix,
                            << server_invoke_start.str();
                 if (is_destroy) {
                     def_server << "ServerDestroy()";
+                } else if (server_ret_accessor.empty()) {
+                    def_server << "ServerCall<void>()";
                 } else {
-                    def_server << "ServerCall()";
+                    def_server << "ServerCall<" << server_ret_accessor << ">()";
                 }
                 def_server << server_invoke_end.str() << ");\n}\n";
                 ++method_ordinal;

test/mp/test/foo-types.h

@@ -41,6 +41,22 @@ struct FooFn; // IWYU pragma: export
 struct FooInterface; // IWYU pragma: export
 } // namespace messages
 
+template <typename T, typename Value, typename Output>
+void CustomBuildField(TypeList<Pinned<T>>, Priority<1>, InvokeContext& invoke_context, Value&& value, Output&& output)
+{
+    BuildField(TypeList<T>(), invoke_context, output, value.value);
+}
+
+template <typename T, typename Input, typename ReadDest>
+decltype(auto) CustomReadField(TypeList<Pinned<T>>, Priority<1>, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest)
+{
+    // read_dest.construct() is used instead of read_dest.update() because Pinned<T>
+    // has no default constructor, so update()'s default-construct-then-fill path fails.
+    // ReadDestTemp<T> is required here: without a pre-existing T to pass to
+    // ReadDestUpdate, we need ReadField to return a T value directly.
+    return read_dest.construct(ReadField(TypeList<T>(), invoke_context, input, ReadDestTemp<T>()));
+}
+
 template <typename Output>
 void CustomBuildField(TypeList<FooCustom>, Priority<1>, InvokeContext& invoke_context, const FooCustom& value, Output&& output)
 {

test/mp/test/foo.capnp

@@ -26,6 +26,8 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") {
     callbackSaved @9 (context :Proxy.Context, arg: Int32) -> (result :Int32);
     callbackExtended @10 (context :Proxy.Context, callback :ExtendedCallback, arg: Int32) -> (result :Int32);
     passCustom @11 (arg :FooCustom) -> (result :FooCustom);
+    returnPinned @26 (vec :List(Int32)) -> (result :List(Int32));
+    throwPinned @27 (vec :List(Int32)) -> (error :List(Int32) $Proxy.exception("mp::test::Pinned<std::vector<int>>"));
     passEmpty @12 (arg :FooEmpty) -> (result :FooEmpty);
     passData @24 (arg :Data) -> (result :Data);
     passMessage @13 (arg :FooMessage) -> (result :FooMessage);

test/mp/test/foo.h

@@ -40,6 +40,19 @@ struct FooEmpty
 {
 };
 
+// Test type that has no default constructor and cannot be copied or moved.
+// Used to stress-test the serialization framework's ReadDestTemp path.
+template <typename T>
+struct Pinned {
+    T value;
+    explicit Pinned(T v) : value(std::move(v)) {}
+    Pinned() = delete;
+    Pinned(const Pinned&) = delete;
+    Pinned& operator=(const Pinned&) = delete;
+    Pinned(Pinned&&) = delete;
+    Pinned& operator=(Pinned&&) = delete;
+};
+
 struct FooMessage
 {
     std::string message;
@@ -83,6 +96,8 @@ class FooImplementation
     int callbackSaved(int arg) { return m_callback->call(arg); }
     int callbackExtended(ExtendedCallback& callback, int arg) { return callback.callExtended(arg); }
     FooCustom passCustom(FooCustom foo) { return foo; }
+    Pinned<std::vector<int>> returnPinned(std::vector<int> vec) { return Pinned<std::vector<int>>{std::move(vec)}; }
+    void throwPinned(std::vector<int> vec) { throw Pinned<std::vector<int>>{std::move(vec)}; }
     FooEmpty passEmpty(FooEmpty foo) { return foo; }
     FooData passData(FooData foo) { return foo; }
     FooMessage passMessage(FooMessage foo) { foo.message += " call"; return foo; }

test/mp/test/test.cpp

@@ -237,6 +237,17 @@ KJ_TEST("Call FooInterface methods")
     KJ_EXPECT(custom_in.v1 == custom_out.v1);
     KJ_EXPECT(custom_in.v2 == custom_out.v2);
 
+    std::vector<int> pinned_in = {1, 2, 3};
+    Pinned<std::vector<int>> pinned_out = foo->returnPinned(pinned_in);
+    KJ_EXPECT(pinned_out.value == pinned_in);
+
+    try {
+        foo->throwPinned(pinned_in);
+        KJ_FAIL_EXPECT("throwPinned should have thrown");
+    } catch (const Pinned<std::vector<int>>& e) {
+        KJ_EXPECT(e.value == pinned_in);
+    }
+
     foo->passEmpty(FooEmpty{});
 
     FooData empty_data_out = foo->passData(FooData{});