Merge pull request github#12467 from RasmusWL/kwargs-parameter-position-fixup

Python/Ruby: Use new parameter position for synthetic hash-splat instead

Author: RasmusWL

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll

@@ -64,7 +64,14 @@ newtype TParameterPosition =
     index = any(Parameter p).getPosition() + 1
   } or
   TSynthStarArgsElementParameterPosition(int index) { exists(TStarArgsParameterPosition(index)) } or
-  TDictSplatParameterPosition()
+  TDictSplatParameterPosition() or
+  // To get flow from a **kwargs argument to a keyword parameter, we add a read-step
+  // from a synthetic **kwargs parameter. We need this separate synthetic ParameterNode,
+  // since we clear content of the normal **kwargs 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.
+  TSynthDictSplatParameterPosition()
 
 /** A parameter position. */
 class ParameterPosition extends TParameterPosition {
@@ -92,6 +99,12 @@ class ParameterPosition extends TParameterPosition {
   /** Holds if this position represents a `**kwargs` parameter. */
   predicate isDictSplat() { this = TDictSplatParameterPosition() }
 
+  /**
+   * Holds if this position represents a **synthetic** `**kwargs` parameter
+   * (see comment for `TSynthDictSplatParameterPosition`).
+   */
+  predicate isSynthDictSplat() { this = TSynthDictSplatParameterPosition() }
+
   /** Gets a textual representation of this element. */
   string toString() {
     this.isSelf() and result = "self"
@@ -108,6 +121,8 @@ class ParameterPosition extends TParameterPosition {
     )
     or
     this.isDictSplat() and result = "**"
+    or
+    this.isSynthDictSplat() and result = "synthetic **"
   }
 }
 
@@ -179,6 +194,8 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
   )
   or
   ppos.isDictSplat() and apos.isDictSplat()
+  or
+  ppos.isSynthDictSplat() and apos.isDictSplat()
 }
 
 // =============================================================================
@@ -324,16 +341,9 @@ abstract class DataFlowFunction extends DataFlowCallable, TFunction {
     )
     or
     // `**kwargs`
-    // since the dataflow library has the restriction that we can only have ONE result per
-    // parameter position, if there is both a synthetic **kwargs and a real **kwargs
-    // parameter, we only give the result for the synthetic, and add local flow from the
-    // synthetic to the real. It might seem more natural to do it in the other
-    // direction, but since we have a clearStep on the real **kwargs parameter, we would have that
-    // content-clearing would also affect the synthetic parameter, which we don't want.
-    ppos.isDictSplat() and
-    if exists(func.getArgByName(_))
-    then result = TSynthDictSplatParameterNode(this)
-    else result.getParameter() = func.getKwarg()
+    ppos.isDictSplat() and result.getParameter() = func.getKwarg()
+    or
+    ppos.isSynthDictSplat() and result = TSynthDictSplatParameterNode(this)
   }
 }
 
@@ -1460,16 +1470,7 @@ class SummaryParameterNode extends ParameterNodeImpl, TSummaryParameterNode {
   override Parameter getParameter() { none() }
 
   override predicate isParameterOf(DataFlowCallable c, ParameterPosition ppos) {
-    sc = c.asLibraryCallable() and
-    ppos = pos and
-    // avoid overlap with `SynthDictSplatParameterNode`
-    not (
-      pos.isDictSplat() and
-      exists(ParameterPosition keywordPos |
-        FlowSummaryImpl::Private::summaryParameterNodeRange(sc, keywordPos) and
-        keywordPos.isKeyword(_)
-      )
-    )
+    sc = c.asLibraryCallable() and ppos = pos
   }
 
   override DataFlowCallable getEnclosingCallable() { result.asLibraryCallable() = sc }

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

@@ -182,11 +182,7 @@ private predicate synthDictSplatArgumentNodeStoreStep(
 private predicate dictSplatParameterNodeClearStep(ParameterNode n, DictionaryElementContent c) {
   exists(DataFlowCallable callable, ParameterPosition dictSplatPos, ParameterPosition keywordPos |
     dictSplatPos.isDictSplat() and
-    (
-      n.getParameter() = callable.(DataFlowFunction).getScope().getKwarg()
-      or
-      n = TSummaryParameterNode(callable.asLibraryCallable(), dictSplatPos)
-    ) and
+    n = callable.getParameter(dictSplatPos) and
     exists(callable.getParameter(keywordPos)) and
     keywordPos.isKeyword(c.getKey())
   )
@@ -237,28 +233,6 @@ class SynthDictSplatParameterNode extends ParameterNodeImpl, TSynthDictSplatPara
   override Parameter getParameter() { none() }
 }
 
-/**
- * Flow step from the synthetic `**kwargs` parameter to the real `**kwargs` parameter.
- * Due to restriction in dataflow library, we can only give one of them as result for
- * `DataFlowCallable.getParameter`, so this is a workaround to ensure there is flow to
- * _both_ of them.
- */
-private predicate dictSplatParameterNodeFlowStep(
-  ParameterNodeImpl nodeFrom, ParameterNodeImpl nodeTo
-) {
-  exists(DataFlowCallable callable |
-    nodeFrom = TSynthDictSplatParameterNode(callable) and
-    (
-      nodeTo.getParameter() = callable.(DataFlowFunction).getScope().getKwarg()
-      or
-      exists(ParameterPosition pos |
-        nodeTo = TSummaryParameterNode(callable.asLibraryCallable(), pos) and
-        pos.isDictSplat()
-      )
-    )
-  )
-}
-
 /**
  * Reads from the synthetic **kwargs parameter to each keyword parameter.
  */
@@ -404,8 +378,6 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   simpleLocalFlowStepForTypetracking(nodeFrom, nodeTo)
   or
   summaryFlowSteps(nodeFrom, nodeTo)
-  or
-  dictSplatParameterNodeFlowStep(nodeFrom, nodeTo)
 }
 
 /**

python/ql/test/experimental/dataflow/TestUtil/DataFlowConsistency.qll

@@ -20,15 +20,6 @@ private class MyConsistencyConfiguration extends ConsistencyConfiguration {
     n instanceof SynthDictSplatParameterNode
   }
 
-  override predicate uniqueParameterNodeAtPositionExclude(
-    DataFlowCallable c, ParameterPosition pos, Node p
-  ) {
-    // TODO: This can be removed once we solve the overlap of dictionary splat parameters
-    c.getParameter(pos) = p and
-    pos.isDictSplat() and
-    not exists(p.getLocation().getFile().getRelativePath())
-  }
-
   override predicate uniqueParameterNodePositionExclude(
     DataFlowCallable c, ParameterPosition pos, Node p
   ) {

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

@@ -443,6 +443,13 @@ 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
     TSplatAllParameterPosition() or
     TAnyParameterPosition() or
     TAnyKeywordParameterPosition()
@@ -1249,6 +1256,8 @@ class ParameterPosition extends TParameterPosition {
   /** Holds if this position represents a hash-splat parameter. */
   predicate isHashSplat() { this = THashSplatParameterPosition() }
 
+  predicate isSynthHashSplat() { this = TSynthHashSplatParameterPosition() }
+
   predicate isSplatAll() { this = TSplatAllParameterPosition() }
 
   /**
@@ -1274,6 +1283,8 @@ class ParameterPosition extends TParameterPosition {
     or
     this.isHashSplat() and result = "**"
     or
+    this.isSynthHashSplat() and result = "synthetic **"
+    or
     this.isSplatAll() and result = "*"
     or
     this.isAny() and result = "any"
@@ -1356,6 +1367,8 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
   or
   ppos.isHashSplat() and apos.isHashSplat()
   or
+  ppos.isSynthHashSplat() and apos.isHashSplat()
+  or
   ppos.isSplatAll() and apos.isSplatAll()
   or
   ppos.isAny() and argumentPositionIsNotSelf(apos)

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

@@ -189,18 +189,6 @@ module LocalFlow {
   }
 
   predicate localFlowStepCommon(Node nodeFrom, Node nodeTo) {
-    exists(DataFlowCallable c | nodeFrom = TSynthHashSplatParameterNode(c) |
-      exists(HashSplatParameter p |
-        p.getCallable() = c.asCallable() and
-        nodeTo = TNormalParameterNode(p)
-      )
-      or
-      exists(ParameterPosition pos |
-        nodeTo = TSummaryParameterNode(c.asLibraryCallable(), pos) and
-        pos.isHashSplat()
-      )
-    )
-    or
     localSsaFlowStep(nodeFrom, nodeTo)
     or
     nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::BlockArgumentCfgNode).getValue()
@@ -648,9 +636,7 @@ private module ParameterNodes {
           )
         or
         parameter = callable.getAParameter().(HashSplatParameter) and
-        pos.isHashSplat() and
-        // avoid overlap with `SynthHashSplatParameterNode`
-        not callable.getAParameter() instanceof KeywordParameter
+        pos.isHashSplat()
         or
         parameter = callable.getParameter(0).(SplatParameter) and
         pos.isSplatAll()
@@ -780,7 +766,7 @@ private module ParameterNodes {
     final override Parameter getParameter() { none() }
 
     final override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
-      c = callable and pos.isHashSplat()
+      c = callable and pos.isSynthHashSplat()
     }
 
     final override CfgScope getCfgScope() { result = callable.asCallable() }
@@ -802,16 +788,7 @@ private module ParameterNodes {
     override Parameter getParameter() { none() }
 
     override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
-      sc = c.asLibraryCallable() and
-      pos = pos_ and
-      // avoid overlap with `SynthHashSplatParameterNode`
-      not (
-        pos.isHashSplat() and
-        exists(ParameterPosition keywordPos |
-          FlowSummaryImpl::Private::summaryParameterNodeRange(sc, keywordPos) and
-          keywordPos.isKeyword(_)
-        )
-      )
+      sc = c.asLibraryCallable() and pos = pos_
     }
 
     override CfgScope getCfgScope() { none() }