Ruby: Improve performance of flow through (hash) splats

github · Sep 25, 2023 · 12ceca8 · 12ceca8
1 parent 3634e0f
commit 12ceca8
Show file tree

Hide file tree

Showing 23 changed files with 5,005 additions and 4,773 deletions.
diff --git a/ruby/ql/lib/codeql/ruby/ApiGraphs.qll b/ruby/ql/lib/codeql/ruby/ApiGraphs.qll
@@ -886,8 +886,6 @@ module API {
       )
       or
       implicitCallEdge(pred, succ)
-      or
-      exists(DataFlow::HashLiteralNode splat | hashSplatEdge(splat, pred, succ))
     }
 
     /**
@@ -986,29 +984,6 @@ module API {
       )
     }
 
-    pragma[nomagic]
-    private DataFlow::Node getHashSplatArgument(DataFlow::HashLiteralNode literal) {
-      result = DataFlowPrivate::TSynthHashSplatArgumentNode(literal.asExpr())
-    }
-
-    /**
-     * Holds if the epsilon edge `pred -> succ` should be generated to account for the members of a hash literal.
-     *
-     * This currently exists because hash literals are desugared to `Hash.[]` calls, whose summary relies on `WithContent`.
-     * However, `contentEdge` does not currently generate edges for `WithContent` steps.
-     */
-    bindingset[literal]
-    pragma[inline_late]
-    private predicate hashSplatEdge(DataFlow::HashLiteralNode literal, ApiNode pred, ApiNode succ) {
-      exists(TypeTracker t |
-        pred = Impl::MkForwardNode(getALocalSourceStrict(getHashSplatArgument(literal)), t) and
-        succ = Impl::MkForwardNode(pragma[only_bind_out](literal), pragma[only_bind_out](t))
-        or
-        succ = Impl::MkBackwardNode(getALocalSourceStrict(getHashSplatArgument(literal)), t) and
-        pred = Impl::MkBackwardNode(pragma[only_bind_out](literal), pragma[only_bind_out](t))
-      )
-    }
-
     pragma[nomagic]
     private DataFlow::LocalSourceNode getAModuleReference(DataFlow::ModuleNode mod) {
       result = mod.getAnImmediateReference()

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
@@ -437,6 +437,7 @@ private module Cached {
       FlowSummaryImplSpecific::ParsePositions::isParsedKeywordParameterPosition(_, name)
     } or
     THashSplatArgumentPosition() or
+    TSynthHashSplatArgumentPosition() or
     TSplatArgumentPosition(int pos) { exists(Call c | c.getArgument(pos) instanceof SplatExpr) } or
     TSynthSplatArgumentPosition() or
     TAnyArgumentPosition() or
@@ -461,14 +462,10 @@ private module Cached {
       FlowSummaryImplSpecific::ParsePositions::isParsedKeywordArgumentPosition(_, name)
     } or
     THashSplatParameterPosition() or
-    // To get flow from a hash-splat argument to a keyword parameter, we add a read-step
-    // from a synthetic hash-splat parameter. We need this separate synthetic ParameterNode,
-    // since we clear content of the normal hash-splat parameter for the names that
-    // correspond to normal keyword parameters. Since we cannot re-use the same parameter
-    // position for multiple parameter nodes in the same callable, we introduce this
-    // synthetic parameter position.
     TSynthHashSplatParameterPosition() or
     TSplatParameterPosition(int pos) {
+      pos = 0
+      or
       exists(Parameter p | p.getPosition() = pos and p instanceof SplatParameter)
     } or
     TSynthSplatParameterPosition() or
@@ -1297,12 +1294,15 @@ class ParameterPosition extends TParameterPosition {
   /** Holds if this position represents a hash-splat parameter. */
   predicate isHashSplat() { this = THashSplatParameterPosition() }
 
+  /** Holds if this position represents a synthetic hash-splat parameter. */
   predicate isSynthHashSplat() { this = TSynthHashSplatParameterPosition() }
 
-  predicate isSynthSplat() { this = TSynthSplatParameterPosition() }
-
+  /** Holds if this position represents a splat parameter at position `n`. */
   predicate isSplat(int n) { this = TSplatParameterPosition(n) }
 
+  /** Holds if this position represents a synthetic splat parameter. */
+  predicate isSynthSplat() { this = TSynthSplatParameterPosition() }
+
   /**
    * Holds if this position represents any parameter, except `self` parameters. This
    * includes both positional, named, and block parameters.
@@ -1334,9 +1334,9 @@ class ParameterPosition extends TParameterPosition {
     or
     this.isAnyNamed() and result = "any-named"
     or
-    this.isSynthSplat() and result = "synthetic *"
-    or
     exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
+    or
+    this.isSynthSplat() and result = "synthetic *"
   }
 }
 
@@ -1366,14 +1366,16 @@ class ArgumentPosition extends TArgumentPosition {
   /** Holds if this position represents any positional parameter. */
   predicate isAnyNamed() { this = TAnyKeywordArgumentPosition() }
 
-  /**
-   * Holds if this position represents a synthesized argument containing all keyword
-   * arguments wrapped in a hash.
-   */
+  /** Holds if this position represents a hash-splat argument. */
   predicate isHashSplat() { this = THashSplatArgumentPosition() }
 
+  /** Holds if this position represents a synthetic hash-splat argument. */
+  predicate isSynthHashSplat() { this = TSynthHashSplatArgumentPosition() }
+
+  /** Holds if this position represents a splat argument at position `n`. */
   predicate isSplat(int n) { this = TSplatArgumentPosition(n) }
 
+  /** Holds if this position represents a synthetic splat argument. */
   predicate isSynthSplat() { this = TSynthSplatArgumentPosition() }
 
   /** Gets a textual representation of this position. */
@@ -1394,9 +1396,11 @@ class ArgumentPosition extends TArgumentPosition {
     or
     this.isHashSplat() and result = "**"
     or
-    this.isSynthSplat() and result = "synthetic *"
+    this.isSynthHashSplat() and result = "synthetic **"
     or
     exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
+    or
+    this.isSynthSplat() and result = "synthetic *"
   }
 }
 
@@ -1429,17 +1433,21 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
   or
   exists(string name | ppos.isKeyword(name) and apos.isKeyword(name))
   or
-  ppos.isHashSplat() and apos.isHashSplat()
-  or
-  ppos.isSynthHashSplat() and apos.isHashSplat()
+  (ppos.isHashSplat() or ppos.isSynthHashSplat()) and
+  (apos.isHashSplat() or apos.isSynthHashSplat())
   or
-  ppos.isSplat(0) and apos.isSynthSplat()
-  or
-  ppos.isSynthSplat() and
-  (apos.isSynthSplat() or apos.isSplat(0))
-  or
-  // Exact splat match
-  exists(int n | apos.isSplat(n) and ppos.isSplat(n))
+  exists(int pos |
+    (
+      ppos.isSplat(pos)
+      or
+      ppos.isSynthSplat() and pos = 0
+    ) and
+    (
+      apos.isSplat(pos)
+      or
+      apos.isSynthSplat() and pos = 0
+    )
+  )
   or
   ppos.isAny() and argumentPositionIsNotSelf(apos)
   or