bytecodealliance / bytecodealliance/wasm-tools

`wasmparser`: Make `VisitOperator` more user friendly

Open
#713 12 comments 0 reactions 0 assignees View on GitHub
wasmparser
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.