Merge pull request github#18934 from aschackmull/ssa/refactor5

SSA: Replace the Guards interface in the SSA data flow integration.

Author: aschackmull

csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll

@@ -1047,8 +1047,17 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
   }
 
   class Guard extends Guards::Guard {
-    predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) {
-      this.getAControlFlowNode() = bb.getNode(i)
+    /**
+     * Holds if the control flow branching from `bb1` is dependent on this guard,
+     * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
+     * guard to `branch`.
+     */
+    predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) {
+      exists(ControlFlow::SuccessorTypes::ConditionalSuccessor s |
+        this.getAControlFlowNode() = bb1.getLastNode() and
+        bb2 = bb1.getASuccessorByType(s) and
+        s.getValue() = branch
+      )
     }
   }
 
@@ -1060,16 +1069,6 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
       conditionBlock.edgeDominates(bb, s)
     )
   }
-
-  /** Gets an immediate conditional successor of basic block `bb`, if any. */
-  ControlFlow::BasicBlock getAConditionalBasicBlockSuccessor(
-    ControlFlow::BasicBlock bb, boolean branch
-  ) {
-    exists(ControlFlow::SuccessorTypes::ConditionalSuccessor s |
-      result = bb.getASuccessorByType(s) and
-      s.getValue() = branch
-    )
-  }
 }
 
 private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;

java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll

@@ -667,22 +667,20 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
   }
 
   class Guard extends Guards::Guard {
-    predicate hasCfgNode(BasicBlock bb, int i) {
-      this = bb.getNode(i).asExpr()
-      or
-      this = bb.getNode(i).asStmt()
+    /**
+     * Holds if the control flow branching from `bb1` is dependent on this guard,
+     * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
+     * guard to `branch`.
+     */
+    predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) {
+      super.hasBranchEdge(bb1, bb2, branch)
     }
   }
 
   /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
   predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch) {
     guard.controls(bb, branch)
   }
-
-  /** Gets an immediate conditional successor of basic block `bb`, if any. */
-  BasicBlock getAConditionalBasicBlockSuccessor(BasicBlock bb, boolean branch) {
-    result = bb.(Guards::ConditionBlock).getTestSuccessor(branch)
-  }
 }
 
 private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;

javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll

@@ -81,7 +81,19 @@ module SsaDataflowInput implements DataFlowIntegrationInputSig {
   class Guard extends js::ControlFlowNode {
     Guard() { this = any(js::ConditionGuardNode g).getTest() }
 
-    predicate hasCfgNode(js::BasicBlock bb, int i) { this = bb.getNode(i) }
+    /**
+     * Holds if the control flow branching from `bb1` is dependent on this guard,
+     * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
+     * guard to `branch`.
+     */
+    predicate controlsBranchEdge(js::BasicBlock bb1, js::BasicBlock bb2, boolean branch) {
+      exists(js::ConditionGuardNode g |
+        g.getTest() = this and
+        bb1 = this.getBasicBlock() and
+        bb2 = g.getBasicBlock() and
+        branch = g.getOutcome()
+      )
+    }
   }
 
   pragma[inline]
@@ -92,14 +104,6 @@ module SsaDataflowInput implements DataFlowIntegrationInputSig {
       branch = g.getOutcome()
     )
   }
-
-  js::BasicBlock getAConditionalBasicBlockSuccessor(js::BasicBlock bb, boolean branch) {
-    exists(js::ConditionGuardNode g |
-      bb = g.getTest().getBasicBlock() and
-      result = g.getBasicBlock() and
-      branch = g.getOutcome()
-    )
-  }
 }
 
 import DataFlowIntegration<SsaDataflowInput>

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll

@@ -74,10 +74,10 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) {
 
 /** Gets the SSA definition node corresponding to parameter `p`. */
 pragma[nomagic]
-SsaImpl::DefinitionExt getParameterDef(NamedParameter p) {
+Ssa::Definition getParameterDef(NamedParameter p) {
   exists(BasicBlock bb, int i |
     bb.getNode(i).getAstNode() = p.getDefiningAccess() and
-    result.definesAt(_, bb, i, _)
+    result.definesAt(_, bb, i)
   )
 }
 
@@ -388,15 +388,15 @@ module VariableCapture {
     Flow::clearsContent(asClosureNode(node), c.getVariable())
   }
 
-  class CapturedSsaDefinitionExt extends SsaImpl::DefinitionExt {
-    CapturedSsaDefinitionExt() { this.getSourceVariable() instanceof CapturedVariable }
+  class CapturedSsaDefinition extends SsaImpl::Definition {
+    CapturedSsaDefinition() { this.getSourceVariable() instanceof CapturedVariable }
   }
 
   // From an assignment or implicit initialization of a captured variable to its flow-insensitive node
   private predicate flowInsensitiveWriteStep(
     SsaDefinitionNodeImpl node1, CapturedVariableNode node2, CapturedVariable v
   ) {
-    exists(CapturedSsaDefinitionExt def |
+    exists(CapturedSsaDefinition def |
       def = node1.getDefinition() and
       def.getSourceVariable() = v and
       (
@@ -412,7 +412,7 @@ module VariableCapture {
   private predicate flowInsensitiveReadStep(
     CapturedVariableNode node1, SsaDefinitionNodeImpl node2, CapturedVariable v
   ) {
-    exists(CapturedSsaDefinitionExt def |
+    exists(CapturedSsaDefinition def |
       node1.getVariable() = v and
       def = node2.getDefinition() and
       def.getSourceVariable() = v and
@@ -772,7 +772,7 @@ class SsaNode extends NodeImpl, TSsaNode {
 class SsaDefinitionNodeImpl extends SsaNode {
   override SsaImpl::DataFlowIntegration::SsaDefinitionNode node;
 
-  SsaImpl::Definition getDefinition() { result = node.getDefinition() }
+  Ssa::Definition getDefinition() { result = node.getDefinition() }
 
   override predicate isHidden() {
     exists(SsaImpl::Definition def | def = this.getDefinition() |
@@ -2478,7 +2478,7 @@ module TypeInference {
       n = def or
       n.asExpr() =
         any(CfgNodes::ExprCfgNode read |
-          read = def.getDefinition().(SsaImpl::DefinitionExt).getARead() and
+          read = def.getDefinition().getARead() and
           not isTypeCheckedRead(read, _) // could in principle be checked against a new type
         )
     )

ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll

@@ -218,18 +218,18 @@ private predicate hasVariableReadWithCapturedWrite(
 
 pragma[noinline]
 deprecated private predicate adjacentDefReadExt(
-  DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2,
+  Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2,
   SsaInput::SourceVariable v
 ) {
   Impl::adjacentDefReadExt(def, _, bb1, i1, bb2, i2) and
   v = def.getSourceVariable()
 }
 
 deprecated private predicate adjacentDefReachesReadExt(
-  DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2
+  Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2
 ) {
   exists(SsaInput::SourceVariable v | adjacentDefReadExt(def, bb1, i1, bb2, i2, v) |
-    def.definesAt(v, bb1, i1, _)
+    def.definesAt(v, bb1, i1)
     or
     SsaInput::variableRead(bb1, i1, v, true)
   )
@@ -242,7 +242,7 @@ deprecated private predicate adjacentDefReachesReadExt(
 }
 
 deprecated private predicate adjacentDefReachesUncertainReadExt(
-  DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2
+  Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2
 ) {
   adjacentDefReachesReadExt(def, bb1, i1, bb2, i2) and
   SsaInput::variableRead(bb2, i2, _, false)
@@ -251,7 +251,7 @@ deprecated private predicate adjacentDefReachesUncertainReadExt(
 /** Same as `lastRefRedef`, but skips uncertain reads. */
 pragma[nomagic]
 deprecated private predicate lastRefSkipUncertainReadsExt(
-  DefinitionExt def, SsaInput::BasicBlock bb, int i
+  Definition def, SsaInput::BasicBlock bb, int i
 ) {
   Impl::lastRef(def, bb, i) and
   not SsaInput::variableRead(bb, i, def.getSourceVariable(), false)
@@ -413,33 +413,6 @@ private module Cached {
 
 import Cached
 
-/**
- * An extended static single assignment (SSA) definition.
- *
- * This is either a normal SSA definition (`Definition`) or a
- * phi-read node (`PhiReadNode`).
- *
- * Only intended for internal use.
- */
-class DefinitionExt extends Impl::DefinitionExt {
-  VariableReadAccessCfgNode getARead() { result = getARead(this) }
-
-  override string toString() { result = this.(Ssa::Definition).toString() }
-
-  override Location getLocation() { result = this.(Ssa::Definition).getLocation() }
-}
-
-/**
- * A phi-read node.
- *
- * Only intended for internal use.
- */
-class PhiReadNode extends DefinitionExt, Impl::PhiReadNode {
-  override string toString() { result = "SSA phi read(" + this.getSourceVariable() + ")" }
-
-  override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() }
-}
-
 class NormalParameter extends Parameter {
   NormalParameter() {
     this instanceof SimpleParameter or
@@ -453,10 +426,10 @@ class NormalParameter extends Parameter {
 
 /** Gets the SSA definition node corresponding to parameter `p`. */
 pragma[nomagic]
-DefinitionExt getParameterDef(NamedParameter p) {
+Definition getParameterDef(NamedParameter p) {
   exists(Cfg::BasicBlock bb, int i |
     bb.getNode(i).getAstNode() = p.getDefiningAccess() and
-    result.definesAt(_, bb, i, _)
+    result.definesAt(_, bb, i)
   )
 }
 
@@ -515,21 +488,24 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
   predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { p.isInitializedBy(def) }
 
   class Guard extends Cfg::CfgNodes::AstCfgNode {
-    predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
+    /**
+     * Holds if the control flow branching from `bb1` is dependent on this guard,
+     * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
+     * guard to `branch`.
+     */
+    predicate controlsBranchEdge(SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch) {
+      exists(Cfg::SuccessorTypes::ConditionalSuccessor s |
+        this.getBasicBlock() = bb1 and
+        bb2 = bb1.getASuccessor(s) and
+        s.getValue() = branch
+      )
+    }
   }
 
   /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
   predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) {
     Guards::guardControlsBlock(guard, bb, branch)
   }
-
-  /** Gets an immediate conditional successor of basic block `bb`, if any. */
-  SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) {
-    exists(Cfg::SuccessorTypes::ConditionalSuccessor s |
-      result = bb.getASuccessor(s) and
-      s.getValue() = branch
-    )
-  }
 }
 
 private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;

rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll

@@ -361,7 +361,18 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
   }
 
   class Guard extends CfgNodes::AstCfgNode {
-    predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
+    /**
+     * Holds if the control flow branching from `bb1` is dependent on this guard,
+     * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
+     * guard to `branch`.
+     */
+    predicate controlsBranchEdge(SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch) {
+      exists(Cfg::ConditionalSuccessor s |
+        this = bb1.getANode() and
+        bb2 = bb1.getASuccessor(s) and
+        s.getValue() = branch
+      )
+    }
   }
 
   /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
@@ -372,14 +383,6 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
       conditionBlock.edgeDominates(bb, s)
     )
   }
-
-  /** Gets an immediate conditional successor of basic block `bb`, if any. */
-  SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) {
-    exists(Cfg::ConditionalSuccessor s |
-      result = bb.getASuccessor(s) and
-      s.getValue() = branch
-    )
-  }
 }
 
 private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;

shared/ssa/codeql/ssa/Ssa.qll

@@ -1434,15 +1434,16 @@ module Make<LocationSig Location, InputSig<Location> Input> {
       /** Gets a textual representation of this guard. */
       string toString();
 
-      /** Holds if the `i`th node of basic block `bb` evaluates this guard. */
-      predicate hasCfgNode(BasicBlock bb, int i);
+      /**
+       * Holds if the control flow branching from `bb1` is dependent on this guard,
+       * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
+       * guard to `branch`.
+       */
+      predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch);
     }
 
     /** Holds if `guard` controls block `bb` upon evaluating to `branch`. */
     predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch);
-
-    /** Gets an immediate conditional successor of basic block `bb`, if any. */
-    BasicBlock getAConditionalBasicBlockSuccessor(BasicBlock bb, boolean branch);
   }
 
   /**
@@ -1891,11 +1892,7 @@ module Make<LocationSig Location, InputSig<Location> Input> {
           |
             DfInput::guardControlsBlock(g, bb, branch)
             or
-            exists(int last |
-              last = bb.length() - 1 and
-              g.hasCfgNode(bb, last) and
-              DfInput::getAConditionalBasicBlockSuccessor(bb, branch) = phi.getBasicBlock()
-            )
+            g.controlsBranchEdge(bb, phi.getBasicBlock(), branch)
           )
         )
       }