Skip to content

Migrate from failure to thiserror and anyhow#436

Merged
sunfishcode merged 4 commits intobytecodealliance:masterfrom
joshtriplett:unfailure
Nov 5, 2019
Merged

Migrate from failure to thiserror and anyhow#436
sunfishcode merged 4 commits intobytecodealliance:masterfrom
joshtriplett:unfailure

Conversation

@joshtriplett
Copy link
Copy Markdown
Member

@joshtriplett joshtriplett commented Nov 3, 2019

The failure crate invents its own traits that don't use std::error::Error (because failure predates certain features added to Error). (For instance, if you have a function returning just Result<SomeType, Box<dyn Error>>, you can't use ? on a failure::error::Error in such a function.) The thiserror and anyhow crates use the new support in std::error::Error, so they interoperate with other usage of the standard Error trait.

Would there be interest in migrating from failure to anyhow (and from failure_derive to thiserror), if someone provided a pull request doing so?

@sunfishcode
Copy link
Copy Markdown
Member

Overall this sounds good.

My one concern is no_std support, which anyhow doesn't yet have. Wasmtime itself doesn't fully support no_std yet, but it is something we're considering. So as long as core-error is a viable path to anyhow supporting no_std in the future, that's good enough for now.

@joshtriplett joshtriplett changed the title Migrate from failure to anyhow? Migrate from failure to thiserror and anyhow Nov 3, 2019
@joshtriplett
Copy link
Copy Markdown
Member Author

joshtriplett commented Nov 3, 2019

I've made this a pull request showing what the progress looks like. This shouldn't be merged until a cranelift release containing the corresponding migration takes place, at which point I can change the git rev references in the Cargo.toml files back to cranelift version numbers.

This PR carefully does not attempt to semantically change or refactor the approach to error-handling in any portion of the code, to ensure that the change remains straightforward to review. Modules using specific differentiated error types move from failure_derive and derive(Fail) to thiserror and derive(Error). Modules boxing all errors opaquely move from failure::Error to anyhow. Modules using String as an error type continue to do so. Code using unwrap or expect continues to do so.

The second and third commits make some minor further improvements based on this, which I noticed while working on this change. The second avoids duplicating error messages in wasmtime that already exist in cranelift. The third takes advantage of anyhow's Debug implementation printing the chain of causes.

Note that in cranelift-debug, I've avoided migrating away from failure, because it depends heavily on the faerie crate which still uses and exposes failure::Error. I'd be happy to migrate that one too (it'll just have many calls to .map_err(|e| e.compat()), or we can leave it until we migrate away from faerie to avoid changing it twice, or we can migrate faerie if that's an option.

The failure crate invents its own traits that don't use
std::error::Error (because failure predates certain features added to
Error); this prevents using ? on an error from failure in a function
using Error. The thiserror and anyhow crates integrate with the standard
Error trait instead.

This change does not attempt to semantically change or refactor the
approach to error-handling in any portion of the code, to ensure that
the change remains straightforward to review. Modules using specific
differentiated error types move from failure_derive and derive(Fail) to
thiserror and derive(Error). Modules boxing all errors opaquely move
from failure::Error to anyhow. Modules using String as an error type
continue to do so. Code using unwrap or expect continues to do so.

Drop Display implementations when thiserror can easily derive an
identical instance.

Drop manual traversal of iter_causes; anyhow's Debug instance prints the
chain of causes by default.

Use anyhow's type alias anyhow::Result<T> in place of
std::result::Result<T, anyhow::Error> whenever possible.
handle_module in wasm2obj manually maps
cranelift_codegen::isa::LookupError values to strings, but LookupError
values already have strings that say almost exactly the same thing.
Rely on the strings from cranelift.
The main() wrapper around rmain() completely matches the behavior of
question-mark-in-main (print error to stderr and return 1), so switch to
question-mark-in-main.
@joshtriplett
Copy link
Copy Markdown
Member Author

Updated for the cranelift 0.47 release.

Copy link
Copy Markdown
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Both crates switched from failure to anyhow; updating lets us avoid a
translation from failure to anyhow within wasmtime-interface-types.
@sunfishcode sunfishcode merged commit 56ce6e9 into bytecodealliance:master Nov 5, 2019
alexcrichton added a commit to alexcrichton/faerie that referenced this pull request Nov 11, 2019
This commit takes a similar migration path as proposed in
bytecodealliance/wasmtime#436 and shares much of the same motivation. The
intention here was originally spawned from using `anyhow` everywhere
inside of `wasmtime`!
alexcrichton added a commit to alexcrichton/faerie that referenced this pull request Nov 11, 2019
This commit takes a similar migration path as proposed in
bytecodealliance/wasmtime#436 and shares much of the same motivation. The
intention here was originally spawned from using `anyhow` everywhere
inside of `wasmtime`!
m4b pushed a commit to m4b/faerie that referenced this pull request Nov 12, 2019
This commit takes a similar migration path as proposed in
bytecodealliance/wasmtime#436 and shares much of the same motivation. The
intention here was originally spawned from using `anyhow` everywhere
inside of `wasmtime`!
coriolinus added a commit to exercism/rust that referenced this pull request Oct 1, 2020
The `failure` crate is getting old, and has largely been superseded
in the ecosystem by the `anyhow` crate, which better uses the standard
library (and which only became possible with Rust v1.34). We should
demonstrate more modern techniques to students.

See:

- bytecodealliance/wasmtime#436
- https://blog.yoshuawuyts.com/error-handling-survey/
- https://internals.rust-lang.org/t/failure-crate-maintenance/12087
coriolinus added a commit to exercism/rust that referenced this pull request Oct 5, 2020
* Grep: replace failure crate with anyhow

The `failure` crate is getting old, and has largely been superseded
in the ecosystem by the `anyhow` crate, which better uses the standard
library (and which only became possible with Rust v1.34). We should
demonstrate more modern techniques to students.

See:

- bytecodealliance/wasmtime#436
- https://blog.yoshuawuyts.com/error-handling-survey/
- https://internals.rust-lang.org/t/failure-crate-maintenance/12087

* rustfmt tests file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants