Migrate to error-chain

[alexcrichton]

Cargo's long since had its own error handling story, but this is embodied best today in the community-norm crate of error-chain. Cargo should use error chain instead of an ancient, not-quite-as-ergonomic solution!

[alexcrichton]

cc @brson

[jluner]

I'm interested in giving it a shot. Is there an obvious first place to start? crates-io seems self-contained.

(Absolute github noob, so please be patient if I need obvious things explained...)

[alexcrichton]

Awesome thanks @jluner! Yeah I think crates-io would be a great starting point, using error-chain there and then starting to migrate this outwards towards Cargo itself.

Unfortunately I don't think there's a great way to "ease" this in. We'll definitely, for the first shot, not want to rename CargoResult. I'd probably recommend something like:

• Comment out most of src/cargo/util/error.rs
• Add in the error-chain macro, defining the types like CargoResult and CargoError
• Define human and internal function to just manufacture string-based errors (e.g. "string".into())
• Comment out handling of errors, such as the functions that print it in src/cargo/lib.rs
• Hit cargo build and fix error cases until the whole crate compiles
• Uncomment commented code in src/cargo/lib.rs, reimplement how errors are displayed
• Start getting tests passing

I think that the trickiest thing here may be the "human" vs "not human" distinction, but I think we can basically manage that with just two custom variants of an error-chain error, plumbing data through like that.

That's unfortunately a little vague, but if you have any questions please feel free to ask! I'm available both here and on IRC

[jluner]

Now that I'm in the thick of it, a stylistic question:

Is it preferable to .map_err(|e| { e.into() }) or .map_err(|e| { CargoError::from(e) }) or is there a third way I should know about?

[rustonaut]

also maybe .map_err(CargoError::from)?

[alexcrichton]

@jluner oh ideally that wouldn't be necessary at all as the ? operator bakes in the Into conversion, right?

[jluner]

There are places that were, in the old scheme, <some operation resulting in e.g. io::Result>.chain_error(...). I'm trying to preserve all the information that was being imparted, so I need to translate that original error type into the new error_chain! equivalent, then chain the original expanded error message to it.

I'm being rather literal in my approach, since I don't have the grounding to know when best to stop doing whatever it used to be doing. All the old error structs are still structs that are wrapped in one of the ErrorKind options, and the is_human plumbing is still there, preserving the original trait's behavior. The NetworkError trait turned into its own error_chain! with maybe_spurious implemented over it, and then is a link in the Cargo* chain.
(edit: This design decision also adds a lot of methods that e.g. invoke git but return a CargoResult, where internally I have to turn the git error into a NetworkError so that it can be automatically turned into the CargoError type).

Once this pass checks out, I'll go back and see about turning e.g. the cause field for ProcessError or ConcreteCargoError into something hooked up via chain_err.

[alexcrichton]

@jluner ah ok, thanks for the info. I think it's ok to not preserve the human/is_human stuff if it gets too onerous, I think we can always come up with a different solution as well for something like that. Ideally we'd only have one error_chain! invocation and the maybe_spurious method would just be defined on the error itself, would that be possible? With error_chain! I think we can defer boxing things up and erasing types until the end which may allow us to just operate over the error type directly to look for spurious network errors.

[alexcrichton]

Oh and I actually though that <io::Result>.chain_err(|| ...) was a thing that error-chain did, right? Or does it only define chain_err for the built-in error type?

[jluner]

One thing I want to do as follow-up to this is put a blog post somewhere about error_chain, since I had to learn some details about it the hard way. Looking at it expanded, it creates the trait

/// Additional methods for `Result`, for easy interaction with this crate.
pub trait CargoResultExt<T, E> {
    /// If the `Result` is an `Err` then `chain_err` evaluates the closure,
    /// which returns *some type that can be converted to `ErrorKind`*, boxes
    /// the original error to store as the cause, then returns a new error
    /// containing the original error.
    fn chain_err<F, EK>(self, callback: F)
    -> ::std::result::Result<T, CargoError>
    where
    F: FnOnce()
    ->
    EK,
    EK: Into<CargoErrorKind>;
}

With the impl

impl <T, E> CargoResultExt<T, E> for ::std::result::Result<T, E> where
 E: ::std::error::Error + Send + 'static

So... maybe it gives that? I've been mapping things under the assumption I needed to, but I don't have the clearest intuition for how a lot of these bounds translate. I'll poke a few places and see if I actually have to map into the chain-defined type, or if I can just chain_err directly.

As for maybe_spurious, it would be possible to impl that directly onto the Error struct or the ErrorKind enum. I don't know if it would be semantically awkward having the ability to ask if non-network errors are spurious or not, but I can look at it.

[alexcrichton]

Ok thanks for the updates! Feel free to keep asking questions :)

I think that the CargoResultExt there should apply for every kind of error we work with today, but lemme know if something looks awry.

[jluner]

I'm on to the bit where I'm re-implementing how errors are displayed, and I'm either in a hole or just not clever enough yet.

The intent of the code is: Given some chain of errors, we're either run in Verbose mode, so we print every error, or we're not Verbose, so starting with the first error we will print errors until the current error is not typed as being shown to the user.

At this point, we start with a &CargoError, so I can do nearly anything I want to that. To get a subsequent error now, I can either rely upon error-chain's provided iter (which just loops over cause), or call cause directly. Either way, from then on I'm stuck working with &Error.

My first thought had been to invent trait Human, give it a default impl for &Error and some real smarts for &CargoError. But I assumed this will work because it would in C# and I'm still thinking like a C# programmer. Instead (and I wrote a simple program to confirm this), once I have the trait object, I can only dispatch to other impls defined on that trait, not on whatever the underlying type might be.

[alexcrichton]

@jluner so I've often thought that Cargo's error handling here to be quite subpar and not great. More often than not Cargo's hiding error information that I wanted to see by default!

Along those lines, I think it's actually ok to not replicate exactly what's happening today. Instead I'd recommend the opposite:

• All errors are printed out by default.
• There's a special Internal variant of Cargo errors which just wraps another Cargo error
• Internal errors (and the stack below them) are hidden by -v by default

That way we explicitly tag errors in Cargo as internal, and otherwise they're always printed. I personally prefer those defaults to defaulting to not print errors for now at least.

Ideally what we'd have is "if you see this error it's a bug in Cargo's error reporting, please report an issue" but we're still relatively far from that, so I'm ok with a middle ground in the meantime :)

Does that sound ok to you?

[jluner]

I'll tackle it this weekend. I finally figured out that downcasting is built into Error, so that should fix my stumbling block.