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: Poor borrow checker error message when extension of borrow happens indirectly (e.g. via method) #54256

Open
ghost opened this issue Sep 15, 2018 · 10 comments
Assignees
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non Lexical Lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ghost
Copy link

ghost commented Sep 15, 2018

EDIT: Go here for a smaller example.

Code:

#![feature(nll)]

extern crate clang;

use std::collections::HashMap;
use std::error::Error;
use clang::*;

#[derive(Default)]
struct SymbolDesc<'a> {
    deps: Vec<Entity<'a>>,
}

fn visit<'a>(_entity: Entity<'a>, _desc: &mut SymbolDesc<'a>) {
    // snip
}

fn main() -> Result<(), Box<Error>> {
    let clang = Clang::new()?;

    let index = Index::new(&clang, false, false);
    let sources = vec!["examples/simple.c", "examples/simple_impl.c"];

    let mut tus = vec![];
    let mut sym_table: HashMap<String, SymbolDesc<'_>> = HashMap::new();

    for source in sources {
        tus.push(index.parser(source).parse()?);

        for child in tus.last().unwrap().get_entity().get_children() {
            if let Some(child_name) = child.get_name() {
                let desc = sym_table.entry(child_name).or_default();
                visit(child, desc);
            }
        }
    }

    Ok(())
}

Error:

error[E0502]: cannot borrow `tus` as mutable because it is also borrowed as immutable
  --> src/main.rs:28:9
   |
28 |         tus.push(index.parser(source).parse()?);
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
29 | 
30 |         for child in tus.last().unwrap().get_entity().get_children() {
   |                      --- immutable borrow occurs here
31 |             if let Some(child_name) = child.get_name() {
32 |                 let desc = sym_table.entry(child_name).or_default();
   |                            --------- borrow used here in later iteration of loop

The crux of the problem is that at line visit(child, desc), table starts borrowing tu because it has type HashMap<String, SymbolDesc<'tu>>. This prevents tus.push(index.parser(source).parse()?) because it needs a mutable borrow.

I wish the error message mentioned that table borrows tu because that's far from obvious. But once you see it, it's understandable why the borrow checker is not happy.

@zackmdavis zackmdavis added the A-diagnostics Area: Messages for errors, warnings, and lints label Sep 15, 2018
@memoryruins
Copy link
Contributor

Note for team: tried it locally; the error message is the same with and without #![feature(nll)].

@estebank estebank added the A-borrow-checker Area: The borrow checker label Sep 16, 2018
@pnkfelix pnkfelix added the A-NLL Area: Non Lexical Lifetimes (NLL) label Sep 18, 2018
@matthewjasper matthewjasper added the NLL-diagnostics Working torwads the "diagnostic parity" goal label Sep 22, 2018
@nikomatsakis nikomatsakis added this to the Edition 2018 RC 2 milestone Sep 25, 2018
@nikomatsakis
Copy link
Contributor

I've added to the RC2 milestone but I consider this a "nice to have". That said, I've been wanting for a while to have borrows be able to explain better how the reference came to be used at a particular spot — this overlaps with other sorts of region errors that could make use of a similar trace. It'd be helpful to brainstorm what we think the error should look like, I think.

@nikomatsakis
Copy link
Contributor

Can we maybe get a standalone example that doesn't use clang?

@ghost
Copy link
Author

ghost commented Oct 30, 2018

@nikomatsakis Here's a smaller example (playground link):

#![feature(nll)]

struct Parent;
struct Child<'a>(Option<&'a Parent>);

// the signature implies that the child borrows the parent
fn adopt<'a>(p: &'a Parent, c: &mut Child<'a>) {
    c.0 = Some(p);
}

fn main() {
    let mut parents: Vec<Parent> = vec![];
    let mut children: Vec<Child<'_>> = vec![];

    for _ in 0..2 {
        parents.push(Parent);
        let p = parents.last().unwrap();
        
        children.push(Child(None));
        let c = children.last_mut().unwrap();
        
        adopt(p, c); // comment this line to make the program compile
    }
}

Error:

error[E0502]: cannot borrow `parents` as mutable because it is also borrowed as immutable
  --> src/main.rs:16:9
   |
16 |         parents.push(Parent);
   |         ^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
17 |         let p = parents.last().unwrap();
   |                 ------- immutable borrow occurs here
18 |         
19 |         children.push(Child(None));
   |         -------- immutable borrow used here, in later iteration of loop

Removing adopt(p, c); will make the program compile, but the error message doesn't mention that line at all.

It's important to note that, even though in this example it's fairly obvious that children borrow parents, in the original post it was not.

@nikomatsakis nikomatsakis removed this from the Rust 2018 Release milestone Nov 13, 2018
@nikomatsakis
Copy link
Contributor

Removing from the Rust 2018 milestone -- this is not a Rust 2018 blocker.

@pnkfelix pnkfelix changed the title Poor borrow checker error message NLL: Poor borrow checker error message when extension of borrow happens indirectly (e.g. via method) Feb 20, 2019
@pnkfelix
Copy link
Member

NLL triage. Made title more informative. P-medium.

@pnkfelix pnkfelix added the P-medium Medium priority label Feb 20, 2019
@oli-obk oli-obk added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label May 24, 2019
@oli-obk
Copy link
Contributor

oli-obk commented May 24, 2019

Diagnostics triage: we believe this issue can be resolved by explaining why something is being borrowed instead of just where. Will need some digging in the NLL diagnostics machinery.

@crlf0710 crlf0710 added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 11, 2020
@Aaron1011
Copy link
Member

This now produces the same error message both with and without #![feature(nll)]

@Aaron1011 Aaron1011 removed the NLL-diagnostics Working torwads the "diagnostic parity" goal label Oct 2, 2021
@ocstl
Copy link

ocstl commented Jan 5, 2022

I don't know if it helps, but even without the adopt function (playground link):

struct Parent;
struct Child<'a>(Option<&'a Parent>);

fn main() {
    let mut parents: Vec<Parent> = vec![];
    let mut children: Vec<Child<'_>> = vec![];

    for _ in 0..2 {
        parents.push(Parent);
        let p = parents.last().unwrap();
        
        children.push(Child(None));
        children.last_mut().unwrap().0 = Some(p);
    }
}

We get the same error message:

error[E0502]: cannot borrow `parents` as mutable because it is also borrowed as immutable
  --> src/main.rs:9:9
   |
9  |         parents.push(Parent);
   |         ^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
10 |         let p = parents.last().unwrap();
   |                 -------------- immutable borrow occurs here
11 |         
12 |         children.push(Child(None));
   |         -------------------------- immutable borrow later used here

@lf-
Copy link
Contributor

lf- commented Jun 16, 2023

I hit this using httparse, in a way that looks like this:

use std::marker::PhantomData;

struct Parser<'headers, 'buf>(PhantomData<(&'headers (), &'buf ())>);

impl<'headers, 'buf> Parser<'headers, 'buf> {
    fn new(_headers: &'headers mut ()) -> Self {
        Self(PhantomData)
    }

    fn get_thing(&self) -> &str {
        "nya"
    }

    fn parse(&mut self, _buf: &'buf [u8]) {}
}

fn main() {
    let mut headers = ();
    let mut buf = Vec::new();
    let mut p = Parser::new(&mut headers);

    p.parse(&buf);
    buf.clear();
    p.get_thing();
}

Playground

Error:

Compiling playground v0.0.1 (/playground)
error[[E0502]](https://doc.rust-lang.org/stable/error_codes/E0502.html): cannot borrow `buf` as mutable because it is also borrowed as immutable
  --> src/main.rs:23:5
   |
22 |     p.parse(&buf);
   |             ---- immutable borrow occurs here
23 |     buf.clear();
   |     ^^^^^^^^^^^ mutable borrow occurs here
24 |     p.get_thing();
   |     ------------- immutable borrow later used here

For more information about this error, try `rustc --explain E0502`.
error: could not compile `playground` (bin "playground") due to previous error

I think that qualitatively the reason that this is confusing to me is that most of the time, if you lend something a reference, it gives it back right after. I wound up confused because in this case, the reference is made to outlive the lifetime of another value and is not given back. So a more helpful diagnostic for this particular problem needs to point out this condition being at play: a lifetime is extended by the function call.

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 A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non Lexical Lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-medium Medium priority 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