-
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
Implement MIR lowering for or-patterns #67668
Conversation
This comment has been minimized.
This comment has been minimized.
c1309e9
to
232d661
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion |
Implement MIR lowering for or-patterns This is the last thing needed to get meaningful run-pass tests for or-patterns. There probably need to be more tests before stabilizing this, but the most important cases should have been covered. Note: we can generate exponentially large MIR CFGs when using or-patterns containing bindings, type ascriptions, or that are for a match arm with a guard. `src/test/mir-opt/exponential-or.rs` shows the best case for what we currently do. cc #54883 closes #60350 closes #67514 cc @Centril r? @pnkfelix
☀️ Try build successful - checks-azure |
Queued 844409e with parent 2ee25da, future comparison URL. |
@@ -31,6 +30,8 @@ fn main() { | |||
let x = Ok(3); | |||
let Ok(y) | Err(y) = x; | |||
//~^ ERROR or-pattern is not allowed in a `const` | |||
//~| ERROR constant contains unimplemented expression 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.
What is going on here? cc @ecstatic-morse @oli-obk
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.
We're generating actual branches in the MIR now, which const qualif is erroring for.
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.
What surprised me though was that we've added support for control flow to const qualif, so I didn't expect this specific error here. Should we open an issue to follow up?
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.
You're right, this is from the ProjectionElem::Downcast
. Changing from y
to _
avoids the extra errors.
Perf is done and looks good. |
This comment has been minimized.
This comment has been minimized.
4e7127c
to
ba2a379
Compare
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.
Just some nits.
99eefa8
to
1aa85f9
Compare
assert_eq!(search((true, false, false)), 5); | ||
assert_eq!(search((true, false, true)), 6); | ||
assert_eq!(search((true, true, false)), 7); | ||
assert_eq!(search((true, true, true)), 8); |
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.
This is weird and mind-bending, but correct once you bang your head against the wall enough trying to comprehend what's going on.
impure guard code is yucky.
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.
Its possible the test would be a little less mind-bending if it also included this variant on search
(play):
fn search_old_style(target: (bool, bool, bool)) -> u32 {
let x = ((false, true), (false, true), (false, true));
let mut guard_count = 0;
match x {
// ((a, _) | (_, a), (b @ _, _) | (_, b @ _), (c @ false, _) | (_, c @ true)) |
((a, _), (b @ _, _), (c @ false, _)) |
((a, _), (b @ _, _), (_, c @ true)) |
((a, _), (_, b @ _), (c @ false, _)) |
((a, _), (_, b @ _), (_, c @ true)) |
((_, a), (b @ _, _), (c @ false, _)) |
((_, a), (b @ _, _), (_, c @ true)) |
((_, a), (_, b @ _), (c @ false, _)) |
((_, a), (_, b @ _), (_, c @ true))
if {
guard_count += 1;
(a, b, c) == target
} =>
{
guard_count
}
_ => unreachable!(),
}
}
this is of course just the original OR-patterns now expanded into the old syntax. Seeing that made me immediately understand what was going on in the test.
Now, whether this language feature (mixing multi-pattern arms and guards) is an anti-pattern in disguise is another matter entirely ...
(I'll admit that when I initially read examples like these, I wrongly assume that we commit to the pattern, check the guard, and when the guard fails, move on to the next arm. That is of course not what happens, which is probably for the best, but it definitely can make impure guards very confusing.)
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.
Looked through the tests and they all seem reasonable. Perhaps would want a run-pass test that also checks the sugared matches (if-let
, while-let
, for
etc).
1aa85f9
to
876bc41
Compare
This comment has been minimized.
This comment has been minimized.
876bc41
to
ed49d91
Compare
This comment has been minimized.
This comment has been minimized.
ed49d91
to
19377b9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
19377b9
to
56dc63e
Compare
📌 Commit a781aed402778bbbed9494261a71b28f4544b611 has been approved by |
⌛ Testing commit a781aed402778bbbed9494261a71b28f4544b611 with merge d8001e113550e7254016c3ee22dbd06e2cb7d781... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-azure |
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
Also add some comments
a781aed
to
8dbbe4d
Compare
@bors r=pnkfelix |
📌 Commit 8dbbe4d has been approved by |
To ensure this doesn't bitrot, @bors p=1 rollup=never |
Implement MIR lowering for or-patterns This is the last thing needed to get meaningful run-pass tests for or-patterns. There probably need to be more tests before stabilizing this, but the most important cases should have been covered. Note: we can generate exponentially large MIR CFGs when using or-patterns containing bindings, type ascriptions, or that are for a match arm with a guard. `src/test/mir-opt/exponential-or.rs` shows the best case for what we currently do. cc #54883 closes #60350 closes #67514 cc @Centril r? @pnkfelix
☀️ Test successful - checks-azure |
or_patterns: add regression test for rust-lang#68785 Fixes rust-lang#68785. (Fixed by rust-lang#67668.) cc rust-lang#54883 r? @estebank
This is the last thing needed to get meaningful run-pass tests for or-patterns. There probably need to be more tests before stabilizing this, but the most important cases should have been covered.
Note: we can generate exponentially large MIR CFGs when using or-patterns containing bindings, type ascriptions, or that are for a match arm with a guard.
src/test/mir-opt/exponential-or.rs
shows the best case for what we currently do.cc #54883
closes #60350
closes #67514
cc @Centril
r? @pnkfelix