Skip to content

Shared CFG: add defaulted getWhileElse/getForeachElse to AstSig#21931

Open
yoff wants to merge 1 commit into
mainfrom
yoff/python-shared-cfg-loop-else
Open

Shared CFG: add defaulted getWhileElse/getForeachElse to AstSig#21931
yoff wants to merge 1 commit into
mainfrom
yoff/python-shared-cfg-loop-else

Conversation

@yoff
Copy link
Copy Markdown
Contributor

@yoff yoff commented Jun 2, 2026

Lifts the shared-controlflow changes out of #21921 so they can be reviewed independently. No behavioural change for existing languages (Java, C#, Ruby, Swift, Go, …) — both new predicates default to none() and the Make0 loop-edge case extensions only fire when a language overrides them.

Motivation

Python's upcoming new CFG library (#21921) needs to model while-else / for-else, where the else block runs when the loop condition becomes false (rather than via a break). The shared CFG didn't previously have predicates exposing those else blocks.

Changes

  • Adds defaulted getWhileElse(WhileStmt) and getForeachElse(ForeachStmt) to AstSig (shared/controlflow/codeql/controlflow/ControlFlowGraph.qll).
  • Extends Make0 in three places (while exit, foreach empty-collection exit, foreach post-body exit) to route through the else block if one exists.

Verification

Copilot AI review requested due to automatic review settings June 2, 2026 13:48
@yoff yoff requested a review from a team as a code owner June 2, 2026 13:48
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

Adds shared-CFG signature hooks for loop else blocks so languages that support while-else / for-else (notably Python) can expose that structure to the shared control-flow graph without impacting existing languages.

Changes:

  • Added defaulted AstSig.getWhileElse(WhileStmt) and AstSig.getForeachElse(ForeachStmt) predicates (default none()).
  • Updated shared CFG construction (Make0) to route certain loop-exit edges through an else block when provided by a language implementation.
  • Added a shared/controlflow change note documenting the new signature predicates.
Show a summary per file
File Description
shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Extends the shared CFG AST signature with defaulted loop-else accessors and threads them into loop edge construction.
shared/controlflow/change-notes/2026-05-19-loop-else.md Documents the new defaulted AstSig predicates for loop else blocks.

Copilot's findings

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

---
category: feature
---
* The `AstSig` signature gains two new defaulted predicates `getWhileElse` and `getForeachElse`, allowing languages (like Python) to model `while-else` / `for-else` constructs where the `else` branch is taken when the loop condition becomes false (rather than via a `break`). Existing languages that do not provide these predicates retain the previous behaviour.
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 wouldn't add a change note. This isn't a user-facing change.

… AstSig

Adds three new defaulted signature predicates to the shared CFG library:

- getWhileElse / getForeachElse: `else` block of a while/for loop, if
  any (used by Python's `while-else` / `for-else` constructs).
- getCatchType: type expression of a catch clause, if any (used by
  Python's `except SomeExpr:` where the catch type is a runtime
  expression that needs CFG evaluation).

Each predicate defaults to `none()`, so behaviour is unchanged for any
language that doesn't override it (verified by re-running
java/ql/test/library-tests/controlflow/).

The Make0 succession rules are extended:
- WhileStmt/ForeachStmt: route the loop-exit edge through the else
  block before reaching the after-position.
- CatchClause: route the matching-evaluation through the type
  expression (if present) before reaching the after-value position.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@yoff yoff force-pushed the yoff/python-shared-cfg-loop-else branch from 2eaf01c to 4d2296d Compare June 5, 2026 08:12
default AstNode getTryElse(TryStmt try) { none() }

/**
* Gets the `else` block of this `while` loop statement, if any.
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.

Suggested change
* Gets the `else` block of this `while` loop statement, if any.
* Gets the `else` block of `while` loop statement `loop`, if any.

default AstNode getWhileElse(WhileStmt loop) { none() }

/**
* Gets the `else` block of this `foreach` loop statement, if any.
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.

Suggested change
* Gets the `else` block of this `foreach` loop statement, if any.
* Gets the `else` block of `foreach` loop statement `loop`, if any.

default AstNode getForeachElse(ForeachStmt loop) { none() }

/**
* Gets the type expression of this catch clause, if any.
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.

Suggested change
* Gets the type expression of this catch clause, if any.
* Gets the type expression of catch clause `catch`, if any.

exists(MatchingSuccessor t |
n1.isBefore(catchclause) and
(
n2.isBefore(getCatchType(catchclause))
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.

t is unbound in this disjunct.

*
* Only some languages (e.g. Python) support `while-else` constructs.
*/
default AstNode getWhileElse(WhileStmt loop) { none() }
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.

Let's stick to just one new predicate

Suggested change
default AstNode getWhileElse(WhileStmt loop) { none() }
default AstNode getLoopElse(LoopStmt loop) { none() }

The semantics of a loop-else block is mainly related to whether the loop exits via its condition or a break, so it's not really tied to the specifics of while vs. foreach.

* statically resolved, this defaults to `none()` and no CFG node
* is created.
*/
default Expr getCatchType(CatchClause catch) { none() }
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.

This is unrelated to the PR description - was this meant to go in a different PR?

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.

Also, unless absolutely necessary, I don't think this should be a point of parameterisation - rather, I think we ought to be able to come up with a CFG for catch that solves the needs of all languages. We should probably discuss this offline and put a fix in a different PR when we agree on the solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants