Identify unreachable subpatterns more reliably#80632
Conversation
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. |
|
⌛ Trying commit 5082bb16c11aedfb927c5d8e2702444ff2abe9c5 with merge 6e126cb311d93b2142d8eae09719d614926e04b7... |
|
☀️ Try build successful - checks-actions |
|
Queued 6e126cb311d93b2142d8eae09719d614926e04b7 with parent fde6927, future comparison URL. @rustbot label: +S-waiting-on-perf |
|
Finished benchmarking try commit (6e126cb311d93b2142d8eae09719d614926e04b7): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
There was a problem hiding this comment.
I've just left a few comments about the comments. Apologies if I made any mistakes about the meaning; it's a little late. I've only skimmed the code so far, so I'll take a better look soon. My appreciation for the complexity of handling or-patterns correctly continues to grow.
|
I realized it's much clearer if we keep a set of reachable subpatterns instead of a set of unreachable ones. There were negations everywhere it was hurting my head. |
|
Thanks, these changes definitely make the code clearer! Looks good to me. @bors r+ |
|
📌 Commit ae6fcab has been approved by |
|
Thanks for letting me know it was confusing ^^ |
|
☀️ Test successful - checks-actions |
|
Will this be backported to the beta branch? |
|
No idea |
|
@Nadrieril @varkor This seems to have caused a performance regression after merging. The regression is in a benchmark specifically for pattern match which is impacted by this change. The |
|
Hm, the perf run we did was neutral... Given that the last commit is purely about naming, the cause must be the rebase I did after the perf run. The |
|
@Nadrieril the question now is is whether this is an acceptable performance regression. 1% on a stress benchmark is not a cause for great concern, but perhaps trying to gain that performance back might be helpful. |
|
Hm, well I can't reproduce the perf loss locally with |
|
I'll label this with @rustbot label T-compiler (not 100% sure though) |
|
Backports declined (compiler meeting notes) @rustbot label -beta-nominated -stable-nominated |
In #80104 I used
Spans to identify unreachable sub-patterns in the presence of or-patterns during exhaustiveness checking. In #80501 it was revealed thatSpans are complicated and that this was not a good idea.Instead, this PR identifies subpatterns logically: as a path in the tree of subpatterns of a given pattern. I made a struct that captures a set of such subpatterns. This is a bit complex, but thankfully self-contained; the rest of the code does not need to know anything about it.
Fixes #80501. I think I managed to keep the perf neutral.
r? @varkor