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 compilation error with vec.swap(0, vec.len() - 1), while vec.push(vec.len()) works. #74319

Open
marcospb19 opened this issue Jul 14, 2020 · 11 comments
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@marcospb19
Copy link
Contributor

I tried compiling this code (here is the playgound link):

fn main() {
    let mut vec = vec![];
    
    vec.push(vec.len());
    vec.push(vec.len() - 1);    // This works!
    vec.swap(0, vec.len() - 1); // Error
}

I expected a Successful compilation, but this happened:

error[E0502]: cannot borrow `vec` as immutable because it is also borrowed as mutable
 --> src/main.rs:6:17
  |
6 |     vec.swap(0, vec.len() - 1);
  |     --- ----    ^^^ immutable borrow occurs here
  |     |   |
  |     |   mutable borrow later used by call
  |     mutable borrow occurs here

Note 1: that the line above the error also contains vec.len(), but it compiles.
Note 2: vec.swap(0, vec.len()); produces the same error (nothing to do with - 1).

Meta

rustc --version --verbose:

rustc 1.44.1
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.44.1
LLVM version: 10.0

Also confirmed in the playground against Nightly and Beta.

@marcospb19 marcospb19 added the C-bug Category: This is a bug. label Jul 14, 2020
@jonas-schievink
Copy link
Contributor

two-phase borrows don't seem to work with autoderef at all, but I'm not sure if there's a canonical issue for that

@dlight
Copy link

dlight commented Jul 14, 2020

Is autoderef happening in both of them or just in the vec.swap(..) case?

@jonas-schievink
Copy link
Contributor

push is a method on Vec, so no autoderef is needed. swap is defined on slices, so one autoderef step is needed to call it.

@dlight
Copy link

dlight commented Jul 14, 2020

swap is defined on slices but also on Vec, so no autoderef should be needed.

edit: actually, no, the method on Vec is the slice one.. this should be displayed in the docs somehow. it is

@jonas-schievink
Copy link
Contributor

No, that is in "Methods from Deref<Target = [T]>"

@Alexendoo Alexendoo added A-borrow-checker Area: The borrow checker T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 15, 2020
@nbdd0121
Copy link
Contributor

That line is actually

<[_]>::swap(<Vec<_> as DerefMut>::deref_mut(&mut vec), 0, Vec::len(&vec) - 1);

And currently the <Vec<_> as DerefMut>::deref_mut would have to evaluate before Vec::len, which will fail borrowck.

@marcospb19
Copy link
Contributor Author

I ran into the same issue again.

This did not compiled:

merge_sort(&mut v[..v.len()]);
merge_sort(&mut v[v.len()..]);

But this did:

let temp = v.len() / 2;
merge_sort(&mut v[..temp]);
merge_sort(&mut v[temp..]);

The signature:

fn merge_sort(v: &mut [i32]) { ... }

I'm out of context, are there any other other opened issues referencing this problem?

Will Polonius be able to fix it?

@nbdd0121
Copy link
Contributor

nbdd0121 commented Aug 6, 2020

Will Polonius be able to fix it?

No. As I mentioned earlier, the desugared code will use a mutable borrow before an immutable one, so this code should not compile, otherwise it is unsound in general. To make NLL work across autoderef would likely be a langauge-level major change because the compiler'll need to move the location of autoderef.

@yume-chan
Copy link

yume-chan commented Jul 3, 2021

This doesn't compile:

vec[vec.len() - 1] = 0;

But this does: (playground)

use std::ops::IndexMut;
*vec.index_mut(vec.len() - 1) = 0;

The doc of IndexMut says

container[index] is actually syntactic sugar for *container.index_mut(index), but only when used as a mutable value.

No, clearly not, there is magic between them.


But this doesn't compile

*vec.get_mut(vec.len() - 1).unwrap() = 0;

Nor this:

*std::ops::IndexMut::index_mut(&mut vec, vec.len() - 1) = 0;

What's the difference?

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 3, 2021

No, clearly not, there is magic between them.

container[index] is indeed just a syntactic sugar, but it does not have two-phase borrow enabled, while two-phase borrow is enabled for a plain method call. This is actually intentional.

// Deref/indexing can be desugared to a method call,
// so maybe we could use two-phase here.
// See the documentation of AllowTwoPhase for why that's
// not the case today.
allow_two_phase_borrow: AllowTwoPhase::No,

Related: #49434

@pnkfelix
Copy link
Member

pnkfelix commented Dec 14, 2022

(I had put a big comment here but then I realized that this issue isn't specifically about IndexMut, so I moved my comment over to #58419, which is specifically about IndexMut (I think)...)

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants