relate closure_substs.parent_substs() to parent fn in NLL

[aliemjay]

Fixes #98589

The discrepancy between early- and late-bound lifetimes is because we map early-bound lifetimes into those found in the closure_substs while late-bound lifetimes are mapped into liberated free regions:

rust/compiler/rustc_borrowck/src/universal_regions.rs

Lines 255 to 261 in 5f98537

	tcx.for_each_free_region(&closure_substs, |fr| {
	region_mapping.push(fr);
	});
	for_each_late_bound_region_defined_on(tcx, typeck_root_def_id, |r| {
	region_mapping.push(r);
	});

r? @rust-lang/types

[compiler-errors]

src/librustdoc/clean/auto_trait.rs:294:23

huh, was this code that's failing committed after nll was enabled on this crate or something? if so, then maybe it's ok? regardless, maybe we can do a crater run for this.

@bors try

[bors]

⌛ Trying commit 48ba16b3bc55d3d41cc3a226a321292467966e6f with merge 430fd2d8141f9ec36db37c8821e1f5bb3a620a79...

[compiler-errors]

someone should run @ craterbot check in approximately 2.5 hours when this try build is done, i will be asleep 😴

[bors]

💔 Test failed - checks-actions

[compiler-errors]

of course it doesn't try-build... i totally didn't think that through.

[aliemjay]

rustdoc regressed because the following passed on nightly:

struct Ty<'a, 'b>(&'a &'b ());

impl<'a, 'b> Ty<'a, 'b> {
    fn f() {
        || { Self::f(); };
    }
}

but it doesn't compile on stable because of #98852.

Now I think this fix should be backported to avoid users relying on this behavior.

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 499a0823b7bdcf222ed2d200dc0b6622d7684a50 with merge cb1f7c96119295882e08b09b4bd01051669c071b...

[bors]

☀️ Try build successful - checks-actions
Build commit: cb1f7c96119295882e08b09b4bd01051669c071b (cb1f7c96119295882e08b09b4bd01051669c071b)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-98835 created and queued.
🤖 Automatically detected try build cb1f7c96119295882e08b09b4bd01051669c071b
🔍 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

[craterbot]

🚧 Experiment pr-98835 is now running

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

[craterbot]

🎉 Experiment pr-98835 is completed!
📊 28 regressed and 5 fixed (238905 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

[aliemjay]

crater showed 5 root regressions:

charcoal-1.1.0 doesn't compile on the current stable versions and was one of the "fixed" crates by #95565.

Other 4 regressions (detailed below) are indeed stable regressions. They are variants of #98589 that somehow previously passed lexical region resolution (as well as NLL). An example regression is the one from parse-hyperlink-0.23.3, it passes on stable but is rejected by this PR

fn test<'a>()
where
    'a, 'a, // <- makes sure `'a` is early-bound; late-bound lifetimes are already rejected
{
    let f = |_: &'a str| {};
    f(&String::new());
}

A few notes on these regressions:

• This is not a soundness fix. It just fixes language semantics IMO. The difference in behavior between early- and late-bound lifetimes is another source of confusion.
• It only affects users of explicit lifetime annotations on closure signature and body only if one of the lifetimes happens to be early-bound.
• They were rejected by AST borrowck.
• The migration is trivial in most cases: removing lifetime annotations is sufficient, however innernet requires replacing Self constructor with the struct name.
• On the impact: parse-hyperlinks has 6 dependent crates, others don't have any.
• When NLL reaches stable without this fix, the impact MAY be greater since NLL allows more code to compile with this behavior.

[compiler-errors]

r? @nikomatsakis

[aliemjay]

Here is my attempt for relnotes. It may need polishing and the code example may be removed to reduce verbosity.

---

This change fixes a correctness bug that affects lifetime annotations in closure signature and body. For example, the following was incorrectly accepted:

async fn test<'a>() { // `'a`, being a lifetime parameter, must be longer than the entire function body
    let f = |_: &'a str| {}; // the argument is required to outlive `'a`
    f(&String::new()); // an argument of shorter lifetime is passed!
}

The code that relies on this bug must remove the lifetime annotation from the closure to allow the compiler to infer an appropriate lifetime. However, migration may require additional subtle changes:

• If the annotation is on closure args, one will likely stumble on another bug with a documented workaround.
• Use of Self constructor inside the closure can be overly restrictive and must then replaced with the struct name. Example.

[steffahn]

• It only affects users of explicit lifetime annotations on closure signature and body only if one of the lifetimes happens to be early-bound.
• They were rejected by AST borrowck.

Here’s an interesting code example that checks neither of these boxes. (Slightly modified from #100725)

pub struct Bigger<'a> {
    _marker: &'a (),
}
impl<'a> Bigger<'a> {
    pub fn get_addr(byte_list: &'a mut Vec<u8>) -> &mut u8 {
        let _ = byte_list.iter_mut().map(|item| {
            Self::other(item);
        }).collect::<Vec<_>>();

        byte_list.push(0);
        byte_list.last_mut().unwrap()
    }

    pub fn other<'b: 'a>(_value: &'b mut u8) {
        panic!()
    }
}

Doesn’t involve explicit lifetime annotations in the closure and compiles successfully since Rust 1.2.0, broken in 1.64 beta (bisects to this PR).

Edit: Oh, wait, my bad, the Self of course stands for Bigger::<'a> so there is a lifetime in the closure. Still, the point that this was not rejected by AST borrow checking still stands.

[jackh726]

Yeah, it is indeed because of Self there. I'm curious if #100375 fixes it. If not, I think we should consider reverting this.

[aliemjay]

For the record, #100375 doesn't change this behavior.