Skip to content
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

Implement WitType, WitLoad and WitStore for pointer types #3063

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

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Dec 19, 2024

Motivation

It's not uncommon to want to store some values in the heap, but still be able to copy them to and from guest Wasm modules. For that to work, heap smart pointer types should implement the WitType, WitLoad and WitStore traits.

Proposal

Create a new macro with the code to delegate implement the traits by delegating the items to the wrapped type, and use the macro to implement the traits for Box, Rc, and Arc.

Test Plan

Unit tests were modified to wrap all tested types in Arc, Rc and Box containers and check that it doesn't affect the loading and storing of the types.
Unit tests were added to check derived WitType, WitLoad and WitStore implementations for types that have Arc, Rc and Box fields.

Release Plan

  • These changes introduce a backwards compatible feature, so they follow the usual release cycle.

Links

@jvff jvff added the enhancement New feature or request label Dec 19, 2024
@jvff jvff added this to the Mainnet milestone Dec 19, 2024
@jvff jvff requested a review from afck December 19, 2024 13:59
@jvff jvff self-assigned this Dec 19, 2024
type Dependencies = T::Dependencies;

fn wit_type_name() -> ::std::borrow::Cow<'static, str> {
T::wit_type_name()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not show the wrapper as well ($wrapper<T::wit_type_name()>)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the wrapper types aren't part of the WIT standard :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but that leads me to a different thought. This should be empty, and the T should be a dependency 🤔 I'll fix it, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I was wrong. The problem is that if I do that, then the same wit_type_name is used for two different types (T and Box<T>), and they end up having different wit_type_declarations, which is a problem when generating the .wit files. So I had to revert to make Box<T> identical to T for it to work.

jvff added 19 commits December 20, 2024 18:51
Prepare to reuse code to implement the traits for smart pointer types.
Ensure values in the heap can be copied to and from guest Wasm modules.
Prepare to test more things in the helper function.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the fields are handled as if the field was not boxed.
Ensure that the fields are handled as if the field was not boxed.
Ensure that the fields are handled as if the field was not boxed.
Ensure values in the heap can be copied to and from guest Wasm modules.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the fields are handled as if the field was not boxed.
Ensure values in the heap can be copied to and from guest Wasm modules.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the type is treated exactly the same as if the type was not
wrapped.
Ensure that the fields are handled as if the field was not boxed.
@jvff jvff force-pushed the implement-wit-traits-for-pointer-types branch from b69c53c to 532da70 Compare December 20, 2024 18:51
@jvff
Copy link
Contributor Author

jvff commented Dec 20, 2024

I updated the PR to add tests, but I'll add wrapped slices support in a separate PR because it will also require a few new tests.

@deuszx deuszx self-requested a review December 20, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants