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 breaks usage of AddAssign #48129

Closed
Aaron1011 opened this issue Feb 10, 2018 · 7 comments
Closed

NLL breaks usage of AddAssign #48129

Aaron1011 opened this issue Feb 10, 2018 · 7 comments
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Feb 10, 2018

When trying to reproduce #48071, I came across another issue in libcompiler_builtins. The original errors were:

error[E0503]: cannot use `result` because it was mutably borrowed
   --> rustc/compiler_builtins_shim/../../libcompiler_builtins/src/float/add.rs:179:46
    |
179 |     if round_guard_sticky == 0x4 { result += result & one; }
    |                                    ------    ^^^^^^ use of borrowed `result`
    |                                    |
    |                                    borrow of `result` occurs here

error[E0503]: cannot use `a` because it was mutably borrowed
  --> rustc/compiler_builtins_shim/../../libcompiler_builtins/src/float/pow.rs:18:18
   |
18 |             a *= a;
   |             -    ^ use of borrowed `a`
   |             |
   |             borrow of `a` occurs here

error[E0503]: cannot use `product_high` because it was mutably borrowed
   --> rustc/compiler_builtins_shim/../../libcompiler_builtins/src/float/mul.rs:173:25
    |
173 |         product_high += product_high & one;
    |         ------------    ^^^^^^^^^^^^ use of borrowed `product_high`

I managed to reduce the second error to this example (playground):

#![feature(nll)]

use std::ops::*;

#[derive(Copy, Clone)]
struct Foo;

impl AddAssign for Foo {
    fn add_assign(&mut self, _rhs: Foo) {
        panic!()
    }
}

impl Foo {
    fn use_self(&mut self, _other: Foo) {
        panic!()
    }
}

fn bar() {
    let mut a = Foo;
    let mut b = Foo;
    
    b.use_self(b);
    a += a;
}

fn main() {}

which fails to compile with this error:

error[E0503]: cannot use `a` because it was mutably borrowed
  --> src/main.rs:25:10
   |
25 |     a += a;
   |     -    ^ use of borrowed `a`
   |     |
   |     borrow of `a` occurs here

As expected, removing #![feature(nll)] causes a += a to compile, but b.use_self(b); to fail with this error:

error[E0503]: cannot use `b` because it was mutably borrowed
  --> src/main.rs:24:16
   |
24 |     b.use_self(b);
   |     -          ^ use of borrowed `b`
   |     |
   |     borrow of `b` occurs here

I assume that this last error is due to the lack of two-phase borrows on ast-borrowck, and is not something that will be fixed. However, I expect NLL to compile both the operator and 'desuraged operator' version.

@Aaron1011
Copy link
Member Author

I'd like to work on this.

@Aaron1011
Copy link
Member Author

Oddly enough, the precise syntax used to invoke AddAssign.add_assign seems to be what causes it: (playground):

#![feature(nll)]

use std::ops::*;

#[derive(Copy, Clone)]
struct Foo;

impl AddAssign for Foo {
    fn add_assign(&mut self, _rhs: Foo) {
        panic!()
    }
}

fn bar() {
    let mut a = Foo;
 
    a += a;   
    a.add_assign(a);

}

fn main() {}

@Aaron1011
Copy link
Member Author

I happened to run this with a slightly outdated nightly build. It seems that this is a regression: the last snippet worked on rustc 1.25.0-nightly (bacb5c58d 2018-01-26), and is currently broken on rustc 1.25.0-nightly (3bcda48a3 2018-02-09)

@Aaron1011
Copy link
Member Author

Upon further investigation, this appears to be intentional: https://github.com/rust-lang/rust/pull/47489/files#diff-b3d7798e2a3c43eca68203216e36de65R544

However, this ends up breaking existing code in libcompiler_builtins, and seems extremely inconsistent (explicitly calling an operator method like add_assign will make it compile).

@pnkfelix: Seeing as this was your PR, do you think it makes sense to extend two-phase borrows to overloaded operators in order to preserve compatibility with existing code?

@pietroalbini pietroalbini added A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. WG-compiler-nll T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 11, 2018
@abusch
Copy link

abusch commented Feb 11, 2018

I ran into this as well. I have some code like the following that suddenly stopped compiling with the latest nightly:

#![feature(nll)]

use std::num::Wrapping;

fn main() {
    let mut x = Wrapping(1234u32);
    x ^= x >> 2;
    
    println!("{}", x);
}

It does compile and runs fine without the nll feature.

@nikomatsakis
Copy link
Contributor

I think extending two-phase borrows to cover this case seems reasonable.

@Manishearth
Copy link
Member

Manishearth commented Feb 13, 2018

The use_self isn't necessary to show that this is a regression, see #48175

#![feature(nll)]
use std::ops::*;

#[derive(Copy, Clone)]
struct Au(u32);

impl Add<Au> for Au {
    type Output = Au;
    fn add(self, other: Au) -> Au {
        Au(self.0 + other.0)
    }
}

impl AddAssign<Au> for Au {
    fn add_assign(&mut self, other: Au) {
        *self = Au(self.0 + other.0)
    }
}

fn main() {
    let mut foo = vec![Au(4), Au(5), Au(6)];
    foo[2] += foo[2];
}

(playpen)

@sapphire-arches sapphire-arches added this to the NLL: Valid code works milestone Feb 13, 2018
sapphire-arches added a commit to sapphire-arches/rust that referenced this issue Feb 14, 2018
We need two-phase borrows of ops to be in the initial NLL release since without
them lots of existing code will break. Fixes rust-lang#48129
Manishearth added a commit to Manishearth/rust that referenced this issue Feb 19, 2018
…, r=nikomatsakis

Allow two-phase borrows of &mut self in ops

We need two-phase borrows of ops to be in the initial NLL release since without them lots of existing code will break. Fixes rust-lang#48129.
CC @pnkfelix  and @nikomatsakis

r? @pnkfelix
Manishearth added a commit to Manishearth/rust that referenced this issue Feb 24, 2018
…, r=nikomatsakis

Allow two-phase borrows of &mut self in ops

We need two-phase borrows of ops to be in the initial NLL release since without them lots of existing code will break. Fixes rust-lang#48129.
CC @pnkfelix  and @nikomatsakis

r? @pnkfelix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) 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

6 participants