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

Destructuring boxes into multiple mutable references seems broken #30104

Closed
ben0x539 opened this issue Nov 29, 2015 · 8 comments · Fixed by #59114
Closed

Destructuring boxes into multiple mutable references seems broken #30104

ben0x539 opened this issue Nov 29, 2015 · 8 comments · Fixed by #59114
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled.

Comments

@ben0x539
Copy link
Contributor

This fails with "cannot mutably borrow more than once" errors

fn push(mut node: Box<(i32, i32)>) {
    let (ref mut left, ref mut right) = *node;
}

Same here:

fn push(mut node: Box<(i32, i32)>) {
    let box (ref mut left, ref mut right) = node;
}

This works:

fn push(mut node: Box<(i32, i32)>) {
    let (ref mut left, ref mut right) = *&mut *node;
}

This also works:

fn push(node: &mut (i32, i32)) {
    let (ref mut left, ref mut right) = *node;
}

Same behavior with structs instead of tuples. Boxes are weird, destructuring into mutable borrows is weird, so I have no idea what is going on there.

@jethrogb
Copy link
Contributor

#![feature(box_patterns)]

box pattern

There. Now other people can find this bug.

@aidanhs
Copy link
Member

aidanhs commented Jun 2, 2016

More keywords: match, if let.
(no, they don't work either)

Two workarounds are to either a) use *&mut *x or b) bring std::ops::DerefMut in and use *x.deref_mut():

use std::ops::DerefMut;
fn main() {
    let mut x = Box::new((1, 2));
    { let (ref mut a, ref mut b) = *&mut *x; }
    { let (ref mut a, ref mut b) = *x.deref_mut(); }
}

@Mark-Simulacrum Mark-Simulacrum added the A-borrow-checker Area: The borrow checker label Jun 23, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 24, 2017
@jonhoo
Copy link
Contributor

jonhoo commented Oct 31, 2017

Is this a dupe of #16223?

@Mark-Simulacrum
Copy link
Member

I don't think so, but am uncertain. Might have the same underlying cause, though -- hard to tell.

@arielb1
Copy link
Contributor

arielb1 commented Oct 31, 2017

This is as defined by RFC rust-lang/rfcs#130

@arielb1 arielb1 closed this as completed Oct 31, 2017
@jethrogb
Copy link
Contributor

jethrogb commented Oct 31, 2017

I don't fully understand it, but that RFC sounds like it was intended to make Box more like other types. This issue is pointing out a discrepancy between Box and other types.

e.g., compare the code from the original issue with

fn push(mut node: ::std::cell::RefMut<(i32, i32)>) {
    let (ref mut left, ref mut right) = *node;
}

(compiles)

This sounds to me like Box is still getting special treatment and the RFC has not been fully implemented yet, therefore this issue should remain open.

@arielb1
Copy link
Contributor

arielb1 commented Nov 1, 2017

@jethrogb

So the RFC was just poorly-defined. I think the solution to mutating in match should be able to be extended to handle this.

@arielb1 arielb1 reopened this Nov 1, 2017
@shepmaster
Copy link
Member

3 of these 4 cases fail with Rust 1.25.0. The good news is that with NLL, all 4 work! 🎉

use std::ops::{Deref, DerefMut};

// Fails
fn smart_two_field(v: &mut Wrap<(i32, i32)>) {
    let _a = &mut v.0;
    let _b = &mut v.1;
}

fn smart_destructure(v: &mut Wrap<(i32, i32)>) {
    let (ref mut _head, ref mut _tail) = **v;
}

// Fails
fn box_two_field(v: &mut Box<(i32, i32)>) {
    let _a = &mut v.0;
    let _b = &mut v.1;
}

// Fails
fn box_destructure(v: &mut Box<(i32, i32)>) {
    let (ref mut _head, ref mut _tail) = **v;
}

// My own smart pointer
struct Wrap<T>(T);

impl<T> Deref for Wrap<T> {
    type Target = T;
    fn deref(&self) -> &T {
        &self.0
    }
}

impl<T> DerefMut for Wrap<T> {
    fn deref_mut(&mut self) -> &mut T {
        &mut self.0
    }
}

fn main() {}

@shepmaster shepmaster added NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. labels May 8, 2018
memoryruins added a commit to memoryruins/rust that referenced this issue Aug 14, 2018
frewsxcv added a commit to frewsxcv/rust that referenced this issue Aug 17, 2018
[nll] add tests for rust-lang#48697 and rust-lang#30104

Adds tests for the following issues:
- rust-lang#48697 ``[NLL] ICE: unexpected region for local data with reference to closure``
- rust-lang#30104 ``Destructuring boxes into multiple mutable references seems broken``

r? @nikomatsakis
bors added a commit that referenced this issue Aug 17, 2018
Rollup of 11 pull requests

Successful merges:

 - #52858 (Implement Iterator::size_hint for Elaborator.)
 - #53321 (Fix usage of `wasm_target_feature`)
 - #53326 ([nll] add regression test for issue #27868)
 - #53347 (rustc_resolve: don't allow paths starting with `::crate`.)
 - #53349 ([nll] add tests for #48697 and #30104)
 - #53357 (Pretty print btreemap for GDB)
 - #53358 (`{to,from}_{ne,le,be}_bytes` for unsigned integer types)
 - #53406 (Do not suggest conversion method that is already there)
 - #53407 (make more ported compile fail tests more robust w.r.t. NLL)
 - #53413 (Warn that `#![feature(rust_2018_preview)]` is implied when the edition is set to Rust 2018.)
 - #53434 (wasm: Remove --strip-debug argument to LLD)

Failed merges:

r? @ghost
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-borrow-checker Area: The borrow checker C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. 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.

7 participants