Python: fix shared CFG exception-handler reachability - #22380
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes shared Python CFG/SSA regressions affecting exception-handler reachability and module-export phi uses.
Changes:
- Routes exceptional completion through canonical CFG nodes.
- Preserves synthetic normal-exit uses for generic SSA definitions.
- Adds regression tests for both fixes.
Show a summary per file
| File | Description |
|---|---|
python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll |
Uses canonical CFG-node mapping for exception flow. |
python/ql/lib/semmle/python/dataflow/new/internal/SsaImpl.qll |
Supports phi definitions when resolving uses. |
python/ql/test/library-tests/ControlFlow/shared-cfg-exceptions/test.py |
Adds yield and import exception cases. |
python/ql/test/library-tests/ControlFlow/shared-cfg-exceptions/ExceptionReachabilityTest.ql |
Checks typed-handler reachability. |
python/ql/test/library-tests/ControlFlow/shared-cfg-exceptions/ExceptionReachabilityTest.expected |
Records expected exception results. |
python/ql/test/library-tests/dataflow-new-ssa/test.py |
Adds conditional module-export coverage. |
python/ql/test/library-tests/dataflow-new-ssa/SsaTest.ql |
Checks synthetic exit uses for phi definitions. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/7 changed files
- Comments generated: 0
- Review effort level: Balanced
9055769 to
94aa7e2
Compare
0394945 to
c69ba9a
Compare
94aa7e2 to
a93d631
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
a93d631 to
3dfc77e
Compare
| or | ||
| mayThrow(ast) and | ||
| n.isIn(ast) and | ||
| n.injects(ast) and |
There was a problem hiding this comment.
Hmm, I think perhaps you need to add the relevant nodes to postOrInOrder instead. Otherwise, if these are leaf nodes, then you end up without a separate after-node, which means that unconditional exceptions cannot be represented.
(The fact that we have always = false may save us for now, but flipping that boolean wouldn't work, so it seems a bit too brittle.)
There was a problem hiding this comment.
I guess perhaps we could add a consistency check to verify that beginAbruptCompletion always sit on isIn or isAdditional nodes. I believe that's an implicit assumption by the library.
Summary
Fixes exception-handler reachability in the shared Python CFG introduced by #21921 and now present on
main. Testing the dataflow switch-over in #21925 surfaced the bug.Leaf expressions that may throw, including bare
yieldand plain imports, were not always connected to typed exception-handler entries. The fix uses the canonical shared-CFGinjectsmapping when starting exceptional completion, so these expressions reach the appropriate catch entry.Tests
Adds focused inline coverage for:
yieldreaching aGeneratorExithandler;ImportErrorhandler.DCA impact
The switch-over DCA in #38556 exposed effects of the missing exception edges in mypy, CPython, and youtube-dl. Restoring these edges recovers the affected catch reachability and downstream flow.
Validation
codeql test run python/ql/test/library-tests/ControlFlow/shared-cfg-exceptionscodeql query format --check-only python/ql/test/library-tests/ControlFlow/shared-cfg-exceptions/ExceptionReachabilityTest.ql