Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions python/extractor/semmle/populator.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ def main(sys_path = sys.path[:]):
if options.language_version:
last_version = options.language_version[-1]
update_analysis_version(last_version)
# Worker processes are spawned rather than forked on macOS, so they do
# not inherit the value set above; they re-read it from the environment
# as this module did on import. Set it there too, or `--lang` would take
# effect in this process only, and on one platform only.
os.environ["CODEQL_EXTRACTOR_PYTHON_ANALYSIS_VERSION"] = last_version

found_py2 = False
if get_analysis_major_version() == 2 and options.extract_stdlib:
Expand Down
16 changes: 15 additions & 1 deletion python/extractor/semmle/python/parser/ast.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from blib2to3.pgen2 import token
from ast import literal_eval
from semmle.python import ast
from semmle.util import get_analysis_major_version
from blib2to3.pgen2.parse import ParseError
import sys

Expand Down Expand Up @@ -981,7 +982,20 @@ def visit_except_clause(self, node):
if len(node.children) > 1:
type = self.visit(node.children[1], LOAD)
if len(node.children) > 3:
name = self.visit(node.children[3], STORE)
# The grammar rule `'except' [test [(',' | 'as') test]]` is shared
# between two incompatible readings of a fourth child, so the
# separator token and the analysis version together decide:
# `except A as e:` binds an alias, in every version;
# `except A, e:` binds an alias when extracting Python 2, where
# that is the canonical idiom;
# `except A, B:` is an unparenthesized tuple of exception types
# otherwise -- PEP 758, Python 3.14+.
if is_token(node.children[2], "as") or get_analysis_major_version() == 2:
name = self.visit(node.children[3], STORE)
else:
elts = [type, self.visit(node.children[3], LOAD)]
type = ast.Tuple(elts, LOAD)
set_location(type, node.children[1].start, node.children[3].end)
Comment on lines +995 to +998

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.

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.py tries 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). There except Exception, e: really is the alias binding, so with this change e flips from a Store to a Load of 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).

Copy link
Copy Markdown
Author

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 e became a Load of an undefined name.

Gated as you suggested in 117fc6b. as binds an alias in every version; a comma binds an alias when get_analysis_major_version() == 2 and 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.py drives parser.parse directly with the version flipped and pins all four combinations: comma and as, py2 and py3, plus the parenthesized form that must bind no alias in either. Removing the gate fails the py2 case, and pytest tests/test_parser.py still passes 37.

Happy to add a --lang=2 query test under Imports/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.

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.

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.

Copy link
Copy Markdown
Author

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_except extracts with --lang=2 and 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: --lang did not reach the worker processes. populator.main honours it by calling update_analysis_version, but that rebinds a global in the process that parses the options, while extraction happens in an ExtractorPool — and on macOS those workers are spawned, not forked, so they re-read CODEQL_EXTRACTOR_PYTHON_ANALYSIS_VERSION and saw the default of 3. So --lang=2 meant Python 2 on Linux and Python 3 on macOS. My first version of this test passed under CODEQL_EXTRACTOR_PYTHON_ANALYSIS_VERSION=2 and failed under --lang=2 on 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.ql fails, 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 the relaxed_except*.py query tests from the earlier commit.
  • python/extractor pytest — 116 passed, including the 37 parser tests.

return type, name

def visit_del_stmt(self, node):
Expand Down
2 changes: 1 addition & 1 deletion python/extractor/semmle/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

#Semantic version of extractor.
#Update this if any changes are made
VERSION = "7.1.8"
VERSION = "7.1.9"

PY_EXTENSIONS = ".py", ".pyw"

Expand Down
10 changes: 10 additions & 0 deletions python/extractor/tests/parser/exceptions_relaxed.py
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
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.
1 change: 1 addition & 0 deletions python/ql/test/2/extractor-tests/relaxed_except/options
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
semmle-extractor-options: --lang=2
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 |
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
19 changes: 19 additions & 0 deletions python/ql/test/2/extractor-tests/relaxed_except/test.py
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
| imports_test.py:27:1:27:25 | Import | Import of 'func2' is not used. |
| imports_test.py:34:1:34:14 | Import | Import of 'module2' is not used. |
| imports_test.py:116:1:116:41 | Import | Import of 'not_a_fixture' is not used. |
| relaxed_except.py:12:1:12:68 | Import | Import of 'NeverUsed' is not used. |
26 changes: 26 additions & 0 deletions python/ql/test/query-tests/Imports/unused/relaxed_except.py
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
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
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
Loading