-
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 2024: Implement eat-one-layer #123512
Match ergonomics 2024: Implement eat-one-layer #123512
Conversation
0c430f7
to
2f730c1
Compare
if let Some(&mut Some(&x)) = &Some(&mut Some(0)) { | ||
let _: u32 = x; | ||
} | ||
if let Some(&Some(&mut x)) = &mut Some(& Some(0)) { | ||
let _: u32 = x; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are weird, shouldn't we be eating references in the same order as in the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this typecheck?
if let Some(&Some(x)) = &mut Some(&Some(0)) {
let _: &mut u32 = x;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this typecheck?
Not any longer, pushed a fix so we now properly downgrade the mutability of the inherited reference.
Shouldn't we be eating references in the same order as in the type?
No, that would be unnecessarily restrictive, as demonstrated by this newly added test:
if let Some(Some(&mut x)) = &Some(Some(&mut 0)) {
let _: &u32 = x;
}
(let &mut x = &&mut 0
does not work currently, but I think it should, will try to fix edit: also works now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I would not expect these tests to work personally.
I would expect we want a rule like: let &mut x = <expr>; x: T
iff let x = <expr>; x: &mut T
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any good reason we should restrict it to that, when the currently implemented rule (if the pattern can match against the inherited reference, do so; otherwise match against a reference in the scrutinee type) is strictly more useful. let Some(&mut x) = &Some(&mut 0)
is more convenient than let &Some(&mut (ref x)) = &Some(&mut 0)
, why should we force people to use the latter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't let Some(&x) = &Some(&mut 0)
work just as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that would make x
an &mut {integer}
instead of an &{integer}
, and then the compiler yells at you that you can't move out of a shared reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. Well, I'm happy to merge your design, this is an implementation review not a design review. I'll just need some time to wrap my head around the new commits
cf7cdfc
to
e5b63c2
Compare
Some changes occurred in match checking cc @Nadrieril |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, that looks quite good. I'm ok with most of the implementation, I'd just like to try and simplify calc_default_binding_mode
further because it's becoming thorny.
compiler/rustc_hir_typeck/src/pat.rs
Outdated
AdjustMode::ResetAndConsumeRef(ref_pat_mutbl) => { | ||
let mutbls_match = def_bm.0 == ByRef::Yes(ref_pat_mutbl); | ||
if pat.span.at_least_rust_2024() && self.tcx.features().ref_pat_eat_one_layer_2024 { | ||
let max_ref_mutbl = cmp::min(max_ref_mutbl, ref_pat_mutbl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intuitively I'd expect we can just cap def_bm
by max_ref_mutbl
just before returning from the function, and cap it by ref_pat_mutbl
at the end of this if
. Does that work? The peel_off_references
change I just suggested could help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a slight tweak to this function, but didn't go as far as that. The capping by max_ref_mutbl
has to be gated by edition/feature gate, so needs to stay inside the if
.
@rustbot author |
☔ The latest upstream changes (presumably #123991) made this pull request unmergeable. Please resolve the merge conflicts. |
a4640f4
to
3efbe3e
Compare
Looks good, ty! @bors r+ |
…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#122811 (Move `SourceMap` initialization) - rust-lang#123512 (Match ergonomics 2024: Implement eat-one-layer) - rust-lang#123811 (Use queue-based `RwLock` on more platforms) - rust-lang#123859 (Remove uneeded clones now that TrustedStep implies Copy) - rust-lang#123979 (Subtype predicates only exist on inference types, so we can allow them to register opaque types within them.) - rust-lang#124016 (Outline default query and hook provider function implementations) - rust-lang#124023 (Allow workproducts without object files.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#123512 - Jules-Bertholet:ref-pat-eat-one-layer-2024, r=Nadrieril Match ergonomics 2024: Implement eat-one-layer r? `@Nadrieril` cc rust-lang#123076 `@rustbot` label A-edition-2024 A-patterns
r? @Nadrieril
cc #123076
@rustbot label A-edition-2024 A-patterns