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

Erroneous borrow error from 2018 edition's NLL migration mode. #52967

Closed
nrc opened this issue Aug 2, 2018 · 4 comments
Closed

Erroneous borrow error from 2018 edition's NLL migration mode. #52967

nrc opened this issue Aug 2, 2018 · 4 comments
Assignees
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nrc
Copy link
Member

nrc commented Aug 2, 2018

I have:

pub fn infer_defaults(&mut self, project_dir: &Path) -> CargoResult<()> {
    ....
    match &mut self.target_dir {
            Inferrable::Specified(Some(ref mut path)) if path.is_relative() => {
                *path = project_dir.join(&path);
            }
            _ => {},
        }
    ...
}

(from https://github.com/rust-lang-nursery/rls/blob/master/src/config.rs#L238-L277)

And I get the following warning:

warning[E0502]: cannot borrow `_` as mutable because it is also borrowed as immutable
   --> src/config.rs:259:40
    |
257 |         match &mut self.target_dir {
    |               -------------------- immutable borrow occurs here
258 |             // We require an absolute path, so adjust a relative one if it's passed.
259 |             Inferrable::Specified(Some(ref mut path)) if path.is_relative() => {
    |                                        ^^^^^^^^^^^^ mutable borrow occurs here
...
262 |             _ => {},
    |             - borrow later used here
    |
    = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
            It represents potential unsoundness in your code.
            This warning will become a hard error in the future.

The warning is the same without the &mut.

It seems to me this code is fine and there should not be a warning.

cc @nikomatsakis @pnkfelix

@nrc nrc added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-NLL Area: Non Lexical Lifetimes (NLL) labels Aug 2, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Aug 2, 2018

Smaller example (playground):

fn main() {
    let mut stuff = ("left", "right");
    match stuff {
        (ref mut left, _) if *left == "left" => { }
        _ => {}
    }
}

which emits the diagnostic:

warning[E0502]: cannot borrow `stuff.0` as mutable because it is also borrowed as immutable
 --> src/main.rs:4:10
  |
3 |     match stuff {
  |           ----- immutable borrow occurs here
4 |         (ref mut left, _) if *left == "left" => { }
  |          ^^^^^^^^^^^^ mutable borrow occurs here
5 |         _ => {}
  |         - borrow later used here
  |
  = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
          It represents potential unsoundness in your code.
          This warning will become a hard error in the future.

@pnkfelix
Copy link
Member

pnkfelix commented Aug 2, 2018

Here's the (very?) good news: this is not an NLL bug, per se.

In particular, when we opt into NLL explicitly, the example works (playground):

#![feature(nll)]

fn main() {
    let mut stuff = ("left", "right");
    match stuff {
        (ref mut left, _) if *left == "left" => { }
        _ => {}
    }
}

There is a bug in how I linked the migration mode into the 2018 edition (PR #52681). Basically, its not toggling on enough switches, and one of the ones I missed is causing something to break with the new way we handle match.

@pnkfelix pnkfelix changed the title Erroneous borrow error from NLL Erroneous borrow error from 2018 edition's NLL migration mode. Aug 2, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Aug 2, 2018

This issue strikes me as an argument that we should turn on something analogous to compare-mode=nll in compiletest, except specialized to just the 2018 edition opt-in, rather than turning on NLL via debug switches.

@pnkfelix pnkfelix self-assigned this Aug 2, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Aug 2, 2018

In particular, you see the error with -Z borrowck=mir, but not with -Z borrowck=mir -Z two-phase-borrows. That's a pretty strong hint that I forgot to have edition=2008 imply two-phase-borrows.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Aug 2, 2018
bors added a commit that referenced this issue Aug 2, 2018
…ase-borrows, r=Mark-Simulacrum

NLL migration in the 2018 edition needs two-phase borrows too!

NLL migration in the 2018 edition needs two-phase borrows too!

Fix #52967.
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) 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

2 participants