Skip to content

Python: fix PEP 758 except A, B: extraction in the default parser - #22386

Open
aausch wants to merge 2 commits into
github:mainfrom
aausch:aausch/python-pep758-legacy-parser
Open

Python: fix PEP 758 except A, B: extraction in the default parser#22386
aausch wants to merge 2 commits into
github:mainfrom
aausch:aausch/python-pep758-legacy-parser

Conversation

@aausch

@aausch aausch commented Aug 19, 2026

Copy link
Copy Markdown

What

except A, B: (PEP 758, Python 3.14+) is extracted by the default parser using the Python 2 reading: B becomes an alias binding (Store) instead of a use (Load). Queries that reason about whether a name is used then misfire — py/unused-import reports the import of B as unused.

Why

blib2to3/Grammar.txt shares one rule between both readings:

except_clause: 'except' [test [(',' | 'as') test]]

and visit_except_clause never looked at the separator, so a fourth child was always bound as an alias:

if len(node.children) > 3:
    name = self.visit(node.children[3], STORE)

This patch checks the separator token: as binds an alias as before, , builds a tuple of exception types with Load context and no alias.

Only the two-type form was affected. except A, B, C: already extracted correctly, and parenthesized forms were never affected.

Verification

  • Matches the existing expectation. With this change the default parser reproduces the checked-in python/extractor/tests/parser/exceptions_new.expected — the tree-sitter parser's expected AST for this exact syntax — byte for byte, locations and contexts included.
  • No collateral change. Dumping the default parser's AST for all 37 files in tests/parser/ before and after, only the two files containing PEP 758 syntax differ.
  • New parser test. tests/parser/exceptions_relaxed.py is unsuffixed, so the harness asserts the two parsers produce identical ASTs. It fails on main and passes with this change. pytest tests/test_parser.py → 37 passed.
  • End to end. Patching this file into the codeql-bundle-v2.26.3 extractor and re-running Imports/UnusedImport.ql over the reproduction removes the false positive, while a genuinely unused import in the same file is still reported.
  • New query tests. python/ql/test/query-tests/Imports/unused/ gains coverage for two-, three- and four-type chains through the real extraction path. Reverting the extractor fix in a 2.26.3 bundle makes relaxed_except.py report Import of 'Beta' is not used. and the test fail.

Chains longer than two

Worth recording, because the two cases behave differently and the difference is not obvious.

clause default parser result today
except A, B: parses (as Python 2) wrong AST reaches the queries
except A, B, C: SyntaxError Module.py_ast falls back to tree-sitter, AST is correct
except A, B, C, D: SyntaxError same, correct

except_clause: 'except' [test [(',' | 'as') test]] admits exactly one trailing
test, so three or more types cannot parse at all. The fallback in
semmle/python/modules.py then rescues them. Only the two-type form is silently
wrong, precisely because it is the only one the default parser accepts.

This has a consequence for testing that cost me a first attempt: the two cases
cannot share a file. Any three-type clause fails the default parser, sends the
whole file to tree-sitter, and masks the two-type behaviour completely. So the
query tests are split into relaxed_except.py and relaxed_except_long.py,
with a comment in each explaining why. For the same reason each name is used in
exactly one clause — a name that also appears in a parenthesized clause is a use
regardless, and hides the defect.

I have deliberately not changed the grammar to accept longer chains. The
fallback already yields correct results, and a Grammar.txt change is a much
larger and riskier diff than the defect warrants. It may still be worth doing:
the fallback costs a failed parse per affected file, which measured at roughly
541ms versus 9ms for a neighbouring file in the same run. Happy to open that
separately if you would like it.

Trade-off

Python 2 source of the form except ValueError, e: that parses under the Python 3 grammar will now be read as a tuple of exception types rather than an alias binding. This input is genuinely ambiguous between the two language versions and the grammar rule cannot distinguish them. The tree-sitter parser (#20990) already resolves it in favour of PEP 758, and the unsuffixed parser tests require the two parsers to agree, so this change aligns them. Happy to gate it on the matched grammar instead if you would rather preserve the Python 2 reading — that needs the grammar identity threaded from parser/__init__.py::parse into ast.convert, which I left out to keep this minimal.

Fixes #22387


Investigated with assistance from Claude Code. Every result quoted above was executed against the real 2.26.3 bundle and the repo's own test harness, not inferred.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes default Python parser extraction of PEP 758 exception lists, aligning it with the tree-sitter parser.

Changes:

  • Distinguishes as aliases from comma-separated exception types.
  • Adds parser parity regression coverage.
  • Documents the corrected extraction behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
python/extractor/semmle/python/parser/ast.py Extracts except A, B: as a load-context tuple.
python/extractor/tests/parser/exceptions_relaxed.py Tests exception-list and alias variants across parsers.
python/ql/lib/change-notes/2026-08-19-legacy-parser-relaxed-except.md Records the parser fix and query impact.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

aausch and others added 2 commits August 20, 2026 11:10
The grammar rule shared by both readings is

    except_clause: 'except' [test [(',' | 'as') test]]

and `visit_except_clause` ignored the separator token, always treating the
fourth child as an alias to bind. So `except A, B:` extracted `B` as a Store
rather than a use, which is the Python 2 reading. Queries that reason about
whether a name is used then report false positives; `py/unused-import` flags
the import of `B` as unused.

The tree-sitter parser already extracts this as a tuple of exception types
(github#20990), so the two parsers disagreed. `tests/parser/exceptions_relaxed.py`
is an unsuffixed parser test, which asserts the two parsers produce identical
ASTs; it fails without this change.

With the fix, the default parser reproduces the existing
`tests/parser/exceptions_new.expected` byte for byte, and of the 37 parser
test files only the two containing PEP 758 syntax change at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ngth

The parser test added with the fix pins the AST. These pin the behaviour a user
actually sees, through the real extraction path including the tree-sitter
fallback, and they cover chains longer than two.

Chains of three or more behave differently from chains of two, which is worth
having written down. `except A, B, C:` fails the default parser outright, so
`Module.py_ast` falls back to tree-sitter and the result is already correct.
`except A, B:` parses successfully under the Python 2 reading, so the fallback
never fires and the bad AST reaches the queries. That is why only the two-type
form produced a false positive.

It also means the two cases cannot share a file: any three-type clause sends
the whole file to tree-sitter and masks the two-type behaviour. Hence
relaxed_except.py and relaxed_except_long.py, with a comment in each saying so.
Each name is used in exactly one clause for the same reason -- a name reused in
a parenthesized clause is a use regardless, and hides the defect.

Verified by reverting the extractor fix in a 2.26.3 bundle: relaxed_except.py
then reports `Import of 'Beta' is not used.` and the test fails.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@aausch
aausch force-pushed the aausch/python-pep758-legacy-parser branch from 03c2f1e to 896d8d7 Compare August 20, 2026 09:10
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.

Python: PEP 758 except A, B: extracted as a Python 2 alias, causing py/unused-import false positives

2 participants