rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.37k stars 12.72k forks source link

Borrow checker bug with double borrow #108704

Open MatiasLyyra opened 1 year ago

MatiasLyyra commented 1 year ago

Hi,

I came across following weird borrow check behaviour. If you uncomment elements = &mut elements[idx].children; and comment line below, program compiles ok. I think this is a bug.

I tried this code:

struct Root {
    children: Vec<Node>,
}

struct Node {
    name: String,
    children: Vec<Node>,
}

fn merge_tree(root: &mut Root, path: Vec<String>) {
    let mut elements = &mut root.children;

    for p in path.iter() {
        for (idx, el) in elements.iter_mut().enumerate() {
            if el.name == *p {
                // elements = &mut elements[idx].children;
                elements = &mut el.children;
                break;
            }
        }
    }
}

I expected to see this happen: No compiler error.

Instead, this happened: Following compiler error:

error[E0499]: cannot borrow `*elements` as mutable more than once at a time
  --> src/main.rs:29:26
   |
29 |         for (idx, el) in elements.iter_mut().enumerate() {
   |                          ^^^^^^^^^^^^^^^^^^^
   |                          |
   |                          `*elements` was mutably borrowed here in the previous iteration of the loop
   |                          first borrow used here, in later iteration of loop

Meta

rustc --version --verbose:

rustc 1.67.1 (d5a82bbd2 2023-02-07)
binary: rustc
commit-hash: d5a82bbd26e1ad8b7401f6a718a9c57c96905483
commit-date: 2023-02-07
host: aarch64-apple-darwin
release: 1.67.1
LLVM version: 15.0.6
Backtrace

``` N/A ```

823984418 commented 1 year ago

In fact, your question is equivalent to

fn merge_tree(root: &mut Root, path: Vec<String>) {
    let mut elements = &mut root.children;

    if let Some((idx, el)) = elements.iter_mut().enumerate().next() {
        // elements = &mut elements[idx].children;
        elements = &mut el.children;
    }
    if let Some((idx, el)) = elements.iter_mut().enumerate().next() {
        // elements = &mut elements[idx].children;
        elements = &mut el.children;
    }
}
NipaJ commented 1 year ago

I experimented more with this, and found that this is also equivalent. Also found a very suspicious workaround.

#[derive(Clone)]
pub struct Node {
    pub children: Vec<Node>,
}

pub fn works(mut root: Vec<Node>) {
    let mut null = Vec::new();
    let mut elements = &mut root;

    {
        let mut tmp = &mut null;
        if let Some(el) = elements.get_mut(0) {
            tmp = &mut el.children;
        }
        elements = tmp;
    }

    elements.clear();
}

pub fn breaks(mut root: Vec<Node>) {
    let mut elements = &mut root;

    if let Some(el) = elements.get_mut(0) {
        elements = &mut el.children;
    }

    elements.clear();
}
823984418 commented 1 year ago
pub fn breaks(mut root: Vec<Node>) {
    let mut elements = &mut root;

    if let Some(el) = elements.get_mut(0) {
        elements = &mut el.children;
    }

    elements.clear();
}

Note that these two pieces of code are not equivalent.

The code equivalent to works is:

pub fn breaks(mut root: Vec<Node>) {
    let mut null = Vec::new();
    let mut elements = &mut root;

    {
        if let Some(el) = elements.get_mut(0) {
            elements = &mut el.children;
        } else {
            elements = &mut null;
        }
    }

    elements.clear();
}

This is because get_mut is a method of &mut [T].

pub fn breaks(mut root: Vec<Node>) {
    let mut elements = &mut root;

    {
        if let Some(el) = elements.deref_mut().get_mut(0) { // element's ownership is taken away by deref_mut
            elements = &mut el.children; // this assignment extends the duration of ownership taken by deref_mut to the same as the lifetime of element
        } else {
            // Do Nothing
        }
        // element was borrowed all the lifetime, but only one branch returned it
    }

    elements.clear();
}
Enselic commented 4 months ago

The original code compiles with -Zpolonius.