Should we put a log::trace! before every unwrap_validated()?

[wucke13]

No description provided.

[florianhartung]

Instead of having a log::trace! before every unwrap_validated(), I think we should do the trace! call inside the unwrap_validated() method. The caller then has to provide a message (or reason) for why the unwrap is safe.

A possible implementiation for unwrap_validated could be:

pub fn unwrap_validated(self, reason: impl Display) {
    log::trace!("Unwrapping Option because of prior validation. Reasoning: {msg}");
    self.unwrap_or_else(||
        panic!("Validation guarantees this to be `Some(_)`, but it is `None`. Reason this should have been safe to unwrap: {reason}")
    )
}

Here is also an example of how this could be called:

GLOBAL_GET => {
    let global_idx = wasm.read_var_u32(
        .unwrap_validated("The GLOBAL_GET instruction should be followed by a 32-bit integer.") as GlobalIdx;
    let global = store.globals.get(global_idx)
        .unwrap_validated("The global index a GLOBAL_GET instruction should be valid");
    stack.push_value(global.value.clone());
}

However, now the naming unwrap_validated does not make sense anymore because usually unwrap methods don't take any arguments.

[wucke13]

However, now the naming unwrap_validated does not make sense anymore because usually unwrap methods don't take any arguments.

unwrap_or takes an argument. I think having this particular thing is not an issue at all.

[florianhartung]

I feel like unwrap_or should not be able to panic. I suggest fn expect_validated(reason: &str) instead.