regression 1.51: lifetime may not live long enough with &impl Error

[camelid]

This compiles on stable and beta, but not nightly (1.51):

use std::error::Error;
use std::iter;

fn walk_source_chain(error: &impl Error) {
    for e in iter::successors(error.source(), |e| e.source()) {
        println!("{}", e);
    }
}

Error with nightly-2021-01-27:

error: lifetime may not live long enough
 --> src/lib.rs:5:51
  |
5 |     for e in iter::successors(error.source(), |e| e.source()) {
  |                                                -- ^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
  |                                                ||
  |                                                |return type of closure is Option<&'2 dyn std::error::Error>
  |                                                has type `&'1 &dyn std::error::Error`

error: aborting due to previous error

Changing the closure parameter pattern to match &e fixes the borrow checker error in nightly.

Originally posted by @mzabaluev in #80949 (comment)

[camelid]

Regressed yesterday (!):

searched nightlies: from nightly-2020-12-01 to nightly-2021-01-28
regressed nightly: nightly-2021-01-26
searched commits: from 1d0d76f to f4eb5d9
regressed commit: d3163e9

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-apple-darwin
Reproduce with:

cargo bisect-rustc --regress=error --start=2020-12-01 --preserve 

---

I suspect #75180. cc @KodrAus @m-ou-se

@rustbot label: -E-needs-bisection +T-libs

[camelid]

From that PR:

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.

Given that someone ran into this so quickly (the day after it was merged), should that decision be reconsidered?

[apiraino]

Assigning P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[mzabaluev]

In the recent Libs meeting we considered this acceptable breakage.

I find it startling that breaking existing code that isn't wrong could be considered acceptable.

[camelid]

Nominating for T-libs discussion and removing T-compiler label (I don't think this issue has a compiler component).

@rustbot label: +I-nominated -T-compiler

[sfackler]

I find it startling that breaking existing code that isn't wrong could be considered acceptable.

If we never considered a change that could break not-wrong existing code acceptable, we would, for example, never be able to add a new method to a trait, or add an implementation of a trait to an existing type.

[yaahc]

related: eyre-rs/stable-eyre#4

[m-ou-se]

We discussed this in the library team meeting again just now, and agreed that this breakage still seems acceptable. Closing this issue for now.

If this turns out to cause any significant breakage in the ecosystem, feel free to re-open.

[KodrAus]

It's unfortunate that we no longer get that auto-deref for free that lops off the 'short borrow. As noted in #80949 this can be fixed by using a reference binding:

fn walk_source_chain(error: &impl Error) {
    for e in iter::successors(error.source(), |&e| e.source()) {
        println!("{}", e);
    }
}

Or alternatively by accepting a &(dyn Error + 'static) when you can instead, since it's still a more useful bound:

fn walk_source_chain(error: &(dyn Error + 'static)) {
    for e in error.chain() {
        println!("{}", e);
    }
}

To make impl Error more useful, we could look at adding a chain method to the Error trait itself:

trait ChainExt: Error {
    fn chain(&self) -> Chain
    where
        Self: Sized + 'static,
    {
        <dyn Error + 'static>::chain(self)
    }
}

impl<E: Error> ChainExt for E {}

fn _assert_object_safe(_: &dyn ChainExt) {
    
}

fn walk_source_chain(error: &(impl Error + 'static)) {
    for e in error.chain() {
        println!("{}", e);
    }
}

which wouldn't clash with this:

fn walk_source_chain(error: &(dyn Error + 'static)) {
    for e in error.chain() {
        println!("{}", e);
    }
}

but would with this:

fn dyn_walk_source_chain<'a>(error: &'a &'a (dyn Error + 'static)) {
    for e in error.chain() { // <-- would now fail to assert that `&'a (dyn Error + 'static)` is `'static`
        println!("{}", e);
    }
}

That's basically the same cause as above.

[Mark-Simulacrum]

beta crater:

• https://crater-reports.s3.amazonaws.com/beta-1.51-1/beta-2021-02-14/reg/strerror-0.6.3/log.txt
• https://crater-reports.s3.amazonaws.com/beta-1.51-1/beta-2021-02-14/reg/eyre-0.6.5/log.txt
• https://crater-reports.s3.amazonaws.com/beta-1.51-1/beta-2021-02-14/reg/simple-eyre-0.3.0/log.txt