Ruby: Rework hash splat argument/parameter matching

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

@@ -1528,9 +1528,14 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
   or
   exists(string name | ppos.isKeyword(name) and apos.isKeyword(name))
   or
-  (ppos.isHashSplat() or ppos.isSynthHashSplat()) and
+  ppos.isHashSplat() and
   (apos.isHashSplat() or apos.isSynthHashSplat())
   or
+  // no case for `apos.isSynthHashSplat() and ppos.isSynthHashSplat()`, since
+  // direct keyword matching is possible
+  ppos.isSynthHashSplat() and
+  apos.isHashSplat()
+  or
   exists(int pos, boolean hasActualSplatParam, boolean hasActualSplatArg |
     (
       ppos.isSplat(pos) and

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

@@ -662,7 +662,7 @@ private module Cached {
       )
     } or
     deprecated TSplatContent(int i, Boolean shifted) { i in [0 .. 10] } or
-    THashSplatContent(ConstantValue::ConstantSymbolValue cv) or
+    deprecated THashSplatContent(ConstantValue::ConstantSymbolValue cv) or
     TCapturedVariableContent(VariableCapture::CapturedVariable v) or
     // Only used by type-tracking
     TAttributeName(string name) { name = any(SetterMethodCall c).getTargetName() }
@@ -686,24 +686,16 @@ private module Cached {
     TUnknownElementContentApprox() or
     TKnownIntegerElementContentApprox() or
     TKnownElementContentApprox(string approx) { approx = approxKnownElementIndex(_) } or
-    THashSplatContentApprox(string approx) { approx = approxKnownElementIndex(_) } or
     TNonElementContentApprox(Content c) { not c instanceof Content::ElementContent } or
     TCapturedVariableContentApprox(VariableCapture::CapturedVariable v)
 
   cached
   newtype TDataFlowType =
     TLambdaDataFlowType(Callable c) { c = any(LambdaSelfReferenceNode n).getCallable() } or
-    // In order to reduce the set of cons-candidates, we annotate all implicit (hash) splat
-    // creations with the name of the method that they are passed into. This includes
-    // array/hash literals as well (where the name is simply `[]`), because of how they
-    // are modeled (see `Array.qll` and `Hash.qll`).
-    TSynthHashSplatArgumentType(string methodName) {
-      methodName = any(SynthHashSplatArgumentNode n).getMethodName()
-    } or
     TUnknownDataFlowType()
 }
 
-class TElementContent = TKnownElementContent or TUnknownElementContent or THashSplatContent;
+class TElementContent = TKnownElementContent or TUnknownElementContent;
 
 import Cached
 
@@ -1118,18 +1110,6 @@ private module ParameterNodes {
    *
    * by adding read steps out of the synthesized parameter node to the relevant
    * keyword parameters.
-   *
-   * In order to avoid redundancy (and improve performance) in cases like
-   *
-   * ```rb
-   * foo(p1: taint(1), p2: taint(2))
-   * ```
-   *
-   * where direct keyword matching is possible, we use a special `HashSplatContent`
-   * (instead of reusing `KnownElementContent`) when we construct a synthesized hash
-   * splat argument (`SynthHashSplatArgumentNode`) at the call site, and then only
-   * add read steps out of this node for actual hash-splat arguments (which will use
-   * a normal `KnownElementContent`).
    */
   class SynthHashSplatParameterNode extends ParameterNodeImpl, TSynthHashSplatParameterNode {
     private DataFlowCallable callable;
@@ -1436,24 +1416,7 @@ module ArgumentNodes {
         not cv.isSymbol(_)
       )
     |
-      if call instanceof CfgNodes::ExprNodes::HashLiteralCfgNode
-      then
-        /*
-         * Needed for cases like
-         *
-         * ```rb
-         * hash = { a: taint, b: safe }
-         *
-         * def foo(a:, b:)
-         *   sink(a)
-         * end
-         *
-         * foo(**hash)
-         * ```
-         */
-
-        c.isSingleton(Content::getElementContent(cv))
-      else c.isSingleton(THashSplatContent(cv))
+      c.isSingleton(Content::getElementContent(cv))
     )
   }
 
@@ -1935,11 +1898,8 @@ DataFlowType getNodeType(Node n) {
     result = TLambdaDataFlowType(c)
   )
   or
-  result = TSynthHashSplatArgumentType(n.(SynthHashSplatArgumentNode).getMethodName())
-  or
   not n instanceof LambdaSelfReferenceNode and
   not mustHaveLambdaType(n, _) and
-  not n instanceof SynthHashSplatArgumentNode and
   result = TUnknownDataFlowType()
 }
 
@@ -2165,11 +2125,6 @@ class ContentApprox extends TContentApprox {
       result = "approximated element " + approx
     )
     or
-    exists(string s |
-      this = THashSplatContentApprox(s) and
-      result = "approximated hash-splat position " + s
-    )
-    or
     exists(Content c |
       this = TNonElementContentApprox(c) and
       result = c.toString()
@@ -2209,8 +2164,6 @@ ContentApprox getContentApprox(Content c) {
   result =
     TKnownElementContentApprox(approxKnownElementIndex(c.(Content::KnownElementContent).getIndex()))
   or
-  result = THashSplatContentApprox(approxKnownElementIndex(c.(Content::HashSplatContent).getKey()))
-  or
   result = TNonElementContentApprox(c)
 }
 

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

@@ -629,7 +629,7 @@ module Content {
    *
    * we have an implicit hash-splat argument containing `{:a => 1, :b => 2, :c => 3}`.
    */
-  class HashSplatContent extends ElementContent, THashSplatContent {
+  deprecated class HashSplatContent extends Content, THashSplatContent {
     private ConstantValue::ConstantSymbolValue cv;
 
     HashSplatContent() { this = THashSplatContent(cv) }
@@ -797,7 +797,6 @@ class ContentSet extends TContentSet {
   private Content getAnElementReadContent() {
     exists(Content::KnownElementContent c | this.isKnownOrUnknownElement(c) |
       result = c or
-      result = THashSplatContent(c.getIndex()) or
       result = TUnknownElementContent()
     )
     or
@@ -815,8 +814,6 @@ class ContentSet extends TContentSet {
     |
       type = result.(Content::KnownElementContent).getIndex().getValueType()
       or
-      type = result.(Content::HashSplatContent).getKey().getValueType()
-      or
       includeUnknown = true and
       result = TUnknownElementContent()
     )

ruby/ql/test/library-tests/dataflow/api-graphs/VerifyApiGraphExpectations.expected

@@ -1,2 +1,2 @@
-failures
 testFailures
+failures

ruby/ql/test/library-tests/dataflow/flow-summaries/semantics.expected

@@ -144,14 +144,14 @@ edges
 | semantics.rb:108:5:108:5 | b | semantics.rb:110:27:110:27 | b | provenance |  |
 | semantics.rb:108:9:108:18 | call to source | semantics.rb:108:5:108:5 | b | provenance |  |
 | semantics.rb:108:9:108:18 | call to source | semantics.rb:108:5:108:5 | b | provenance |  |
-| semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | semantics.rb:109:10:109:34 | ...[...] | provenance |  |
-| semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | semantics.rb:109:10:109:34 | ...[...] | provenance |  |
-| semantics.rb:109:19:109:19 | a | semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | provenance |  |
-| semantics.rb:109:19:109:19 | a | semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | provenance |  |
-| semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | semantics.rb:110:10:110:34 | ...[...] | provenance |  |
-| semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | semantics.rb:110:10:110:34 | ...[...] | provenance |  |
-| semantics.rb:110:27:110:27 | b | semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | provenance |  |
-| semantics.rb:110:27:110:27 | b | semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | provenance |  |
+| semantics.rb:109:10:109:28 | call to s15 [element :foo] | semantics.rb:109:10:109:34 | ...[...] | provenance |  |
+| semantics.rb:109:10:109:28 | call to s15 [element :foo] | semantics.rb:109:10:109:34 | ...[...] | provenance |  |
+| semantics.rb:109:19:109:19 | a | semantics.rb:109:10:109:28 | call to s15 [element :foo] | provenance |  |
+| semantics.rb:109:19:109:19 | a | semantics.rb:109:10:109:28 | call to s15 [element :foo] | provenance |  |
+| semantics.rb:110:10:110:28 | call to s15 [element :bar] | semantics.rb:110:10:110:34 | ...[...] | provenance |  |
+| semantics.rb:110:10:110:28 | call to s15 [element :bar] | semantics.rb:110:10:110:34 | ...[...] | provenance |  |
+| semantics.rb:110:27:110:27 | b | semantics.rb:110:10:110:28 | call to s15 [element :bar] | provenance |  |
+| semantics.rb:110:27:110:27 | b | semantics.rb:110:10:110:28 | call to s15 [element :bar] | provenance |  |
 | semantics.rb:114:5:114:5 | a | semantics.rb:116:14:116:14 | a | provenance |  |
 | semantics.rb:114:5:114:5 | a | semantics.rb:116:14:116:14 | a | provenance |  |
 | semantics.rb:114:5:114:5 | a | semantics.rb:119:17:119:17 | a | provenance |  |
@@ -1269,14 +1269,14 @@ nodes
 | semantics.rb:108:5:108:5 | b | semmle.label | b |
 | semantics.rb:108:9:108:18 | call to source | semmle.label | call to source |
 | semantics.rb:108:9:108:18 | call to source | semmle.label | call to source |
-| semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | semmle.label | call to s15 [hash-splat position :foo] |
-| semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | semmle.label | call to s15 [hash-splat position :foo] |
+| semantics.rb:109:10:109:28 | call to s15 [element :foo] | semmle.label | call to s15 [element :foo] |
+| semantics.rb:109:10:109:28 | call to s15 [element :foo] | semmle.label | call to s15 [element :foo] |
 | semantics.rb:109:10:109:34 | ...[...] | semmle.label | ...[...] |
 | semantics.rb:109:10:109:34 | ...[...] | semmle.label | ...[...] |
 | semantics.rb:109:19:109:19 | a | semmle.label | a |
 | semantics.rb:109:19:109:19 | a | semmle.label | a |
-| semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | semmle.label | call to s15 [hash-splat position :bar] |
-| semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | semmle.label | call to s15 [hash-splat position :bar] |
+| semantics.rb:110:10:110:28 | call to s15 [element :bar] | semmle.label | call to s15 [element :bar] |
+| semantics.rb:110:10:110:28 | call to s15 [element :bar] | semmle.label | call to s15 [element :bar] |
 | semantics.rb:110:10:110:34 | ...[...] | semmle.label | ...[...] |
 | semantics.rb:110:10:110:34 | ...[...] | semmle.label | ...[...] |
 | semantics.rb:110:27:110:27 | b | semmle.label | b |