Python: simplify decorator-detection predicates to pure AST match#21926
Open
yoff wants to merge 1 commit into
Open
Python: simplify decorator-detection predicates to pure AST match#21926yoff wants to merge 1 commit into
yoff wants to merge 1 commit into
Conversation
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>
Contributor
There was a problem hiding this comment.
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, andhasPropertyDecoratorto matchfunc.getADecorator().(Name).getId()against builtin decorator names. - Added/updated rationale comments in
DataFlowDispatch.qllexplaining the shift to a syntactic AST match. - Added a
minorAnalysischange 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. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Factor-out from the Python shared-CFG dataflow migration stack (#21919/#21920/#21921/#21923/#21925).
The internal predicates
isStaticmethod,isClassmethod, andhasPropertyDecorator(inDataFlowDispatch.qll) previously required the decorator'sNameNodeto satisfyisGlobal()— i.e. no SSA definition reaches the name use. That filter was correct but unnecessarily indirect:staticmethod,classmethod, andpropertyare builtins.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
Namedirectly 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.:With the previous code,
foowas 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/hasOverloadDecoratortoo?Those predicates also use the
NameNode.isGlobal()pattern, but their target names (contextmanagerfromcontextlib,overloadfromtyping) are imported, not builtin. TheisGlobal()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
python/ql/{src,lib}queries compile.library-tests/dataflow,library-tests/PointsTo/general, andquery-tests/Functionspass.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 theCfg::qualification on the two remaining contextmanager/overload predicates).