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

Diagnostics for two-phase borrows are confusing and unhelpful #77826

Closed
Rik-de-Kort opened this issue Oct 11, 2020 · 5 comments
Closed

Diagnostics for two-phase borrows are confusing and unhelpful #77826

Rik-de-Kort opened this issue Oct 11, 2020 · 5 comments
Labels
D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Rik-de-Kort
Copy link

When I have a struct with two methods on the impl, both borrowing self mutably, I cannot chain the functions, like s.second(s.first()): I get a borrow checker error. Instead, I have to assign s.first() to an auxiliary variable. Might be related to #21906 but could not find any other similar issues.

struct S{}

impl S{
    fn first(&mut self) -> bool{
        true
    }
    
    fn second(&mut self, b: bool) {
        if b { println!{"Hello, world!"}; }
    }
}

fn main() {
    let mut s = S{};
    s.second(s.first());
}

I expected to see this happen: a simple print of "hello world!"

Instead, this happened:

   Compiling playground v0.0.1 (/playground)
error[E0499]: cannot borrow `s` as mutable more than once at a time
  --> src/main.rs:15:14
   |
15 |     s.second(s.first())
   |     - ------ ^ second mutable borrow occurs here
   |     | |
   |     | first borrow later used by call
   |     first mutable borrow occurs here

error: aborting due to previous error

For more information about this error, try `rustc --explain E0499`.
error: could not compile `playground`

To learn more, run the command again with --verbose.

It works fine if we use an auxiliary variable, which looks quite ugly to my mind:

fn main() {
    let mut s = S{};
    let f = s.first();
    s.second(s.first());
}

Playground link

@Rik-de-Kort Rik-de-Kort added the C-bug Category: This is a bug. label Oct 11, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 11, 2020

This is 'intended' behavior in that it's not a bug: your code desugars to

S::second(&mut s, S::first(&mut s))

and now you can see that it's borrowed for second before the call to first. That said, the behavior is definitely unintuitive; see #56254.

@jyn514 jyn514 added D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed C-bug Category: This is a bug. labels Oct 11, 2020
@jyn514 jyn514 changed the title Method composition with &mut self not borrowchecking Diagnostics for multi-stage borrows are confusing and unhelpful Oct 11, 2020
@jyn514 jyn514 changed the title Diagnostics for multi-stage borrows are confusing and unhelpful Diagnostics for two-phase borrows are confusing and unhelpful Oct 11, 2020
@camelid
Copy link
Member

camelid commented Oct 11, 2020

Implementing #77834 should fix this.

@Rik-de-Kort
Copy link
Author

This is 'intended' behavior in that it's not a bug: your code desugars to

S::second(&mut s, S::first(&mut s))

and now you can see that it's borrowed for second before the call to first. That said, the behavior is definitely unintuitive; see #56254.

This means I can learn something here, because I thought Rust was call by value? So wouldn't (or shouldn't) the bytecode calculate S::first(&mut s) first before entering the function? Or is there some parallel evaluation of arguments magic going on so that the compiler can't guarantee this?

@jyn514
Copy link
Member

jyn514 commented Oct 12, 2020

Rust borrows the arguments in order: because &mut s comes before the call to first(), it's mutably borrowed before and can't be borrowed again. 'call by value' doesn't have anything to do with it.

@jyn514
Copy link
Member

jyn514 commented Dec 12, 2020

I'm going to close this as a duplicate of #77834, but I agree the diagnostic here should be improved.

@jyn514 jyn514 closed this as completed Dec 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants