proxy: fix BuildList to use non-const iteration for interface types #304

IIUC this isn't really a callback flow, so the terminology had me confused about the test. Might be better to add another (Bar) interface to make it more clear.

diff --git a/test/mp/test/foo-types.h b/test/mp/test/foo-types.h
index b96eabfcfb..96506a9f50 100644
--- a/test/mp/test/foo-types.h
+++ b/test/mp/test/foo-types.h
@@ -40,4 +40,5 @@ struct FooCallback; // IWYU pragma: export
 struct FooFn; // IWYU pragma: export
 struct FooInterface; // IWYU pragma: export
+struct BarInterface; // IWYU pragma: export
 } // namespace messages

diff --git a/test/mp/test/foo.capnp b/test/mp/test/foo.capnp
index edf7841c8a..0896274209 100644
--- a/test/mp/test/foo.capnp
+++ b/test/mp/test/foo.capnp
@@ -37,5 +37,5 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") {
     callIntFnAsync [@21](/bitcoin-core-multiprocess/contributor/21/) (context :Proxy.Context, arg :Int32) -> (result :Int32);
     passDataPointers [@22](/bitcoin-core-multiprocess/contributor/22/) (arg :List(Data)) -> (result :List(Data));
-    listCallbacks [@24](/bitcoin-core-multiprocess/contributor/24/) (context :Proxy.Context, n :Int32) -> (result :List(FooCallback));
+    listBars [@24](/bitcoin-core-multiprocess/contributor/24/) (context :Proxy.Context, n :Int32) -> (result :List(BarInterface));
 }

@@ -49,4 +49,9 @@ interface ExtendedCallback extends(FooCallback) $Proxy.wrap("mp::test::ExtendedC
 }

+interface BarInterface $Proxy.wrap("mp::test::Bar") {
+    destroy [@0](/bitcoin-core-multiprocess/contributor/0/) (context :Proxy.Context) -> ();
+    value [@1](/bitcoin-core-multiprocess/contributor/1/) (context :Proxy.Context) -> (result :Int32);
+}
+
 interface FooFn $Proxy.wrap("ProxyCallback<std::function<int()>>") {
     destroy [@0](/bitcoin-core-multiprocess/contributor/0/) (context :Proxy.Context) -> ();
diff --git a/test/mp/test/foo.h b/test/mp/test/foo.h
index 17aeeb8388..5c881117ce 100644
--- a/test/mp/test/foo.h
+++ b/test/mp/test/foo.h
@@ -60,13 +60,4 @@ public:
 };

-//! Concrete FooCallback that returns a fixed value, used by listCallbacks tests.
-class SimpleCallback : public FooCallback
-{
-public:
-    explicit SimpleCallback(int value) : m_value(value) {}
-    int call(int) override { return m_value; }
-    int m_value;
-};
-
 class ExtendedCallback : public FooCallback
 {
@@ -75,4 +66,27 @@ public:
 };

+//! A second, arbitrary interface used to test returning a
+//! list of interface objects.
+class Bar
+{
+public:
+    virtual ~Bar() = default;
+    virtual int value() = 0;
+};
+
+//! Concrete Bar that returns a fixed value, used by listBars tests.
+class SimpleBar : public Bar
+{
+public:
+    explicit SimpleBar(int value) : m_value(value) {}
+    int value() override { return m_value; }
+    int m_value;
+};
+
 class FooImplementation
 {
@@ -99,9 +113,9 @@ public:
     int passFn(std::function<int()> fn) { return fn(); }
     std::vector<FooDataRef> passDataPointers(std::vector<FooDataRef> values) { return values; }
-    std::vector<std::unique_ptr<FooCallback>> listCallbacks(int n)
+    std::vector<std::unique_ptr<Bar>> listBars(int n)
     {
-        std::vector<std::unique_ptr<FooCallback>> result;
+        std::vector<std::unique_ptr<Bar>> result;
         result.reserve(n);
-        for (int i = 0; i < n; ++i) result.push_back(std::make_unique<SimpleCallback>(i));
+        for (int i = 0; i < n; ++i) result.push_back(std::make_unique<SimpleBar>(i));
         return result;
     }
diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp
index 41ca506c62..41aa4d67fd 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -263,9 +263,9 @@ KJ_TEST("Call FooInterface methods")
     // BuildList with interface element types, which requires non-const iteration
     // so unique_ptr::release() can transfer ownership to the proxy server.
-    std::vector<std::unique_ptr<FooCallback>> callbacks{foo->listCallbacks(3)};
-    KJ_REQUIRE(callbacks.size() == 3u);
+    std::vector<std::unique_ptr<Bar>> bars{foo->listBars(3)};
+    KJ_REQUIRE(bars.size() == 3u);
     for (int i = 0; i < 3; ++i) {
-        KJ_REQUIRE(callbacks[i] != nullptr);
-        KJ_EXPECT(callbacks[i]->call(0) == i);
+        KJ_REQUIRE(bars[i] != nullptr);
+        KJ_EXPECT(bars[i]->value() == i);
     }
 }

re: #304 (review)

IIUC this isn't really a callback flow, so the terminology had me confused about the test. Might be better to add another (Bar) interface to make it more clear.

Yeah you are right. The server is returning callback objects in the sense of "you can call me back later by using these objects" not taking callback objects that it can use to call into the client later. So your example makes more sense and its better to avoid the callback term here.

re: #304 (review)

Applied suggested changes. I tried to do something a simpler adding a FooInterface method that returns a list of FooInterfaces, but it didn't work out because there's no C++ FooInterface class with virtual methods. It might make sense to add virtual methods and have separate FooInterface / FooImplementation classes which would make the test a little more realistic. But it would also require defining each method twice and overriding it, so seem better to keep current test setup.

In commit "proxy: fix BuildList to use non-const iteration for interface types" (a0b01d73698597293e48771cad2b4cb8e0ba16a0)

Note, while this change mostly restores behavior to what it was before #277, passing elem to BuildField below as an lvalue reference regardless of whether the container is lvalue or rvalue, it might make more sense to pass elem as an rvalue if the container is an rvalue, or as an lvalue if it's an lvalue with something like:

for (auto&& elem : value) {
    if constexpr (std::is_lvalue_reference_v<Value&&>) {
        BuildField(/*...*/, elem);
    } else {
        BuildField(/*...*/, std::move(elem));
    }
}

This could be safer because it could allow BuildField to distinguish based on the way the container is being used and only move from temporary containers that are about to be destroyed, not containers passed as references.

Also it might be better to replace for (auto&& elem : value) with for (auto it = elem.begin(); it != elem.end(); ++it) like the previous code had since this would could also preserve rvaluedness of dereferencing the iterator, if the iterator deferences as a proxy object instead of a reference like vector<bool>`

Which is just to say that a0b01d73698597293e48771cad2b4cb8e0ba16a0 is a minimal change restoring ability to return lists of interface pointers, but it might make sense to actually increase safety here as well.

re: #304 (review)

Latest push implements all these suggestions so rvaluedness is preserved and the code should be safer and potentially work more types. It is a little more verbose and complicated now, but I think not too bad.