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

Pattern errors are too imprecise and should be removed in favor of MIR borrowck #45600

Closed
comex opened this issue Oct 28, 2017 · 22 comments · Fixed by #68376
Closed

Pattern errors are too imprecise and should be removed in favor of MIR borrowck #45600

comex opened this issue Oct 28, 2017 · 22 comments · Fixed by #68376
Assignees
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. F-move_ref_pattern `#![feature(move_ref_pattern)]` NLL-complete Working towards the "valid code works" goal P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@comex
Copy link
Contributor

comex commented Oct 28, 2017

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) => {}
    }
}
@Mark-Simulacrum Mark-Simulacrum added A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 30, 2017
@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Dec 21, 2017
@nikomatsakis
Copy link
Contributor

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
Copy link
Member

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 pnkfelix added NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. and removed NLL-deferred labels Dec 19, 2018
@pnkfelix
Copy link
Member

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 pnkfelix reopened this Dec 19, 2018
@pnkfelix pnkfelix added NLL-complete Working towards the "valid code works" goal and removed NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. labels Dec 19, 2018
@pnkfelix
Copy link
Member

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

@pnkfelix pnkfelix added the P-medium Medium priority label Dec 19, 2018
@pnkfelix
Copy link
Member

oh also I think this is probably related to #54987

@pnkfelix
Copy link
Member

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

@goffrie
Copy link
Contributor

goffrie commented Jan 23, 2019

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
Copy link
Contributor

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
Copy link
Contributor

oli-obk commented Apr 24, 2019

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
Copy link
Contributor

We could remove the checks only in 2018, right?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 24, 2019

Yes

@estebank
Copy link
Contributor

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

@matthewjasper matthewjasper changed the title error "cannot bind by-move into a pattern guard" (+ others) is too imprecise and should be removed in favor of MIR borrowck Pattern errors are too imprecise and should be removed in favor of MIR borrowck May 4, 2019
@matthewjasper
Copy link
Contributor

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
Copy link
Contributor

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

@oli-obk oli-obk added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label May 24, 2019
@estebank
Copy link
Contributor

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
Copy link
Contributor

Centril commented Jun 28, 2019

So this is just for https://doc.rust-lang.org/error-index.html#E0009 as discussed in @matthewjasper's report? in #15287 (comment)?

@estebank
Copy link
Contributor

I think it is indeed.

@Centril
Copy link
Contributor

Centril commented Jun 30, 2019

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
Copy link
Member

scottmcm commented Jul 1, 2019

@estebank I personally like the answer in #15287 (comment) 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
Copy link
Contributor

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
Copy link
Contributor

Centril commented Jan 11, 2020

@lily-commure I don't believe we have any current plans. We have however relaxed x @ Some(y) under a feature gate (#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 Centril added I-nominated and removed A-diagnostics Area: Messages for errors, warnings, and lints C-feature-request Category: A feature request, i.e: not implemented / a PR. I-nominated labels Jan 11, 2020
@Centril Centril self-assigned this Jan 17, 2020
@Centril Centril removed the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 17, 2020
@Centril
Copy link
Contributor

Centril commented Jan 17, 2020

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.

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 A-NLL Area: Non-lexical lifetimes (NLL) E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. F-move_ref_pattern `#![feature(move_ref_pattern)]` NLL-complete Working towards the "valid code works" goal P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.