Implement Error for &(impl Error)

[KodrAus]

Opening this up just to see what it breaks. It's unfortunate that &(impl Error) doesn't actually implement Error. If this direct approach doesn't work out then I'll try something different, like an Error::by_ref method.

EDIT: This is a super low-priority experiment so feel free to cancel it for more important crater runs! 🙂

---

Stabilization Report

Why?

We've been working for the last few years to try "fix" the Error trait, which is probably one of the most fundamental in the whole standard library. One of its issues is that we commonly expect you to work with abstract errors through dyn Trait, but references and smart pointers over dyn Trait don't actually implement the Error trait. If you have a &dyn Error or a Box<dyn Error> you simply can't pass it to a method that wants a impl Error.

What does this do?

This stabilizes the following trait impl:

impl<'a, T: Error + ?Sized + 'static> Error for &'a T;

This means that &dyn Error will now satisfy a impl Error bound.

It doesn't do anything with Box<dyn Error> directly. We discussed how we could do Box<dyn Error> in the thread here (and elsewhere in the past), but it seems like we need something like lattice-based specialization or a sprinkling of snowflake compiler magic to make that work. Having said that, with this new impl you can now get a impl Error from a Box<dyn Error> by dereferencing it.

What breaks?

A crater run revealed a few crates broke with something like the following:

// where e: &'short &'long dyn Error
err.source()

previously we'd auto-deref that &'short &'long dyn Error to return a Option<&'long dyn Error> from source, but now will call directly on &'short impl Error, so will return a Option<&'short dyn Error>. The fix is to manually deref:

// where e: &'short &'long dyn Error
(*err).source()

In the recent Libs meeting we considered this acceptable breakage.

[rust-highfive]

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

[KodrAus]

@bors try

[bors]

⌛ Trying commit 39466bc with merge 353a0a223066e9f46a1d935f2271a26c3c5002bb...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 353a0a223066e9f46a1d935f2271a26c3c5002bb (353a0a223066e9f46a1d935f2271a26c3c5002bb)

[KodrAus]

@craterbot check

[craterbot]

👌 Experiment pr-75180 created and queued.
🤖 Automatically detected try build 353a0a223066e9f46a1d935f2271a26c3c5002bb
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[KodrAus]

With this trait impl, the following code would begin to compile:

use std::{fmt, error::Error};

#[derive(Debug)]
pub struct MyError;

impl fmt::Display for MyError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "my error")
    }
}

impl Error for MyError {}

fn is_err<T: Error>() {}

fn main() {
    is_err::<&MyError>();
    is_err::<&dyn Error>();
    is_err::<&(dyn Error + 'static)>();
}

This is an example of code that no longer compiles with this change:

use std::{fmt, error::Error};

#[derive(Debug)]
pub struct MyError;

impl fmt::Display for MyError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "my error")
    }
}

impl Error for MyError {}
impl<'a> Error for &'a MyError {}

It fails with:

error[E0119]: conflicting implementations of trait `std::error::Error` for type `&MyError`:
  --> src\lib.rs:13:1
   |
13 | impl<'a> Error for &'a MyError {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: conflicting implementation in crate `std`:
           - impl<'a, T> std::error::Error for &'a T
             where T: std::error::Error, T: ?Sized;

Looks like eyre flushes out another source of breakage from auto-deref:

error: lifetime may not live long enough
  --> examples\custom_handler.rs:65:60
   |
65 |         let errors = iter::successors(Some(error), |error| error.source());
   |                                                     ------ ^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
   |                                                     |    |
   |                                                     |    return type of closure is std::option::Option<&'2 dyn std::error::Error>
   |                                                     has type `&'1 &dyn std::error::Error`

which can be fixed by:

- let errors = iter::successors(Some(error), |error| error.source());
+ let errors = iter::successors(Some(error), |error| (*error).source());

cc @yaahc

So far I haven't found breakage in the test suites of anyhow, thiserror, error-chain, or failure.

We could try and come up with some object-safe adapter to implement Error for references to an impl Error, or do we just expect that the only effective way to work with errors generically is going to be through dyn Error rather than impl Error?

[yaahc]

@KodrAus what's the goal of this change?

[KodrAus]

@yaahc Sorry to drop you into a random PR like that!

The goal is to try make impl Error more usable. Right now:

• &dyn Error
• &MyError
• Box<dyn Error>

all don’t implement the Error trait. So if you have a &dyn Error you can’t pass it to a method that wants impl Error.

We have coherence issues with impl Error for Box<dyn Error>, but with this change you could at least still get an impl Error from a Box<dyn Error> by dereferencing it. When I noticed this impl at least doesn’t break anything in rustc I thought I’d try it on the wider ecosystem.

[yaahc]

aah yea, I've wanted a way to abstract over types that deref to error like Box<dyn Error> and types that impl error for a while, I've tried finding ways to do it but have so far been unsuccessful. I've been placing my bets on specialization fixing this issue so we can just implement Error for Box<dyn Error> but I'm excited about the prospect of finding other solutions in the short term.

Last time I tried implementing a trait for this I think the problem was that I'd need higher kinded types to express it.

[KodrAus]

I've been placing my bets on specialization fixing this issue so we can just implement Error for Box

Hmm, so I think impl Error for Box<dyn Error> (ideally via impl<T: Error + ?Sized> Error for Box<T>) causes these impls to conflict:

// in core
impl<T> From<T> for T;

// in std
impl<'a, E: Error + 'a> From<E> for Box<dyn Error + 'a>;

which I think specialization doesn't quite cover so we might need a sprinkling of compiler magic to work around it 🤔 The impl<'a, E: Error + 'a> From<E> for Box<dyn Error + 'a> side of the conflict would be covered by a more general impl on Box<T> though:

// in alloc
impl<T, U: ?Sized> From<T> for Box<U> where T: Unsize<U>;

That still conflicts with impl<T> From<T> for T, but might make a better target for some compiler magic if it wouldn't break inference all through the ecosystem... At any rate that doesn't seem to prevent us from trying to do the impl this PR adds.

On top of this PR, there is also the conservative method-based approach to look at for converting Box<dyn Error> into impl Error:

impl dyn Error {
    pub fn as_error(self: &Box<Self>) -> Compat<&Self>;

    pub fn into_error(self: Box<Self>) -> Compat<Box<Self>>;
}

pub struct Compat<T>(T);

impl<'a, T: ?Sized + Error> Error for Compat<&'a T>;

impl<T: ?Sized + Error> Error for Compat<Box<T>>;

I guess this is all stuff to track in the Error Handling Group once it's bootstrapped? 🙂

[yaahc]

Hmm, so I think impl Error for Box<dyn Error> (ideally via impl<T: Error + ?Sized> Error for Box<T>) causes these impls to conflict:

which I think specialization doesn't quite cover

would you mind elaborating on this one? I had thought that the second impl would work if we marked the first one as default.

[KodrAus]

I had thought that the second impl would work if we marked the first one as default.

I'd thought so too... but it didn't work out when I tried it. I had a browse back through the RFC and I think my understanding of specialization is just wrong. I've been thinking of it a bit like the CSS !important operator that just forces one rule to be more applicable than another by slapping a default into the mix. But I think specialization still relies on one of the impls being specialized over applying to a subset of the types of another. Maybe like some kind of scoped !important.

The limitations section of the RFC talks a bit about this, but it's hard to understand specialization casually so I might still be misunderstanding things.

[yaahc]

I had thought that the second impl would work if we marked the first one as default.

I'd thought so too... but it didn't work out when I tried it. I had a browse back through the RFC and I think my understanding of specialization is just wrong. I've been thinking of it a bit like the CSS !important operator that just forces one rule to be more applicable than another by slapping a default into the mix. But I think specialization still relies on one of the impls being specialized over applying to a subset of the types of another. Maybe like some kind of scoped !important.

The limitations section of the RFC talks a bit about this, but it's hard to understand specialization casually so I might still be misunderstanding things.

hmm, im in a similar boat of not being sure how close to reality my understanding of specialization is. I know there's some edge cases about lifetimes wrt specialization, so the <E: Error + 'a> might be responsible for the breakdown? If that's the case then I wonder if specialization + the Unsize based From impl which currently overlaps would end up working out, since it doesn't expose any of the lifetimes in it's interface.

If we cant find a way to impl Error for box<dyn Error> that would not be great, but maybe not the worst thing in the world. If we can add a std::error::Report type that essentially replaces box<dyn Error> we could explicitly not include the 'a bound in the From impl which might let us still impl Error on the type that we'd want people to be using for dyn Errors in the future.

[craterbot]

🚧 Experiment pr-75180 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-75180 is completed!
📊 16 regressed and 7 fixed (116018 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[KodrAus]

So the results of the crater run are finally in!

We have a few regressions:

We now need to manually deref &&dyn Error in order to get a source with the right lifetime:

• eyre
• strerror

Some crates now have unnecessary implementations of Error for &T: Error:

• chainerror

Some that can no longer use String::as_ref to get an impl Into<Box<dyn Error>>:

• neptungen
• mars_discovery
• c_string
• sv

I’d like to work through the implications for &&dyn Error a bit more, to see if there’s a nicer way than (*error).source() but I think this change is worth pursuing more to fix one of the papercuts of working with Error.

What do you think @yaahc @rust-lang/libs?

[BurntSushi]

@KodrAus I think my initial reaction here is that since there is breakage, it might be better if a change like this were considered by the error handling working group. Mostly just so that it's considered in the context of other changes we might like to make.

[yaahc]

@BurntSushi still waiting on that last vote to establish the project group :(

rust-lang/rfcs#2965 (comment)

[crlf0710]

The project group has been established. I'll mark this PR as S-blocked on that project group's decisions.

[m-ou-se]

@bors r+

[bors]

📌 Commit bbf5001 has been approved by m-ou-se

[lopopolo]

Hi folks, the Crater docs mention that it discovers and tests Rust codebases on GitHub. Is this still true? This PR broke an unpublished project of mine, see artichoke/artichoke#1069. Should I have expected a ticket or to be listed as a regression in this ticket?

RubyException has std::error::Error as a supertrait which doesn't appear to be mentioned in the breakage categories above.

I turned out to not need this impl for Artichoke to compile, but if I did, i'd be in a tricky spot in CI which must pass on both stable and nightly (for building rustdoc).

error[E0119]: conflicting implementations of trait `std::error::Error` for type `&(dyn error::RubyException + 'static)`:
   --> artichoke-backend/src/error.rs:128:1
    |
128 | impl error::Error for &dyn RubyException {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `std`:
            - impl<'a, T> std::error::Error for &'a T
              where T: std::error::Error, T: ?Sized;

error[E0119]: conflicting implementations of trait `std::error::Error` for type `&(dyn error::RubyException + 'static)`:
   --> artichoke-backend/src/error.rs:128:1
    |
128 | impl error::Error for &dyn RubyException {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `std`:
            - impl<'a, T> std::error::Error for &'a T
              where T: std::error::Error, T: ?Sized;

error: aborting due to previous error

For more information about this error, try `rustc --explain E0119`.
error: could not document `artichoke-backend`

[yaahc]

Hi folks, the Crater docs mention that it discovers and tests Rust codebases on GitHub. Is this still true? This PR broke an unpublished project of mine, see artichoke/artichoke#1069. Should I have expected a ticket or to be listed as a regression in this ticket?

to my knowledge it does still lookup git repos. I am not super familiar with the crater UI so I might be misinterpreting this but I think what happened is that your crate was failing to compile on with both versions of std, before and after this change. This ends up causing crater to just ignore the error and assume it was unrelated to the change introduced.

Here are the errors it ran into:

• Before: https://crater-reports.s3.amazonaws.com/pr-75180/master%23119d2a1a98fe87d4ae6cabf12134a0ef2fb95851/gh/artichoke.artichoke/log.txt
• After: https://crater-reports.s3.amazonaws.com/pr-75180/try%23353a0a223066e9f46a1d935f2271a26c3c5002bb/gh/artichoke.artichoke/log.txt

I don't really know what this means though, it seems like they discard the root cause error. :/

[Dylan-DPC-zz]

that's not your repo fault but the way crater is configured sometimes it has issues with some of the repositories since crater is sand-boxed and other stuff

[lopopolo]

@yaahc @Dylan-DPC I pulled the rustops/crates-build-env container and Artichoke builds successfully in it, but I did notice that the rustup-installed cargo initiated an install of the toolchain specified in Artichoke's rust-toolchain file. I'm guessing this type of network activity is what the crater sandbox prevents.

Given that crater's intent is to build projects with specific versions of rustc, I think it makes sense to ignore rust-toolchain files in the repository if they are present.

[yaahc]

@lopopolo I went ahead and opened an issue on crater so hopefully this gets fixed before too long.

[lopopolo]

@yaahc thank you!

[pietroalbini]

@yaahc for future reference, the "errors" section means an internal Crater error. If the crate failed to build both before and after the change it will be marked as "build-fail".