fix: remove unsafe code in resolve_error

[brice-stacks]

No description provided.

[Acaccia-stacks]

Not convinced here. We are using an assumption that wasmtime::Error implementation will not change. We have no guarantee it's the case.
The unsafe code is tried and true and will remain correct if Wasmtime changes its error implementation.

[brice-stacks]

I think the unsafe version also relies on that assumption. We'd really prefer not to use unsafe unless it is necessary, like in loading the wasm module. At least an issue with this version would cause a compile-time error, not strange runtime behavior.

[Acaccia-stacks]

There wouldn't be more of a compile-time error actually, both root_cause and downcast are part of anyhow, so both would have a compilation error in this case. My bad, I thought downcast was part of std::error.

[Acaccia-stacks]

However, with the new code, if the error becomes something like
anyhow::Error::new(UnknowNewErr) where UnknownNewErr::source() is &VmExecutionError,
downcast won't be able to find the VmExecutionError, while root_cause.downcast_ref would.

However, it seems that root_cause.downcast_ref would fail if one day, VmExecutionError's source isn't a None anymore... :D

[brice-stacks]

How about we add this after the downcast check?

        debug_assert!(
            !e.root_cause().is::<VmExecutionError>(),
            "VmExecutionError reachable via root_cause() but not downcast(): {e:?}"
        );

[Acaccia-stacks]

Yup, sounds great!
Why not just assert! though? I don't like debug_assert! because it creates different execution paths between debug and release...

[brice-stacks]

I used the debug_assert! to avoid any overhead in release builds, since this is not something that we need to check every time, it should either fail in tests, or never fail at all.

[Acaccia-stacks]

@brice-stacks , a little follow-up on our discussion yesterday, where I said we could avoid the unsafe code if we could make VmExecutionError implement Clone.
It doesn't seem obvious to me how we could do it easily, since some WasmError wrap the wasmtime errors, which are acutally anyhow errors, which cannot be cloned...

[brice-stacks]

@brice-stacks , a little follow-up on our discussion yesterday, where I said we could avoid the unsafe code if we could make VmExecutionError implement Clone. It doesn't seem obvious to me how we could do it easily, since some WasmError wrap the wasmtime errors, which are acutally anyhow errors, which cannot be cloned...

Bummer. Ok, I'll come back to this and investigate a bit more.