Skip to content

anyhow instead of error_chain #2721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 30, 2021
Merged

Conversation

rbtcollins
Copy link
Contributor

@rbtcollins rbtcollins commented Apr 14, 2021

Drop error_chain which is no longer supported, evolving, causes code deprecation warnings, forces everything into one surfaced massive enum and generally is breaking our brains. Use anyhow! for most errors, and ThisError for errors used multiple times or for which we do need to match on.

@rbtcollins rbtcollins force-pushed the anyhow branch 3 times, most recently from 9d5c60a to d9ad312 Compare April 22, 2021 14:14
@rbtcollins rbtcollins changed the title anyhow anyhow instead of error_chain Apr 26, 2021
@rbtcollins rbtcollins marked this pull request as ready for review April 26, 2021 21:28
@kinnison
Copy link
Contributor

I have started to review this but I think there's something squiffy about the commit series because the delta github shows me includes things definitely already merged to master.

Could you please rebase this and clean up the commit series to just be your changes?

anyhow provides significantly better error tooling than error_chain,
without the derivation support, thiserror fills that gap.
SyncError provides adaption to wrap around error_chain, both where we
use it, and if any libraries rustup is using use it. Once introduced we
can start incrementally migrating function and error at at time to
anyhow without requiring a single big bang commit.
@rbtcollins rbtcollins force-pushed the anyhow branch 2 times, most recently from fe5077f to 8add447 Compare April 30, 2021 04:49
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

There are some inconsistencies in use of downcast() vs downcast_ref() in a few places, and a few extra .clone()s which might be avoidable (though they're in test paths)

I've highlighted one or two things, but otherwise I'm up for merging this modulo you having a final pass for happiness and tweaking the few items I mention.

@rbtcollins
Copy link
Contributor Author

downcast vs downcast_ref - depends on whether the error is being rethrown or consumed generally: I wouldn't call it inconsistent so much as having different contexts. Of course, spread over a year some inconsistency can always occur

The main UI impact is that using the anyhow formatters removes some
custom code at a slight UI change - rather than repeating the err! macro
output around each item in the backtrace, we get it just once (see the
comment in common.rs).
@kinnison
Copy link
Contributor

If the light is green, the trap is clean!

@rbtcollins rbtcollins merged commit 7a051f7 into rust-lang:master Apr 30, 2021
@rbtcollins rbtcollins deleted the anyhow branch April 30, 2021 18:46
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