Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dubious region behaviour affected by &_ annotations #36082

Closed
arielb1 opened this issue Aug 28, 2016 · 3 comments
Closed

Dubious region behaviour affected by &_ annotations #36082

arielb1 opened this issue Aug 28, 2016 · 3 comments
Labels
A-lifetimes Area: Lifetimes / regions P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@arielb1
Copy link
Contributor

arielb1 commented Aug 28, 2016

Meta

$ rustc -V
rustc 1.13.0-dev (528c6f3ed 2016-08-25)

STR

This code compiles and runs successfully

use std::cell::RefCell;

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

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

    x.borrow_mut();
}

But adding a &_ type annotation causes a double borrow panic:

use std::cell::RefCell;

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

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

    x.borrow_mut();
}

Notes

"Specification-wise", the type annotation changing the code behaviour is rather dubious. The root cause is this gem of a function.

The behaviour is actually documented in the code:

    // Rule B. `let x: &[...] = [foo().x]`. The rvalue `[foo().x]`
    // would have an extended lifetime, but not `foo()`.

That "common case" does not apply in today's Rust, of course.

Of course, changing this now would be a [breaking-change], but we are landing a similar significant change by @KiChjang (#36029). I will post my take of the rules on a rust-memory-model issue, because @ubsan nicely asked me to (maybe that should be on the RFCs repo instead?).

cc @nrc @pnkfelix @ubsan

@arielb1 arielb1 added A-lifetimes Area: Lifetimes / regions A-amusing I-needs-decision Issue: In need of a decision. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 28, 2016
@arielb1
Copy link
Contributor Author

arielb1 commented Dec 25, 2016

Nominating since this seemed to disappear.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Dec 29, 2016

Discussing in @rust-lang/compiler meeting. This does seem like a kind of "spec bug", at the least -- in that the spec for temp lifetimes wasn't fully revised in light of other language changes. It'd be good to measure potential impact. We could use @brson's craterbomb tool to run tests.

If we do decide to change, we can also do a kind of warning period where we report if the lifetime of some temporary with a custom dtor will change as a result of this change.

@nikomatsakis
Copy link
Contributor

triage: P-medium

@arielb1 will make a branch to investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants