Skip to content

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

Merged
merged 3 commits into from
Jun 30, 2025

Conversation

tausbn
Copy link
Contributor

@tausbn tausbn commented Jun 26, 2025

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.

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.
@tausbn tausbn marked this pull request as ready for review June 27, 2025 11:53
@Copilot Copilot AI review requested due to automatic review settings June 27, 2025 11:53
@tausbn tausbn requested a review from a team as a code owner June 27, 2025 11:54
@tausbn tausbn requested a review from IdrissRio June 27, 2025 11:54
Copy link
Contributor

@Copilot Copilot AI left a 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.

Copy link
Contributor

@IdrissRio IdrissRio left a 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.

@tausbn
Copy link
Contributor Author

tausbn commented Jun 27, 2025

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!

@IdrissRio IdrissRio linked an issue Jun 30, 2025 that may be closed by this pull request
Copy link
Contributor

@IdrissRio IdrissRio left a 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 the free_cad failure might be related to the changes in this PR.
    Also stage-timing seems a bit off.

I'll block this to prevent the merge while we wait for @tausbn’s input.

@tausbn
Copy link
Contributor Author

tausbn commented Jun 30, 2025

  • Free_cad: Exit code 99 due to Java heap out-of-memory during query evaluation.

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.

  • ddnet: failed with ECONNRESET during artifact finalization. To me, this looks like a transient network issue, unrelated to the changes.

Agreed.

I will do a DCA re-run to make sure what we see is just noise, even though the free_cad failure might be related to the changes in this PR.
Also stage-timing seems a bit off.

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 python/cpython). To me this just looks like a bit of wobble.

I'll block this to prevent the merge while we wait for @tausbn’s input.

Let's wait and see if the ddnet failure disappears in your run. Otherwise I think it should be fine to merge this.

Copy link
Contributor

@IdrissRio IdrissRio left a comment

Choose a reason for hiding this comment

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

DCA Looks good!

@tausbn tausbn merged commit 184dd5b into main Jun 30, 2025
14 checks passed
@tausbn tausbn deleted the tausbn/python-fix-match-as-identifier branch June 30, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extraction error with tsg-python
2 participants