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

Borrow checker gets confused with a mutable pointer which should be moved #27487

Closed
eefriedman opened this issue Aug 3, 2015 · 11 comments · Fixed by #59114
Closed

Borrow checker gets confused with a mutable pointer which should be moved #27487

eefriedman opened this issue Aug 3, 2015 · 11 comments · Fixed by #59114
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled.

Comments

@eefriedman
Copy link
Contributor

pub struct FragmentRepr(Vec<FragmentRepr>);
pub fn accepted(mut entry: &mut FragmentRepr) {
    loop {
        let FragmentRepr(ref mut discrs) = *{entry};
        entry = &mut discrs[0];
    }
}
pub fn rejected(mut entry: &mut FragmentRepr) {
    loop {
        let FragmentRepr(ref mut discrs) = *entry;
        entry = &mut discrs[0];
    }
}
fn main() {}

Rust accepts the first function, but rejects the second with "error: cannot borrow entry.0 as mutable more than once at a time". It would be nice not to be forced to insert random curly braces to convince the borrow checker that my code is correct.

@barosl
Copy link
Contributor

barosl commented Aug 3, 2015

I think the reason why the first is accepted is that blocks unintentionally move out the values of references in Rust. I believe this behavior is there by accident, so there's a question: is there an "official" way to do so?

@arielb1
Copy link
Contributor

arielb1 commented Aug 3, 2015

If entry isn't moved, then the issue here is that borrowck doesn't reason about &mut FragmentRepr lacking drop glue - any such drop glue could invalidate discrs in the second example.

@bluss
Copy link
Member

bluss commented Aug 4, 2015

@barosl I think the identity function solution is the best.

/// Identity function. Move the input to output.
fn moving<T>(x: T) -> T { x }

let current: &mut _;

// Force move like this
moving(current).next

// we can also write it like this:
let tmp = current;
tmp.next

// or this:
{current}.next

@steveklabnik steveklabnik added the A-borrow-checker Area: The borrow checker label Aug 4, 2015
@steveklabnik
Copy link
Member

Triage: so is this a bug or not?

@Mark-Simulacrum
Copy link
Member

@arielb1 Is the bug here "that borrowck doesn't reason about &mut FragmentRepr lacking drop glue"?

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 22, 2017
@memoryruins
Copy link
Contributor

memoryruins commented Jan 30, 2019

Triage: both functions compile (as well as when applied to a FragmentRepr) on 2018 edition.
2015 errors:

error[E0499]: cannot borrow `entry.0` as mutable more than once at a time
  --> src/main.rs:11:26
   |
11 |         let FragmentRepr(ref mut discrs) = *entry;
   |                          ^^^^^^^^^^^^^^ mutable borrow starts here in previous iteration of loop
...
14 | }
   | - mutable borrow ends here

error[E0506]: cannot assign to `entry` because it is borrowed
  --> src/main.rs:12:9
   |
11 |         let FragmentRepr(ref mut discrs) = *entry;
   |                          -------------- borrow of `entry` occurs here
12 |         entry = &mut discrs[0];
   |         ^^^^^^^^^^^^^^^^^^^^^^ assignment to borrowed `entry` occurs here

error: aborting due to 2 previous errors

rustc: 1.32.0

edit: removing fixed-by-nll tag as I am not certain how it affects drop glue. pointing this out in another triage thread.

@memoryruins memoryruins added NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. and removed NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. labels Jan 30, 2019
@arielb1
Copy link
Contributor

arielb1 commented Feb 6, 2019

This is indeed intentionally fixed by NLL. &mut FragmentRepr can't possibly have drop glue (it is Copy).

@arielb1 arielb1 added the NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. label Feb 6, 2019
@arielb1
Copy link
Contributor

arielb1 commented Feb 6, 2019

keywords: "copy-vs-move", "orphaned borrow". I seriously need to find the root test for this.

@arielb1
Copy link
Contributor

arielb1 commented Feb 6, 2019

duplicate of #46361

@arielb1
Copy link
Contributor

arielb1 commented Feb 6, 2019

The test is

// run-pass
#![feature(nll)]
struct List<T> {
value: T,
next: Option<Box<List<T>>>,
}
fn to_refs<T>(mut list: &mut List<T>) -> Vec<&mut T> {
let mut result = vec![];
loop {
result.push(&mut list.value);
if let Some(n) = list.next.as_mut() {
list = n;
} else {
return result;
}
}
}
fn main() {
let mut list = List { value: 1, next: None };
let vec = to_refs(&mut list);
assert_eq!(vec![&mut 1], vec);
}

@pnkfelix
Copy link
Member

okay since this has a test I won't tag it as E-needstest. (Presumably when NLL is turned on across the board we'll just remove #![feature(nll)] from all those tests.)

But I'm still going to leave it open since our policy is to leave NLL-fixed-by-NLL bugs open until we turn NLL on across the board

bors added a commit that referenced this issue Mar 11, 2019
Enable NLL migrate mode on the 2015 edition

Blocked on #58739

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable the `-Ztwo-phase-borrows` flag on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests
* Remove the `-Zborrowck=compare` option
* Remove the `-Ztwo-phase-borrows` flag. It's kept so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
bors added a commit that referenced this issue Apr 22, 2019
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
bors added a commit that referenced this issue Apr 22, 2019
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants