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

if let keeps borrows too long #43407

Closed
oxalica opened this issue Jul 22, 2017 · 6 comments · Fixed by #59114
Closed

if let keeps borrows too long #43407

oxalica opened this issue Jul 22, 2017 · 6 comments · Fixed by #59114
Labels
A-lifetimes Area: Lifetimes / regions C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled.

Comments

@oxalica
Copy link
Contributor

oxalica commented Jul 22, 2017

Borrows in patterns are kept during the whole if let expression, including the else branch, which is counterintuitive and annoying.
For example, this doesn't compile.

let mut x = Some(1);
if let Some(&ref p) = x.as_ref() {
    // do something with p
} else {
    x = Some(2); // error: x is still borrowed
}

To pass the borrow check, you have to write ugly code like this.

let mut x = Some(1);
let mut flag = false;
if let Some(&ref p) = x.as_ref() {
    // do something with p
} else {
    flag = true;
}
if flag {
    x = Some(2);
}
@qnighy
Copy link
Contributor

qnighy commented Jul 22, 2017

This seems to be the known limitation of the current borrow checker. #35132 is one of the similar issues.

@oxalica
Copy link
Contributor Author

oxalica commented Jul 22, 2017

Unlike match mentioned in #35132, if let with else has two braces and seems to have two different lexical scope naturally, and borrows should only live the first scope.
On the other hand, although non-lexical lifetimes do fix both issues, I think this one is easier to resolve by simply desugering code like the latter I've shown.

@qnighy
Copy link
Contributor

qnighy commented Jul 22, 2017

So you meant to change desugaring of if let, the current behavior of which leads to the very NLL example?
To me it looks a bit difficult to apply your desugaring in general, since if let can have types other than ().

Perhaps rust-lang/rfcs#2068 is also related?

@codyps
Copy link
Contributor

codyps commented Jul 23, 2017

I don't think desugaring will be an issue. At least handling a return value works with the example below (using fake macro-like syntax, and likely in need of some NoDrop to avoid issues where the expression is Drop)

if let $p:pattern = $ei:expr {
    $em:expr
} else {
    $ef:expr
}

Transformed to:

{
    let mut flag = false;
    let mut v = if let $p = $ei {
       $em
    } else {
        flag = true;
        unsafe { uninitialized() }
    };
    if flag {
        v = $ef;
    }
    v
}

A specific example (https://play.rust-lang.org/?gist=205359c0924e50efa8974f1f73250419&version=stable)

let mut x = Some(1);
let res = {
    let mut flag = false;
    let mut v = if let Some(&ref p) = x.as_ref() {
        // do something with p
        println!("{}", p);
        p * 3
    } else {
        flag = true;
        unsafe { std::mem::uninitialized() }
    };
    if flag {
        x = Some(2);
        v = 7;
    }
   v
};

@Mark-Simulacrum Mark-Simulacrum added A-lifetimes Area: Lifetimes / regions C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Jul 26, 2017
@scottmcm
Copy link
Member

There are flagless desugars too, like

loop {
    {
        if let $p:pattern = $ei:expr {
            break $em:expr
        }
    }
    break $ef:expr
}

(Or the nicer version with labeled blocks, if inside the compiler or if that RFC is accepted)

@memoryruins
Copy link
Contributor

Triage: OP's example now compiles on 2018.
2015 errors:

error[E0506]: cannot assign to `x` because it is borrowed
 --> src/main.rs:6:9
  |
3 |     if let Some(&ref p) = x.as_ref() {
  |                           - borrow of `x` occurs here
...
6 |         x = Some(2); // error: x is still borrowed
  |         ^^^^^^^^^^^ assignment to borrowed `x` occurs here

rustc: 1.32.0

@memoryruins memoryruins added the NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. label Jan 30, 2019
bors added a commit that referenced this issue Mar 11, 2019
Enable NLL migrate mode on the 2015 edition

Blocked on #58739

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable the `-Ztwo-phase-borrows` flag on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests
* Remove the `-Zborrowck=compare` option
* Remove the `-Ztwo-phase-borrows` flag. It's kept so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
bors added a commit that referenced this issue Apr 22, 2019
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
bors added a commit that referenced this issue Apr 22, 2019
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants