-
Notifications
You must be signed in to change notification settings - Fork 235
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 a wasmparser -> wasm-encoder trait #1628
Conversation
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.
How should user errors be handled?
I think this'll be best guided by using this module elsewhere. I think it's ok as-is and the introduction of using this in other locations can help figure out how best, if at all, to return custom errors.
Should the utility methods be monomorphised or use dynamic dispatch?
I think as-is is fine, we can see how bad the blowup is, if at all, later on.
Add documentation (I'd appreciate some help with this)
Happy to help, if you wouldn't mind moving missing_docs
to just the trait I think that should be a reasonable balance for now. No need to document all the trait methods.
@alexcrichton Thanks for your feedback! I've tried to address most of it and made some changes. Feel free to make further tweaks on this branch as I'll now be in my weekend :) |
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.
I've pushed up a few minor changes, but I'd like to also poke at this a bit more to see if internally wasm-tools can use this in a few places.
Thanks for the review and for pushing the fixes! That sounds good to me and would help with checking if the design works well in practice. Have a good weekend! |
I did some quick testing of how this would work with my usecase and my code is a lot cleaner now :) There are two aspects I'd like to still address:
|
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.
I'm not sure if I'm sold on the user-configurable error here but I'm happy to try it out and see how it works. I was hoping that this might be possible as an adapter like ReencodeWithError<R: Reencode>
where the outer type would store an error to get returned later (or something like that), but that's a bit wonky too.
Well ... I guess ... that's possible too. A user reencoder could store an optional error inside itself and check that afterwards. If we don't go for a user error I'll use that, but it's not my preferred API (though I'm very open to variants of the current one) |
Yeah sorry I don't mean to say that I don't like |
I am very happy that we're discussing alternatives. I must say that I also don't love that the error has to be that invasive (an associated type default would slighty help hide it better), so I'm really interested how the error type now works for you in practice for using it inside wasm-tools, and how we can perhaps improve this first design form it
I absolutely agree :D |
@alexcrichton Do you want to explore this PR a bit more to see how existing code can be cleaned up and are there some other changed / improvements that I can make? |
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.
I think this is good to go as-is so I'm going to flag for merge and it can be iterated more on in-tree as necessary over time too.
Thank you sooo much for your help with designing, prototyping, and improving this API and for your valuable feedback! |
And thank you for pushing on this and helping to iterate on it! |
Progress towards #1588, follow-up to #1627
Thanks to @alexcrichton for coming up with the design!
This PR provides a new module,
reencoder
, which contains theWasmParserToWasmEncoder
trait with utility functions to convert betweenwasmparser
andwasm-encoder
types. Every default implementation is also provided as a free function in thereencoder::utils
module. In this PR, only types that are needed to re-encode core WASM modules are included, but future PRs could extend this.This PR also does not yet replace similar functionality in other wasm-tools crates, e.g. the
wasm_mutate::mutators::translate::Translator
trait, which could in follow-up PRs be cleaned up to use this general-purpose re-encoder infrastructure instead.Utility conversion functions added in #1627 are now implemented in terms of the
RoundtripReencoder
all-default implementation of theWasmParserToWasmEncoder
. While most interfaces stay the same (and #1627 has not been published yet), some methods now returnResult
s.Unresolved questions and TODOs
reencoder::Error
enum and make every method fallible if that's preferable.