Merge pull request #22158 from aschackmull/ruby/cfg-swap

Ruby: Replace CFG with shared implementation

Author: aschackmull

ruby/ql/consistency-queries/CfgConsistency.ql

@@ -1,31 +1,2 @@
-import codeql.ruby.controlflow.internal.ControlFlowGraphImpl::Consistency as Consistency
-import Consistency
-import codeql.ruby.AST
 import codeql.ruby.CFG
-import codeql.ruby.controlflow.internal.Completion
-import codeql.ruby.controlflow.internal.ControlFlowGraphImpl as CfgImpl
-
-/**
- * All `Expr` nodes are `PostOrderTree`s
- */
-query predicate nonPostOrderExpr(Expr e, string cls) {
-  cls = e.getPrimaryQlClasses() and
-  not exists(e.getDesugared()) and
-  not e instanceof BodyStmt and
-  exists(AstNode last, Completion c |
-    CfgImpl::last(e, last, c) and
-    last != e and
-    c instanceof NormalCompletion
-  )
-}
-
-query predicate scopeNoFirst(CfgScope scope) {
-  Consistency::scopeNoFirst(scope) and
-  not scope = any(StmtSequence seq | not exists(seq.getAStmt())) and
-  not scope =
-    any(Callable c |
-      not exists(c.getAParameter()) and
-      not c.getBody().hasEnsure() and
-      not exists(c.getBody().getARescue())
-    )
-}
+import ControlFlow::Consistency

ruby/ql/consistency-queries/DataFlowConsistency.ql

@@ -22,17 +22,6 @@ private module Input implements InputSig<Location, RubyDataFlow> {
     not isNonConstantExpr(n.asExpr())
   }
 
-  predicate multipleArgumentCallExclude(ArgumentNode arg, DataFlowCall call) {
-    // An argument such as `x` in `if not x then ...` has two successors (and hence
-    // two calls); one for each Boolean outcome of `x`.
-    exists(CfgNodes::ExprCfgNode n |
-      arg.argumentOf(call, _) and
-      n = call.asCall() and
-      arg.asExpr().getASuccessor(any(ConditionalSuccessor c)).getASuccessor*() = n and
-      n.getASplit() instanceof Split::ConditionalCompletionSplit
-    )
-  }
-
   predicate uniqueTypeExclude(Node n) {
     n =
       any(DataFlow::CallNode call |

ruby/ql/lib/change-notes/2026-08-24-cfg-swap.md

@@ -0,0 +1,14 @@
+---
+category: breaking
+---
+* The Ruby control flow graph implementation has been completely replaced. This
+  affects a number of queries slightly. The CFG now includes additional nodes
+  to more accurately represent certain constructs. This also means that any
+  existing code that implicitly relies on very specific details about the CFG
+  may need to be updated. The CFG no longer uses splitting, which means that
+  AST nodes now have a unique CFG node representation. In particular,
+  `ControlFlowNode.getAstNode` has changed its meaning. The AST-to-CFG mapping
+  remains one-to-many, but now for a different reason. It used to be because of
+  splitting, but now it's because of additional "helper" CFG nodes. To get the
+  (now canonical) CFG node for a given AST node, use
+  `Stmt.getControlFlowNode()` instead.

ruby/ql/lib/codeql/ruby/CFG.qll

@@ -3,4 +3,3 @@
 import codeql.Locations
 import controlflow.ControlFlowGraph
 import controlflow.CfgNodes as CfgNodes
-import controlflow.BasicBlocks

ruby/ql/lib/codeql/ruby/ast/Call.qll

@@ -63,7 +63,7 @@ class Call extends Expr instanceof CallImpl {
       TCfgScope(result) = viableCallableLambda(c, _)
     )
     or
-    result = getTarget(TNormalCall(this.getAControlFlowNode()))
+    result = getTarget(TNormalCall(this.getControlFlowNode()))
   }
 
   override AstNode getAChild(string pred) {

ruby/ql/lib/codeql/ruby/ast/Statement.qll

@@ -6,22 +6,25 @@ private import codeql.ruby.CFG
 private import internal.AST
 private import internal.TreeSitter
 private import internal.Variable
-private import codeql.ruby.controlflow.internal.ControlFlowGraphImpl as CfgImpl
 
 /**
  * A statement.
  *
  * This is the root QL class for all statements.
  */
 class Stmt extends AstNode, TStmt {
-  /** Gets a control-flow node for this statement, if any. */
-  CfgNodes::AstCfgNode getAControlFlowNode() { result.getAstNode() = this }
+  /** Gets the control-flow node for this statement, if any. */
+  ControlFlowNode getControlFlowNode() { result.injects(this) }
 
-  /** Gets a control-flow entry node for this statement, if any */
-  AstNode getAControlFlowEntryNode() { result = CfgImpl::getAControlFlowEntryNode(this) }
+  /**
+   * DEPRECATED: Use `getControlFlowNode()` instead.
+   *
+   * Gets a control-flow node for this statement, if any.
+   */
+  deprecated CfgNodes::AstCfgNode getAControlFlowNode() { result.getAstNode() = this }
 
   /** Gets the control-flow scope of this statement, if any. */
-  CfgScope getCfgScope() { result = CfgImpl::getCfgScope(this) }
+  CfgScope getCfgScope() { result = getEnclosingCallable(this) }
 
   /** Gets the enclosing callable, if any. */
   Callable getEnclosingCallable() { result = this.getCfgScope() }

ruby/ql/lib/codeql/ruby/ast/internal/Constant.qll

@@ -95,7 +95,7 @@ private module Propagation {
     or
     isIntExpr(e.(ConstantReadAccess).getValue(), i)
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isInt(n, i))
+    isInt(e.getControlFlowNode(), i)
   }
 
   predicate isFloat(ExprCfgNode e, float f) {
@@ -153,7 +153,7 @@ private module Propagation {
     or
     isFloatExpr(e.(ConstantReadAccess).getValue(), f)
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isFloat(n, f))
+    isFloat(e.getControlFlowNode(), f)
   }
 
   predicate isRational(ExprCfgNode e, int numerator, int denominator) {
@@ -175,7 +175,7 @@ private module Propagation {
     or
     isRationalExpr(e.(ConstantReadAccess).getValue(), numerator, denominator)
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isRational(n, numerator, denominator))
+    isRational(e.getControlFlowNode(), numerator, denominator)
   }
 
   predicate isComplex(ExprCfgNode e, float real, float imaginary) {
@@ -197,7 +197,7 @@ private module Propagation {
     or
     isComplexExpr(e.(ConstantReadAccess).getValue(), real, imaginary)
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isComplex(n, real, imaginary))
+    isComplex(e.getControlFlowNode(), real, imaginary)
   }
 
   overlay[local]
@@ -309,7 +309,7 @@ private module Propagation {
     or
     isStringExpr(e.(ConstantReadAccess).getValue(), s)
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isString(n, s))
+    isString(e.getControlFlowNode(), s)
   }
 
   predicate isSymbol(ExprCfgNode e, string s) {
@@ -334,7 +334,7 @@ private module Propagation {
     or
     isSymbolExpr(e.(ConstantReadAccess).getValue(), s)
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isSymbol(n, s))
+    isSymbol(e.getControlFlowNode(), s)
   }
 
   predicate isRegExp(ExprCfgNode e, string s, string flags) {
@@ -359,7 +359,7 @@ private module Propagation {
     or
     isRegExpExpr(e.(ConstantReadAccess).getValue(), s, flags)
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isRegExp(n, s, flags))
+    isRegExp(e.getControlFlowNode(), s, flags)
   }
 
   predicate isBoolean(ExprCfgNode e, boolean b) {
@@ -381,7 +381,7 @@ private module Propagation {
     or
     isBooleanExpr(e.(ConstantReadAccess).getValue(), b)
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isBoolean(n, b))
+    isBoolean(e.getControlFlowNode(), b)
   }
 
   predicate isNil(ExprCfgNode e) {
@@ -403,7 +403,7 @@ private module Propagation {
     or
     isNilExpr(e.(ConstantReadAccess).getValue())
     or
-    forex(ExprCfgNode n | n = e.getAControlFlowNode() | isNil(n))
+    isNil(e.getControlFlowNode())
   }
 }
 
@@ -566,7 +566,7 @@ private predicate isArrayExpr(Expr e, ArrayLiteralCfgNode arr) {
   // control flow paths.
   // Note(hmac): I don't think this is necessary, as `getSource` will not return
   // results if the source is a phi node.
-  forex(ExprCfgNode n | n = e.getAControlFlowNode() | isArrayConstant(n, arr))
+  isArrayConstant(e.getControlFlowNode(), arr)
   or
   // if `e` is an array, then `e.freeze` is also an array
   e.(MethodCall).getMethodName() = "freeze" and

ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll

@@ -833,6 +833,10 @@ private module AssignOperationDesugar {
         )
       )
     }
+
+    final override predicate excludeFromControlFlowTree(AstNode n) {
+      n = any(ScopeResolutionAssignOperation sao).getLeftOperand()
+    }
   }
 
   /** An assignment operation where the left-hand side is a method call. */