Uh oh!
There was an error while loading. Please reload this page.
Python: inline init_module_submodule_defn into ImportResolution - #21930
Merged
Conversation
The new-dataflow ImportResolution module only used semmle.python.essa.SsaDefinitions for the 5-line helper predicate SsaSource::init_module_submodule_defn. Inline it locally and drop the dependency on legacy SsaDefinitions. This is the only remaining direct import of semmle.python.essa.* in the new dataflow stack, so dropping it makes the layering cleaner. Semantic noop on the current SSA: SsaSourceVariable.getName() and GlobalVariable.getId() both project the same DB column (variable(_,_,result)), and the old call's 'init.getEntryNode() = f' join was just constraining init = package via Scope.getEntryNode()'s functional uniqueness. RA dump of accesses.ql confirms only the expected predicate-rename shuffle; all 70 dataflow + ApiGraphs library tests pass. This factors out commit 8cab5a2 from the larger shared-CFG migration #21925. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes an unnecessary dependency on legacy Python ESSA definitions in the new-dataflow import resolution logic by inlining a small helper predicate, and simplifies internal decorator-detection predicates to rely on direct AST matching (with a corresponding change note).
Changes:
- Inlined
SsaSource::init_module_submodule_defnintoImportResolution.qllto avoid importingsemmle.python.essa.SsaDefinitions. - Simplified internal predicates for detecting
@staticmethod,@classmethod, and@propertyto match ASTNamenodes directly instead of going via CFG +isGlobal(). - Added a change note documenting the (rare) behavior change when these decorator names are shadowed.
Show a summary per file
| File | Description |
|---|---|
| python/ql/lib/semmle/python/dataflow/new/internal/ImportResolution.qll | Inlines a small helper predicate to drop an ESSA definitions dependency from the new dataflow stack. |
| python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll | Switches decorator detection for static/class/property decorators to direct AST Name matching with updated rationale comments. |
| python/ql/lib/change-notes/2026-06-01-decorator-predicate-simplification.md | Documents the decorator predicate simplification and the (rare) shadowing-related semantic change. |
Copilot's findings
- Files reviewed: 3/3 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. |
| exists(NameNode id | id.getId() = "classmethod" and id.isGlobal() | | ||
| func.getADecorator() = id.getNode() | ||
| ) | ||
| // See `isStaticmethod` for the rationale for matching on the AST `Name` |
Contributor
There was a problem hiding this comment.
You could refactor the functionality into a predicate hasDecorator(string id) { ...} and then transfer the comment to the re-usable implementation. This probably applies to any possible decorator
Contributor
There was a problem hiding this comment.
Or maybe hasBuiltinDecorator since the comment applies for code using python features and less reliably so if somebody defines a random decorator
yoff
changed the base branch from
main
to
yoff/python-simplify-decorator-predicatesJune 3, 2026 07:34
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Factored out of #21925 (shared-CFG migration).
The new-dataflow
ImportResolutionmodule only importedsemmle.python.essa.SsaDefinitionsto call the 5-line helperSsaSource::init_module_submodule_defn. Inlining it locally drops the dependency on legacy SSA definitions — makingnew/internal/ImportResolution.qllindependent ofessa/.Semantic-noop argument
SsaSourceVariable.getName()andVariable.getId()both project the same DB columnvariable(_, _, result), so the rename in the inlined body is safe.init_module_submodule_defn(var.getSourceVariable(), package.getEntryNode())constrainedinit = packagevia the joininit.getEntryNode() = package.getEntryNode()plusvar.getScope() = init. WithScope.getEntryNode()being functional, this is equivalent to passingpackagedirectly.var instanceof GlobalVariablecheck in the old body is now expressed via the parameter type.Verification
python/ql/test/library-tests/dataflow/global-flow/accesses.qlshows only the expected predicate-rename shuffle (no semantic shape change in any other predicate).python/ql/test/library-tests/dataflowandpython/ql/test/library-tests/ApiGraphspass without reblessing.Investigation note (negative result worth recording)
I had originally hoped to fold two other ESSA-bypass rewrites into this PR — switching
definitionFlowStepfromAssignmentDefinitiontoDefinitionNode + NameNode, and switchingModuleVariableNode.getAWritefromEssaNodeDefinition.definedBytoNameNode.defines— but they are not semantic noops onmain. Old ESSA'sEssaNodeDefinition(and henceAssignmentDefinition) is liveness-pruned atSsaCompute.qll:108-113, so bypassing it reintroduces dead writes.accesses.qlimmediately picks up the deadg_mod = []line. Those rewrites have to stay on the flip PR (#21925) where they're justified by the shared-SSA prune semantics.Stacked under #21926.