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>
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 ofTrap
to have a variant of its representation where it stores ananyhow::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 justanyhow::Error
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.
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
vsDebug
and we'll be able to get a structure stack of causal errors.
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))))
leoyvens commented on Issue #1753:
The suggested
impl<T> From<T> for Trap where T: Into<anyhow::Error>
conflicts with the reflexive implimpl<T> std::convert::From<T> for T
sinceTrap
itself implementsError
. I did add a conversionBox<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 toTrapReason
which allowed for some nice refactorings.
Trap::message
is confusing to me, imo along withDisplay
andDebug
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 ownedString
.
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>
alexcrichton commented on Issue #1753:
This all looks great to me, thanks again for this!
Last updated: Dec 23 2024 at 12:05 UTC