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

Erroneous loop diagnostic in nll #56113

Merged
merged 3 commits into from
Feb 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/librustc_mir/borrow_check/nll/explain_borrow/find_use.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,7 @@ impl<'cx, 'gcx, 'tcx> UseFinder<'cx, 'gcx, 'tcx> {

None => {
if p.statement_index < block_data.statements.len() {
queue.push_back(Location {
statement_index: p.statement_index + 1,
..p
});
queue.push_back(p.successor_within_block());
} else {
queue.extend(
block_data
Expand Down
451 changes: 278 additions & 173 deletions src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | for &x in &vector {
| -------
| |
| immutable borrow occurs here
| immutable borrow used here, in later iteration of loop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikomatsakis this change is for the worse, I don't remember you saying that with this approach some things were going to be worse but I couldn't find the discussion about it and if this is one of those cases or it's just an error in the code :).

| immutable borrow later used here
LL | let cap = vector.capacity();
LL | vector.extend(repeat(0)); //~ ERROR cannot borrow
| ^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
Expand All @@ -17,7 +17,7 @@ LL | for &x in &vector {
| -------
| |
| immutable borrow occurs here
| immutable borrow used here, in later iteration of loop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here.

| immutable borrow later used here
...
LL | vector[1] = 5; //~ ERROR cannot borrow
| ^^^^^^ mutable borrow occurs here
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ LL | borrow(&*v); //[ast]~ ERROR cannot borrow
| ^^^ immutable borrow occurs here
...
LL | *x = box 5;
| -- mutable borrow used here, in later iteration of loop
| -- mutable borrow later used here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better


error[E0502]: cannot borrow `*v` as immutable because it is also borrowed as mutable
--> $DIR/borrowck-lend-flow-loop.rs:109:16
|
LL | **x += 1;
| -------- mutable borrow used here, in later iteration of loop
| -------- mutable borrow later used here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

less good, but this case is sort of crazy. (It's a bit hard to tell, actually, what is meant by "later" here -- it's supposed to be after the immutable borrow, I guess -- and in that case, we do go around the loop before we reach **x += 1)

LL | borrow(&*v); //[ast]~ ERROR cannot borrow
| ^^^ immutable borrow occurs here
...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time
LL | 1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
| ---- ^^^^^^ second mutable borrow occurs here
| |
| first borrow used here, in later iteration of loop
| first borrow later used here
...
LL | _ => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
| ------ first mutable borrow occurs here
Expand All @@ -13,7 +13,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time
--> $DIR/borrowck-mut-borrow-linear-errors.rs:15:30
|
LL | 1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
| ---- first borrow used here, in later iteration of loop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two changes in this files also happened in AST and we already approved that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an analogous message in
https://github.com/rust-lang/rust/blob/06ec1a422a5ff147c3a09af7eb80db77459e797b/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.ast.stderr#L4-L20

Were you referring to a different file when you said "also happened in AST" ? Or do you mean that the above diagnostic is analogous to what NLL is being changed to produced above?

(To be clear, I think AST is currently producing a third totally different diagnostic message right now for this case...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note in particular that the AST-borrowck error message is highlighting a use of x in the second match arm, while NLL is highlighting a use of addr in the first match arm.)

I'm not saying I object to the change. I'm just trying to understand the comment you have written here on github about it.

| ---- first borrow later used here
LL | //[mir]~^ ERROR [E0499]
LL | 2 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
| ^^^^^^ second mutable borrow occurs here
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time
LL | 1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
| ---- ^^^^^^ second mutable borrow occurs here
| |
| first borrow used here, in later iteration of loop
| first borrow later used here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe less good, but the example is also kind of weird

...
LL | _ => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
| ------ first mutable borrow occurs here
Expand All @@ -13,7 +13,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time
--> $DIR/borrowck-mut-borrow-linear-errors.rs:15:30
|
LL | 1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
| ---- first borrow used here, in later iteration of loop
| ---- first borrow later used here
LL | //[mir]~^ ERROR [E0499]
LL | 2 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
| ^^^^^^ second mutable borrow occurs here
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/borrowck/mut-borrow-outside-loop.nll.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ LL | let inner_second = &mut inner_void; //~ ERROR cannot borrow
| ^^^^^^^^^^^^^^^ second mutable borrow occurs here
LL | inner_second.use_mut();
LL | inner_first.use_mut();
| ----------- first borrow used here, in later iteration of loop
| ----------- first borrow later used here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better


error: aborting due to 2 previous errors

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | let v: Vec<&str> = line.split_whitespace().collect();
| ^^^^ borrowed value does not live long enough
...
LL | acc += cnt2;
| --- borrow used here, in later iteration of loop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway the error is still ungreat :)

| --- borrow later used here
...
LL | }
| - `line` dropped here while still borrowed
Expand Down
49 changes: 49 additions & 0 deletions src/test/ui/nll/issue-53773.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#![feature(nll)]

struct Archive;
struct ArchiveIterator<'a> {
x: &'a Archive,
}
struct ArchiveChild<'a> {
x: &'a Archive,
}

struct A {
raw: &'static mut Archive,
}
struct Iter<'a> {
raw: &'a mut ArchiveIterator<'a>,
}
struct C<'a> {
raw: &'a mut ArchiveChild<'a>,
}

impl A {
pub fn iter(&self) -> Iter<'_> {
panic!()
}
}
impl Drop for A {
fn drop(&mut self) {}
}
impl<'a> Drop for C<'a> {
fn drop(&mut self) {}
}

impl<'a> Iterator for Iter<'a> {
type Item = C<'a>;
fn next(&mut self) -> Option<C<'a>> {
panic!()
}
}

fn error(archive: &A) {
let mut members: Vec<&mut ArchiveChild<'_>> = vec![];
for child in archive.iter() {
members.push(child.raw);
//~^ ERROR borrow may still be in use when destructor runs [E0713]
}
members.len();
}

fn main() {}
16 changes: 16 additions & 0 deletions src/test/ui/nll/issue-53773.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0713]: borrow may still be in use when destructor runs
--> $DIR/issue-53773.rs:43:22
|
LL | members.push(child.raw);
| ^^^^^^^^^
LL | //~^ ERROR borrow may still be in use when destructor runs [E0713]
LL | }
| - here, drop of `child` needs exclusive access to `*child.raw`, because the type `C<'_>` implements the `Drop` trait
LL | members.len();
| ------- borrow later used here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good

|
= note: consider using a `let` binding to create a longer lived value

error: aborting due to previous error

For more information about this error, try `rustc --explain E0713`.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ LL | foo.mutate();
| ^^^^^^^^^^^^ mutable borrow occurs here
LL | //~^ ERROR cannot borrow `foo` as mutable
LL | println!("foo={:?}", *string);
| ------- immutable borrow used here, in later iteration of loop
| ------- immutable borrow later used here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better


error: aborting due to previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0597]: `x` does not live long enough
--> $DIR/regions-escape-loop-via-variable.rs:11:13
|
LL | let x = 1 + *p;
| -- borrow used here, in later iteration of loop
| -- borrow later used here
LL | p = &x;
| ^^ borrowed value does not live long enough
LL | }
Expand Down
8 changes: 4 additions & 4 deletions src/test/ui/span/regions-escape-loop-via-vec.nll.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ LL | while x < 10 { //~ ERROR cannot use `x` because it was mutably borrowed
| ^ use of borrowed `x`
LL | let mut z = x; //~ ERROR cannot use `x` because it was mutably borrowed
LL | _y.push(&mut z);
| -- borrow used here, in later iteration of loop
| -- borrow later used here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better


error[E0503]: cannot use `x` because it was mutably borrowed
--> $DIR/regions-escape-loop-via-vec.rs:6:21
Expand All @@ -18,15 +18,15 @@ LL | while x < 10 { //~ ERROR cannot use `x` because it was mutably borrowed
LL | let mut z = x; //~ ERROR cannot use `x` because it was mutably borrowed
| ^ use of borrowed `x`
LL | _y.push(&mut z);
| -- borrow used here, in later iteration of loop
| -- borrow later used here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better


error[E0597]: `z` does not live long enough
--> $DIR/regions-escape-loop-via-vec.rs:7:17
|
LL | _y.push(&mut z);
| -- ^^^^^^ borrowed value does not live long enough
| |
| borrow used here, in later iteration of loop
| borrow later used here
...
LL | }
| - `z` dropped here while still borrowed
Expand All @@ -38,7 +38,7 @@ LL | let mut _y = vec![&mut x];
| ------ borrow of `x` occurs here
...
LL | _y.push(&mut z);
| -- borrow used here, in later iteration of loop
| -- borrow later used here
LL | //~^ ERROR `z` does not live long enough
LL | x += 1; //~ ERROR cannot assign
| ^^^^^^ use of borrowed `x`
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/vec/vec-mut-iter-borrow.nll.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | for x in &mut xs {
| -------
| |
| first mutable borrow occurs here
| first borrow used here, in later iteration of loop
| first borrow later used here
LL | xs.push(1) //~ ERROR cannot borrow `xs`
| ^^ second mutable borrow occurs here

Expand Down