-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Python: Allow use of match
as an identifier
#19895
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
Conversation
This previously only worked in certain circumstances. In particular, assignments such as `match[1] = ...` or even just `match[1]` would fail to parse correctly. Fixing this turned out to be less trivial than anticipated. Consider the fact that ``` match [1]: case (...) ``` can either look the start of a `match` statement, or it could be a type ascription, ascribing the value of `case(...)` (a call) to the item at index 1 of `match`. To fix this, then, we give `match` the identifier and `match` the statement the same precendence in the grammar, and additionally also mark a conflict between `match_statement` and `primary_expression`. This causes the conflict to be resolved dynamically, and seems to do the right thing in all cases.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
LGTM 👍
Quick (non-blocking) question: would it make sense to run DCA to check that none of the existing projects fail due to some subtle corner case?
I guess that if anything unexpected happens, it would still be caught in the nightly run.
Oh yes, I completely forgot to do that. We usually do that for any change, no matter how small (unless it literally doesn't touch any code at all), so thanks for the reminder! |
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.
Two projects failed in DCA:
- Free_cad: Exit code 99 due to Java heap out-of-memory during query evaluation.
- ddnet: failed with
ECONNRESET
during artifact finalization. To me, this looks like a transient network issue, unrelated to the changes.
I will do a DCA re-run to make sure what we see is just noise, even though thefree_cad
failure might be related to the changes in this PR.
Alsostage-timing
seems a bit off.
I'll block this to prevent the merge while we wait for @tausbn’s input.
FreeCAD has been OOMing for a while (one of the quality queries we ported away from points-to recently had some changes that caused this -- see #19641). It's unlikely to be related to these changes.
Agreed.
I had a look at the stage timings, and I don't think there's cause for concern. A bunch of queries got 6 seconds slower, and a bunch of queries got 6-10 seconds faster (mostly on
Let's wait and see if the |
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.
DCA Looks good!
This previously only worked in certain circumstances. In particular, assignments such as
match[1] = ...
or even justmatch[1]
would fail to parse correctly.Fixing this turned out to be less trivial than anticipated. Consider the fact that
can either look the start of a
match
statement, or it could be a type ascription, ascribing the value ofcase(...)
(a call) to the item at index 1 ofmatch
.To fix this, then, we give
match
the identifier andmatch
the statement the same precendence in the grammar, and additionally also mark a conflict betweenmatch_statement
andprimary_expression
. This causes the conflict to be resolved dynamically, and seems to do the right thing in all cases.