Rust: Pattern errors are too imprecise and should be removed in favor of MIR borrowck

[comex]

Update by @pnkfelix :

NLL (aka MIR-borrowck) addresses the first case given in the issue, but not this one:

struct Foo(String, String);
fn x(f: Foo) {
    match f {
        Foo(a, ref b) => {} 
    }
}

Original bug report follows

---

For this code:

struct Foo;
fn main() {
    match Foo {
        x @ Foo if true => x,
        _ => Foo,
    };
}

rustc produces:

error[E0008]: cannot bind by-move into a pattern guard
 --> test.rs:4:9
  |
4 |         x @ Foo if true => x,
  |         ^^^^^^^ moves value into pattern guard

This error comes from check_legality_of_move_bindings in librustc_const_eval/check_match.rs.

The message is at least arguably incorrect: since x is not used in the pattern guard, it doesn't make much sense to claim that the code "moves value into pattern guard".

I suppose you could argue that all bindings in a pattern are always moved/copied to the pattern guard, and the fact that this one is unused doesn't matter. Under that interpretation, it's a matter of improving the error message, and this issue is a dupe of rust-lang/rfcs#684 which seeks to improve that error message in general.

However, I think the code should be allowed, as it's not unsound. I'm not too familiar with compiler internals, but it seems like once MIR borrow checking is finished, it would catch any actually-unsound moves into pattern guards, and this error could be removed altogether. Is that correct?

The other two errors generated by check_legality_of_move_bindings have similar issues:

1. "cannot bind by-move with sub-bindings"

For one thing, this error seems to be a subset of a different one from elsewhere in check_match.rs, "pattern bindings are not allowed after an @" - at least, I can't think of any code that would trigger this and not that. (Feel free to let me know what I'm missing.) But in any case, this is overly conservative. If we have

struct Foo(i32);

then it should be legal to match x @ Foo(y) (because y is Copy). Again, this seems like something MIR borrowck might be able to handle better, although the frontend might have to make sure the copy comes from the right place (pre- or post-move).

1. "cannot bind by-move and by-ref in the same pattern"

I don't even know what this error is trying to prevent. The following works fine even with the existing borrowck (f properly becomes a partially moved value):

struct Foo(String, String);
fn x(f: Foo) {
    match f {
        Foo(a, _) => {
            let b = &f.1;
        }
    }
}

So what is wrong with putting b into the pattern?

fn x(f: Foo) {
    match f {
        Foo(a, ref b) => {}
    }
}

[nikomatsakis]

I'm marking this as deferred. I believe that indeed MIR-based borrowck can generalize many of these patterns, particularly once @pnkfelix's work is done.

[pnkfelix]

Re-triaging for #56754. NLL-fixed-by-NLL(via PRs #49870 and #54034). Normally that would mean I'd leave the issue open, but I think the tracking issue for #15287 already serves in terms of tracking the feature that is being requested here. (Note that when you have NLL enabled, on nightly, you get a suggestion to enable #![feature(bind_by_move_pattern_guards)].) So, just closing as effectively fixed.

[pnkfelix]

Ah there is the second example that was given in the issue:

struct Foo(String, String);
fn x(f: Foo) {
    match f {
        Foo(a, ref b) => {} 
    }
}

which still errors under NLL.

The argument given above is that we already allow partial moves (when you use _ rather than ref b), so it is not clear why we do not use partial moves here to support that code.

I'll reopen this issue to allow discussion of that case (which is indeed something that we might fix in the future.)

[pnkfelix]

in terms of the re-triaging effort, I'll call this NLL-complete, P-medium.

[pnkfelix]

oh also I think this is probably related to #54987

[pnkfelix]

which means it is probably related to Centril/rfcs#16

[goffrie]

ran into a similar issue recently - the following code is rejected:

fn bind(a: &Option<String>, b: Option<String>) {
    match (a, b) {
        (&Some(ref x), Some(y)) => println!("{} {}", x, y), 
        _ => (),
    }
}

error[E0009]: cannot bind by-move and by-ref in the same pattern
 --> src/lib.rs:3:29
  |
3 |         (&Some(ref x), Some(y)) => println!("{} {}", x, y), 
  |                -----        ^ by-move pattern here
  |                |
  |                both by-ref and by-move used

but ref x is not really borrowing the matched value (a, b) - rather, it borrows from *a.

[estebank]

I made a test on top of #60125 (which continues evaluating after "misc checking 2" errors) disabling check_match. No currently compiling code stops compiling and the errors barely regress: a few wording changes but the actual problematic cases are caught and even mention the pattern. We could change the wording if necessary. That being said, a subset of currently rejected code would now be accepted, like the example above, which seems like it could be problematic because people could write code that compiles in latest stable but wouldn't compile in older rustc. @rust-lang/compiler is this an appropriate type of change, where _more_ code is accepted? The mir borrow checker catches every single case that we _want_ to reject, which is a subset of what we currently reject.

[oli-obk]

I think the main problem is that the 2015 edition doesn't use NLL yet, so we'd suddenly accept unsound code if we removed the match checks.

[estebank]

We could remove the checks only in 2018, right?

[oli-obk]

Yes

[estebank]

I want to briefly bring this ticket up in tomorrow's meeting.

[matthewjasper]

Changed the title to make it clearer that this refers to the errors that are not part of #15287. The next steps for this are:

• Check that the lang team is happy to remove the restriction without an RFC.
• Add a feature gate to remove these errors.
• Review users of ExprUseVisitor to see if they handle the new cases correctly.
• Write tests.

[nikomatsakis]

Discussed in compiler team meeting, removing nomination as it seems like we sort of know what steps we need now.

[estebank]

Check that the lang team is happy to remove the restriction without an RFC.

@rust-lang/lang can you confirm wether relying on the MIR borrowck instead of checking binding modes in patterns ("cannot bind by-move and by-ref in the same pattern") without an RFC?

[Centril]

So this is just for https://doc.rust-lang.org/error-index.html#E0009 as discussed in @matthewjasper's report? in https://github.com/rust-lang/rust/issues/15287#issuecomment-489308359?

[estebank]

I think it is indeed.

[Centril]

I would say we should deal with #15287 first and then revisit this once that's done.
A small RFC might be in order once we've done that.

[scottmcm]

@estebank I personally like the answer in https://github.com/rust-lang/rust/issues/15287#issuecomment-491043259 of thinking of these restrictions as "AST borrowck details", and thus removing them if MIR borrowck is sufficient in in the spirit of NLL's approval.

[lily-commure]

Is there a plan for this now that #15287 is done? I just ran into the confusing "cannot bind by-move and by-ref in the same pattern" message today.

[Centril]

@lily-commure I don't believe we have any current plans. We have however relaxed x @ Some(y) under a feature gate (https://github.com/rust-lang/rust/issues/65490) for now. Perhaps we should to the same here. I'll nominate for the language team to discuss this for next Thursday, if we have time. If @matthewjasper could attend that would be good. :)

[Centril]

We discussed this on yesterday's language team meeting. Our conclusion was that there's no good (e.g. soundness related) reason to keep this an error and that it would be an improvement to allow this code. We noted that a good test suite would be needed before stabilizing. Moreover, we thought that it should remain an error to move out of a field where the data type implements Drop (though it is an independent check).

I will implement this change along with adding the tests, with a new fresh tracking issue.