Stream: git-wasmtime

Topic: wasmtime / Issue #1753 impl From<anyhow::Error> for Trap

Wasmtime GitHub notifications bot (May 25 2020 at 19:18):

github-actions[bot] commented on Issue #1753:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (May 26 2020 at 14:32):

alexcrichton commented on Issue #1753:

Thanks for this! I'm a little wary to do it, however, because it's a lossy conversion. The original error object contains important contextual information (like the backtrace, list of error contexts, etc), and that all gets lost on conversion into a Trap. One thing we could consider, however, is to refactor the internals of Trap to have a variant of its representation where it stores an anyhow::Error perhaps?

One thing that'd also be great is if we could support anything that's E: Error or otherwise support other error types via generics rather than just anyhow::Error

Wasmtime GitHub notifications bot (May 26 2020 at 15:16):

leoyvens commented on Issue #1753:

Doing a better conversion is out of my depth, but I'd expect that those improvements can be made backwards compatibly. Right now this is the best that can be done with the public API, so this is just making the public API more ergonomic.

I didn't make this generic over E: Error because I didn't want to create any backwards compatibility hazard wrt to coherence, but I can do so if that's preferred.

Wasmtime GitHub notifications bot (May 27 2020 at 15:50):

alexcrichton commented on Issue #1753:

We've already got an enum TrapReason, so how about doing something like this:

impl<T> From<T> for Trap where T: Into<anyhow::Error> {
    // ...
}

enum TrapReason {
    // ...
    Error(anyhow::Error),
}

impl Error for Trap {
    fn source(&self) { /* ... */ }
}

We can figure out later if it's necessary to expose the underlying error's original type, but otherwise for now that will at least sidestep issues with Display vs Debug and we'll be able to get a structure stack of causal errors.

Wasmtime GitHub notifications bot (May 27 2020 at 18:13):

sunfishcode commented on Issue #1753:

FWIW, there are a few existing places that could use that too:

$ rg 'Trap::new\(format!\("\{:\?\}"'
crates/wasmtime/src/linker.rs
410:                        Err(error) => Trap::new(format!("{:?}", error)),

crates/c-api/src/error.rs
14:        Box::new(wasm_trap_t::new(Trap::new(format!("{:?}", self.error))))

Wasmtime GitHub notifications bot (May 27 2020 at 22:20):

leoyvens commented on Issue #1753:

The suggested impl<T> From<T> for Trap where T: Into<anyhow::Error> conflicts with the reflexive impl impl<T> std::convert::From<T> for T since Trap itself implements Error. I did add a conversion Box<dyn std::error::Error + Send + Sync>.

Based on the suggestions given and the uses in the project, I added a Error(Box<dyn std::error::Error + Send + Sync>) variant to TrapReason which allowed for some nice refactorings.

Trap::message is confusing to me, imo along with Display and Debug there are too many ways of converting a Trap to a string. But I kept it, though the return type had to be changed to an owned String.

Wasmtime GitHub notifications bot (May 27 2020 at 22:38):

github-actions[bot] commented on Issue #1753:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (May 29 2020 at 20:24):

alexcrichton commented on Issue #1753:

This all looks great to me, thanks again for this!

Last updated: Nov 22 2024 at 16:03 UTC