bytecodealliance / wasm-tools

CLI and Rust libraries for low-level manipulation of WebAssembly modules
Apache License 2.0
1.3k stars 234 forks source link

Added serde support to wit-encoder #1647

Closed MendyBerger closed 2 months ago

MendyBerger commented 3 months ago

This PR add serde support to wit-encoder.

Why is wit-parser's serde not enough?

alexcrichton commented 2 months ago

One of the reasons I was hesitant to do this originally is that this would add a second JSON representation of a WIT package which is subtly incompatible with the wit-parser JSON representation and I was afraid of confusion that might cause. Could your use case be adapted to use the JSON representation of wit-parser instead perhaps? For example adding Deserialize in addition to Serialize shouldn't be too too hard. As for id-vs-name to me that can be seen as an upside too because it avoids the need to perform name resolution itself, but I do agree it's more difficult to track.

MendyBerger commented 2 months ago

The main reason I need this is to allow adding arbitrary wit to an existing wit package. If there are references by id, that breaks that ability. I'm trying to make that hand writable. Having IDs there would make it error prone.

I definitely get the problem with having multiple json representations. How open would you be to only having the json from wit-encoder? I'm working on a tool that converts wit-parser's resolve into a list of wit-encoder packages. That would mean that we can take a resolve, convert it into wint-encoder, and get the json from the wit-encoder.

As for adding Deserialize to wit-parser: I don't think that you can build up an arena from json.

Thoughts @alexcrichton?

alexcrichton commented 2 months ago

I would prefer to not change the "main" json representation from what it is today because using an id-based encoding makes it much less error-prone to interpret the JSON output. For example it doesn't require all consumers to perform their own name resolution and such. Instead a directly usable form of the AST is processed.

I'd personally prefer to push on adding custom deserialization/etc to be able to deserialize from JSON. That may require changes in id-arena but I suspect that would be reasonable to do upstream as well.

MendyBerger commented 2 months ago

@alexcrichton if that's the only way forward, how would you suggest that we deal with adding arbitrary wit to an existing package, without having to update the ids constantly. We need this ability for wasi-webgpu.

alexcrichton commented 2 months ago

I don't know the specifics of your use case so it's hard for me to propose a precise solution to it. I agree that modifying the JSON output of wit-parser would not be easy. I don't really understand how JSON fits into your workflow because the naive answer I'd have to adding custom WIT would be to add more *.wit files adjacent to the original WIT files, but if JSON is in the mix I don't know how that affects things

MendyBerger commented 2 months ago

Having separate *.wit files, won't let us augment existing interfaces.

MendyBerger commented 2 months ago

Thanks @alexcrichton for the help and guidance with this!