bytecodealliance / bytecodealliance/wasm-tools
`wasmparser`: Make `VisitOperator` more user friendly
- Dominant language
- Rust
- Stars
- 1.8k
- Forks
- 351
- Avg merge
- 16h 57m
- Merged PRs (30d)
- 38
Description
At the moment the new `VisitOperator` trait requires users to implement each and every of the over 500 variants.
This quickly becomes a mundane task, especially if there are only a few of the `visit` methods that are of importance to a user.
## Idea 1
One idea is to introduce another default implemented method `visit_fallback` or `visit_default` (name not final) which acts similar to a default arm in a `match` expression. All `visit_*` methods would be default implemented to forward to this new `visit_fallback` method and the `visit_fallback` itself would be default implemented with something generic such as `unimplemented!()`.
```rust
pub trait VisitOperator {
type Output;
fn visit_fallback(&mut self, offset: usize) -> Self::Output {
unimplemented!()
}
fn visit_unreachable(&mut self, offset: usize) -> Self::Output {
self.visit_fallback(offset)
}
fn visit_nop(&mut self, offset: usize) -> Self::Output {
self.visit_fallback(offset)
}
// etc...
}
```
### Advantages
This design would allow users to incrementally implement the `VisitOperator` trait and also would allow to only define small subsets of the `VisitOperator` trait API surface. An example where this could be useful is the `wasmprinter` crate which simply prints `newline` for most of the operators and could make heavy use of the `visit_fallback` method there.
### Downsides
The huge downside to this design is that it is no longer clear to users if they have forgotten to implement a variant. Also if `wasmparser` is updated an added new variants users are no longer informed about this. I think this is a rather big disadvantage and was wondering how we could maybe find a solution that still informs users that want to take care about each and every `visit_*` method themselves.
---
## Idea 2
We could introduce yet another `VisitOperator`-like trait. Let's call it `VisitOperatorFallback`.
It is very similar to the original `VisitOperator` trait in that it houses a visit method for each Wasm operator.
The difference to the original `VisitOperator` trait is that it has this new `visit_fallback` method and all the aforementioned default implementations.
Users can decide to either implement the original `VisitOperator` or `VisitOperatorFallback` trait.
In case they implement the `VisitOperatorFallback` (or just subsets of it due to its default impls) then the type also receive an auto implementation of `VisitOperator` that simply forwards to the methods of the `VisitOperatorFallback`.
```rust
pub trait VisitOperatorFallback {
type Output;
fn visit_fallback(&mut self, offset: usize) -> Self::Output {
unimplemented!()
}
fn visit_unreachable(&mut self, offset: usize) -> Self::Output {
self.visit_fallback(offset)
}
fn visit_nop(&mut self, offset: usize) -> Self::Output {
self.visit_fallback(offset)
}
// etc...
}
impl VisitOperator for T
where
T: VisitOperatorFallback,
{
type Output = ::Output;
fn visit_unreachable(&mut self, offset: usize) -> Self::Output {
::visit_unreachable(self, offset)
}
fn visit_nop(&mut self, offset: usize) -> Self::Output {
::visit_nop(self, offset)
}
// etc...
}
```
### Advantages
With this design we have the same set of advantages as with solution 1 if users decide to implement `VisitOperatorFallback` trait.
If users only care about a subset of the visit methods they can implement `VisitOperatorFallback`. If they care about all the visit methods they can implement `VisitOperator` trait and still be updated about changes properly as with the original design.
### Disadvantages
We now require 2 massive traits instead of just 1. However, we could use `macro_rules` or proc. macros to generate them to keep them in sync.
Contributor guide
Assessment
This issue has not been assessed yet.