Stream: git-wasmtime

Topic: wasmtime / issue #1056 VerifierStepResult is confusing

Wasmtime GitHub notifications bot (May 04 2022 at 20:37):

cfallin labeled issue #1056:

The verifier functions have an API that could probably be simplified. Functions which return VerifierStepResult must by convention also take an out-param VerifierErrors that will contain non-fatal errors.

Moreover, the T in VerifierStepResult<T> seems to always be set to (), so it's unused.

It seems the out param is redundant with the error that's present in the Result hidden VerifierStepResult. I think slightly modifying the interface of VerifierStepResult would avoid this out param:

• let the Ok type be VerifierErrors, in case we only have non-fatal errors (they could be called "warnings").
• let the Err type stay the same (VerifierErrors too), and contain non-fatal and fatal errors, if there was at least one fatal error.

Then we wouldn't need the errors outparam anymore, which looks cleaner and "more Rusty". It might mean that a few users of the fatal! etc macros would need to have their own errors variable, but that seems OK.

Thoughts?

Wasmtime GitHub notifications bot (May 04 2022 at 20:37):

cfallin labeled issue #1056:

The verifier functions have an API that could probably be simplified. Functions which return VerifierStepResult must by convention also take an out-param VerifierErrors that will contain non-fatal errors.

Moreover, the T in VerifierStepResult<T> seems to always be set to (), so it's unused.

It seems the out param is redundant with the error that's present in the Result hidden VerifierStepResult. I think slightly modifying the interface of VerifierStepResult would avoid this out param:

• let the Ok type be VerifierErrors, in case we only have non-fatal errors (they could be called "warnings").
• let the Err type stay the same (VerifierErrors too), and contain non-fatal and fatal errors, if there was at least one fatal error.

Then we wouldn't need the errors outparam anymore, which looks cleaner and "more Rusty". It might mean that a few users of the fatal! etc macros would need to have their own errors variable, but that seems OK.

Thoughts?

Wasmtime GitHub notifications bot (Oct 05 2024 at 20:38):

aidenfoxivey commented on issue #1056:

@cfallin This issue seems finished - could it be a candidate for being closed?

Wasmtime GitHub notifications bot (Oct 05 2024 at 20:43):

cfallin commented on issue #1056:

I don't think the core issue is actually resolved at all; and in the linked PR it mentions that it resolved a subpart of the problem only. This could still be a good starter-issue for someone wanting to do some cleanups.

Wasmtime GitHub notifications bot (Oct 05 2024 at 20:52):

aidenfoxivey commented on issue #1056:

I don't think the core issue is actually resolved at all; and in the linked PR it mentions that it resolved a subpart of the problem only. This could still be a good starter-issue for someone wanting to do some cleanups.

Ah neat, I see. I'll give it a read. :)

Wasmtime GitHub notifications bot (Oct 29 2025 at 12:45):

bahbah94 commented on issue #1056:

Hi, I would like to contribute to this.
Where exactly does this originate, i see that the T param has been removed from the src/verifier/mod.rs
it seems i need to modify the type of VerifierResult, if this is the correct approach, are there tests suits to run to check correctness

Wasmtime GitHub notifications bot (Oct 29 2025 at 13:01):

bahbah94 commented on issue #1056:

If im not wrong
#[inline] pub fn as_result(&self) -> VerifierStepResult { if self.is_empty() { Ok(()) } else { Err(()) } }
the core problem lies here, where its returning Ok(()) and respectively Err(()). ---- > perhaps we should we return OK(self)/Err(self)

Wasmtime GitHub notifications bot (Nov 06 2025 at 17:32):

fitzgen closed issue #1056:

The verifier functions have an API that could probably be simplified. Functions which return VerifierStepResult must by convention also take an out-param VerifierErrors that will contain non-fatal errors.

Moreover, the T in VerifierStepResult<T> seems to always be set to (), so it's unused.

It seems the out param is redundant with the error that's present in the Result hidden VerifierStepResult. I think slightly modifying the interface of VerifierStepResult would avoid this out param:

• let the Ok type be VerifierErrors, in case we only have non-fatal errors (they could be called "warnings").
• let the Err type stay the same (VerifierErrors too), and contain non-fatal and fatal errors, if there was at least one fatal error.

Then we wouldn't need the errors outparam anymore, which looks cleaner and "more Rusty". It might mean that a few users of the fatal! etc macros would need to have their own errors variable, but that seems OK.

Thoughts?

Wasmtime GitHub notifications bot (Nov 06 2025 at 17:32):

fitzgen commented on issue #1056:

Hey @bahbah94, this issue is pretty old and I don't think there's too much we need to do here actually. The verifier code has aged pretty well and been extended as necessary without problems, so I don't think it is worth spending time on this one.

If you are still looking for something Cranelift-y to do, I'd suggest #11971. Thanks!

Wasmtime GitHub notifications bot (Nov 08 2025 at 11:28):

bahbah94 commented on issue #1056:

@fitzgen Thanks for the reply. I'll have a look at #11971.

Last updated: Dec 06 2025 at 06:05 UTC