Tracking issue for removing temporary lifetime extension by borrow hint

[arielb1]

UPDATE: Mentoring instructions for removing the various bits of dead code found below.

---

This is the summary issue for the removal of temporary lifetime extension by borrow hint.

What is this issue about

One issue that languages with deterministic destruction have to face is the lifetime of expression temporaries. To avoid confusion, temporaries have to be destroyed in some definite time, and temporaries living for too short or too long a time can cause subtle errors. The Rust rules for that - nikomatsakis/rust-memory-model#17 - are somewhat complicated.

The implementation used to follow the rules, but it also contained a leftover from the old days of Rust - issue #36082:

Normally, temporaries in Rust live until the containing arena, which in simple cases is the containing statement. For example, in this code the RefCell borrows only live to the end of their containing statements, so this code does not panic:

use std::cell::RefCell;

fn main() {
    let r = 0;
    let s = 0;
    let x = RefCell::new((&r,s));

    let val = x.borrow().0;
    println!("{}", val);

    x.borrow_mut().1 += 1;
}

However, due to said leftover, adding an &_ or &mut _ type hint would make the borrow live until the end of the containing block, which means this code would keep a long borrow and panic due to a double borrow:

use std::cell::RefCell;

fn main() {
    let r = 0;
    let s = 0;
    let x = RefCell::new((&r,s));

    let val: &_ = x.borrow().0; //~ WARN this temporary used to live longer
    println!("{}", val);

    x.borrow_mut().1 += 1;
}

This "feature" is an oversight from the old days of Rust, and therefore it is removed starting from Rust 1.16.

However, some code might be relying on the changed temporary lifetimes, and in some cases might not even compile without it:

use std::cell::RefCell;

fn main() {
    let mut r = 0;
    let s = 0;
    let x = RefCell::new((&mut r,s)); //~ WARN this temporary used to live longer

    let val: &_ = x.borrow().0;
    println!("{}", val);
}

How to fix this warning/error

Use an explicit variable with an explicit lifetime instead of the temporary:

use std::cell::RefCell;

fn main() {
    let r = 0;
    let s = 0;
    let x = RefCell::new((&r,s));

    {
        let b = x.borrow();
        let val: &_ = b.0;
        println!("{}", val);
    }

    x.borrow_mut().1 += 1;
}

and

use std::cell::RefCell;

fn main() {
    let mut r = 0;
    let s = 0;
    let x = RefCell::new((&mut r,s));

    let b = x.borrow();
    let val: &_ = b.0;
    println!("{}", val);
}

[nikomatsakis]

Given that this behavior has been changed, and is unlikely to be changed back, I am going to close this issue -- there is (at this moment) no further work to be done here. However, if you encounter this error and have questions or concerns, please feel free to comment. If we find there is further work to be done, we will re-open the issue. Thanks!

[nikomatsakis]

Actually, I take that back. The further work to be done is probably removing the error message that sends users to this issue after a suitable period of time. In particular, I think it would be a good thing to remove after the current nightly reaches beta.

[Mark-Simulacrum]

I think that it's perhaps time to consider removing this message. cc @rust-lang/compiler (and nominating).

[nikomatsakis]

I agree. Thank you @Mark-Simulacrum for bringing it up, I've been meaning to do the same. The main thing that's been holding me from making an "rfcbot" motion is that the typical procedure is to do a crater run to estimate the impact and I've not gotten around to it yet.

[Mark-Simulacrum]

@nikomatsakis That looks like a potential easy or at least mentorable issue to make this a hard error so we can do the crater run; does that sound correct? Could you write up some quick steps?

[nikomatsakis]

I was wrong, we don't need a crater run. We already made it a hard error. We just have to kill code related to this. I'll provide a few pointers, this is indeed a good mentoring issue.

[nikomatsakis]

Tracking this change introduced a small amount of cruft in the compiler. In particular, some data structures track the 'temporary region', and since that changed, those structures are updated to have both the old and new values. In particular, the Categorization<'tcx> structure contains a variant with an extra field:

#[derive(Clone, PartialEq)]
pub enum Categorization<'tcx> {
    // temporary val, argument is its scope
    Rvalue(ty::Region<'tcx>, ty::Region<'tcx>),
    //     ^^^^^^^^^^^^^^^^ this data is the new temporary lifetime, after the change
    //                       ^^^^^^^^^^^^^^^^ this data is the *old* lifetime, before the change
    ...
}

If you ripgrep around, you will see that this field is mostly unused, except in borrowck where it is used to report this warning.

If we remove that variant, you'll be able to follow a trail of breadcrumbs of code that can be removed. For example, MemCategorizationCx::cat_rvalue no longer needs this argument. Eventually, following this trail should let us delete cruft like temporary_scope2() in region.rs.

[venkatagiri]

@nikomatsakis I would like to work on this.
From going over the code and removing the extra field in Categorization structure, feels like all the changes done by #39066 needs to be removed.
Everything from shrunk_rvalue_scopes to temp_lifetime_was_shrunk.
I wasn't able to figure out where exactly the lifetime was shrunk that needs to be retained so that they continue to show errors.
Am I in the right direction?

[nikomatsakis]

@venkatagiri

From going over the code and removing the extra field in Categorization structure, feels like all the changes done by #39066 needs to be removed.

Yes, that is basically correct. This is the PR that introduced this infrastructure, because we were changing some values from one thing to another and we wanted to give warnings to the affected code.

But, that said, we don't want to revert that PR. In other words, we want to keep the new values that the PR introduced, we just want to stop tracking the old values that got replaced (and stop warning about it).

I wasn't able to figure out where exactly the lifetime was shrunk that needs to be retained so that they continue to show errors.

I'm not sure I understand your question here.

Am I in the right direction?

Sounds like it!

[nikomatsakis]

@venkatagiri (ps, feel free to ping me on IRC or gitter sometime -- gitter is maybe better if you don't have a persistent IRC connection)