Skip to content

[cpp] Overhaul resource ownership and record passing #1327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

TartanLlama
Copy link
Contributor

@TartanLlama TartanLlama commented Jun 26, 2025

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 alexcrichton requested a review from cpetig June 27, 2025 14:42
@alexcrichton
Copy link
Member

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.

Copy link
Collaborator

@cpetig cpetig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +98 to +109
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;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@@ -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_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

}

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

/// 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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would coarse and fine also make sense for Rust?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

cpetig commented Jun 28, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants