-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Match ergonomics means bindings have different types in patterns and match arm; cannot deref references in pattern #64586
Comments
I thought that this was the case where you'd want to use |
@estebank: could you clarify? |
@varkor my bad, I misunderstood what you were talking about. Using |
Yes, that's the whole point of default match bindings though. The fact that S(...) is matched against a reference means that every binding inside also becomes a reference. The only annoying part is that there's no way to force a move instead of getting a reference in that case. I think we should absolutely have either |
It's a somewhat common trick to use let () = &&3u8;
^^ expected u8, found () This confused me for quite a while. |
@Centril pointed out that currently, fn foo(x: &S) {
let _: u32 = match x {
S(mut y) => y,
_ => 0,
};
} would not type-check, because |
Woah, |
Do you mean that this is preventing us from allowing to move arr.iter().map(|&(mut y)| .. ) I really hope we can enable a way to move out some pattern constituents explicitly in the presence of match-default-bindings, and I really prefer Me and many other people already expected
(That comment was referring to what I wrote here: #50008) |
We could probably permit some examples that are currently forbidden, but we would not be able to make it behave consistently (e.g. with regards to |
@varkor Before considering using the edition mechanism, I think it would be prudent to crater run a change and see if anyone is relying on the |
Since there's an edition coming up soon, I'm wondering if the binding reset issue should be given consideration again? There was a reddit post of someone running into the issue and it causing confusion. In the case that there's something blocking that, would it be a good idea to have at least some additional diagnostics? It's hard to make any sense of what is happening for newcomers currently. |
I was planning to prototype changes to match ergonomics to address this issue, but I haven't found time to do so yet. I think that would be a good first step in determining the viability of the change. |
FWIW I hit this fairly regularly. It is quite annoying to have to write struct S(u32, Vec<i32>);
fn foo(x: &S) {
match x {
S(y, v) => {
let y = *y;
// match arm goes here
}
}
} or struct S(u32);
fn foo(x: &S) {
let _: &u32 = match *x {
S(y, ref v) => {
// match arm goes here
}
}
} I end up often preferring the latter, even if that means adding I would like to be able to just write struct S(u32, Vec<i32>);
fn foo(x: &S) {
match x {
S(&y, v) => {
// match arm goes here
}
}
} but sadly that is not possible. |
I agree, I run into this all the time and it's always frustrating me that it doesn't work this way, which would be more useful and it would be more in line with expectations. Is there still a chance that this can be fixed in the next edition? |
Could we try to get this for the 2024 edition? I seem to remember someone ran a crater run to see if changing the " |
@rustbot labels +I-lang-nominated Based on the recent discussion about this, let's nominate to discuss it. |
EDIT: see the better proposal two posts below this one Alright, here's the status as far as I can tell. Two things come up regularly (1 2 3 4 5 6 etc) with regards to match ergonomics:
The two issues are somewhat independent but I propose we fix them both. More precisely I propose:
The first part is a breaking change; example of code that would be broken: fn foo(opt: &Option<u32>) -> u32 {
match opt {
Some(mut x) => {
x += 1;
x
}
None => 0
}
} Under this proposal, it would error with " I note that there is a different RFC (here) that would offer instead the option to write Of note is the question of what the behavior of match &Some(&Some(0)) {
Some(Some(&x)) => {} // x: &i32 or i32?
_ => {}
} I have a personal preference for |
The more I look into it the more I find deep discussion on what is or isn't ergonomic and subtle behavior with nested references. I just discovered that |
Alright, I have a precise proposal now (will edit this post if I discover new edge cases). MotivationTwo things come up regularly (1 2 3 4 5 6 etc) with regards to match ergonomics:
I propose to fix all three at once, particularly since together they make it possible to change 1 in such a way that Current behaviorHere is the current behavior of match ergonomics in terms of the mental model I like:
Some illustrative examples (and more: playground): // No way to get a `x: &&T` here since there isn't a `&T` in memory it could point to.
// Hence why we keep only one layer of inherited references.
if let Some(Some(x)) = &Some(&Some(0)) {
let _: &u32 = x;
}
if let &Some(Some(x)) = &Some(&Some(0)) {
let _: &u32 = x;
}
if let Some(&Some(x)) = &Some(&Some(0)) {
let _: u32 = x;
}
if let &Some(&Some(x)) = &Some(&Some(0)) {
let _: u32 = x;
}
// We have one layer of inherited ref, and an underlying place of type `&&T`.
if let Some(x) = &&Some(&&0) {
let _: &&&u32 = x;
}
// The `&` derefs the inherited ref plus one actual ref.
if let Some(&x) = &&Some(&&0) {
let _: &u32 = x;
}
// Compare `x` and `&ref x`.
if let Some(x) = &Some(&0) {
let _: &&u32 = x;
}
// Here the `&` derefs both layers, so the `ref` can only recover one.
if let Some(&ref x) = &Some(&0) {
let _: &u32 = x;
}
// `mut x` dereferences the inherited reference
if let Some(mut x) = &Some(0) {
let _: u32 = x;
}
if let Some(mut x) = &Some(&0) {
let _: &u32 = x;
}
// `&mut` behaves just like `&`
if let Some(Some(x)) = &mut Some(&mut Some(0)) {
let _: &mut u32 = x;
}
// Mixing `&` and `&mut` inherited references gives `&`.
if let Some(Some(x)) = &mut Some(&Some(0)) {
let _: &u32 = x;
} Proposal
|
Thanks for the summary and for reviving this issue. :)
I don't understand the problem here. You can use But if I understand correctly, having this as a pattern is required to have an automatic transition from the current patterns to the new ones where So IOW this footgun is already present with match ergonomics... before match ergonomics, if I had e.g. Basically I am suggesting, maybe this code should get a future-compat warningand eventually a breaking change, if we can get away with it: if let Some(mut x) = &Some(0) {
let _: u32 = x;
} And the (potentially manual) fix is if let Some(&x) = &Some(0) {
let mut x = x;
let _: u32 = x;
} Do we know how common this pattern is out there?
Wow, this is an even more strange case. So we dereference and then take a reference again and then have a local mutable copy of that...? Again I wonder if that's really something we want to allow in patterns; mutating that // old:
if let Some(mut x) = &Some(&0) {
let _: &u32 = x;
}
// new:
if let Some(&x) = &Some(&0) {
let mut x = &x;
let _: &u32 = x;
} This also better reflects the lifetime |
Yes this does seem confusing on first sight. Could you give examples showing in which sense this is already the current behavior and why it is the only sensible behavior? |
I don't know what you mean by "before match ergonomics", but that's not current behavior. let y = Some(0);
if let Some(mut x) = y {
x = 2;
}
assert_eq!(y, Some(0)); If you want to change the scrutinee place you need A different way of saying this:
And the weird resetting behavior does not change the fact that
Idk, I really like the uniformity of "any binding can be declared mutable". Maybe clippy can lint against your case, but I've definitely had cases where it was sensible to declare a mutable if let Some(mut pat) = &arm.pat {
while let AtBinding { subpattern } = pat.kind {
pat = subpattern;
}
process(pat)
} And well, I see the point you're making, my playground example above is likely surprising to newcomers. But that's a different issue which is orthogonal to this one, if you'd like to discuss this further I'd prefer to do it in a separate issue.
That's incorrect.
To be precise, the thing that's the only sensible behavior is the fact that we collapse all layers of inherited references to one. This is necessary, e.g.: if let Some(Some(x)) = &Some(&Some(0)) {
let _: &u32 = x;
} Here there's no if let Some(&x) = &&&&&&Some(&&&&0) {
// The first 6 collapsed into one, then got eaten.
// After that, `&` ate exactly the outer ref inside the `Some`, leaving 3 refs.
let _: &&&u32 = x;
} Having said that, there's only three possibilities for the behavior of
1 is the current behavior; it requires an actual reference. 2 is the one I propose. 3 is backwards-incompatible since it would require if let Some(&&x) = &Some(&0) {
let _: u32 = x;
} |
It may be worth thinking about how deref patterns work as part of this proposal (not strongly related but does overlap somewhat). The current idea there is in #87121 (comment). Basically, this allows a one-time |
Ah, that's an interesting point, thanks for raising it. Assuming you choose if let Some(&x) = &Some(Box::new(0)) {
/// ...
} Under current rust this is simply a type error. Under my proposal without deref patterns however, If you choose something different than Whatever the case however, it's fixable over an edition. Do you think deref patterns could be ready for edition 2024? |
Oh, you are right of course... not sure what I was thinking. 🙈
Yeah that is nice indeed. Though having
I was refering to your new extension of " Maybe not; I think I missed the part where the pattern still requires that there is at least one reference being eaten, but it may be an implicit one.
Specifically you mean "and also the actual reference if any, and fail if there is not at least one being eaten"? As in, when there are implicit references and the binding has type My intuitive expectation would be (3). But honestly the moment there are ever multiple references being 'eaten' it is somewhat confusing, and you gave a good reason for that, so "eat as many references as you can but always at least one" (aka (2)) also makes a lot of sense and might even be easier to explain than (3). |
I guess I was not particularly explicit about that. Since this only affects desugaring into non-match-ergonomic patterns, I'd expect the most general lifetimes that make sense. It most definitely will not be introducing extra places just to take references of them, that'd be silly.
Ah yes you're correct, I'll edit.
It is and isn't, because matching a |
This is still unresolved but last I asked, it seemed like most were in favor of working without any extra syntax. At least as long as there are no potential problems.
How ready :) String deref patterns are in the compiler but not stable. I think the generic proposal could be implemented at any time, just hasn't had anybody able to pick it up. |
Ah right. We are eating at most one "real" |
Hm, that would still require some interaction with
Ideal would be fully ready. If you have deref patterns for std types by 2024 and you later allow it for user types, same kind of breakage would happen. You could circumvent that by requiring Given the size of the feature I'd expect it to stay nightly only for a while anyway, I don't expect full deref patterns could be ready for the edition. Would be unfortunate to wait for 2027 though. Maybe that can motivate someone to pick up the work? :D |
@rustbot labels -I-lang-nominated The proposal above was discussed in the 2023-12-13 T-lang design meeting. The consensus was general agreement about the goals. We probably want @Nadrieril expressed that with agreement on those goals, he can invest further time in writing code, performing crater runs, etc. On the basis of what we learn from that additional work, we hope to build the confidence needed in the details to stabilize some solution. As we had reduced attendance by the time the consensus formed, we decided to confirm this on Zulip, and we also rechecked this via meeting consensus on the 2023-12-20 T-lang triage call.1 Everyone involved also wanted to express our appreciation of @Nadrieril for working to push this forward. Please renominate this for T-lang when there are further questions that the team can answer or further evidence or analysis for us to consider. Footnotes
|
Since there was general agreement that we wanted to fix this, can I add this (or #105647 more likely) to the 2024 edition project board? |
@Nadrieril: Thanks, i've added it to the board. Next time we discuss the board, we'll prioritize it. |
Thank you! I'd be happy to attend if my input would be helpful. Also I'm expecting the final proposal to require fewer moving parts than the one I wrote earlier here; I'm experimenting atm. |
Actually, the thing that requires an edition is fixing #105647, not this one. The two issues may or may not be independent in the end |
Hi! I decided I don't have the bandwidth to handle this in time for the edition. If anyone wants to pick up the edition-related work, it's actually much simpler than I thought when I wrote up that proposal. I wrote a short summary over on #105647 (comment). |
I've made some progress on implementation here: #122978 |
(From @Nadrieril on the tracking issue:)
To me " I mention this because I also slightly prefer the (backwards incompatible) "eat-one-layer" variant of " The fact that eat-one-layer is more forward-compatible with deref patterns, essentially because it matches this mental model, makes it worth considering here, I think. |
FWIW I agree with this, I hope eat-one-layer is what we can get eventually. But there are challenges: it's a silent change in behavior, which is generally seen as undesirable even across edition boundaries, and also it requires more complex migration lint logic compared to the eat-two-layers proposal. Also, there are situations where eat-one-layer is not obviously more intuitive: fn main() {
if let Some(Some(x)) = &Some(&Some(0)) {
let _: &u8 = x; // under all proposals
}
if let Some(&Some(x)) = &Some(&Some(0)) {
let _: u8 = x; // under today and eat-two-layers
let _: &u8 = x; // under eat one layer - the added & ends up being a noop
}
} |
One thought that might simplify both the migration and that unintuitive case: it does not seem very useful to use a Could old editions somehow accept both the eat-both-layers form and the eat-one-layer form, then lint on the former? For example: if let Some(&x) = &Some(&0) {
let _: i32 = x; // existing behavior; eat-both-layers; now an edition lint
let _: &i32 = x; // new edition behavior; eat-one-layer
}
if let Some(&&x) = &Some(&0) {
let _: i32 = x; // currently errors; eat-one-layer; now accepted in all editions
} This is still a change in behavior, but there is at least the usual path of "fix lints to get something that works the same way in both editions." Applying this to the unintuitive example, we can lint (or even error) on the no-op of if let Some(&Some(x)) = &Some(&Some(0)) {
let _: i32 = x; // existing behavior; eat-both-layers; now an edition lint
let _: &i32 = x; // new edition behavior; eat-one-layer; still a lint (or error)
}
if let Some(&&Some(x)) = &Some(&Some(0)) {
let _: i32 = x; // currently errors; eat-one-layer; now accepted in all editions
} |
Fair, but beware that this is also the most controversial part of it. There's a risk of wasting a lot of energy on this when the rest is less controversial and shippable. |
I would also note: the eat-two-layers proposal is backward compatible and strictly better than what we have today, and we can always do the transition to eat-one-layer in 2027. |
I would strongly advice going with eat-one-layer in 2024 edition. IMO eating one layer is a lot more intuitive. And generally the earlier you do a change, the better.
While this is true, I'm hoping we can provide migration tools that change the code so that it works as if it was in the previous edition.
I think that the non-intuitive part here is not eat-one-layer- |
Is it? Most of the time it will cause a type error. Cases where this can cause a silent change in behavior seem pretty rare (things like
I'm personally not convinced either way. In eat-both-layers, Regarding migration, we can easily provide a tool that makes 2021 code forwards-compatible with 2024 (by adding a few extra The situation is similar with the |
Consider the following:
Here, the variable binding
y
has type&u32
outside the pattern. However, inside the pattern it has typeu32
. The fact that these types do not align is confusing, as it means you can't dereferencey
inside the pattern like this:and instead have to dereference it outside the pattern:
Is there any reason this was chosen to be the case, or is required to be the case? As it stands, this behaviour is very counter-intuitive.
The text was updated successfully, but these errors were encountered: