Move from `error-chain` to `thiserror` #394

pwoolcoc · 2020-03-23T14:19:25Z

This is a first attempt at removing error-chain and using thiserror instead.

A couple notes:

error-chain sets up an Error/ErrorKind pattern, while thiserror seems to work better with a single Error enum. Some of the API choices I made in this PR are a result of this difference, I tried to keep the call sites similar as much as possible.
When rebasing, the Cargo.lock had quite a few conflicts. Instead of risking trying to resolve them manually, I deleted the lock file and re-created it. Apologies, that seems to have caused quite a large diff for that file.

Closes #344

ordian

Thanks for the PR!

I've left some comments, but will take a closer look later this week.

Cargo.toml

src/bin/add/main.rs

src/bin/upgrade/main.rs

ordian · 2020-03-23T14:52:33Z

src/errors.rs

+    #[error("{0}")]
+    Custom(String),
+
+    /// Wraps another error in order to provide more context


hmm, is it a common way of doing this thing?
I see that https://github.com/dtolnay/anyhow for example has a context function for this. Maybe thiserror has something similar?

Unless I'm missing something, thiserror allows you to add source for an error variant by one of three ways:

prepending the field with #[from],

prepending the field with #[source], or

naming the field source

In all cases, the source of the error variant is kept in the variant itself, and I can't find any way to add this source information alongside the Error enum in the same way that error-chain did with the Error/ErrorKind setup. I could rename the enum to be ErrorKind and have an Error struct that stores the ErrorKind and a source, but then we are basically reimplementing error-chain by hand and we lose the benefits of using thiserror since we won't be able to take advantage of the code generation that it does to allow us to convert those source errors to our Error enum.

It just feels like a something is not right here, either:

thiserror is not the right tool for the job, or

we should redesign the error types exposed in the library

and I'll need more time to think about this

I've got an idea about this that I'll try out today

Thinking a little more about this, I think using a combination of thiserror and anyhow might be the right way to go. thiserror is good for defining the error types for the library and deriving the Error trait on them, and anyhow seems perfect for working with them within the application itself. I'll give that a try and see how it turns out

any updates on this? (also needs merging with master)

If we need to add some context to each error, maybe the error type should be

struct Error { pub error: ErrorKind, context: &'static str, }

or something along the lines, or we could add more variants of errors to the lib.

yea that's what I'm in the process of moving to. sorry I haven't been more responsive, I'm still working on this and will hopefully have an update for you tomorrow

no worries :) take your time and thank you for the PR!

pwoolcoc · 2020-04-07T00:26:34Z

@ordian ok, I think this might be the way to go. I've kept the thiserror enum but return values all use anyhow::Result. This seems to be a good mix between library-specific errors & adding ad-hoc context to an error

ordian

So here we're keeping the lib Error type, but all functions return anyhow::Result. Is a user expected to downcast the error to the lib Error? What are other possible values of the error type besides &'static str?
TBH I'm not convinced this is an improvement. I would reassess what kind of static string errors we return and maybe create enum variants for them. Also not sure how with_context messages are helpful in the lib.

Curious what @killercup thinks about this.

src/metadata.rs

ordian · 2020-04-07T09:20:27Z

src/bin/add/main.rs

-        if let Some(backtrace) = err.backtrace() {
-            eprintln!("Backtrace: {:?}", backtrace);


is it not possible to keep backtraces with anyhow? :(

anyhow can display backtraces as long as it was complied by nightly, since the stdlib backtrace feature is still unstable. I've tried to convince failure::Fail::backtrace to accept an anyhow::Error and try to get a backtrace out of it, but haven't had any luck. anyhow::Error will deref to a dyn Error + Send + Sync + 'static, but Fail::backtrace complains that it is unsized.

I've added a feature that will allow a user to compile with backtrace support as long as they are on nightly, but if there is a better way I'd be more than happy to use it.

@killercup are backtraces used for identifying bugs in cargo-edit if it fails to run on some input? maybe it's not that helpful for the users after all

I could have sworn that anyhow brought in a dependency on backtrace-rs on stable but apparently not. I don't mean to shamelessly plug my own stuff but if you want stable backtraces you can swap from anyhow to eyre which is a fork that adds support for custom context types, which you could define to include a backtrace::Backtrace

Losing backtraces is kind of a shame from a technical standpoint, but I don't think they are being relied on by anyone using cargo-edit, because we have such amazing error descriptions! 😉

Kidding aside, I think this is fine. When we finally end up packaging binaries we can compile them with nightly and get some backtraces for free.

@yaahc, I just got what the name is about. Amazing. I'm also totally fine with using your fork. Or switching to it in a later PR.

killercup · 2020-04-07T11:30:45Z

This is quite the diff, I'll try to read it sometime today.

Also cc @yaahc for errors in a small-but-in-production code base :)

yaahc · 2020-04-07T18:01:54Z

I'm not really clear on which part is the library and which part isn't, assuming everything in src/bin is the bin and everything else is lib?

So here we're keeping the lib Error type, but all functions return anyhow::Result. Is a user expected to downcast the error to the lib Error?

If you're going to immediately erase the error type then there is very little value to using thiserror. For library errors if you intend for the errors to be handleable by the caller then you almost certainly shouldn't use anyhow, if you still need context errors then what you want is a higher level error type that contains the root cause error types so you can just do map_err(HigherError::RelevantVariant).

pwoolcoc · 2020-04-07T19:22:37Z

thanks @ordian @killercup @yaahc , I think I know where to go with it.

@yaahc

As @yaahc pointed out, if we immediately erase the error type by converting it to `anyhow::Error` then there's not much reason to have a library error type. This commit puts _all_ errors generated in the library into the Error enum and removes use of `anyhow` from the library completely. Note that `anyhow` is still used in the 3 binaries.

pwoolcoc · 2020-04-07T20:13:23Z

r? @ordian (and @yaahc if you are available!)

ordian · 2020-04-07T20:38:40Z

src/errors.rs

+
+    /// Failed to parse a version for a dependency
+    #[error("The version `{0}` for the dependency `{1}` couldn't be parsed")]
+    ParseReqVersion(String, String, #[source] semver::ReqParseError),
+
+    /// Failed to parse a version for a dependency
+    #[error("The version `{0}` for the dependency `{1}` couldn't be parsed")]
+    ParseVersion(String, String, #[source] semver::SemVerError),
+
+    /// An invalid crate version requirement was encountered
+    #[error("Invalid crate version requirement")]
+    InvalidCrateVersionReq(#[source] semver::ReqParseError),
+
+    /// Unable to get crate name from a URI
+    #[error("Unable to obtain crate informations from `{0}`.\n")]
+    ParseCrateNameFromUri(String),
+
+    /// Unable to get a crate from a git repository
+    #[error("Failed to fetch crate from git")]
+    FetchCrateFromGit(#[source] reqwest::Error),
+
+    /// Received an invalid response from a git repository
+    #[error("Git response not a valid `String`")]
+    InvalidGitResponse(#[source] std::io::Error),
+
+    /// Could not read manifest contents
+    #[error("Failed to read manifest contents")]
+    ManifestReadError(#[source] std::io::Error),
+
+    /// Could not parse Cargo.toml
+    #[error("Unable to parse Cargo.toml")]
+    ManifestParseError(#[source] Box<Error>),
+
+    /// Cargo.toml contained invalid TOML
+    #[error("Manifest not valid TOML")]
+    ManifestInvalidToml(#[source] toml_edit::TomlError),
+
+    /// Could not found Cargo.toml
+    #[error("Failed to find Cargo.toml")]
+    ManifestNotLocated(#[source] std::io::Error),
+
+    /// Could not get cargo metadata
+    #[error("Failed to get cargo file metadata")]
+    GetCargoMetadata(#[source] std::io::Error),
+
+    /// Could not get current directory
+    #[error("Failed to get current directory")]
+    GetCwd(#[source] std::io::Error),
+
+    /// Could not set output colour
+    #[error("Failed to set output colour")]
+    SetOutputColour(#[source] std::io::Error),
+
+    /// Could not write upgrade message
+    #[error("Failed to write upgrade message")]
+    WriteUpgradeMessage(#[source] std::io::Error),
+
+    /// Could not clear output colour
+    #[error("Failed to clear output colour")]
+    ClearOutputColour(#[source] std::io::Error),
+
+    /// Could not write upgraded versions
+    #[error("Failed to write upgrade versions")]
+    WriteUpgradeVersions(#[source] std::io::Error),
+
+    /// Could not print upgrade message
+    #[error("Failed to print upgrade message")]
+    PrintUpgradeMessage(#[source] std::io::Error),
+
+    /// Could not truncate Cargo.toml
+    #[error("Failed to truncate Cargo.toml")]
+    TruncateCargoToml(#[source] std::io::Error),
+
+    /// Could not write updated Cargo.toml
+    #[error("Failed to write updated Cargo.toml")]
+    WriteUpdatedCargoToml(#[source] std::io::Error),
+
+    /// Missing Version Field
+    #[error("Missing version field")]
+    MissingVersionField,
+
+    /// Could not write new manifest contents
+    #[error("Failed to write new manifest contents")]
+    WriteNewManifestContents(#[source] Box<Error>),
+
+    /// Could not open Cargo.toml
+    #[error("Unable to open local Cargo.toml")]
+    OpenLocalManifest(#[source] Box<Error>),
+
+    /// Git repo URL seems incomplete
+    #[error("Git repo url seems incomplete")]
+    IncompleteGitUrl,
+
+    /// Could not parse git repo URL
+    #[error("Unable to parse git repo URL")]
+    ParseGitUrl,
+
+    /// Found a virtual manifest instead of an actual manifest
+    #[error("Found virtual manifest, but this command requires running against an  actual package in this workspace. Try adding `--workspace`.")]
+    VirtualManifest,


oh boy, I didn't expect that many variants of errors 😅

I understand that on one hand we want to provide as much details about the error as possible, but on the other hand, users of this library won't need to differentiate between SetOutputColour and WriteUpgradeMessage. IMHO it's fine to shrink all io errors into one, all toml parse errors into one, etc.

🤣 yea, agreed that it's a lot. I've cut it way down in my latest commit, let me know what you think

ordian · 2020-04-08T20:41:12Z

src/errors.rs

+    /// A ReqParseError from the semver crate
+    #[error(transparent)]
+    SemVerParse(#[from] semver::ReqParseError),
+    /// A SemVerError from the semver crate
+    #[error(transparent)]
+    SemVer(#[from] semver::SemVerError),


can we merge these into one?

Unfortunately the source errors are completely separate types so we'd either lose those or have to cast them to trait objects or something

ordian · 2020-04-08T20:41:36Z

src/errors.rs

+    /// Unable to get a crate from a git repository
+    #[error("Failed to fetch crate from git")]
+    FetchCrateFromGit(#[source] reqwest::Error),


and this into Reqwest error

ordian · 2020-04-08T20:42:12Z

src/errors.rs

+    /// Unable to find the specified resource
+    #[error("The resource `{0}` could not be found.")]
+    NotFound(String),
+
+    /// Unable to find the specified resource
+    #[error("The resource `{0}` could not be found in `{1}`.")]
+    NotFoundIn(String, String),


these to also look similar

I split them up because the second error message is helpful in giving a location for the error, i.e. it ends up looking like "the resource serde could not be found in dev-dependencies", and I liked having that location information.

ordian · 2020-04-08T20:42:23Z

src/errors.rs

+    /// Failed to parse a version for a dependency
+    #[error("The version `{0}` for the dependency `{1}` couldn't be parsed")]
+    ParseReqVersion(String, String, #[source] semver::ReqParseError),
+
+    /// Failed to parse a version for a dependency
+    #[error("The version `{0}` for the dependency `{1}` couldn't be parsed")]
+    ParseVersion(String, String, #[source] semver::SemVerError),


ordian · 2020-04-08T20:44:33Z

src/errors.rs

+    /// A value was expected but we got `None` instead
+    #[error("Expected a value")]
+    ExpectedValue,


The previous name ReadHomeDirFailure was more descriptive, maybe we should remove it and map the err to an io error?

ordian · 2020-04-08T20:49:53Z

src/bin/add/main.rs

-        if let Some(backtrace) = err.backtrace() {
-            eprintln!("Backtrace: {:?}", backtrace);


@killercup are backtraces used for identifying bugs in cargo-edit if it fails to run on some input? maybe it's not that helpful for the users after all

killercup · 2020-04-09T11:05:34Z

Error handling is a hot topic in Rust right now -- like very spring, it seems -- and I love the discussion here, this is good stuff.

I have two more points. Don't read them as must-haves but just as random suggestions.

Looking at this huge error enum, would it make sense to instead split this up in an hierarchical fashion, so pretty much each module/file has its own Error type, and all the function would return Result<T, OurLocalError>? I've done this in some code bases at work and it was fine when it was just one or two layers of nesting.

The main advantages are that the errors are more local and easier to read, and that the cause stack is very explicit. Also, since it's public enums, a consumer of the library (incl. us in the code for the binaries) can actually match on them. The main downside is that it's a large refactoring for no directly visible gain.
Can we use #[non_exhaustive] on our public error types?

pwoolcoc · 2020-04-20T17:58:35Z

sorry for the radio silence, I'm still working on this and will try to update here as soon as I can

pwoolcoc · 2020-10-27T19:41:34Z

I think I'm going to close this...I've tried a couple more things since the last time I updated this PR and haven't been happy with any of them and I don't think I'm going to come back to it. Sorry!

pwoolcoc changed the title ~~Issue 344 switch to thiserror~~ Move from error-chain to thiserror Mar 23, 2020

ordian reviewed Mar 23, 2020

View reviewed changes

Paul Woolcock added 7 commits April 1, 2020 20:15

Move cargo-edit lib to thiserror

9aa7e86

Use thiserror for cargo-add

09e3b4f

use thiserror for cargo-rm

3e2a6d9

use thiserror for cargo-upgrade

2a8e63c

Remove leading :: from import paths

650f4f7

change these loops to while-lets

ddebe9d

Add anyhow to crate lib

c1acd7c

Paul Woolcock added 4 commits April 6, 2020 20:32

Add anyhow to cargo-add

373bbb6

Add anyhow to cargo-rm

d18e7a6

Add anyhow to cargo-upgrade

4f04abe

rustfmt pass

87290d8

ordian reviewed Apr 7, 2020

View reviewed changes

Reduce the number of possible errors

2760f26

ordian reviewed Apr 8, 2020

View reviewed changes

pwoolcoc closed this Oct 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from `error-chain` to `thiserror` #394

Move from `error-chain` to `thiserror` #394

pwoolcoc commented Mar 23, 2020 •

edited

Loading

ordian left a comment •

edited

Loading

ordian Mar 23, 2020

pwoolcoc Mar 25, 2020 •

edited

Loading

ordian Mar 25, 2020

pwoolcoc Mar 26, 2020

pwoolcoc Mar 26, 2020

ordian Mar 30, 2020

ordian Apr 1, 2020

pwoolcoc Apr 1, 2020

ordian Apr 1, 2020

pwoolcoc commented Apr 7, 2020

ordian left a comment

ordian Apr 7, 2020

pwoolcoc Apr 7, 2020

ordian Apr 8, 2020

yaahc Apr 8, 2020 •

edited

Loading

killercup Apr 9, 2020

killercup commented Apr 7, 2020

yaahc commented Apr 7, 2020

pwoolcoc commented Apr 7, 2020

pwoolcoc commented Apr 7, 2020

ordian Apr 7, 2020

pwoolcoc Apr 8, 2020

ordian Apr 8, 2020

pwoolcoc Apr 9, 2020 •

edited

Loading

ordian Apr 8, 2020

ordian Apr 8, 2020

pwoolcoc Apr 9, 2020

ordian Apr 8, 2020

ordian Apr 8, 2020

ordian Apr 8, 2020

killercup commented Apr 9, 2020

pwoolcoc commented Apr 20, 2020

pwoolcoc commented Oct 27, 2020

		if let Some(backtrace) = err.backtrace() {
		eprintln!("Backtrace: {:?}", backtrace);

Move from error-chain to thiserror #394

Move from error-chain to thiserror #394

Conversation

pwoolcoc commented Mar 23, 2020 • edited Loading

ordian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwoolcoc Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwoolcoc commented Apr 7, 2020

ordian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaahc Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup commented Apr 7, 2020

yaahc commented Apr 7, 2020

pwoolcoc commented Apr 7, 2020

pwoolcoc commented Apr 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwoolcoc Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup commented Apr 9, 2020

pwoolcoc commented Apr 20, 2020

pwoolcoc commented Oct 27, 2020

Move from `error-chain` to `thiserror` #394

Move from `error-chain` to `thiserror` #394

pwoolcoc commented Mar 23, 2020 •

edited

Loading

ordian left a comment •

edited

Loading

pwoolcoc Mar 25, 2020 •

edited

Loading

yaahc Apr 8, 2020 •

edited

Loading

pwoolcoc Apr 9, 2020 •

edited

Loading