bytecodealliance / bytecodealliance/wasm-tools

Impossible to implement `VisitOperator::simd_visitor` generically

Open
#2,181 5 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1.8k
Forks
351
Avg merge
16h 57m
Merged PRs (30d)
38

Description

I have a snippet of code similar to this in my code base, after I made changes to upgrade to a more recent version of wasm-tools:

```rust
pub(crate) struct JoinVisitor(pub(crate) L, pub(crate) R);

macro_rules! join_visit {
($( @$proposal:ident $op:ident $({ $($arg:ident: $argty:ty),* })? => $visit:ident ($($ann:tt)*))*) => {
$(fn $visit(&mut self $($(,$arg: $argty)*)?) -> Self::Output {
(self.0.$visit($($($arg.clone()),*)?), self.1.$visit($($($arg),*)?))
})*
}
}

impl<'a, L, R> wasmparser::VisitOperator<'a> for JoinVisitor
where
L: wasmparser::VisitOperator<'a>,
R: wasmparser::VisitOperator<'a>,
{
type Output = (L::Output, R::Output);
wasmparser::for_each_visit_operator!(join_visit);
}

impl<'a, L, R> wasmparser::VisitSimdOperator<'a> for JoinVisitor
where
L: wasmparser::VisitSimdOperator<'a>,
R: wasmparser::VisitSimdOperator<'a>,
{
wasmparser::for_each_visit_simd_operator!(join_visit);
}
```

But currently this seems to still fail to visit SIMD opcodes if I'm calling `JoinVisitor::visit_operator`. From the looks of it that's because there is a new function I need to implement `VisitOperator::simd_visitor`.

However I cannot implement this method in a generic-preserving way. If I do the naive thing

```rust
impl<'a, L, R> VisitOperator<'a> for JoinVisitor
where
L: VisitOperator<'a> + VisitSimdOperator<'a>,
R: VisitOperator<'a> + VisitSimdOperator<'a>,
{
fn simd_visitor(&mut self) -> Option<...> { Some(self) }
}
```

then I will have this code build, but the `JoinVisitor: VisitOperator` will now require an implementation of `VisitSimdOperator`. As far as I can tell there is no way to construct this such a way that `simd_visitor` returns `Some` only if `L + R: VisitSimdOperator`. An obvious

```rust
fn simd_visitor(
&mut self,
) -> Option<&mut dyn wasmparser::VisitSimdOperator<'a, Output = Self::Output>>
where
L: wasmparser::VisitSimdOperator<'a>,
R: wasmparser::VisitSimdOperator<'a>,
{
Some(self)
}
```

won't work because this method has stricter requirements than the trait being `impl`d, nor will this:

```rust
fn simd_visitor(
&mut self,
) -> Option<&mut dyn wasmparser::VisitSimdOperator<'a, Output = Self::Output>> {
Some(JoinVisitor(self.0.simd_visitor()?, self.1.simd_visitor()?))
}
```

due to the signature requiring `&mut dyn VisitSimdOperator` to be returned.

Besides this particular snag, `simd_visitor` is also a plain footgun in that implementing a `VisitSimdOperator` is not all that's required on each of the implementors to support visiting SIMD operators, and the failure mode is a *runtime* error.

---

In the world where we know we'll only have `VisitOperator` and `VisitSimdOperator` this could be addressed by adding `visit_operator` to `VisitSimdOperator` as well (which would delegate to its supertrait's `visit_operator` if it isn't a SIMD opcode,) but that won't work out nicely if future delivers another `Visit*Operator` that's unrelated to `VisitSimdOperator`.

The only other way to address this that I can see is to add a new associated type to `VisitOperator` along the lines of `type SimdVisitor` that `simd_visitor` would return. Defaulting to `Infallible` or something.

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.