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

reset default binding mode when we pass through a & pattern #48448

Merged

Conversation

nikomatsakis
Copy link
Contributor

Fixes #46688.

r? @cramertj

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2018
@cramertj
Copy link
Member

I'm adding some examples here to make sure I understand the behavior:

let (x, &(y, ref z)) = &(1, (2, 3));
// x: &i32
// y: i32
// z: &i32

let (x, &Some(Some(y))) = &(1, Some(Some(5)));
// x: &i32
// y: i32

So the following would be an error: "cannot move out of borrowed content"?

let Outer { &Inner { x } } = &Outer { Inner { "".to_string() } };

But this one would work:

let Outer { &Inner { ref x } } = &Outer { Inner { "".to_string() } };

It seems a little bit odd that & switches the whole inner pattern to "by-value" mode, with no way to explicitly request the opposite (e.g. let ref Foo { x, y } = Foo { x: 10, y: 10 };). I could imagine limiting the shift to by-value to just the individual variable binding (e.g. &x rather than &Foo { x }), which would prevent this "stuck in by-value mode" problem. One nice thing about this restriction is that it would be backwards-compatible to loosen it in the future. However, I don't know that we need to restrict ourselves like that-- this seems like a strange corner case of deeply nested matching that wouldn't come up often in practice, and you could always opt to place the & on individual bindings when you need more granular behavior.

If you're feeling up to it, I wouldn't mind having more tests of this sort in order to more clearly demonstrate the desired behavior, but basically r=me.

@nikomatsakis
Copy link
Contributor Author

I'm adding some examples here to make sure I understand the behavior:

Yes

So the following would be an error: "cannot move out of borrowed content"?
let Outer { &Inner { x } } = &Outer { Inner { "".to_string() } };

Yes

But this one would work:
let Outer { &Inner { ref x } } = &Outer { Inner { "".to_string() } };

Yes

If you're feeling up to it, I wouldn't mind having more tests of this sort in order to more clearly demonstrate the desired behavior, but basically r=me.

OK

@nikomatsakis
Copy link
Contributor Author

Actually @cramertj your first two examples were wrong:

let (x, &(y, ref z)) = &(1, (2, 3));
//                          ^^^^^^ should be `&(2, 3)`, else you get an error

the other has a similar problem.

@nikomatsakis
Copy link
Contributor Author

Actually, all of them do.

@nikomatsakis
Copy link
Contributor Author

So maybe you were expecting different behavior. =)

@cramertj
Copy link
Member

Okay, I guess that makes sense-- it's switching the binding mode, but still pattern-matching normally using the reference pattern. So this will only work for dereferencing values which were references originally. This seems like an unfortunate limitation-- it would be nice to be able to specify that other values should be taken by-value-- this would basically make & into the move keyword suggested previously. However, this seems like a backwards-compatible extension, so I'm fine with leaving as-is for now.

@nikomatsakis
Copy link
Contributor Author

@cramertj I see. I do sort of want the move keyword, and I agree it could be handy to be able to "switch to ref mode" in the middle of pattern matching. But those also seem like fairly substantial changes (the move keyword, in particular, was discussed but ultimately decided against) that would probably require a new RFC (well, at least the idea of having & patterns be applicable to something that does not have & type).

@nikomatsakis
Copy link
Contributor Author

@cramertj so r=you?

@cramertj
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2018

📌 Commit 31f66a0 has been approved by cramertj

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2018
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 24, 2018
…ssue-46688, r=cramertj

reset default binding mode when we pass through a `&` pattern

Fixes rust-lang#46688.

r? @cramertj
bors added a commit that referenced this pull request Feb 25, 2018
Rollup of 15 pull requests

- Successful merges: #47689, #48110, #48197, #48296, #48386, #48392, #48404, #48415, #48441, #48448, #48452, #48481, #48490, #48499, #48503
- Failed merges:
@bors bors merged commit 31f66a0 into rust-lang:master Feb 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants