VerifierStepResult is confusing

[bnjbvr]

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?

[sunfishcode]

@6A Do you have any thoughts here?

[71]

@bnjbvr's suggestion does feel more "rusty" (and cleaner), but two details bother me with it:

1. Now we have to basically return a Result<Vec, Vec>, which is not as lightweight as I intended.
2. If we have multiple non-fatal errors from say, five function calls in a row, that means we have to merge five different Vecs, which is likely not as efficient as what we do right now.

We would also have to take care of merging each vector, which would lead to this kind of code:

let a = verify_foo()?;
let b = verify_bar()?;
a.extend(&mut b);
let c = verify_baz()?;
a.extend(&mut c);

IIRC I also wanted to get rid of the T, but for some reason we kept it. If we don't see a problem with the deletion, then I'm fine with removing it.

[aidenfoxivey]

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

[cfallin]

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.

[aidenfoxivey]

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. :)

[bahbah94]

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

[bahbah94]

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)

[fitzgen]

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!

[bahbah94]

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