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

Non-lexical borrow scopes and better treatment of nested method calls #811

Closed
pnkfelix opened this issue Feb 5, 2015 · 41 comments
Closed

Non-lexical borrow scopes and better treatment of nested method calls #811

pnkfelix opened this issue Feb 5, 2015 · 41 comments
Labels

Comments

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 5, 2015

In general, it can be useful to have support for borrows that aren't tied to lexical scopes.

Related links:

UPDATE: The relevant RFCs have been accepted. See rust-lang/rust#43234 to track implementation progress here.

@pnkfelix pnkfelix added the postponed label Feb 5, 2015
@nikomatsakis nikomatsakis changed the title Single-entry / multiple-exit regions for borrows Non-lexical borrow scopes and better treatment of nested method calls Apr 16, 2015
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jun 12, 2015
Most of these are old, but some specific messages for specific tests:

* trait-contravariant-self.rs: failed due to a soundess hole:
  rust-lang@05e3248

* process-detatch: rust-lang@15966c3
  says "this test is being ignored until signals are implemented" That's
  not happening for a long time, and when it is, we'll write tests for
  it.

* deep-vector{,2}.rs: "too big for our poor macro infrastructure", and has
  been ignored over a year.

* borrowck-nested-calls.rs's FIXME rust-lang#6268 was closed in favor of
  rust-lang/rfcs#811

* issue-15167.rs works properly now
* issue-9737.rs works properly now
* match-var-hygiene.rs works properly now
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jun 12, 2015
Most of these are old, but some specific messages for specific tests:

* trait-contravariant-self.rs: failed due to a soundess hole:
  rust-lang@05e3248

* process-detatch: rust-lang@15966c3
  says "this test is being ignored until signals are implemented" That's
  not happening for a long time, and when it is, we'll write tests for
  it.

* deep-vector{,2}.rs: "too big for our poor macro infrastructure", and has
  been ignored over a year.

* borrowck-nested-calls.rs's FIXME rust-lang#6268 was closed in favor of
  rust-lang/rfcs#811

* issue-15167.rs works properly now
* issue-9737.rs works properly now
* match-var-hygiene.rs works properly now

Addresses a chunk of rust-lang#3965
@barosl
Copy link
Contributor

@barosl barosl commented Jun 24, 2015

@mike-marcacci pointed out that the single-entry, multiple-exit regions issue also applies to the if let struct. While a code like

if let Some(val) = map.get_mut(&id) {
   /* ... */
} else {
   map.insert(id, new_val);
}

seems "innocent", it is actually just a syntax sugar for the match block, so the borrow lasts until the end of the else block. This particular case could be improved, or at least diagnostics could be, but I'm not sure fixing the specific point without introducing a general solution is a good idea.

thepowersgang added a commit to thepowersgang/rust that referenced this issue Jul 25, 2015
Most of these are old, but some specific messages for specific tests:

* trait-contravariant-self.rs: failed due to a soundess hole:
  rust-lang@05e3248

* process-detatch: rust-lang@15966c3
  says "this test is being ignored until signals are implemented" That's
  not happening for a long time, and when it is, we'll write tests for
  it.

* deep-vector{,2}.rs: "too big for our poor macro infrastructure", and has
  been ignored over a year.

* borrowck-nested-calls.rs's FIXME rust-lang#6268 was closed in favor of
  rust-lang/rfcs#811

* issue-15167.rs works properly now
* issue-9737.rs works properly now
* match-var-hygiene.rs works properly now

Addresses a chunk of rust-lang#3965
@flavius
Copy link

@flavius flavius commented Sep 13, 2015

What's the state of this feature? Any new plans and/or changes with this?

@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Oct 1, 2015

@flavius basically, the HIR/MIR work needs to be completed first.

@ebiggers
Copy link

@ebiggers ebiggers commented Feb 21, 2016

I would like to see this implemented. I believe I ran into this with the following code which is descending a prefix tree and inserting new nodes:

struct TrieNode {
    bitmask: u32,
    terminal: bool,
    children: Vec<Box<TrieNode>>,
}

impl TrieNode {
    fn new() -> TrieNode {
        TrieNode {
            bitmask: 0,
            terminal: false,
            children: Vec::with_capacity(2),
        }
    }

    fn insert(&mut self, word: &Vec<u8>) {
        let mut node = self;
        for b in word {
            let bit = 1u32 << b;
            let index = (node.bitmask & (bit - 1)).count_ones() as usize;
            if (node.bitmask & bit) == 0 {
                node.children.insert(index, Box::new(TrieNode::new()));
                node.bitmask |= bit;
            }
            node = &mut node.children[index];
        }
        node.terminal = true;
    }
}

The problematic line is this one:

node = &mut node.children[index];

This causes the Rust compiler to spew a page of errors:

test.rs:22:17: 22:30 error: cannot borrow `node.children` as mutable more than once at a time [E0499]
test.rs:22                 node.children.insert(index, Box::new(TrieNode::new()));
               ^~~~~~~~~~~~~
test.rs:22:17: 22:30 help: run `rustc --explain E0499` to see a detailed explanation
test.rs:25:25: 25:38 note: previous borrow of `node.children` occurs here; the mutable borrow prevents subsequent moves, borrows, or modification of `node.children` until the borrow ends
test.rs:25             node = &mut node.children[index];
                   ^~~~~~~~~~~~~
test.rs:28:6: 28:6 note: previous borrow ends here
test.rs:16     fn insert(&mut self, word: &Vec<u8>) {
...
test.rs:28     }
           ^
test.rs:25:13: 25:45 error: cannot assign to `node` because it is borrowed [E0506]
test.rs:25             node = &mut node.children[index];
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.rs:25:25: 25:38 note: borrow of `node` occurs here
test.rs:25             node = &mut node.children[index];
                   ^~~~~~~~~~~~~
test.rs:25:25: 25:38 error: cannot borrow `node.children` as mutable more than once at a time [E0499]
test.rs:25             node = &mut node.children[index];
                   ^~~~~~~~~~~~~
test.rs:25:25: 25:38 help: run `rustc --explain E0499` to see a detailed explanation
test.rs:25:25: 25:38 note: previous borrow of `node.children` occurs here; the mutable borrow prevents subsequent moves, borrows, or modification of `node.children` until the borrow ends
test.rs:25             node = &mut node.children[index];
                   ^~~~~~~~~~~~~
test.rs:28:6: 28:6 note: previous borrow ends here
test.rs:16     fn insert(&mut self, word: &Vec<u8>) {
...
test.rs:28     }

This can be "fixed" by replacing the line with this:

let tmp = node;
node = &mut tmp.children[index];

In my opinion this adds no value and simply serves as a means of appeasing the borrow checker. The program is correct either way; it seems to be purely a limitation of the borrow checker that it cannot determine this and the programmer has to explicitly add a temporary variable.

I understand there may be concern about making the borrow checker more complicated. However, I feel that usability takes precedence here. It's no secret that writing code the Rust compiler can prove is "safe" is one of the largest barriers to using Rust. Any change that makes the compiler smarter and able to correctly recognize more safe code as safe is, in my opinion, an improvement (at least, provided that it continues to follow well-defined rules).

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Feb 21, 2016

@ebiggers

Your issue here is reborrow vs. move, which is distinct from non-lexical borrows. You can also move node using &mut {node}.children[index] (see https://www.reddit.com/r/rust/comments/46rii1/when_to_borrow_move_or_copy/).

@ehiggs
Copy link

@ehiggs ehiggs commented Mar 21, 2016

Is this related to places where matching an option won't compile using as_mut but can be fixed up by using Some(ref mut ...)?

e.g. code like this can fail because foo is still borrowed:

match foo.as_mut() {
    Some(x) => x.bar(), // fn bar(&mut self)
    None => foo = Some(Foo::new(7))
} 

It can be worked around like

match foo {
    Some(ref mut x) => x.bar(),
    None => foo = Some(Foo::new(7))
}
@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Mar 22, 2016

That's a different issue - non-lexical borrows.

@ehiggs
Copy link

@ehiggs ehiggs commented Mar 22, 2016

@arielb1, ok. Is there an issue for that? It seems like the two chunks of code I pasted should be semantically equivalent. @bluss on IRC pointed me to this one suggesting that it was similar to the issue I faced.

@glaebhoerl
Copy link
Contributor

@glaebhoerl glaebhoerl commented Mar 22, 2016

@arielb1 The title of this issue is "Non-lexical borrow scopes and"...

@bluss
Copy link
Member

@bluss bluss commented Mar 22, 2016

@ehiggs I wouldn't call that a work around, it's the idiomatic solution.

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Mar 22, 2016

@glaebhoerl

Oops wrong RFC - I thought this was copy-vs-move.

@aidanhs
Copy link
Member

@aidanhs aidanhs commented Jul 18, 2016

Think this is related:

fn main() {
    let mut a = 1;
    let c;
    {
        let b = &mut a;
        c = &*b;
    }
    let d = &a;
}
cannot borrow `a` as immutable because it is also borrowed as mutable
@dbrgn
Copy link

@dbrgn dbrgn commented Jul 19, 2016

@aidanhs no, I think that is correct. If you visualize the implicit scopes:

fn main() {
  let mut a = 1;
  { 
    let c;
    {
      let b = &mut a;
      {
        c = &*b;
      }
    }
    let d = &a;
  }
}

...then you can see that you're trying to borrow a while it's still borrowed mutably by b.

While borrowing a mutable reference to a value, that refrence is the only way to access that value at all.

@ebkalderon
Copy link

@ebkalderon ebkalderon commented Oct 17, 2016

Just curious, what is the current status of this issue? Will work on non-lexical borrows resume now that MIR has landed on Nightly?

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Nov 8, 2016

Not sure if that was intentional or not, but beta and nightly allow the following piece of code to work (not panic at runtime):

use std::cell::RefCell;

fn main() {
    let mut data = RefCell::new(None);
    let inner = if data.borrow().is_none() {
        let d = 1337;
        *data.borrow_mut() = Some(d);
        d
    } else {
        data.borrow().unwrap()
    };
}

(found here: http://stackoverflow.com/questions/40482981/if-condition-remains-borrowed-in-body )

@eddyb
Copy link
Member

@eddyb eddyb commented Nov 8, 2016

It was intentional, the previous behavior was a long-standing bug that was only partially fixed before.

@radix
Copy link

@radix radix commented Dec 2, 2016

Can this ticket be closed? I see that all three of the linked issues in the description are closed. If it still needs to be open, is it also still appropriate to have the "postponed" label?

@istankovic
Copy link

@istankovic istankovic commented Dec 2, 2016

@radix The issue #29775 was linked to this one and closed at some point, however, I've just checked and it is still relevant. So if this one is to be closed, I suggest to reopen #29775 since it is stil relevant and represents a very basic use case.

@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Dec 2, 2016

Can this ticket be closed?

Why would we close it?

I see that all three of the linked issues in the description are closed.

That's because they're duplicates of this one.

If it still needs to be open, is it also still appropriate to have the "postponed" label?

Yes, as generally this means "we considered it but decided that it's not the right time yet." This is still true, as porting the borrowck to MIR is still a blocker.

@radix
Copy link

@radix radix commented Dec 2, 2016

@steveklabnik I apologize. I was just unclear on the status since recent comments seemed to indicate it, or some parts of it, had been fixed. I also didn't realize that the links in the description were duplicates, I assumed they were dependents. My error. I should have worded my message more carefully. I didn't mean to say that I think it should be closed, just that I was curious about its status.

Thank you for the information.

@istankovic I'm not sure which #29775 you are referring to, as I can't find a reference to that number on this page.

@istankovic
Copy link

@istankovic istankovic commented Dec 2, 2016

@radix sorry, I meant rust-lang/rust#29975

@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Dec 2, 2016

@radix it's all good! This is a very long-running and complicated issue, it's very easy to have done that. 😄

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 10, 2017

A rather long write-up about nested method calls specifically:

https://internals.rust-lang.org/t/accepting-nested-method-calls-with-an-mut-self-receiver/4588

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Jun 27, 2017

porting the borrowck to MIR is still a blocker.

What’s the status on this?


As far as I can tell, borrows being lexical means not only that some programs need to be more verbose, but also that some programs are impossible (at least with the desired API/characteristics). For example, I just tried to write something like this (reduced here):

pub enum Token {
    Whitespace,
    Ident(String),
    Number(i32),
}

pub struct Parser {
    /* ... */
}

impl Parser {
    pub fn advance_without_skipping_whitespace(&mut self) -> &Token {
        unimplemented!()
    }

    pub fn advance(&mut self) -> &Token {
        loop {
            match self.advance_without_skipping_whitespace() {
                &Token::Whitespace => {}
                result => return result
            }
        }
    }
}

Playpen

… and got this error message:

error[E0499]: cannot borrow `*self` as mutable more than once at a time
  --> <anon>:18:19
   |
18 |             match self.advance_without_skipping_whitespace() {
   |                   ^^^^
   |                   |
   |                   second mutable borrow occurs here
   |                   first mutable borrow occurs here
...
23 |     }
   |     - first borrow ends here

error: aborting due to previous error

The problem is the "first" borrow lasting for the whole function rather than just one iteration of the loop. I think that’s because the borrowed value is (in some code paths) returned, and borrow are lexical? I can’t find a way to rewrite this that compiles in today’s Rust, even tail recursion has the same problem as a loop.

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Jun 27, 2017

some programs are impossible

That may have been a bit over-dramatic, I found a work-around. (Not returning the result of advance_without_skipping_whitespace(), using break instead of return, and than returning self.current_token() outside the loop.)

But the same kind of issues happen all the time when porting existing parsing code to this new API. Enough that this approach is not viable, I may have to do somewhat-expensive cloning everywhere.

@pnkfelix
Copy link
Member Author

@pnkfelix pnkfelix commented Jun 27, 2017

porting the borrowck to MIR is still a blocker.

What’s the status on this?

It is in-progress. Some steps have recently landed, but its not 100% working yet.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 25, 2018

I think we can basically close this in favor of rust-lang/rust#43234

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.