Python: Use new parameter position for synthetic **kwargs instead

We wanted to ensure that a callable did not have multiple parameters
with same parameter position. Originally we fixed this with
02b3a1b (like Ruby). This commit
reverts that and solves it by introducing a new parameter position
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

@@ -181,11 +181,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())
   )
@@ -236,28 +232,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.
  */
@@ -403,8 +377,6 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   simpleLocalFlowStepForTypetracking(nodeFrom, nodeTo)
   or
   summaryFlowSteps(nodeFrom, nodeTo)
-  or
-  dictSplatParameterNodeFlowStep(nodeFrom, nodeTo)
 }
 
 /**