Make the match lowering state machine explicit#128515
Closed
Nadrieril wants to merge 8 commits intorust-lang:masterfrom
Closed
Make the match lowering state machine explicit#128515Nadrieril wants to merge 8 commits intorust-lang:masterfrom
Nadrieril wants to merge 8 commits intorust-lang:masterfrom
Conversation
`match_candidates` can change the `otherwise_block`, so the previous code was morally incorrect (if not in practice, for some reason).
rustbot
added
S-waiting-on-review
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Zalathar
reviewed
Aug 2, 2024
Comment on lines
+1960
to
+1961
| all_except_last | ||
| .all(|candidate| matches!(candidate.state, CandidateState::Complete { .. })) |
Member
There was a problem hiding this comment.
It looks like this is overly strict, because we can also have CandidateState::Expanded with remaining_match_pairs.is_empty().
That seems to be the cause of the assertion failure in debug-assertion builds.
Member
|
With the existing code, it's nice that the code that handles |
Member
Author
|
Yeahh, it's not great, but reusing code as before requires a 4th state I think (see the last commit) |
Member
|
Hmm yeah, I tried a simplification that I had in mind and it almost worked, but not quite. To me, it feels like the code is not quite ready to be broken up into crisp candidate states, like there's some other change that needs to happen first to make this smoother. But I'm not sure what that change is. |
Member
Author
|
I know what you mean. The other change I have in mind is to choose success/failure blocks before processing candidates, which would mean we can do the "post-processing" of or-patterns before passing them through |
Collaborator
|
☔ The latest upstream changes (presumably #122792) made this pull request unmergeable. Please resolve the merge conflicts. |
Member
Author
|
More thought needed then |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One of the trickiest parts of match lowering is what happens to the
Candidates. They're used for a bunch of things at once, especially for or-patterns. This PR moves the important data ofCandidateinto an enum, to make the various stages of processing explicit.This is a bit rough, I'm open to feedback on whether this is clear enough. cc @Zalathar in particular.
r? @ghost