Implement a wasmparser -> wasm-encoder trait

[juntyr]

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 the WasmParserToWasmEncoder trait with utility functions to convert between wasmparser and wasm-encoder types. Every default implementation is also provided as a free function in the reencoder::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 the WasmParserToWasmEncoder. While most interfaces stay the same (and #1627 has not been published yet), some methods now return Results.

Unresolved questions and TODOs

• How should user errors be handled? At the moment, many trait methods cannot error, and the others can only return pre-defined errors. We could add an extra variant to the reencoder::Error enum and make every method fallible if that's preferable.
• Should the utility methods be monomorphised or use dynamic dispatch?
• Add documentation (I'd appreciate some help with this)

[alexcrichton]

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.

[juntyr]

@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 :)

[alexcrichton]

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.

[juntyr]

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!

[juntyr]

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:

• user error handling has now come up for me - I'll submit a PR for that later (I think I'll give the error a generic parameter that is infallible by default)
  • should other (all) methods also be allowed to return an error
  • unfortunately we cannot provide a from impl for the user type, but maybe we could add a helper function for a double-from (takes any type that can be converted into the user error)
• inserting sections when they don't exist in the original - in my case, I always need an import section. While I can detect the case when no import section is present, I don't have access to the module at the point. I'd like to figure out a way for the reencode-a-full-module-helper to support this case. Any ideas?

[alexcrichton]

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.

[juntyr]

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)

[alexcrichton]

Yeah sorry I don't mean to say that I don't like Error<E>, only that it might be worth considering something else. That being said I agree ergonomically this is probably better so let's stick with Error<E>

[juntyr]

Yeah sorry I don't mean to say that I don't like Error<E>,

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

only that it might be worth considering something else.

I absolutely agree :D

[juntyr]

@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?

[alexcrichton]

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.

[juntyr]

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!

[alexcrichton]

And thank you for pushing on this and helping to iterate on it!