`RefType::new` returning an option is inconvenient

[nagisa]

I’m migrating a crate from a wasmparser 0.99 to 0.104 and there are significant changes around the ValType, introduced as part of https://github.com/bytecodealliance/wasm-tools/pull/701.

One of the things that I’ve been doing is something along the lines of:

impl VisitOperator<'a> for ... {
    fn visit_ref_null(&mut self, t: ValType) -> Self::Output {
        self.values.push(t);
        Ok(())
    }
    ...
}

thus effectively tracking the types that are on the stack. In order to adapt to the new API, I’m looking at something like this instead:

impl VisitOperator<'a> for ... {
    fn visit_ref_null(&mut self, t: HeapType) -> Self::Output {
        self.values.push(ValueType::Ref(RefType::new(true, t)));
        Ok(())
    }
    ...
}

but now there's a need to put in an unwrap in there or to handle the None that's returned by RefType::new in some other way. To me it seems somewhat counterintuitive that it is necessary to jump through hoops like these, given that I’ve already have in my hands something that has been successfully parsed.

[nagisa]

Looking at the code it seems that if visit_ref_null argument was replaced with RefType, then it would no longer be able to support the entire range of function type indices…

(FWIW I’d be happy to work on fixing this, but some suggestions on the approach to take would be appreciated – e.g. can we make RefType support all the indices?)

[alexcrichton]

Out of curiosity is this a context where a validator is present? If so the type information should (at least in theory) be available from the validator where you shouldn't have to maintain your own separate stack.

On the other than though this is sort of inherent given the representation of RefType. If you're not validating then RefType can't represent any u32-based index contained in a HeapType, meaning that something's gotta happen. One option would perhaps be to push the must-be-at-most-20-bits requirement into HeapType so parsing fails here instead of reaching the visit_ref_null method, though.

If there's a validator on-hand, though, then I think ideally that would be consulted for the type stack instead of having to need to keep a duplicate stack for that.

[nagisa]

Unfortunately the code I’m working with does not involve validation and is built to work with modules that wouldn't necessarily pass validation (largely for performance reasons, not as a functional requirement.)

[alexcrichton]

Ah ok, that makes sense. Does your usage have affodance for unreachable code, and if so could an unrepresentable heap type be considered roughly equivalent to unreachable? Otherwise though I think there's basically not a great way of handling this today