Skip to content
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

Identify unreachable subpatterns more reliably #80632

Merged
merged 9 commits into from
Feb 7, 2021

Conversation

Nadrieril
Copy link
Member

In #80104 I used Spans to identify unreachable sub-patterns in the presence of or-patterns during exhaustiveness checking. In #80501 it was revealed that Spans 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

@Nadrieril Nadrieril added F-or_patterns `#![feature(or_patterns)]` A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns labels Jan 2, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2021
@Nadrieril
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 2, 2021

⌛ Trying commit 5082bb16c11aedfb927c5d8e2702444ff2abe9c5 with merge 6e126cb311d93b2142d8eae09719d614926e04b7...

@bors
Copy link
Contributor

bors commented Jan 3, 2021

☀️ Try build successful - checks-actions
Build commit: 6e126cb311d93b2142d8eae09719d614926e04b7 (6e126cb311d93b2142d8eae09719d614926e04b7)

@rust-timer
Copy link
Collaborator

Queued 6e126cb311d93b2142d8eae09719d614926e04b7 with parent fde6927, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 3, 2021
@rust-timer
Copy link
Collaborator

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 rollup- to bors.

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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 3, 2021
Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@varkor varkor added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2021
@Nadrieril
Copy link
Member Author

Nadrieril commented Feb 1, 2021

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.
Also I incorporated the reviews.

@Nadrieril Nadrieril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2021
@varkor
Copy link
Member

varkor commented Feb 7, 2021

Thanks, these changes definitely make the code clearer! Looks good to me.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 7, 2021

📌 Commit ae6fcab has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2021
@bors
Copy link
Contributor

bors commented Feb 7, 2021

⌛ Testing commit ae6fcab with merge 36ecbc9...

@Nadrieril
Copy link
Member Author

Nadrieril commented Feb 7, 2021

Thanks for letting me know it was confusing ^^

@bors
Copy link
Contributor

bors commented Feb 7, 2021

☀️ Test successful - checks-actions
Approved by: varkor
Pushing 36ecbc9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 7, 2021
@bors bors merged commit 36ecbc9 into rust-lang:master Feb 7, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 7, 2021
@Nadrieril Nadrieril deleted the fix-80501 branch February 7, 2021 20:28
@lnicola
Copy link
Member

lnicola commented Feb 8, 2021

Will this be backported to the beta branch?

@Nadrieril
Copy link
Member Author

No idea

@rylev
Copy link
Member

rylev commented Feb 9, 2021

@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 check match query was responsible for the regression.

@Nadrieril
Copy link
Member Author

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 match-stress-enum bench is very sensitive, I expect that I undid a lucky gain from a concurrent PR due to inlining or something like that.

@rylev
Copy link
Member

rylev commented Feb 10, 2021

@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.

@Nadrieril
Copy link
Member Author

Hm, well I can't reproduce the perf loss locally with incremental=true, and if I set incremental=false then I get a ~1% perf gain instead... I'm guessing something else must have changed on master in the mean time, but that sounds hard to diagnose. I think I'll pass on this one sorry.

@Mark-Simulacrum Mark-Simulacrum added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Feb 12, 2021
@apiraino
Copy link
Contributor

I'll label this with @rustbot label T-compiler

(not 100% sure though)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 18, 2021
@apiraino
Copy link
Contributor

Backports declined (compiler meeting notes)

@rustbot label -beta-nominated -stable-nominated

@rustbot rustbot removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns F-or_patterns `#![feature(or_patterns)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spurious unreachable_patterns with macro
10 participants