-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Python: fix PEP 758 except A, B: extraction in the default parser
#22386
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
Open
aausch
wants to merge
4
commits into
github:main
Choose a base branch
from
aausch:aausch/python-pep758-legacy-parser
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
66573bb
Python: fix PEP 758 `except A, B:` in the default parser
aausch 896d8d7
Python: add query tests for unparenthesized except chains of every le…
aausch 117fc6b
Python: keep the Python 2 reading of `except A, e:`
aausch 7d9ae21
Python: test the Python 2 except reading through extraction, not a un…
aausch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| try: | ||
| a | ||
| except b, c: | ||
| d | ||
| except (e, f): | ||
| g | ||
| except h as i: | ||
| j | ||
| except k: | ||
| l |
4 changes: 4 additions & 0 deletions
4
python/ql/lib/change-notes/2026-08-19-legacy-parser-relaxed-except.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: fix | ||
| --- | ||
| * Fixed the extraction of PEP 758 `except A, B:` clauses by the default (non-tree-sitter) Python parser. Previously the second exception type was extracted as a Python 2 style alias binding, so it was recorded as a `Store` rather than a use. This caused false positives from queries that reason about whether a name is used, such as `py/unused-import`. When extracting Python 2 (`--lang=2`), `except A, e:` continues to bind `e` as an alias, since that is what the syntax means in that version. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| semmle-extractor-options: --lang=2 |
3 changes: 3 additions & 0 deletions
3
python/ql/test/2/extractor-tests/relaxed_except/relaxed_except.expected
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| | 6 | ValueError | err (definition) | | ||
| | 12 | ValueError | other (definition) | | ||
| | 18 | ValueError, TypeError | none | |
26 changes: 26 additions & 0 deletions
26
python/ql/test/2/extractor-tests/relaxed_except/relaxed_except.ql
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * The types of each `except` clause, and the name it binds. In Python 2 the | ||
| * comma form binds a name and has a single type; reading it as a PEP 758 tuple | ||
| * instead would give two types and no name. | ||
| */ | ||
|
|
||
| import python | ||
|
|
||
| from ExceptStmt handler, string types, string name | ||
| where | ||
| types = | ||
| concat(Expr type | | ||
| type = handler.getType() | ||
| | | ||
| type.toString(), ", " order by type.getLocation().getStartColumn() | ||
| ) and | ||
| ( | ||
| exists(Name bound | bound = handler.getName() | | ||
| bound.isDefinition() and name = bound.getId() + " (definition)" | ||
| or | ||
| not bound.isDefinition() and name = bound.getId() + " (use)" | ||
| ) | ||
| or | ||
| not exists(handler.getName()) and name = "none" | ||
| ) | ||
| select handler.getLocation().getStartLine(), types, name |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # When extracting Python 2, `except A, e:` binds `e`. It is not a PEP 758 | ||
| # unparenthesized tuple of exception types, which is what the same syntax means | ||
| # from Python 3.14 on. | ||
| try: | ||
| unlikely() | ||
| except ValueError, err: | ||
| print err | ||
|
|
||
| # `as` means the same thing in every version. | ||
| try: | ||
| unlikely() | ||
| except ValueError as other: | ||
| print other | ||
|
|
||
| # A parenthesized tuple is several types, and binds nothing. | ||
| try: | ||
| unlikely() | ||
| except (ValueError, TypeError): | ||
| pass |
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
26 changes: 26 additions & 0 deletions
26
python/ql/test/query-tests/Imports/unused/relaxed_except.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # PEP 758 allows unparenthesized exception types when there is no `as` clause. | ||
| # Every name below is used as an exception type, so no import here is unused. | ||
| # `NeverUsed` is imported and never used, and is the one expected result. | ||
| # | ||
| # Each name appears in exactly one clause on purpose: a name that also appeared | ||
| # in a parenthesized clause would be a use regardless, and would mask the | ||
| # behaviour under test. | ||
| # | ||
| # This file deliberately contains no `except A, B, C:` clause. Three or more | ||
| # unparenthesized types fail the default parser, which sends the whole file to | ||
| # the tree-sitter parser and would likewise mask it. | ||
| from relaxed_except_defs import Alpha, Beta, Delta, Gamma, NeverUsed | ||
|
|
||
|
|
||
| def unparenthesized(): | ||
| try: | ||
| pass | ||
| except Alpha, Beta: | ||
| raise | ||
|
|
||
|
|
||
| def parenthesized(): | ||
| try: | ||
| pass | ||
| except (Gamma, Delta): | ||
| raise |
26 changes: 26 additions & 0 deletions
26
python/ql/test/query-tests/Imports/unused/relaxed_except_defs.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| class Alpha(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class Beta(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class Gamma(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class Delta(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class Epsilon(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class NeverUsed(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class Zeta(Exception): | ||
| pass |
17 changes: 17 additions & 0 deletions
17
python/ql/test/query-tests/Imports/unused/relaxed_except_long.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Three or more unparenthesized exception types. These fail the default parser | ||
| # and are extracted by the tree-sitter parser instead; all names are still uses. | ||
| from relaxed_except_defs import Delta, Epsilon, Gamma, Zeta | ||
|
|
||
|
|
||
| def three(): | ||
| try: | ||
| pass | ||
| except Gamma, Delta, Epsilon: | ||
| raise | ||
|
|
||
|
|
||
| def four(): | ||
| try: | ||
| pass | ||
| except Gamma, Delta, Epsilon, Zeta: | ||
| raise |
Oops, something went wrong.
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.
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.
I think this might actually be a problem for Python 2, which is still officially supported 😅
The default (blib2to3) parser is the primary one (
modules.pytries it first, tree-sitter is only the fallback), and this branch is version-agnostic, so it also kicks in when we extract in Python 2 mode (CODEQL_EXTRACTOR_PYTHON_ANALYSIS_VERSION=2). Thereexcept Exception, e:really is the alias binding, so with this changeeflips from aStoreto aLoadof an undefined name, and the exception stops being bound. That's the canonical py2 idiom, so it's not exactly a rare construct.Could we gate the tuple reading on the version? Something like keeping the old alias branch when
get_analysis_major_version() == 2, and only building the tuple otherwise. The 3+ types case isn't affected (three unparenthesized types aren't valid py2 anyway, so the tree-sitter fallback is fine there).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.
Good catch — and it was worse than a mislabelled AST: in Python 2 mode the binding disappeared entirely, so
ebecame aLoadof an undefined name.Gated as you suggested in 117fc6b.
asbinds an alias in every version; a comma binds an alias whenget_analysis_major_version() == 2and builds the tuple otherwise. Three or more types are unaffected — not valid py2, and the default grammar rejects them regardless of version, so the tree-sitter fallback covers them.The file-driven parser tests can't express this, since they run at the default analysis version with no per-fixture override. So
python/extractor/tests/test_except_clause.pydrivesparser.parsedirectly with the version flipped and pins all four combinations: comma andas, py2 and py3, plus the parenthesized form that must bind no alias in either. Removing the gate fails the py2 case, andpytest tests/test_parser.pystill passes 37.Happy to add a
--lang=2query test underImports/unused/as well if you would rather see it through the real extraction path — I left it out because the unit test pins the exact decision site.I have also rewritten the "Trade-off" section of the description, which described the behaviour this replaces.
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.
A more appropriate solution would be to add an extractor test in
python/ql/test/2/extractor-tests. I think that's preferable to a bespoke unit test.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.
Agreed — done in 7d9ae21, and the unit test is gone.
python/ql/test/2/extractor-tests/relaxed_exceptextracts with--lang=2and pins, per handler, the types and whether the bound name is a definition, so it asserts what a query actually sees rather than the AST shape. Removing the version gate makes it fail.Getting there turned up something you may want to know independently of this PR:
--langdid not reach the worker processes.populator.mainhonours it by callingupdate_analysis_version, but that rebinds a global in the process that parses the options, while extraction happens in anExtractorPool— and on macOS those workers are spawned, not forked, so they re-readCODEQL_EXTRACTOR_PYTHON_ANALYSIS_VERSIONand saw the default of3. So--lang=2meant Python 2 on Linux and Python 3 on macOS. My first version of this test passed underCODEQL_EXTRACTOR_PYTHON_ANALYSIS_VERSION=2and failed under--lang=2on the same machine, which is what gave it away. The commit also sets the environment variable alongside the global, so the flag means the same thing on both platforms. Real Python 2 extraction was never affected — the action sets that variable itself and children inherit it. Happy to split that into its own PR if you would rather it not ride along.I also bumped the extractor version, which the first commit should have done.
Verified with codeql 2.26.3, this branch's extractor patched into it:
python/ql/test/2/extractor-tests— 10 passed.hidden/test.qlfails, but it fails identically against the unpatched 2.26.3 extractor (extra| .hidden/inner |and| folder |rows), so it is not from this branch.python/ql/test/query-tests/Imports— all 17 passed, which also re-confirms therelaxed_except*.pyquery tests from the earlier commit.python/extractorpytest— 116 passed, including the 37 parser tests.