Skip to content

Python: simplify decorator-detection predicates to pure AST match#21926

Open
yoff wants to merge 1 commit into
mainfrom
yoff/python-simplify-decorator-predicates
Open

Python: simplify decorator-detection predicates to pure AST match#21926
yoff wants to merge 1 commit into
mainfrom
yoff/python-simplify-decorator-predicates

Conversation

@yoff
Copy link
Copy Markdown
Contributor

@yoff yoff commented Jun 1, 2026

Factor-out from the Python shared-CFG dataflow migration stack (#21919/#21920/#21921/#21923/#21925).

The internal predicates isStaticmethod, isClassmethod, and hasPropertyDecorator (in DataFlowDispatch.qll) previously required the decorator's NameNode to satisfy isGlobal() — i.e. no SSA definition reaches the name use. That filter was correct but unnecessarily indirect:

  • staticmethod, classmethod, and property are builtins.
  • Even when a class body redefines one (staticmethod = ... inside the class), the class body has not started executing yet at the decorator position, so Python uses the builtin at that point.

This PR matches the decorator's AST Name directly instead, dropping the CFG/SSA detour.

Semantic change

There is a small behavioural difference. The previous isGlobal() check rejected module-level shadowing of these builtins, e.g.:

def staticmethod(x): ...  # shadows the builtin in this module

class C:
    @staticmethod
    def foo(): ...  # at the decorator position, `staticmethod` is the local one

With the previous code, foo was not classified as a staticmethod. With the new code, it is. Shadowing these three names at module scope is essentially never seen in practice, so this is a negligible false-positive risk in exchange for much cleaner code. Documented in a change note.

Why not hasContextmanagerDecorator / hasOverloadDecorator too?

Those predicates also use the NameNode.isGlobal() pattern, but their target names (contextmanager from contextlib, overload from typing) are imported, not builtin. The isGlobal() check legitimately filters out cases where these names refer to a local variable rather than the imported one, which is a more realistic shadowing scenario. So they stay as-is.

Verification

  • All 361 python/ql/{src,lib} queries compile.
  • All 88 tests in library-tests/dataflow, library-tests/PointsTo/general, and query-tests/Functions pass.

This PR has no dependencies on the rest of the migration stack and can land independently. After it merges, the corresponding predicate changes in yoff/python-shared-cfg-dataflow-flip (#21925) collapse to a noop (just the Cfg:: qualification on the two remaining contextmanager/overload predicates).

The internal predicates that identify `@staticmethod`, `@classmethod` and
`@property` decorators previously required the decorator's `NameNode` to
satisfy `isGlobal()` (i.e. no SSA def reaches the decorator's name use).
That filter was correct but unnecessarily indirect: these three names
are builtins, and even when a class body redefines one, the class body
has not started executing at the decorator position, so Python uses the
builtin.

Match the decorator's AST `Name` directly instead, dropping the CFG/SSA
detour. The slight semantic change — `isGlobal()` would have rejected
module-level shadowing of these builtins — is negligible in practice
and explicitly documented in the change note.

`hasContextmanagerDecorator` and `hasOverloadDecorator` keep the
`NameNode.isGlobal()` check because their target names (`contextmanager`,
`overload`) are imported, not builtin, and local shadowing is a real
concern.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 1, 2026 14:05
@yoff yoff requested a review from a team as a code owner June 1, 2026 14:05
Copy link
Copy Markdown
Contributor

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.

Pull request overview

Simplifies internal Python dataflow decorator-detection predicates by matching decorator AST Name nodes directly (instead of going through CFG/SSA isGlobal()), as part of the shared-CFG dataflow migration refactoring stack.

Changes:

  • Updated isStaticmethod, isClassmethod, and hasPropertyDecorator to match func.getADecorator().(Name).getId() against builtin decorator names.
  • Added/updated rationale comments in DataFlowDispatch.qll explaining the shift to a syntactic AST match.
  • Added a minorAnalysis change note documenting the (rare) behavior change for shadowed builtin decorator names.
Show a summary per file
File Description
python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll Switched builtin decorator predicates from CFG/SSA-based resolution checks to direct AST Name matching and added explanatory comments.
python/ql/lib/change-notes/2026-06-01-decorator-predicate-simplification.md Added change note describing the predicate simplification and its (rare) semantic impact.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment on lines +259 to +263
// The decorator is *syntactically* a `Name` "staticmethod" — we don't
// care which variable it resolves to. `staticmethod` is a builtin and
// is almost never shadowed in a module-level scope; even if a class
// redefines `staticmethod` in its body, the class body has not started
// executing yet at the decorator position, so Python uses the builtin.
---
category: minorAnalysis
---
* Simplified the internal predicates that detect `@staticmethod`, `@classmethod` and `@property` decorators to match the decorator's AST `Name` directly, rather than going through the CFG and requiring the name to resolve globally. Code that shadows these three builtin decorators at the module-scope will now be classified by the decorator name alone; in practice, shadowing these names is extremely rare and the call-graph results are unchanged.
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.

2 participants