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

NLL: lost "borrowed value needs to live until here" (diagnostic regression compared to AST-borrowck) #54382

Closed
pnkfelix opened this issue Sep 20, 2018 · 4 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal P-high High priority

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Sep 20, 2018

Spun off of my investigation of #21114, #54556

Consider this code (play):

#![cfg_attr(use_nll, feature(nll))]

fn main() {
    {
        let mut _thing1 = D(Box::new("thing1"));
        D("other").next(&_thing1)
    }

    ;
}

#[derive(Debug)]
struct D<T: std::fmt::Debug>(T);

impl<T: std::fmt::Debug>  Drop for D<T> {
    fn drop(&mut self) {
        println!("dropping {:?})", self);
    }
}

impl<T: std::fmt::Debug> D<T> {
    fn next<U: std::fmt::Debug>(&self, _other: U) -> D<U> { D(_other) }
    fn end(&self) { }
}

Under AST-borrowck, this emits the diagnostics:

Standard Error

   Compiling playground v0.0.1 (/playground)
error[E0597]: `_thing1` does not live long enough
 --> src/main.rs:6:26
  |
6 |         D("other").next(&_thing1)
  |                          ^^^^^^^ borrowed value does not live long enough
7 |     }
  |     - `_thing1` dropped here while still borrowed
8 | 
9 |     ;
  |     - borrowed value needs to live until here

under NLL, it only emits this:

error[E0597]: `_thing1` does not live long enough
 --> src/main.rs:6:25
  |
6 |         D("other").next(&_thing1)
  |                         ^^^^^^^^ borrowed value does not live long enough
7 |     }
  |     - `_thing1` dropped here while still borrowed

In particular, the note 8 "borrowed value needs to live until here" that highlights the semi-colon after the end of the block is no longer emitted.

My current plan for addressing #54556 is to improve diagnostics. The first step in that path, IMO, would be to figure out how to bring back the note about how long the borrowed value needs to live.

@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal labels Sep 20, 2018
@pnkfelix pnkfelix added this to the Rust 2018 Release milestone Sep 25, 2018
@nikomatsakis
Copy link
Contributor

Current output:

error[E0597]: `_thing1` does not live long enough
 --> src/main.rs:6:25
  |
4 |        {
  |   _____-
  |  |_____|
  | ||
5 | ||         let mut _thing1 = D(Box::new("thing1"));
6 | ||         D("other").next(&_thing1)
  | ||                         ^^^^^^^^ borrowed value does not live long enough
7 | ||     }
  | ||     -
  | ||     |
  | ||_____`_thing1` dropped here while still borrowed
  | |______a temporary with access to the borrow is created here ...
  |        ... and the borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `D`

the labels look good, but the spans associated with them -- woah...

@pnkfelix
Copy link
Member Author

Hmm I wonder why the temp is being given the span of the whole block rather than the blocks tail expr

@pnkfelix
Copy link
Member Author

And likewise, why the drop of the temporary is no longer associated with the span of the semicolon on the whole statement ...

@pnkfelix
Copy link
Member Author

pnkfelix commented Nov 6, 2018

Triage: While I do want to address this, I don't think we would feel all that inclined to backport a fix to the beta channel.

So, tagging as P-high, but removing from the Release milestone.


Also, I'm going to assign this to myself since I've taken a special interest in issues involving tail expressions of blocks.

@pnkfelix pnkfelix added the P-high High priority label Nov 6, 2018
@pnkfelix pnkfelix removed this from the Rust 2018 Release milestone Nov 6, 2018
@pnkfelix pnkfelix self-assigned this Nov 6, 2018
pnkfelix added a commit to pnkfelix/rust that referenced this issue Nov 8, 2018
(I opted to rely on compare-mode=nll rather than opt into
`#![feature(nll)]`, mostly to make it easy to observe the interesting
differences between the AST-borrwock diagnostic and the NLL one.)
pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 12, 2018
…pans-for-temps-and-their-drops, r=davidtwco

More precise spans for temps and their drops

This PR has two main enhancements:

 1. when possible during code generation for a statement (like `expr();`), pass along the span of a statement, and then attribute the drops of temporaries from that statement to the statement's end-point (which will be the semicolon if it is a statement that is terminating by a semicolon).
 2. when evaluating a block expression into a MIR temp, use the span of the block's tail expression (rather than the span of whole block including its statements and curly-braces) for the span of the temp.

Each of these individually increases the precision of our diagnostic output; together they combine to make a much clearer picture about the control flow through the spans.

Fix rust-lang#54382
pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 15, 2018
…pans-for-temps-and-their-drops, r=davidtwco

More precise spans for temps and their drops

This PR has two main enhancements:

 1. when possible during code generation for a statement (like `expr();`), pass along the span of a statement, and then attribute the drops of temporaries from that statement to the statement's end-point (which will be the semicolon if it is a statement that is terminating by a semicolon).
 2. when evaluating a block expression into a MIR temp, use the span of the block's tail expression (rather than the span of whole block including its statements and curly-braces) for the span of the temp.

Each of these individually increases the precision of our diagnostic output; together they combine to make a much clearer picture about the control flow through the spans.

Fix rust-lang#54382
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal P-high High priority
Projects
None yet
Development

No branches or pull requests

2 participants