Ruby: Replace CFG with shared implementation - #22158
Open
aschackmull wants to merge 10 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aschackmull
force-pushed
the
ruby/cfg-swap
branch
3 times, most recently
from
July 10, 2026 13:02
597c62a to
e8c7a76
Compare
aschackmull
force-pushed
the
ruby/cfg-swap
branch
3 times, most recently
from
July 30, 2026 12:03
10337a2 to
8b0d578
Compare
aschackmull
force-pushed
the
ruby/cfg-swap
branch
from
August 6, 2026 08:29
0baafe3 to
f374034
Compare
aschackmull
force-pushed
the
ruby/cfg-swap
branch
4 times, most recently
from
August 11, 2026 12:10
c74162e to
8dcd6e6
Compare
Comment on lines
+16
to
+18
| guardNode.isAfterValue(guard.getAstNode(), s) and | ||
| s.getValue() = branch and | ||
| guardNode.getBasicBlock().dominates(bb) |
aschackmull
marked this pull request as ready for review
August 11, 2026 12:50
aschackmull
added
the
no-change-note-required
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces Ruby’s custom control-flow graph implementation with the shared control-flow library and updates dependent analyses and test baselines.
Changes:
- Instantiates the shared CFG framework for Ruby and removes bespoke CFG, completion, splitting, and basic-block implementations.
- Migrates SSA, data-flow, security, framework, and IDE consumers to shared CFG APIs.
- Refreshes affected expected test results and consistency checks.
Show a summary per file
| File | Description |
|---|---|
ruby/ql/test/library-tests/variables/ssa.expected |
Updates SSA results for the shared CFG. |
ruby/ql/test/library-tests/frameworks/active_record/ActiveRecord.expected |
Updates Active Record data-flow results. |
ruby/ql/test/library-tests/dataflow/type-tracker/TypeTracker.expected |
Updates type-tracking results. |
ruby/ql/test/library-tests/dataflow/local/Nodes.expected |
Updates local data-flow nodes. |
ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql |
Uses shared basic-block types. |
ruby/ql/test/library-tests/controlflow/graph/Nodes.expected |
Updates CFG node results. |
ruby/ql/test/library-tests/controlflow/graph/Cfg.ql |
Uses shared CFG test output. |
ruby/ql/test/library-tests/controlflow/graph/BasicBlocks.ql |
Adapts basic-block tests. |
ruby/ql/test/library-tests/ast/ValueText.expected |
Updates CFG-backed value results. |
ruby/ql/src/queries/variables/DeadStoreOfLocal.ql |
Uses enclosing callable scope. |
ruby/ql/src/queries/performance/DatabaseQueryInLoop.ql |
Updates loop scope lookup. |
ruby/ql/src/experimental/performance/UseDetect.ql |
Uses direct control-flow-node lookup. |
ruby/ql/lib/ide-contextual-queries/printCfg.ql |
Migrates CFG visualization APIs. |
ruby/ql/lib/codeql/ruby/security/ConditionalBypassCustomizations.qll |
Adapts edge-dominance checks. |
ruby/ql/lib/codeql/ruby/frameworks/Sinatra.qll |
Updates callable scope lookup. |
ruby/ql/lib/codeql/ruby/dataflow/SSA.qll |
Simplifies phi-node rendering. |
ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll |
Migrates SSA construction to shared CFG blocks. |
ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll |
Migrates internal data-flow CFG usage. |
ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll |
Updates call scope and file lookup. |
ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll |
Removes bespoke CFG splitting. |
ruby/ql/lib/codeql/ruby/controlflow/internal/NonReturning.qll |
Maps non-returning calls to shared successor types. |
ruby/ql/lib/codeql/ruby/controlflow/internal/Guards.qll |
Adapts guard dominance logic. |
ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll |
Removes the previous CFG implementation. |
ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll |
Removes custom completion modeling. |
ruby/ql/lib/codeql/ruby/controlflow/ControlFlowGraph.qll |
Instantiates and configures the shared CFG. |
ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll |
Migrates Ruby CFG node wrappers. |
ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll |
Removes custom basic-block wrappers. |
ruby/ql/lib/codeql/ruby/CFG.qll |
Drops the removed basic-block import. |
ruby/ql/lib/codeql/ruby/ast/Statement.qll |
Exposes shared control-flow-node mappings. |
ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll |
Excludes synthesized assignment operands from CFG construction. |
ruby/ql/consistency-queries/DataFlowConsistency.ql |
Removes split-specific exclusions. |
ruby/ql/consistency-queries/CfgConsistency.ql |
Uses shared CFG consistency checks. |
Review details
Tip
Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 34/38 changed files
- Comments generated: 0
- Review effort level: Balanced
aschackmull
force-pushed
the
ruby/cfg-swap
branch
2 times, most recently
from
August 20, 2026 11:29
99eb18e to
9859c59
Compare
aschackmull
force-pushed
the
ruby/cfg-swap
branch
from
August 20, 2026 13:08
9859c59 to
879280a
Compare
aschackmull
removed
the
no-change-note-required
hvitved
reviewed
Aug 24, 2026
hvitved
left a comment
hvitved
left a comment
Contributor
There was a problem hiding this comment.
Looks great, thanks for doing this. I have a few minor comments.
| ControlFlowNode getControlFlowNode() { result.injects(this) } | ||
|
|
||
| /** Gets a control-flow node for this statement, if any. */ | ||
| CfgNodes::AstCfgNode getAControlFlowNode() { result.getAstNode() = this } |
Contributor
There was a problem hiding this comment.
I think we should deprecate this predicate.
| } | ||
|
|
||
| private R::Ast::AstNode adjustedGetChild(R::Ast::AstNode parent, int index) { | ||
| exists(R::Ast::WhenClause when | parent = when | |
Contributor
There was a problem hiding this comment.
I have a mild preference for parent = any(R::Ast::WhenClause when | ... (same in other cases below).
| or | ||
| findpattern.getElement(index) = result | ||
| or | ||
| index = 1 + max(int i | exists(findpattern.getElement(i))) and |
Contributor
There was a problem hiding this comment.
Use count instead as in the ArrayPattern case (not sure if there can be 0 elements)?
Comment on lines
+202
to
+203
| result = c.(R::Ast::Toplevel).getABeginBlock() or | ||
| result = c.(R::Ast::Toplevel).getAStmt() or |
Contributor
There was a problem hiding this comment.
Perhaps result = callableGetBodyPart(c, _, _) instead?
Comment on lines
+550
to
+552
| n1.isAfterTrue(ce.getCondition()) and not exists(ce.getThen()) and n2.isAfter(ce) | ||
| or | ||
| n1.isAfterFalse(ce.getCondition()) and not exists(ce.getElse()) and n2.isAfter(ce) |
Contributor
There was a problem hiding this comment.
Should this instead be done in the shared library?
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.
This replaces the Ruby control flow graph with an instantiation of the shared library.