Error messages printed twice

[dtolnay]

trafficstars

The following code shows a reasonably written error type by a library not using error chain, as well as a chained error containing that one as a variant.

#[macro_use]
extern crate error_chain;

mod other_crate {
    use std;
    use std::io;
    use std::fmt::{self, Display};

    #[derive(Debug)]
    pub enum Error {
        Io(io::Error),
    }

    impl std::error::Error for Error {
        fn description(&self) -> &str {
            "IO error"
        }

        fn cause(&self) -> Option<&std::error::Error> {
            match *self {
                Error::Io(ref err) => Some(err),
            }
        }
    }

    impl Display for Error {
        fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
            match *self {
                Error::Io(ref err) => write!(formatter, "IO error: {}", err),
            }
        }
    }

    pub fn f() -> Result<(), Error> {
        Err(Error::Io(io::Error::new(io::ErrorKind::Other, "this message is printed twice")))
    }
}

error_chain! {
    foreign_links {
        OtherCrate(other_crate::Error);
    }
}

fn run() -> Result<()> {
    other_crate::f()?;
    Ok(())
}

quick_main!(run);

The output is:

Error: IO error: this message is printed twice
Caused by: this message is printed twice

This output is redundant but is a consequence of these interactions:

• The crate not using error-chain wants to show reasonable error messages if somebody just prints one of their errors using {}. It would not be nice for their Display implementation to just show "IO error".
• The crate not using error-chain correctly implements cause to expose the underlying IO error.
• Error chain prints the Display representation of every error up the chain of causes.

Are there any patterns for eliminating the redundancy?

[brson]

I'd probably argue that the non-error-chain Error type here is implementing Display incorrectly: it does implement cause, and the purpose of cause is to give access to that information; this pattern does not extend to arbitrary deep nesting of causes - you probably wouldn't want an error string saying "Unable to frob a: Unable to frob b: IO Error: file not found", etc. It's just unmanageable.

That said, it's a common pattern, and it's probably not going away, so it needs a solution.

[dtolnay]

Proposal: in ChainedError::display, as it is iterating through the causes, store the most recently printed message as a String and skip over any message that is fully contained in the most recently printed message.

[Yamakaky]

Seems easy to do, I don't see why is would be bad to skip an identical message. Do you want to try a PR?