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

Incorrect partially moved value error check when pattern matching boxed structures. #16223

Closed
ghost opened this Issue Aug 3, 2014 · 20 comments

Comments

Projects
None yet
@ghost
Copy link

commented Aug 3, 2014

The following code worked on 0.11 bu no longer works on master. Last known good nightly was August 1st.

struct Foo {
    a: Vec<u8>,
    b: Vec<u8>
}

fn main() {
    let foo = box Foo{a: Vec::new(), b: Vec::new()};
    let Foo { a: a, b: b } = *foo;
}

This will create the following error message:

<anon>:8:24: 8:25 error: use of partially moved value: `foo.b`
<anon>:8     let Foo { a: a, b: b } = *foo;
                                ^
<anon>:8:18: 8:19 note: `foo.a` moved here because it has type `collections::vec::Vec<u8>`, which is moved by default (use `ref` to override)
<anon>:8     let Foo { a: a, b: b } = *foo;

If Foo is not boxed this will function correctly.

struct Foo {
    a: Vec<u8>,
    b: Vec<u8>
}

fn main() {
    let foo = Foo{a: Vec::new(), b: Vec::new()};
    let Foo { a: a, b: b } = foo;
}
@goffrie

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2014

As I commented on Reddit, it should be noted that moving explicitly out of foo (which should be inferred (?)) works around the bug:

struct Foo {
    a: Vec<u8>,
    b: Vec<u8>
}

fn move<T>(x: T) -> T { x }

fn main() {
    let foo = box Foo{a: Vec::new(), b: Vec::new()};
    let Foo { a: a, b: b } = *move(foo);
}
@zwarich

This comment has been minimized.

Copy link

commented Aug 4, 2014

This is fallout from #16102. The basic gist of this is that while the borrow checker was intentionally made less intelligent regarding the fields of a T in a Box<T>, it is also still stupid enough to treat let Foo { a: a, b: b } = foo; as two separate moves out of (*foo).a and (*foo).b.

If there is a DerefMove trait in the future that Box implements, then we can remove this special handling of Box and it will appear as a single move into a temporary, followed by two moves from the fields of the temporary, which the borrow checker can track in a field-sensitive manner.

@Gankro

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2015

Borrowck is basically the typesystem, right?

Right?

@brson

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

I'm untagging all pre-1.0 regressions to repurpose it for stable regressions.

@ticki

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2015

This is still a problem.

@ticki

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2016

And ... happened to stumple upon it again.

@ticki

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2016

For anyone running into this issue: The problem can be solved by forcing a move using an identify function (for example, wrapping the value in {}).

@kennytm

This comment has been minimized.

Copy link
Member

commented Mar 27, 2016

The identity function trick won't work nicely with a match when you want to keep the box in another arm:

#[derive(Debug)]
enum E {
    A(String, String),
    B(String, String),
}

impl E {
    fn f(self: Box<Self>) -> Box<Self> {
        match *self { // can't use `match {*self}`, otherwise can't use `self` below.
            E::A(..) => self,  
            E::B(a, b) => Box::new(E::B(b, a)), // <-- E0382 here
        }
    }
}

fn main() {
    println!("{:?}", Box::new(E::A("1".to_owned(), "2".to_owned())).f());
    println!("{:?}", Box::new(E::B("1".to_owned(), "2".to_owned())).f());
}

We could add an "indirection" as a workaround...

        match *self {
            E::A(..) => self,
            e => match e {              // workaround #16223
                E::B(a, b) => Box::new(E::B(b, a)), 
                _ => unreachable!(),    // ugly :(
            }
        }
@frewsxcv

This comment has been minimized.

Copy link
Member

commented Nov 4, 2016

This seems like a similar scenario to this:

struct A {
    some_string: String,
    some_num: u32,
}

impl A {
    fn some_method(self: Box<Self>) {
        some_function(self.some_string, self.some_num)
    }
}

fn some_function(_: String, _: u32) {}

fn main() {}
rustc 1.12.1 (d4f39402a 2016-10-19)
error[E0382]: use of moved value: `self`
 --> <anon>:8:41
  |
8 |         some_function(self.some_string, self.some_num)
  |                       ----------------  ^^^^^^^^^^^^^ value used here after move
  |                       |
  |                       value moved here
  |
  = note: move occurs because `self.some_string` has type `std::string::String`, which does not implement the `Copy` trait

error: aborting due to previous error

@frewsxcv frewsxcv referenced this issue Nov 4, 2016

Merged

remove extra clones from dom event script #14066

4 of 5 tasks complete
@kirillkh

This comment has been minimized.

Copy link

commented Jan 3, 2017

I ran into this or similar issue today. (still exists on nightly)

@jonhoo

This comment has been minimized.

Copy link
Contributor

commented May 12, 2017

Still exists on nightly today, and makes it really hard to work with boxed enums..
Also prevents things like:

match some_boxed_enum {
    box Foo::Bar { ref mut a, ref b } => { ... }
}
@fintelia

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2017

I'm not sure why, but this workaround compiles fine:

struct Foo {
    a: Vec<u8>,
    b: Vec<u8>
}

fn main() {
    let foo = box Foo{a: Vec::new(), b: Vec::new()};
    let (Foo { a: a, b: b }, _) = (*foo, 0);
}

In fact, even one element tuples also work:

fn main() {
    let foo = box Foo{a: Vec::new(), b: Vec::new()};
    let (Foo { a: a, b: b },) = (*foo,);
}
@jonhoo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

For the cases where you want multiple ref muts into the Box (i.e., don't want to move the Box entirely), this ugly hack works:

match (&mut *some_boxed_enum,) {
    (&mut Foo::Bar { ref mut a, ref b },) => { ... }
}
@Robbepop

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

Solution Found

Thanks to bob_twinkles (IRC): He pointed out that with enabled feature NLL (Non-Lexical-Lifetimes) (Link: #43234) the below code works - and it does! So this issue might be fixed when NLL stabilizes. :)


In my current project I have quite a lot of problems with this issue since lots of my code work greatly simplify from having this fixed. The code in question operates on recursive and boxed data structures often seen in ASTs.

mod expr {
    struct BoolConst(bool);
    struct Not { child: Box<AnyExpr> }
    struct IfThenElse { children: Box<IfThenElseChildren> }
    // Exists to only have one `Box` indirection instead of one per child for `IfThenElse`.
    struct IfThenElseChildren {
        cond: AnyExpr,
        then_case: AnyExpr,
        else_case: AnyExpr
    }
}
/// A generic super-type covering all existing expression types in a single enum.
enum AnyExpr {
    BoolConst(expr::BoolConst),
    Not(expr::Not),
    IfThenElse(expr::IfThenElse)
}

For the simplifications (aka optimizations) of the AST I use a transformer that consumes the AST and reconstructs it. This happens until no more optimizations are applicable.

One example simplification is for IfThenElse to swap the then_case and else_case whenever the condition is a Not expression:

For example: if not(A) then B else C is simplified to if A then C else B.

The whole thing in action:

fn transform_cond(ite: expr::IfThenElse) -> TransformOutcome {
    if let box IfThenElseChilds{ cond: AnyExpr::Not(not), then_case, else_case } = ite.childs {
        return TransformOutcome::transformed(
            expr::IfThenElse::new(
                not.into_single_child(),
                else_case,
                then_case
            )
        )
    }
    TransformOutcome::identity(ite)
}

Results in this error:

error[E0382]: use of collaterally moved value: `ite.childs.then_case`
108 |         if let box IfThenElseChilds{ cond: AnyExpr::Not(not), then_case, else_case } = ite.childs {
    |                                                         ---   ^^^^^^^^^ value used here after move
    |                                                         |
    |                                                         value moved here
    |
    = note: move occurs because the value has type `ast2::formulas::not::Not`, which does not implement the `Copy` trait

The proposed work around e.g. enclosing the matched thing with {} does not work with the if-let construct in this case since ite is moved and cannot be accessed afterwards by the TransformOutcome::identity(ite). The work around for this was:

fn transform_cond(ite: expr::IfThenElse) -> TransformOutcome {
    match {ite} {
        expr::IfThenElse{ childs: box IfThenElseChilds{ cond: AnyExpr::Not(not), then_case, else_case }, .. } => {
            return TransformOutcome::transformed(
                expr::IfThenElse::new(
                    not.into_single_child(),
                    else_case,
                    then_case
                )
            )
        }
        ite => TransformOutcome::identity(ite)
    }
}

Which compiles but is rather horrible to pattern match against, especially when imagine that this kind of simplification pattern matching will occure potentially hundreds of times in the entire code base ...

Earlier attemps to fix this were the following:

fn transform_cond(ite: expr::IfThenElse) -> TransformOutcome {
    if ite.childs.cond.kind() == ExprKind::Not {
        if let (AnyExpr::Not(not), then_case, else_case) = ite.childs.into_childs_tuple() {
            return TransformOutcome::transformed(
                expr::IfThenElse::new(
                    not.into_single_child(),
                    else_case,
                    then_case
                )
            )
        }
        unreachable!()
    }
    TransformOutcome::identity(ite)
}

The downsites of this work around are the double-checking and the resulting unreachable!(). Besides that the pattern matching is optimal here.

My hackiest approach was to pattern match with exclusive references to allow the move-access to ite after a failed if-let match and then basically use mem::replace to pull out the values.

fn transform_cond(ite: expr::IfThenElse) -> TransformOutcome {
    if let (&mut AnyExpr::Not(ref mut not), &mut ref mut then_case, &mut ref mut else_case) = ite.as_childs_tuple_mut() {
        use std::mem;
        let not       = mem::replace(not,       unimplemented!());
        let then_case = mem::replace(then_case, unimplemented!());
        let else_case = mem::replace(else_case, unimplemented!());
        return TransformOutcome::transformed(
            expr::IfThenElse::new(
                not.into_single_child(),
                else_case,
                then_case
            )
        )
    }
    TransformOutcome::identity(ite)
}

None of these approaches have proven to be a good fit for an entire growing code base and I would really love to see the initial failed attempt to work as it should be.

Questions

  • Are there any other decent solutions to my problem?
  • How high are the chances of fixing this?
  • I have read that the DerefMove feature would be a rescue to this issue. How and why?

I would love to help but would need a kick start and mentoring help for the issue. :)

@bobtwinkles

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

This appears to be fixed by NLL. A test is still required however.

@Robbepop

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2018

@bobtwinkles I'd like to help to provide an appropriate test code for this use case but maybe need minor mentoring for where to put things and how to actually provide test samples.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

Filing under #47366

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

Marking as E-needstest and E-mentor:

Since this code works now, what we need to do is add it to our test-suite. To do that, you would create a file in src/test/ui/nll named issue-16223.rs containing the example. There are general instructions for adding new tests here.

That said, I can't quite tell what is "the" distinctive example to add here -- @Robbepop maybe you know?

@Robbepop

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

@nikomatsakis Thanks for the initial guidelines. I can try to find and add "the" distinctive test sample for this issue. :)

Robbepop added a commit to Robbepop/rust that referenced this issue Apr 8, 2018

Robbepop added a commit to Robbepop/rust that referenced this issue Apr 8, 2018

Robbepop added a commit to Robbepop/rust that referenced this issue Apr 9, 2018

kennytm added a commit to kennytm/rust that referenced this issue Apr 11, 2018

Rollup merge of rust-lang#49781 - Robbepop:master, r=nikomatsakis
add regression test for rust-lang#16223 (NLL): use of collaterally moved value

Adds regression test for rust-lang#16223 which NLL fixes.

The current downside of this test is that it uses the `#![feature(box_patterns)]` and I haven't come up with a proper test that only uses the `#![feature(nll)]` - however, I don't know if this is even possible to test without `#![feature(box_syntax)]` or `#![feature(box_patterns)]`.

bors added a commit that referenced this issue Apr 11, 2018

Auto merge of #49875 - kennytm:rollup, r=kennytm
Rollup of 12 pull requests

Successful merges:

 - #49525 (Use sort_by_cached_key where appropriate)
 - #49575 (Stabilize `Option::filter`.)
 - #49614 (in which the non-shorthand patterns lint keeps its own counsel in macros)
 - #49665 (Small nits to make couple of tests pass on mips targets.)
 - #49781 (add regression test for #16223 (NLL): use of collaterally moved value)
 - #49795 (Properly look for uninhabitedness of variants in niche-filling check)
 - #49809 (Stop emitting color codes on TERM=dumb)
 - #49850 (core: Inline `From<AllocErr> for CollectionAllocErr`)
 - #49856 (Do not uppercase-lint #[no_mangle] statics)
 - #49863 (fixed typo)
 - #49857 (Fix "fp" target feature for AArch64)
 - #49698 (Merge the std_unicode crate into the core crate)

Failed merges:

bors added a commit that referenced this issue Apr 11, 2018

Auto merge of #49875 - kennytm:rollup, r=kennytm
Rollup of 14 pull requests

Successful merges:

 - #49525 (Use sort_by_cached_key where appropriate)
 - #49575 (Stabilize `Option::filter`.)
 - #49614 (in which the non-shorthand patterns lint keeps its own counsel in macros)
 - #49665 (Small nits to make couple of tests pass on mips targets.)
 - #49781 (add regression test for #16223 (NLL): use of collaterally moved value)
 - #49795 (Properly look for uninhabitedness of variants in niche-filling check)
 - #49809 (Stop emitting color codes on TERM=dumb)
 - #49856 (Do not uppercase-lint #[no_mangle] statics)
 - #49863 (fixed typo)
 - #49857 (Fix "fp" target feature for AArch64)
 - #49849 (Add --enable-debug flag to musl CI build script)
 - #49734 (proc_macro: Generalize `FromIterator` impl)
 - #49730 (Fix ICE with impl Trait)
 - #48270 (Replace `structurally_resolved_type` in casts check.)

Failed merges:
@Robbepop

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

@nikomatsakis I think this can be closed now. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.