implement feature gate bind_by_move_pattern_guards

[andjo403]

implementation of issue #15287

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @frewsxcv (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[andjo403]

I think that this is all that is needed. but I have some concerns about the error messages, shall the old be used or shall a new refering to the feature gate be added?

[alexcrichton]

Thanks for the PR @andjo403! It looks like there may be a failing test on Travis though?

[00:54:23] failures:
[00:54:23] 
[00:54:23] ---- [run-pass] run-pass/bind-by-move-with-guard.rs stdout ----
[00:54:23] 	
[00:54:23] error: compilation failed!
[00:54:23] status: exit code: 101
[00:54:23] command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc /checkout/src/test/run-pass/bind-by-move-with-guard.rs -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass --target=x86_64-unknown-linux-gnu --error-format json -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/bind-by-move-with-guard.stage2-x86_64-unknown-linux-gnu.run-pass.libaux -C prefer-dynamic -o /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/bind-by-move-with-guard.stage2-x86_64-unknown-linux-gnu -Crpath -O -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers
[00:54:23] stdout:
[00:54:23] ------------------------------------------
[00:54:23] 
[00:54:23] ------------------------------------------
[00:54:23] stderr:
[00:54:23] ------------------------------------------
[00:54:23] {"message":"mismatched types","code":{"code":"E0308","explanation":"\nThis error occurs when the compiler was unable to infer the concrete type of a\nvariable. It can occur for several cases, the most common of which is a\nmismatch in the expected type that the compiler inferred for a variable's\ninitializing expression, and the actual type explicitly assigned to the\nvariable.\n\nFor example:\n\n```compile_fail,E0308\nlet x: i32 = \"I am not a number!\";\n//     ~~~   ~~~~~~~~~~~~~~~~~~~~\n//      |             |\n//      |    initializing expression;\n//      |    compiler infers type `&str`\n//      |\n//    type `i32` assigned to variable `x`\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/run-pass/bind-by-move-with-guard.rs","byte_start":668,"byte_end":672,"line_start":20,"line_end":20,"column_start":25,"column_end":29,"is_primary":true,"text":[{"text":"        Some(z) if z == true => { dispose(z); },","highlight_start":25,"highlight_end":29}],"label":"expected struct `std::sync::Arc`, found bool","suggested_replacement":null,"expansion":null}],"children":[{"message":"expected type `std::sync::Arc<bool>`\n   found type `bool`","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":null}
[00:54:23] thread 'main' panicked at 'Some tests failed', /checkout/src/tools/compiletest/src/main.rs:315
[00:54:23] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":null}
[00:54:23] 
[00:54:23] ------------------------------------------
[00:54:23] 
[00:54:23] thread '[run-pass] run-pass/bind-by-move-with-guard.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2472
[00:54:23] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:54:23] 
[00:54:23] 
[00:54:23] failures:
[00:54:23]     [run-pass] run-pass/bind-by-move-with-guard.rs
[00:54:23] 
[00:54:23] test result: FAILED. 2686 passed; 1 failed; 5 ignored; 0 measured

I'll also change this to r? @eddyb, a random member from the compiler team.

[andjo403]

do not know how I missed that, may be too eager to send my first PR to rust :)

[eddyb]

r? @nikomatsakis or @arielb1

[nikomatsakis]

I have to think a bit about how I'd prefer to move forward here. It might be that deploying the ExprUseVisitor (and changing nothing else) suffices for now. But it might be that we want to handle this a bit more comprehensively. If the former, I'll try to write some notes shortly on what to do.

[nikomatsakis]

Hmm, I think that this doesn't suffice. Specifically, there are cases we want to disallow, such as this:

let x = Some(vec![1, 2, 3]);
match x {
    Some(v) if { mem::drop(v); true } => { println!("empty"); }
    Some(v) => { println!("not-empty: {:?}", v); }
    None => { println!("None!"); }
}

The danger is that v the vector will get freed after the first guard is executed.

I think that the RFC specifically stated that we should allow this but only if the guard only uses the value "by reference" (or if it only copies).

One way to do this analysis would be to deploy the ExprUseVisitor. I think the option I prefer at this point is to say that, when the guard executes, we treat the bound values as if there was an implicit shared reference (much like how a closure works). This probably requires an RFC, though I think it's backwards compatible.

In the example above, that would mean that references to v in the pattern guard would be treated as equivalent to *v0 where v0 is a synthetic variable with type &Vec<i32>; hence the mem::drop(v) guard would be equivalent to mem::drop(*v0), which would mean that it'd be an error (since you are moving through a borrowed reference).

[bors]

☔ The latest upstream changes (presumably #42058) made this pull request unmergeable. Please resolve the merge conflicts.

[andjo403]

I do not know how to continue with this PR. Shall I try to implement a check with ExprUseVisitor? the other prefered solution I have no ide of how to do so there I needs some more info to get started.

[alexcrichton]

ping @nikomatsakis, do you have thoughts on @andjo403's last comment?

[nikomatsakis]

Sorry, I've started writing up a few responses but never finished them. I'm not sure the best path either. I suspect that the right answer here lies in leveraging a MIR-based borrowck, but it's not necessarily the case that the right code will just "fall out'. Let me put this another way: with a MIR-based borrowck, I think that we could remove the various hard-coded rules about guards and everything would work just in terms of being sound, but we might accept programs we did not intend to. Basically, we might be tying out hands to details of how we lower match statements that we do not want to. This is one of the "to be addressed" use cases that the first PR will not handle.

Probably the right thing is to close this particular PR, but @andjo403 it'd be very cool to try and start afresh while working on the MIR. I'm not sure exactly what that entails though, there's a bit of discussion still needed I fear.

[andjo403]

no problem I close this I missed that the hard case did not work. thougth that the issue hade been solved by all changes in the last year of the compiler and only forgotten.
thanks for the help.