Merge #242: proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values

97d8770 proxy-types: add CustomHasField hook for nullable decode paths (Sjors Provoost)
8c2f102 refactor: add missing includes to mp/type-data.h (Sjors Provoost)

Pull request description:

  Taken from: bitcoin/bitcoin#34020 (comment)

  Let applications override `CustomHasField` so they can decide to treat certain capnproto values as being unset. For example, when converting `List(Data)` to `vector<shared_ptr<CTransaction>>`, mapping empty `Data` fields to null pointers.

  This safe to do in special cases, like in this example because serialized `CTransaction` representations are never empty. It is also useful to do in this case because Cap'n Proto doesn't currently provide any API for distinguishing between unset and empty data values in a list (although they can be distinguished on the wire).

ACKs for top commit:
  ryanofsky:
    Code review ACK 97d8770. Confirmed no changes since last rebase.

Tree-SHA512: 34a9e9961a6e9334f21c684d9c89be98c85b6269852a6fc169ffdec47efb2a42b1e76e6d52090f448ed1a56d6995a8dde74d2c6a6fec75ca46bfd551a26e54cf

Author: ryanofsky

include/mp/proxy-types.h

@@ -23,9 +23,10 @@ class ValueField
     ValueField(Value&& value) : m_value(value) {}
     Value& m_value;
 
+    const Value& get() const { return m_value; }
     Value& get() { return m_value; }
     Value& init() { return m_value; }
-    bool has() { return true; }
+    bool has() const { return true; }
 };
 
 template <typename Accessor, typename Struct>
@@ -166,10 +167,52 @@ struct ReadDestUpdate
     Value& m_value;
 };
 
-template <typename... LocalTypes, typename... Args>
-decltype(auto) ReadField(TypeList<LocalTypes...>, Args&&... args)
+//! Return whether to read a C++ value from a Cap'n Proto field. Returning
+//! false can be useful to interpret certain Cap'n Proto field values as null
+//! C++ values when initializing nullable C++ std::optional / std::unique_ptr /
+//! std::shared_ptr types.
+//!
+//! For example, when reading from a `List(Data)` field into a
+//! `std::vector<std::shared_ptr<const CTransaction>>` value, it's useful to be
+//! able to interpret empty `Data` values as null pointers. This is useful
+//! because the Cap'n Proto C++ API does not currently provide a way to
+//! distinguish between null and empty Data values in a List[*], so we need to
+//! choose some Data value to represent null if we want to allow passing null
+//! pointers. Since no CTransaction is ever serialized as empty Data, it's safe
+//! to use empty Data values to represent null pointers.
+//!
+//! [*] The Cap'n Proto wire format actually does distinguish between null and
+//! empty Data values inside Lists, and the C++ API does allow distinguishing
+//! between null and empty Data values in other contexts, just not the List
+//! context, so this limitation could be removed in the future.
+//!
+//! Design note: CustomHasField() and CustomHasValue() are inverses of each
+//! other.  CustomHasField() allows leaving Cap'n Proto fields unset when C++
+//! types have certain values, and CustomHasValue() allows leaving C++ values
+//! unset when Cap'n Proto fields have certain values. But internally the
+//! functions get called in different ways. This is because in C++, unlike in
+//! Cap'n Proto not every C++ type is default constructible, and it may be
+//! impossible to leave certain C++ values unset. For example if a C++ method
+//! requires function parameters, there's no way to call the function without
+//! constructing values for each of the parameters. Similarly there's no way to
+//! add values to C++ vectors or maps without initializing those values.  This
+//! is not the case in Cap'n Proto where all values are optional and it's
+//! possible to skip initializing parameters and list elements.
+//!
+//! Because of this difference, CustomHasValue() works universally and can be
+//! used to disable BuildField() calls in every context, while CustomHasField()
+//! can only be used to disable ReadField() calls in certain contexts like
+//! std::optional and pointer contexts.
+template <typename... LocalTypes, typename Input>
+bool CustomHasField(TypeList<LocalTypes...>, InvokeContext& invoke_context, const Input& input)
+{
+    return input.has();
+}
+
+template <typename... LocalTypes, typename Input, typename... Args>
+decltype(auto) ReadField(TypeList<LocalTypes...>, InvokeContext& invoke_context, Input&& input, Args&&... args)
 {
-    return CustomReadField(TypeList<RemoveCvRef<LocalTypes>...>(), Priority<2>(), std::forward<Args>(args)...);
+    return CustomReadField(TypeList<RemoveCvRef<LocalTypes>...>(), Priority<2>(), invoke_context, std::forward<Input>(input), std::forward<Args>(args)...);
 }
 
 template <typename LocalType, typename Input>
@@ -190,6 +233,13 @@ void ThrowField(TypeList<std::exception>, InvokeContext& invoke_context, Input&&
     throw std::runtime_error(std::string(CharCast(data.begin()), data.size()));
 }
 
+//! Return whether to write a C++ value into a Cap'n Proto field. Returning
+//! false can be useful to map certain C++ values to unset Cap'n Proto fields.
+//!
+//! For example the bitcoin `Coin` class asserts false when a spent coin is
+//! serialized. But some C++ methods return these coins, so there needs to be a
+//! way to represent them in Cap'n Proto and a null Data field is a convenient
+//! representation.
 template <typename... Values>
 bool CustomHasValue(InvokeContext& invoke_context, const Values&... value)
 {
@@ -372,7 +422,7 @@ struct ClientException
         void handleField(InvokeContext& invoke_context, Results& results, ParamList)
         {
             StructField<Accessor, Results> input(results);
-            if (input.has()) {
+            if (CustomHasField(TypeList<Exception>(), invoke_context, input)) {
                 ThrowField(TypeList<Exception>(), invoke_context, input);
             }
         }

include/mp/type-data.h

@@ -7,6 +7,9 @@
 
 #include <mp/util.h>
 
+#include <concepts>
+#include <span>
+
 namespace mp {
 template <typename T, typename U>
 concept IsSpanOf =

include/mp/type-function.h

@@ -48,13 +48,13 @@ struct ProxyCallFn
 };
 
 template <typename FnR, typename... FnParams, typename Input, typename ReadDest>
-decltype(auto) CustomReadField(TypeList<std::function<FnR(FnParams...)>>,
+decltype(auto) CustomReadField(TypeList<std::function<FnR(FnParams...)>> types,
     Priority<1>,
     InvokeContext& invoke_context,
     Input&& input,
     ReadDest&& read_dest)
 {
-    if (input.has()) {
+    if (CustomHasField(types, invoke_context, input)) {
         using Interface = typename Decay<decltype(input.get())>::Calls;
         auto client = std::make_shared<ProxyClient<Interface>>(
             input.get(), &invoke_context.connection, /* destroy_connection= */ false);

include/mp/type-interface.h

@@ -85,7 +85,7 @@ decltype(auto) CustomReadField(TypeList<std::unique_ptr<LocalType>>,
     typename Decay<decltype(input.get())>::Calls* enable = nullptr)
 {
     using Interface = typename Decay<decltype(input.get())>::Calls;
-    if (input.has()) {
+    if (CustomHasField(TypeList<LocalType>(), invoke_context, input)) {
         return read_dest.construct(
                                    CustomMakeProxyClient<Interface, LocalType>(invoke_context, std::move(input.get())));
     }
@@ -101,7 +101,7 @@ decltype(auto) CustomReadField(TypeList<std::shared_ptr<LocalType>>,
     typename Decay<decltype(input.get())>::Calls* enable = nullptr)
 {
     using Interface = typename Decay<decltype(input.get())>::Calls;
-    if (input.has()) {
+    if (CustomHasField(TypeList<LocalType>(), invoke_context, input)) {
         return read_dest.construct(
             CustomMakeProxyClient<Interface, LocalType>(invoke_context, std::move(input.get())));
     }

include/mp/type-message.h

@@ -23,13 +23,13 @@ void CustomBuildField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_co
 //! overloads. Defining a CustomReadMessage overload is simpler than defining a
 //! CustomReadField overload because it only requires defining a normal
 //! function, not a template function, but less flexible.
-template <typename LocalType, typename Reader, typename ReadDest>
-decltype(auto) CustomReadField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_context, Reader&& reader,
+template <typename LocalType, typename Input, typename ReadDest>
+decltype(auto) CustomReadField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_context, Input&& input,
                                ReadDest&& read_dest,
-                               decltype(CustomReadMessage(invoke_context, reader.get(),
+                               decltype(CustomReadMessage(invoke_context, input.get(),
                                                           std::declval<LocalType&>()))* enable = nullptr)
 {
-    return read_dest.update([&](auto& value) { if (reader.has()) CustomReadMessage(invoke_context, reader.get(), value); });
+    return read_dest.update([&](auto& value) { if (CustomHasField(TypeList<LocalType>(), invoke_context, input)) CustomReadMessage(invoke_context, input.get(), value); });
 }
 
 //! Helper for CustomPassField below. Call Accessor::init method if it has one,

include/mp/type-optional.h

@@ -30,7 +30,7 @@ decltype(auto) CustomReadField(TypeList<std::optional<LocalType>>,
     ReadDest&& read_dest)
 {
     return read_dest.update([&](auto& value) {
-        if (!input.has()) {
+        if (!CustomHasField(TypeList<LocalType>(), invoke_context, input)) {
             value.reset();
         } else if (value) {
             ReadField(TypeList<LocalType>(), invoke_context, input, ReadDestUpdate(*value));

include/mp/type-pointer.h

@@ -50,7 +50,7 @@ decltype(auto) CustomReadField(TypeList<std::shared_ptr<LocalType>>,
     ReadDest&& read_dest)
 {
     return read_dest.update([&](auto& value) {
-        if (!input.has()) {
+        if (!CustomHasField(TypeList<LocalType>(), invoke_context, input)) {
             value.reset();
         } else if (value) {
             ReadField(TypeList<LocalType>(), invoke_context, input, ReadDestUpdate(*value));
@@ -72,7 +72,7 @@ decltype(auto) CustomReadField(TypeList<std::shared_ptr<const LocalType>>,
     ReadDest&& read_dest)
 {
     return read_dest.update([&](auto& value) {
-        if (!input.has()) {
+        if (!CustomHasField(TypeList<LocalType>(), invoke_context, input)) {
             value.reset();
             return;
         }

src/mp/gen.cpp

@@ -228,6 +228,7 @@ static void Generate(kj::StringPtr src_prefix,
     cpp_client << "#include <" << include_path << ".proxy-types.h>\n";
     cpp_client << "#include <capnp/generated-header-support.h>\n";
     cpp_client << "#include <cstring>\n";
+    cpp_client << "#include <vector>\n";
     cpp_client << "#include <kj/common.h>\n";
     cpp_client << "#include <mp/proxy.h>\n";
     cpp_client << "#include <mp/util.h>\n";

test/mp/test/foo-types.h

@@ -13,12 +13,14 @@
 #include <cstddef>
 #include <mp/test/foo.capnp.h>
 #include <mp/type-context.h>
+#include <mp/type-data.h>
 #include <mp/type-decay.h>
 #include <mp/type-function.h>
 #include <mp/type-interface.h>
 #include <mp/type-map.h>
 #include <mp/type-message.h>
 #include <mp/type-number.h>
+#include <mp/type-pointer.h>
 #include <mp/type-set.h>
 #include <mp/type-string.h>
 #include <mp/type-struct.h>
@@ -56,6 +58,14 @@ decltype(auto) CustomReadField(TypeList<FooCustom>, Priority<1>, InvokeContext&
 
 } // namespace test
 
+template <typename Input>
+bool CustomHasField(TypeList<test::FooData>, InvokeContext& invoke_context, const Input& input)
+{
+    // Cap'n Proto C++ cannot distinguish null vs empty Data in List(Data), so
+    // interpret empty Data as null for this specific type.
+    return input.get().size() != 0;
+}
+
 inline void CustomBuildMessage(InvokeContext& invoke_context,
                         const test::FooMessage& src,
                         test::messages::FooMessage::Builder&& builder)

test/mp/test/foo.capnp

@@ -34,6 +34,7 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") {
     callFn @17 () -> ();
     callFnAsync @18 (context :Proxy.Context) -> ();
     callIntFnAsync @21 (context :Proxy.Context, arg :Int32) -> (result :Int32);
+    passDataPointers @22 (arg :List(Data)) -> (result :List(Data));
 }
 
 interface FooCallback $Proxy.wrap("mp::test::FooCallback") {