feat(rust): Adds the ability to specify custom derive attributes #678
Conversation
|
I am pretty sure the test failure is a flake or not related to my code as I am seeing it in other PRs as well |
There was a problem hiding this comment.
Thanks for this! No worries on the failed test, that's known to be flaky and should be resolved on main soon through ignoring it.
At a high level one concern I have about this is that it won't work for Resource types. That won't implement Serialize/Deserialize and doesn't currently implement PartialEq/Hash, although it perhaps could. This means that implementing traits uniformly for all types may not be sufficient and eventually this may need a form of "ignore this type" or "only derive for this type". That all being said I think it's fine to deal with those situations as they come up later, for now this is suitable and works well so I'd say we should land it and then iterate on further syntaxes later if necessary.
| @@ -159,7 +160,7 @@ pub trait RustGenerator<'a> { | |||
| param_mode: TypeMode, | |||
| sig: &FnSig, | |||
| ) -> Vec<String> { | |||
| let params = self.print_docs_and_params(func, param_mode, &sig); | |||
| let params = self.print_docs_and_params(func, param_mode, sig); | |||
There was a problem hiding this comment.
No worries for this PR, but for future contributions I'd recommend splitting out clippy changes into a separate commit or possibly PR. Makes the "meat" elsewhere a bit easier to review that way.
crates/rust-lib/src/lib.rs
Outdated
| ) { | ||
| let info = self.info(id); | ||
| // We use a hash set to make sure we don't have any duplicates | ||
| let additional_derives: HashSet<String> = additional_derives.into_iter().collect(); |
There was a problem hiding this comment.
One important caveat to be aware of about HashSet<T> is that its iteration order is not defined and not deterministic between runs of a process. This means, for example, that different code is generated from the same WIT source as derives might be shuffled around.
Mind using a BTreeSet (or IndexSet) here instead to preserve order?
There was a problem hiding this comment.
Oh right, I should have thought about the ordering issue. I'll switch that around
There was a problem hiding this comment.
This should be all resolved, but leaving the comment unresolved until you have a chance to take a look at it
I also did a little bit of cleanup of clippy warnings while I was in these files. Closes bytecodealliance#554 Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
thomastaylor312
force-pushed
the
feat/custom_derive_attr
branch
from
4cc9460 to
715b15a
Compare
|
@alexcrichton Should be ready to go for a rereview In regards to this:
I completely agree with the path forward here. I was curious how this would interact with resources, but it definitely seemed easiest to add this to struct/enum types for now and then we can see if there is a way to make it more specific in the future |
|
Looks great to me, thanks! One option is to use the |
|
does this work in |
|
@sehz I was actually going to go open an issue over there in cargo component |
I also did a little bit of cleanup of clippy warnings while I was in these files.
Closes #554