-
Notifications
You must be signed in to change notification settings - Fork 252
Support custom Rust types for list
#1442
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
Conversation
b3b75d8 to
f41ad1f
Compare
|
Status (mostly as a reminder to myself):
|
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work for your use case with Bytes? Receiving a Bytes from the outside world would be Bytes: From<Vec<u8>> which is the best we can do, but sending a Bytes to the outside world would unnecessarily require a copy since the shared-reference of Bytes would be copied into a uniquely owned Vec<u8>
I'm not sure what shared reference you're talking about here. Just to make sure we're on the same page, the |
I was assuming this, but looking at the bytes crate. I'm not sure this is actually true. It sounds like it's capable of converting a I tried swapping out my |
|
I can ask perhaps a different question, are you looking for zero-copy when you pass a |
|
Sorry if I added to the confusion by looking into the bytes crate types. But my original intent wasn't specifically about the When I started implementing this I just figured there was no reason to limit this to straight newtype wrappers and support anything that impl |
|
At the risk of adding to the confusion, I decided to dig into what the bytes crate does in this case. As long as you're not doing anything 'interesting' with the When converting from Then, as long as the
Though there is a call to [Edit: Because I was curious, my assumption is essentially correct, at least on my system / version of glibc. As long as there's more than 512 bytes of data it'll do a check on But again, this PR isn't specifically about the bytes crate's I also pushed more test cases that I wrote to prove to myself that you weren't implying something else, so that's unrelated and I plan to remove the changes around non-canonical lists because that definitely does have a copying problem that I'm not sure how I'd solve. |
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok I see now makes sense, thanks for clarifying!
While this wasn't really the intended purpose of the with key I think it's ok to go ahead and support this. I don't see any problems with it for example per se, and it's a convenient feature to have too.
Thanks for the PR again! With green CI I'd be happy to merge
Any type that implements `From<Vec<T>>`, `Into<Vec<T>>`, `Deref<Target=Vec<T>>`, and `Clone` can be used for a WIT type `list<T>` via the `with` option. The `Deref` and `Clone` impls are only required when the type is used as part of a non-canonical type.
8368d74 to
f7634e9
Compare
| #[derive(Debug, Clone)] | ||
| pub struct NewtypedListOfByte(Vec<u8>); | ||
|
|
||
| impl From<Vec<u8>> for NewtypedListOfByte { | ||
| fn from(value: Vec<u8>) -> Self { | ||
| NewtypedListOfByte(value) | ||
| } | ||
| } | ||
|
|
||
| impl From<NewtypedListOfByte> for Vec<u8> { | ||
| fn from(value: NewtypedListOfByte) -> Self { | ||
| value.0 | ||
| } | ||
| } | ||
|
|
||
| impl Deref for NewtypedListOfByte { | ||
| type Target = Vec<u8>; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note Debug, Clone, and Deref are needed because NewtypedListOfBytes appears in a record and the generated code for records requires them. As far as I can tell, Clone isn't strictly necessary, the generated code just derives Clone on the struct. Commenting out the clone derive here doesn't break anything for me:
wit-bindgen/crates/rust/src/interface.rs
Line 2080 in f7634e9
| derives.insert("Clone".to_string()); |
Ideally that clone would be derived only if all the children implement it, but as far as I can tell the only way to do that requires a bit of a hack using some syntax that seems to be in stable by accident (as well as generating the clone impl without derive). Though this is getting a bit off topic...
|
I removed the changes around non-canonical lists and polished up the tests covering more generated code, making the naming more descriptive, and deduplicating a lot of things. I think this should be ready to go now. |
Any type that implements
From<Vec<T>>,Into<Vec<T>>andcan be used for a WIT typeDeref<Target = Vec<T>>list<T>via thewithoption.This was simpler than I originally expected, by omitting the type names, I can just let the compiler fall back to calling
<Vec<T> as Into<Vec<T>>::intowhen no custom type is used, allowing the codegen to not need to track the custom type.Resolves #1441