-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Simplify or-patterns in MIR building #67549
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
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 |
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
Err(match_pair) | ||
PatKind::Or { ref pats } => { | ||
candidate.match_pairs.extend( | ||
pats.iter().map(|pat| MatchPair::new(match_pair.place.clone(), pat)), |
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 should maybe leave a FIXME here to remove this once we use a strategy that doesn't result in multiplicative blow-up? Although this is perhaps the strategy to use for let
statements.
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.
It's linear in the number of patterns, so I don't think it ought to create blowup.
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.
Hmm, but what happens if you have:
match (x, y, z) {
(0 | 1 | 2, 0 | 1 | 2, 0 | 1 | 2) => {}
_ => {}
}
Won't this simplify to 9 tests in sequence (as if you wrote down the "DNF" of the above) as opposed to testing x in 0..=2
first, then y in 0..=2
, and then z in 0..=2
?
(https://github.com/rust-lang/rfcs/blob/master/text/2535-or-patterns.md#implementation-notes)
bf3159d
to
5a4b25e
Compare
This effectively treating an or-pattern as an and-pattern. If we want a temporary solution here, I would prefer to suppress the ICE. |
Ah, sorry, of course it does 😣 Suppressing the ICE would be a better short-term fix. |
Or-patterns were previously not simplified at all during MIR building, which caused any irrefutable match involving or-patterns to ICE. This doesn't fix every ICE: an irrefutable match involving enum variants will still ICE (#67514), but is a step in the right direction.