[cpp] Overhaul resource ownership and record passing #1327

TartanLlama · 2025-06-26T08:46:29Z

This PR makes several core changes to the way that resources and records are managed in the C++ bindings. If there are any design changes you disagree with, we can work out a middle ground!

Ownership configuration

Introduces three resource ownership configurations for passing resources to imports:

Owning: Generated types will be composed entirely of owning fields
Coarse borrowing: Generated types used as parameters to imports will contain reference types (e.g. std::string_view), unless the record contains an owning handle, in which case the entire record will contain owning types
Fine borrowing: Generated types used as parameters to imports will contain reference types for all fields that are not owning handles

Owning is the default, as it is the easiest to use. Coarse borrowing is intended for uses where you want to avoid unnecessary allocation at the binding boundary. Fine borrowing is for when you really need to avoid those allocations, even if your record contains references, and you're willing to accept that the types become harder to reason about (because only the resource will be moved).

In the case of coarse borrowing and fine borrowing, a <TypeName>Param type will be unconditionally generated for each defined record type. This is different from the Rust bindings, which conditionally generates <TypeName>Result and <TypeName>Param, and takes a flag that enables or disables this behaviour. I figured that with three base types of ownership configuration, the duplicate-if-necessary options would add unnecessary complication.

See the following tests for examples of the three ownership types (it seems that these got into the last PR I made, whoops):

Reference types for record parameters to imports

Currently, imports with record parameters unconditionally take value types, which makes the API harder to use for some use cases, e.g. passing a record containing a wit::vector that you want to keep the contents of.

This PR changes the parameter types of record parameters in a way that is dependent on the ownership configuration chosen:

Ownership configuration	`T` contains an owning handle	`T` does not contain an owning handle
Owning	`T&&`	`T const&`
Coarse borrowing	`T&&`	`TParam const&`
Fine borrowing	`TParam&&`	`TParam const&`

This means that, if the type contains a resource, the user must std::move it, but otherwise they don't, and they don't incur a copy at the binding API boundary.

To support this, std::spans in import arguments are std::span<T> if the type contains an owning handle and std::span<T const> otherwise.

Namespaces of types

Currently there is no differentiation between imported types and exported types. This becomes a problem when resources are stored in records, because we have to store different types for imported and exported records.

This PR unconditionally puts exported types into the exports namespace, separating them from imported types. I believe this follows the behaviour of the Rust bindings.

Borrow handles in records

Borrow handles are now correctly handled when they're stored in records, as previously they were treated as if they were in canonical form already.

There's still some work that needs done on resources inside result, option, variant, and tuple, which will come in a future PR

alexcrichton · 2025-06-27T14:42:56Z

Requesting review from @cpetig, but @cpetig by no means feel pressured to review this. If you're busy please feel free to defer to me and I can take over.

cpetig

I really like the general direction of this patch, especially as it adds something which I have been planning to do for some time.

But I wonder whether the choice to move enums/records/typedefs into the same namespace as the exported functions/resources stems from not having a test which needs them unified/compatible between import and export.

Because I recall implementing the additional logic to satisfy that test case.

cpetig · 2025-06-28T13:35:02Z

crates/cpp/helper-types/wit.h

+  char* begin() {
+    return (char*)data_;
+  }
+  char* end() {
+    return (char*)data_ + length;
+  }
+  char const* begin() const {
+    return (char const*)data_;
+  }
+  char const* end() const {
+    return (char const*)data_ + length;
+  }


cpetig · 2025-06-28T13:37:10Z

crates/cpp/helper-types/wit.h

@@ -113,7 +125,7 @@ template <class T> class vector {
  vector &operator=(vector const &) = delete;
  vector &operator=(vector &&b) {
    if (data_ && length>0) {
-      free(const_cast<uint8_t *>(data_));
+      free(data_);


I agree with the direction of this change I just recall that wit::vector<const T> would trigger this, if I recall correctly. Probably the cause is removed by this new ownership design.

Ah, I see where that might be hit. I think it would be better to generate a const wit::vector<T> instead of a wit::vector<const T>

cpetig · 2025-06-28T13:38:52Z

tests/runtime/records/test.cpp

@@ -1,30 +1,32 @@
 #include <assert.h>
 #include <test_cpp.h>

-std::tuple<uint8_t, uint16_t> exports::test::records::to_test::MultipleResults() {
+namespace test_exports = ::exports::test::records::to_test;


cpetig · 2025-06-28T13:45:02Z

tests/runtime/results/intermediate.cpp

 }

-std::expected<float, ::test::results::test::E> exports::test::results::test::EnumError(float a) {
-    auto result = ::test::results::test::EnumError(a);
+std::expected<float, test_exports::E> test_exports::EnumError(float a) {


This is the part I am less convinced about: That EnumError should be in exports.

Update: It should be, but once a type (record, variant, enum) is both in imports and exports, exports should only contain an alias, not a redefinition, to keep them compatible.

cpetig · 2025-06-28T14:57:31Z

crates/cpp/src/lib.rs

@@ -28,8 +29,6 @@ pub const RESOURCE_EXPORT_BASE_CLASS_NAME: &str = "ResourceExportBase";
 pub const RESOURCE_TABLE_NAME: &str = "ResourceTable";
 pub const OWNED_CLASS_NAME: &str = "Owned";
 pub const POINTER_SIZE_EXPRESSION: &str = "sizeof(void*)";
-// these types are always defined in the non-exports namespace
-const NOT_IN_EXPORTED_NAMESPACE: bool = false;


I can't find the test case this functionality aimed at, but there was a test case where the same interface was imported and exported by a world and enums, typedefs and structs should only be declared once to be compatible across imported and exported interface definitions.

Once we know how this works with the new ownership code, I am happy to get rid of this strange behavior.

I guess I simply need to create a test ;-)

Test was merged as of #1328

cpetig · 2025-06-28T15:02:05Z

crates/cpp/src/lib.rs

+    /// Generated types used as parameters to imports will be "deeply
+    /// borrowing", i.e. contain references rather than owned values
+    /// for all fields that are not resources, which will be owning.
+    FineBorrowing,


I feel this is similar enough to the Rust command line parameter to benefit from unification across these languages, but I can't perfectly map this proposal to Owning vs Borrowing(copy_as_needed)

Would coarse and fine also make sense for Rust?

The current Rust behaviour under Borrowing is equivalent to the Coarse mode here. I'm not familiar enough with Rust's lifetime rules to say whether the Fine mode would make sense for Rust. Passing a record type containing a resource to an import under the Fine mode is like a partial move of the record, where only the resources are moved.

cpetig · 2025-06-28T15:10:32Z

Because I recall implementing the additional logic to satisfy that test case.

This spring Alex has rewritten the test infrastructure to no longer require a separate host side tester - making all of testing much simpler.

cpetig · 2025-07-17T21:27:05Z

I don't want perfect to become the enemy of the good with this patch. It clearly fixes some cases while not solving the export vs import aliasing perfectly.

I feel we should merge this with #1331 and get all tests fixed - perhaps this helps with fixing some more codegen tests, perhaps it creates more work to get all compiling again - but merging without the guardrail in CI is a risk.

cpetig · 2025-07-17T21:32:11Z

@alexcrichton How do you feel about this MR? My review got delayed by personal organizational load not from general disagreement with this patch.

cpetig · 2025-07-17T21:36:14Z

crates/cpp/src/lib.rs

@@ -2095,48 +2147,6 @@ impl<'a, 'b> FunctionBindgen<'a, 'b> {
            operands[0]
        );
    }
-
-    fn has_resources2(&self, ty: &Type) -> bool {
-        match ty {


@TartanLlama I recall that we chatted about requiring call by value for arguments to imported functions whenever there is a resource inside. Is this included in this MR or a separate one, because it feels like this code might become handy there.

cpetig

Clearly a move in the right direction although not covering all edge cases, yet.

alexcrichton · 2025-07-17T21:54:13Z

If you're happy I'm happy, so let's merge!

TartanLlama added 12 commits June 26, 2025 08:46

Use value types for asymmetric API

833c84f

Correct test arguments

9bfd0ee

Update docs

4d82caf

Format

89878de

Review feedback

b432127

Implement different ownership rules

f809b1b

Fix rebase errors

103a132

Remove old file

8077194

Revert wit::string changes

e9e8f01

format

dd39693

Fix borrowed handles in lists

8120c93

Formatting

e31121e

alexcrichton requested a review from cpetig June 27, 2025 14:42

cpetig reviewed Jun 28, 2025

View reviewed changes

cpetig mentioned this pull request Jun 28, 2025

[test] sharing types between import and export #1328

Merged

cpetig reviewed Jul 17, 2025

View reviewed changes

cpetig approved these changes Jul 17, 2025

View reviewed changes

alexcrichton added this pull request to the merge queue Jul 17, 2025

Merged via the queue into bytecodealliance:main with commit 24eebf7 Jul 17, 2025
19 checks passed

[cpp] Overhaul resource ownership and record passing #1327

[cpp] Overhaul resource ownership and record passing #1327

Uh oh!

Conversation

TartanLlama commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ownership configuration

Reference types for record parameters to imports

Namespaces of types

Borrow handles in records

Uh oh!

alexcrichton commented Jun 27, 2025

Uh oh!

cpetig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpetig commented Jun 28, 2025

Uh oh!

cpetig commented Jul 17, 2025

Uh oh!

cpetig commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpetig left a comment

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

TartanLlama commented Jun 26, 2025 •

edited

Loading