So there's been a few (internal and Bugzilla) threads about this, so maybe we should step back and try to understand what's the problem to solve here.
The current implementation of wasmTextToBinary is in C++ and we in the Spidermonkey team have written it, so we know how it works and we can trust it. It's slightly more complicated than it ought to be, because of historical reasons: at some point we had wasmBinaryToText, and having the AST representation helped sharing code then, but now it might be slightly overkill. Having our own implementation also helps with adding our own syntax, as has been done for the GC proposal.
So the requirements for wasmTextToBinary that I can think of would be:
- it's possible to easily add new syntax, like the GC proposal. I suspect we may have a lot of our tests with some custom syntax that's never been official, so using a different wasmTextToBinary would either mean updating our test suite or including our quirks in the new parser.
- something we can trust, not especially in terms of security since this is only for testing, but in terms of having the right binary representation. Also this comment shows that wasmTextToBinary is used by Spidermonkey embedders.
Some nice-to-have but less strong requirements:
- something that's easy to understand and hack on;
- not having an external dependency (which requires synchronization, and/or maintaining our own patches if we want to add custom behaviors);
Andy was having issues with implementing multi-values, because our AST nodes assume only one result is possible per AST node, and this would require some important refactoring, hence the idea to have something else.
In the realm of possibilities, something else could be:
- replacing wasmTextToBinary entirely with something else already existing. I don't know much about wabt/wassemble to tell how they would match (3) in the above list, but using something external in general would require us to maintain our own patches and sync with upstream, which is not ideal. Maybe wassemble's only goal is to be used in Spidermonkey, in which case it wouldn't be external anymore.
- or write our own replacement. Dan had talked about wanting to write a new Rust parser for Cranelift/Wasmtime at some point, since we're depending on wabt for Cranelift to parse WAT text cases. Since it's now possible to add Rust dependencies to the JS engine, this new parser could also be used in Spidermonkey. That would still be an external dependency, though.
- or adding support for supplementary parsers in addition to wasmTextToBinary. This sounds like a maintenance burden, and having duplicate code just for the sake of testing, possibilities to run out of sync between the two parsers, making it complicated to choose among one or the other. Not ideal either.
My opinion right now is that it would just be simpler to fix wasmTextToBinary, simplify it if we feel the need to, and keep on using it. In particular, the only con I've got against it is for point (1), as adding support for multi-value shows. But every other solution implies having to deal with every other item in the list of requirements, which looks like it could mean much more work in the long run.
Lars has already commented about the fact that a new parser be written in JS. Curious to know what other people's thoughts are about this topic.