Skip to content

add Resource::take method to wit_bindgen::rt #753

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

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

dicej
Copy link
Collaborator

@dicej dicej commented Nov 10, 2023

This allows the guest to take back ownership of an exported resource from the host; you can think of it as the reverse of Resource::new. It's a bit awkward to do this for the time being;
WebAssembly/component-model#238 will improve the situation if accepted.

This allows the guest to take back ownership of an exported resource from the
host; you can think of it as the reverse of `Resource::new`.  It's a bit awkward
to do this for the time being;
WebAssembly/component-model#238 will improve the
situation if accepted.

Signed-off-by: Joel Dice <[email protected]>
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This is required and has been tested to work for the WASI-Virt upgrade path.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks! Looks reasonable to me and just a few cosmetic comments.

}

/// Takes back ownership of the object, dropping the resource handle.
pub fn take(resource: Self) -> T
Copy link
Member

Choose a reason for hiding this comment

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

Could this be rename to into_inner perhaps? The take terminology seems derivitive of Option<T> which is ideally only an implementation detail here.

@@ -250,7 +250,18 @@ impl<T: WasmResource> Resource<T> {
where
T: RustResource,
{
let _ = Box::from_raw(rep as *mut T);
let _ = Box::from_raw(rep as *mut Option<T>);
Copy link
Member

Choose a reason for hiding this comment

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

Would you be up for a small refactoring of this module? If so I think it'd be good to have something like

type RawRep<T> = Option<T>;

or something like that where all of these casts and various "raw" methods would work with RawRep<T>. That way future changes to RawRep<T> would help trigger compilation errors where appropriate. Basically I think it'd be good to centralize the representation of Option<T> if possible.

@@ -443,7 +443,7 @@ impl Bindgen for FunctionBindgen<'_, '_> {
.as_deref()
.unwrap()
.to_upper_camel_case();
format!("&*({op} as u32 as usize as *const {name})")
format!("Option::as_ref(&*({op} as u32 as usize as *const Option<{name}>)).unwrap()")
Copy link
Member

Choose a reason for hiding this comment

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

Mind adding a helper method to Resource<T> which is #[doc(hidden)] to call here? That way this could avoid duplicating knowledge of the internal representation across the code generator and the actual source

- add `type RawRep<T> = Option<T>` alias
- rename `Resource::take` to `Resource::into_inner`
- add `Resource::lift_borrow` and use it in code generator

Signed-off-by: Joel Dice <[email protected]>
@dicej
Copy link
Collaborator Author

dicej commented Nov 13, 2023

@alexcrichton thanks for the feedback; I just pushed and update. I wasn't sure about the scope of your RawRep suggestion; please let me know if there's more to it than what I did in the latest commit.

@alexcrichton
Copy link
Member

👍

@alexcrichton alexcrichton merged commit 5f90361 into bytecodealliance:main Nov 13, 2023
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