Breaking change to Generics::lifetimes in v2.0.73

[tarcieri]

The following (contrived extraction from der_derive, see RustCrypto/formats#1471) compiles with syn v2.0.72 but not with v2.0.73:

use syn::{Generics, GenericParam, Lifetime, LifetimeParam};
use proc_macro2::Span;

pub fn process_generics(mut generics: Generics) {
    let _lifetime = generics
        .lifetimes()
        .next()
        .map(|lt| lt.lifetime.clone())
        .unwrap_or_else(|| {
            let lt = Lifetime::new("'__default", Span::call_site());
            generics
                .params
                .insert(0, GenericParam::Lifetime(LifetimeParam::new(lt.clone())));
            lt
        });
}

It looks like it now assumes there could be a Drop impl where before it knew the concrete Lifetimes type did not have one:

error[E0502]: cannot borrow `generics.params` as mutable because it is also borrowed as immutable
  --> src/main.rs:9:25
   |
5  |       let _lifetime = generics
   |                       --------
   |                       |
   |  _____________________immutable borrow occurs here
   | |
6  | |         .lifetimes()
   | |____________________- a temporary with access to the immutable borrow is created here ...
...
9  |           .unwrap_or_else(|| {
   |                           ^^ mutable borrow occurs here
10 |               let lt = Lifetime::new("'__default", Span::call_site());
11 | /             generics
12 | |                 .params
   | |_______________________- second borrow occurs due to use of `generics.params` in closure
...
15 |           });
   |             - ... and the immutable borrow might be used here, when that temporary is dropped and runs the destructor for type `impl Iterator<Item = &LifetimeParam>`

The culprit seems to be ac9e1dd.

[dtolnay]

Fixed in 2.0.74 — sorry about the breakage.

[obi1kenobi]

👋 cargo-semver-checks maintainer here. I'd like to see if we can add a lint for cases like this. Would either of you be open to helping me out with that by answering a few questions?

I want to make sure I get all the edge cases right. I don't think I understand all the nuance here yet, despite having read all the linked issues and a bunch of the Rustonomicon. I'd love your help!

[tarcieri]

@obi1kenobi in this case the concrete type had no Drop impl, whearas with impl Iterator the compiler can no longer assume there is no Drop impl, so it had to worry about the case where it could potentially need to borrow the iterator to run Drop

[obi1kenobi]

Right. My question then is this: in what cases does it matter whether there could be a Drop impl or not? E.g. would Drop have mattered if the iterator held only owned types?

[tarcieri]

I believe it mattered here due to the unwrap_or_else, which mutably captures generics from the outer scope, but generics was being borrowed from by the Lifetimes iterator

[obi1kenobi]

I believe it mattered here due to the unwrap_or_else, which mutably captures generics from the outer scope, but generics was being borrowed from by the Lifetimes iterator

Hmm, I'm confused because I don't see where that explanation involves Drop. How does Drop and the destructor of impl Iterator<Item = &LifetimeParam> come into play? Or do you think the diagnostic is mentioning it spuriously?

[tarcieri]

If a Drop impl were to exist, it would need to mutably borrow the impl Iterator prior to the block running, since it needs to be released so it stops borrowing generics so the block can mutably borrow it.

It might help to make a small contrived example that's a minimal repro.

[obi1kenobi]

Yeah, I'd love a small example even if contrived. I'm still having trouble wrapping my head around this.

Ultimately, I'll need to formalize this down to the point where cargo-semver-checks could generate a "witness" for the breakage — a piece of code that suffers a compilation error as a result of the change to impl Trait and its possible Drop. Any help toward that goal that you can provide would be welcome and appreciated!