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.

[jluner]

Puzzle time:

Given an instance of CargoError I wish to inspect its entire error chain (an iterable sequence of &Error) to determine which of them is typed as an internal error. To do this, the best option I can come up with is to try some kind of downcast/type inspection. To downcast from &Error, it must actually be &'static Error. I can get this, IF instead of starting with CargoError, I instead have &'static CargoError.

So can I turn an instance into a static reference? This obviously can happen somehow, because it's a condition for chaining errors in the first place (chaining requires Error + Send + 'static) And second, does a &'static anything actually save me once I start iterating? Will the subsequent &Error instances still be 'static?

I might dump this question over on Reddit as well, so that it gets an audience.

[rustonaut]

You don't want a `&'static Error` (and won't get it any way in this context) but instead a reference to a Error trait object with `E: Error+'static` which means the underling error struct can't have life times (except static) in it's type. If I remember correctly the type being static is a requirement to create a Error trait object from it (at least if the trait is static). So the requirements are probably already givens.

…

On Sun, May 21, 2017, 05:18 jluner ***@***.***> wrote: Puzzle time: Given an instance of CargoError I wish to inspect its entire error chain (an iterable sequence of &Error) to determine which of them is typed as an internal error. To do this, the best option I can come up with is to try some kind of downcast/type inspection. To downcast from &Error, it must actually be &'static Error. I can get this, IF instead of starting with CargoError, I instead have &'static CargoError. So can I turn an instance into a static reference? This obviously can happen *somehow*, because it's a condition for chaining errors in the first place (chaining requires Error + Send + 'static) And second, does a &'static anything actually save me once I start iterating? Will the subsequent &Error instances still be 'static? I might dump this question over on Reddit as well, so that it gets an audience. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4029 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHR0kcW6FP1BPWWAqmK232Tu_TzLH1zlks5r760KgaJpZM4NYHYK> .

[jluner]

As best as I can manage, the static lifetime bound is only ever real on the first instance I get. Once I borrow its cause, the lifetime is implicitly the method lifetime, and so it can't figure out how to resolve that AND the static bound, and then I'm stuck.

Current version of the code:

fn handle_cause<E>(cargo_err: &E, shell: &mut MultiShell) -> bool where E: Error + 'static {
    let verbose = shell.get_verbose();

    if verbose == Verbose {
        //Print all errors
        print(cargo_err.to_string(), shell);
        let mut err = cargo_err.cause();
        loop {
            let cause = match err { Some(err) => err, None => return true };
            print(cause.to_string(), shell);
            err = cause.cause();
        }
    }
    else {
        //If we're run in a non-Verbose mode, print errors until one marked
        //as Internal appears
        //check that cargo_err is not Internal
        let first: &'static Error = cargo_err; //needed so it can find downcast_ref
        let cerr = first.downcast_ref::<CargoError>();
        if let Some(&CargoError(CargoErrorKind::Internal(..), ..)) = cerr {
            return false;
        }
        print(first.to_string(), shell);
        let mut err = first.cause();
        loop {
            let cause = match err { Some(err) => err, None => return true };
            //check that cause is not Internal
            let cerr = cause.downcast_ref::<CargoError>();
            if let Some(&CargoError(CargoErrorKind::Internal(..), ..)) = cerr {
                return false;
            }

            print(cause.to_string(), shell);
            err = cause.cause();
        }
    }
}

The errors I get are

   Compiling cargo v0.20.0 (file:///D:/workspace/cargo)
error[E0495]: cannot infer an appropriate lifetime for automatic coercion due to conflicting requirements
   --> src/cargo/lib.rs:229:37
    |
229 |         let first: &'static Error = cargo_err; //needed so it can find downcast_ref
    |                                     ^^^^^^^^^
    |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the body at 212:91...
   --> src/cargo/lib.rs:212:92
    |
212 | fn handle_cause<E>(cargo_err: &E, shell: &mut MultiShell) -> bool where E: Error + 'static {
    |                                                                                            ^
note: ...so that reference does not outlive borrowed content
   --> src/cargo/lib.rs:229:37
    |
229 |         let first: &'static Error = cargo_err; //needed so it can find downcast_ref
    |                                     ^^^^^^^^^
    = note: but, the lifetime must be valid for the static lifetime...
note: ...so that expression is assignable (expected &'static std::error::Error + 'static, found &std::error::Error)
   --> src/cargo/lib.rs:229:37
    |
229 |         let first: &'static Error = cargo_err; //needed so it can find downcast_ref
    |                                     ^^^^^^^^^

[jluner]

And I found this: rust-lang/rust#35943

So: introspection is off the table.
That leaves inspecting something I can get ahold of from the Error trait, which is really just the string that we're going to be printing. @alexcrichton any objection to using a magic prefix on all Internal-typed errors?

[rustonaut]

You need to change (at least)`let first: &'static Error = cargo_err; //needed so it can find downcast_ref` to `let first: &Error = cargo_err` The `'static` constraint of `downcast_ref` is one the trait you cast, not the reference to it. The other problem is, that `downcast_ref` excerpts a trait object while `cargo_err` is some specific type implementing Error (1) if call it mainly with a error returned by cause or similar maybe change the parameter to `cargo_err: &Error`. (1): wrt. Genetics rustc will create a copy of the function for each different type it is called with, so it's a generalization at compiler time while trait objects are a generalization at runtime. I'm not aware of anythink like a compiler time `downcast` (except maybe specialization) through there are work arounds for some problem cases.

…

On Sun, May 21, 2017, 07:54 jluner ***@***.***> wrote: As best as I can manage, the static lifetime bound is only ever real on the first instance I get. Once I borrow its cause, the lifetime is implicitly the method lifetime, and so it can't figure out how to resolve that AND the static bound, and then I'm stuck. Current version of the code: fn handle_cause<E>(cargo_err: &E, shell: &mut MultiShell) -> bool where E: Error + 'static { let verbose = shell.get_verbose(); if verbose == Verbose { //Print all errors print(cargo_err.to_string(), shell); let mut err = cargo_err.cause(); loop { let cause = match err { Some(err) => err, None => return true }; print(cause.to_string(), shell); err = cause.cause(); } } else { //If we're run in a non-Verbose mode, print errors until one marked //as Internal appears //check that cargo_err is not Internal let first: &'static Error = cargo_err; //needed so it can find downcast_ref let cerr = first.downcast_ref::<CargoError>(); if let Some(&CargoError(CargoErrorKind::Internal(..), ..)) = cerr { return false; } print(first.to_string(), shell); let mut err = first.cause(); loop { let cause = match err { Some(err) => err, None => return true }; //check that cause is not Internal let cerr = cause.downcast_ref::<CargoError>(); if let Some(&CargoError(CargoErrorKind::Internal(..), ..)) = cerr { return false; } print(cause.to_string(), shell); err = cause.cause(); } } } The errors I get are Compiling cargo v0.20.0 (file:///D:/workspace/cargo) error[E0495]: cannot infer an appropriate lifetime for automatic coercion due to conflicting requirements --> src/cargo/lib.rs:229:37 |229 | let first: &'static Error = cargo_err; //needed so it can find downcast_ref | ^^^^^^^^^ | note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the body at 212:91... --> src/cargo/lib.rs:212:92 |212 | fn handle_cause<E>(cargo_err: &E, shell: &mut MultiShell) -> bool where E: Error + 'static { | ^ note: ...so that reference does not outlive borrowed content --> src/cargo/lib.rs:229:37 |229 | let first: &'static Error = cargo_err; //needed so it can find downcast_ref | ^^^^^^^^^ = note: but, the lifetime must be valid for the static lifetime... note: ...so that expression is assignable (expected &'static std::error::Error + 'static, found &std::error::Error) --> src/cargo/lib.rs:229:37 |229 | let first: &'static Error = cargo_err; //needed so it can find downcast_ref | ^^^^^^^^^ — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4029 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHR0kU-2ndrOCu-lwbscH3p7nk8YvOngks5r79GigaJpZM4NYHYK> .

[alexcrichton]

@jluner yeah whatever hacks here are necessary to get working are fine by me! A prefix or something like that is ok.

It may also suffice to use unsafe transmutes here perhaps? I'm not sure if that'll actually work out though.

[rustonaut]

I wrote my last answer from my phone+mail and didn't saw the problem wrt. to cause not returning Error+'static which is quite, uh, surprising ;=)

I agree with @alexcrichton here that in this case a transmute (only from &Error => &Error+'static) might be ok, as you will have a hard time getting a cause not complying with the 'static bound out of a Error complying with 'static e.g. something like this (didn't compile it, so might be not completely right):

fn handle_cause<E>(cargo_err: &E, shell: &mut MultiShell) -> bool 
    where E: Error + 'static 
{
    //...
    else {
        //get trait object from concrete error
        let mut err: &(Error+'static) = &cargo_err as &(Error+'static);
        loop {
            if let Some(&CargoError(CargoErrorKind::Internal(..), ..)) = err.downcast_ref::<CargoError>() {
                return false;
            }
            print(cause.to_string(), shell);
            err = match cause.cause() {
                Some(err) => unsafe { mem::transmut::<&Error, &(Error+'static)>(err) },
                None  => return true
            }
    }

note that this kind of relies that in praxis you will only get a Error complying with 'static out of a cause call on a E: Error+'static, through I'm not 100% sure if I didn't overlook something here

[jluner]

Unsafe transmutes typecheck.

I'm having the function require that the error being inspected be moved into it. I don't know that it's strictly necessary, since none of the references we generate during inspection can escape the function, and we don't mutate anything. Also in order to be chained in the first place, the error must be Error + 'static, so I'm in theory only recovering a fact that we already know to be true.

Onward to fixing up the tests. I'm at 1 failing in the build tests. Since apparently cargo test will finish after the first test module that has a failing test, this doesn't help me quickly see how many other things I need to clean up, but it does feel like I'm making progress.

[alexcrichton]

Awesome @jluner! Want to throw up a PR so I can help give some feedback as well? Wouldn't want too many tests to get updated only to have to change again!

[jluner]

I'll try and push it up tonight. This will be my first github pull request, so there may be a tooling learning curve. The "good" news is that so far I haven't had to change the substance of tests, and the current failure may in fact be due to something I introduced (code is taking what is per the test the wrong branch). I haven't materially changed the substance of error messages being passed along, but that should be easy enough to do once the error-chain stuff is in a stable place. For the first pass I'm reluctant to adjust tests to match the new code output.

[alexcrichton]

Oh sure no worries! Let me know if you've got any questions and I'd be more than willing to help out!

[alexcrichton]

This has been done thanks to @jluner!

[jluner]

I've got a half-finished attempt at changing to the normal names (Result, Error) waiting for me to figure out why my computer is crashing. Do you want a new issue for that, or will a bare PR suffice?

[alexcrichton]

Ah yeah that's fine, just a PR sounds good!