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

Refactor pattern-matching usefulness algorithm #65160

Closed
wants to merge 87 commits into from

Conversation

Nadrieril
Copy link
Member

While working on #53820, I felt uneasy because the usefulness algorithm was quite a complex piece of code and it wasn't clear to me how the different parts interacted. The basic algorithm is clear enough and well explained at the top of the file, but additions were made to support integer ranges, slice patterns and various edge cases like private fields. Those additions made some sense individually but didn't seem to fit in a larger coherent whole.
In this PR, I propose a rework of the algorithm, that integrates advanced features like slice patterns natively. The algorithm describes different steps from the one in the paper, but it actually does the exact same work when restricted to normal constructors + wildcard patterns. This rewrite in particular removes the special-casing of the wildcard pattern, which I found added a lot of cognitive complexity to the code. This PR also largely increases separation of concerns between specialization, witness reconstruction, and handling of special features like integer ranges. I personally feel that the code is now much easier to follow.

This is a fairly large PR; I felt it only made sense as a whole, but if needed I could try splitting it up.
This is for the most part pure refactoring: a lot of the commits simply move code around. I made them small so it should be easy to see that each step preserves behaviour. The only real change in behaviour is regarding exhaustiveness diagnostics for array/slice patterns: they have been slightly altered and are no longer cut off at some arbitrary-seeming length. They have also improved when slice_patterns is enabled.
I recommend the following reading order: first read the description of the new algorithm at the top of the lastest version of _match.rs, then commit-by-commit. I believe tests pass after every commit.

This PR is mostly me being unable to resist refactoring everything I see, so the changes may be a bit all over the place. I hope it all makes sense in the end.
Thanks for taking some time to review !

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2019
@eddyb
Copy link
Member

eddyb commented Oct 6, 2019

r? @pnkfelix or @nikomatsakis cc @varkor @Centril

@bors
Copy link
Contributor

bors commented Oct 6, 2019

☔ The latest upstream changes (presumably #65089) made this pull request unmergeable. Please resolve the merge conflicts.

src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
}).collect()
}
PatKind::Or { ref pats } => {
pats.iter().flat_map(|pat| pat_constructors(tcx, param_env, pat, ty)).collect()
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you've familiarized yourself in-depth with this code, would you like to tackle exhaustiveness for or-patterns as well? (Hope you don't mind @dlrobertson -- then you can focus more on MIR)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, don't mind at all. Having more eyes on the code is always better 😄

Copy link
Member Author

@Nadrieril Nadrieril Oct 7, 2019

Choose a reason for hiding this comment

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

I would like to, yes !

@Centril Centril added F-or_patterns `#![feature(or_patterns)]` F-slice_patterns `#![feature(slice_patterns)]` labels Oct 6, 2019
@varkor
Copy link
Member

varkor commented Oct 7, 2019

I've started reviewing this. Let's check to see whether this affects perf at all.

@bors try

@bors
Copy link
Contributor

bors commented Oct 7, 2019

⌛ Trying commit 92bf5a2d22ffd7cdefb0ed22ceaf18e291a1cfa5 with merge 3c8eb891c46a383549ee6d6d3936bec0dee6f238...

@varkor
Copy link
Member

varkor commented Oct 7, 2019

cc @arielb1

@bors
Copy link
Contributor

bors commented Oct 7, 2019

☀️ Try build successful - checks-azure
Build commit: 3c8eb891c46a383549ee6d6d3936bec0dee6f238 (3c8eb891c46a383549ee6d6d3936bec0dee6f238)

@varkor

This comment has been minimized.

@rust-timer

This comment has been minimized.

@varkor

This comment has been minimized.

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 left some comments. Some of them may be out of date, because I was commenting as I was looking through each commit, which often got refactored later, so ignore any that don't seem relevant any more. Overall, these changes seem really good, and make things a lot clearer ❤️ The diagnostics improvements as a result are a nice bonus, too!

Let's just check that nothing regresses, but other than that, I'm pretty happy with how things are here. You've put a lot of effort into improving this code; thank you!

src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
@varkor
Copy link
Member

varkor commented Oct 7, 2019

@rust-timer build 3c8eb891c46a383549ee6d6d3936bec0dee6f238

1 similar comment
@Mark-Simulacrum
Copy link
Member

@rust-timer build 3c8eb891c46a383549ee6d6d3936bec0dee6f238

@rust-timer
Copy link
Collaborator

Queued 3c8eb891c46a383549ee6d6d3936bec0dee6f238 with parent 59a31c8, future comparison URL.

@Centril
Copy link
Contributor

Centril commented Oct 8, 2019

r? @varkor

@rust-highfive rust-highfive assigned varkor and unassigned pnkfelix Oct 8, 2019
All the constructed PatStacks end up having `head_ctors` called on them,
so this does not unnecessarily precompute anything.
Stop checking that cached constructors are valid. Avoid cloning
constructors when not necessary.
This reverts some of the or-patterns future-proofing, for the sake of
performance.
@pietroalbini
Copy link
Member

/AzurePipelines run

@pietroalbini
Copy link
Member

Sorry for the noise, trying to figure out why PR builders don't work.

/AzurePipelines help

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bors
Copy link
Contributor

bors commented Oct 19, 2019

☔ The latest upstream changes (presumably #65598) made this pull request unmergeable. Please resolve the merge conflicts.

@Nadrieril
Copy link
Member Author

So the lastest merge conflict is taking a big toll on me to rebase. I also have the feeling that this PR has become so unwieldy that maintainers will struggle to find time to review it. If it is going to wait around a while (and need several more complex rebases), maybe splitting this PR up into smaller meaningful chunks could make everyone's job easier; but that would not be worth it if the PR is almost mergeable.
Does this sound like it would be worth the additional effort ?

@Centril
Copy link
Contributor

Centril commented Oct 20, 2019

@Nadrieril I would personally try to split it up; what should make it both easier to merge and review, and especially the latter is important as match checking is important for soundness.

@varkor
Copy link
Member

varkor commented Oct 20, 2019

I think that as long as the rebases aren't requiring significant changes in how things work, and the performance issue can be resolved, it could be easier to just go for this one PR, as it's already been looked at by two of us.

That said, it's up to you: if splitting up would be easier, then go for that.

@JohnCSimon
Copy link
Member

Ping from triage.
@Nadrieril This PR has sat idle for the last few days. Can you please address the merge conflicts?
CC @varkor
Thank you!

@Nadrieril
Copy link
Member Author

I have decided to split this into smaller bits

@Nadrieril Nadrieril closed this Oct 27, 2019
bors added a commit that referenced this pull request Nov 1, 2019
Clarify pattern-matching usefulness algorithm

This PR clarifies a bit the usefulness algorithm by emphasizing that each row of the matrix can be seen as a sort of stack from which we pop constructors. It also moves code around to increase separation of concerns.

This is part of my splitting of #65160 into smaller PRs.
bors added a commit that referenced this pull request Nov 4, 2019
Clarify pattern-matching usefulness algorithm

This PR clarifies a bit the usefulness algorithm by emphasizing that each row of the matrix can be seen as a sort of stack from which we pop constructors. It also moves code around to increase separation of concerns.

This is part of my splitting of #65160 into smaller PRs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-or_patterns `#![feature(or_patterns)]` F-slice_patterns `#![feature(slice_patterns)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.