Merge pull request #15175 from tamasvajk/feature/arg-param-mapping

C#: Improve arg-param mapping logic to better handle arguments passed to `params` parameters

Author: tamasvajk

csharp/ql/lib/change-notes/2023-12-20-arg-param-mapping.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+
+* The `Call::getArgumentForParameter` predicate has been reworked to add support for arguments passed to `params` parameters.

csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll

@@ -163,7 +163,48 @@ private predicate isMaybeNullArgument(Ssa::ExplicitDefinition def, MaybeNullExpr
   |
     p = pdef.getParameter().getUnboundDeclaration() and
     arg = p.getAnAssignedArgument() and
-    not arg.getEnclosingCallable().getEnclosingCallable*() instanceof TestMethod
+    not arg.getEnclosingCallable().getEnclosingCallable*() instanceof TestMethod and
+    (
+      p.isParams()
+      implies
+      (
+        isValidExplicitParamsType(p, arg.getType()) and
+        not exists(Call c | c.getAnArgument() = arg and hasMultipleParamsArguments(c))
+      )
+    )
+  )
+}
+
+/**
+ * Holds if the type `t` is a valid argument type for passing an explicit array
+ * to the `params` parameter `p`. For example, the types `object[]` and `string[]`
+ * of the arguments on lines 4 and 5, respectively, are valid for the parameter
+ * `args` on line 1 in
+ *
+ * ```csharp
+ * void M(params object[] args) { ... }
+ *
+ * void CallM(object[] os, string[] ss, string s) {
+ *   M(os);
+ *   M(ss);
+ *   M(s);
+ * }
+ * ```
+ */
+pragma[nomagic]
+private predicate isValidExplicitParamsType(Parameter p, Type t) {
+  p.isParams() and
+  t.isImplicitlyConvertibleTo(p.getType())
+}
+
+/**
+ * Holds if call `c` has multiple arguments for a `params` parameter
+ * of the targeted callable.
+ */
+private predicate hasMultipleParamsArguments(Call c) {
+  exists(Parameter p | p = c.getTarget().getAParameter() |
+    p.isParams() and
+    exists(c.getArgument(any(int i | i > p.getPosition())))
   )
 }
 

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

@@ -602,18 +602,6 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   nodeTo.(ObjectCreationNode).getPreUpdateNode() = nodeFrom.(ObjectInitializerNode)
 }
 
-pragma[noinline]
-private Expr getImplicitArgument(Call c, int pos) {
-  result = c.getArgument(pos) and
-  not exists(result.getExplicitArgumentName())
-}
-
-pragma[nomagic]
-private Expr getExplicitArgument(Call c, string name) {
-  result = c.getAnArgument() and
-  result.getExplicitArgumentName() = name
-}
-
 /**
  * Holds if `arg` is a `params` argument of `c`, for parameter `p`, and `arg` will
  * be wrapped in an array by the C# compiler.
@@ -624,11 +612,7 @@ private predicate isParamsArg(Call c, Expr arg, Parameter p) {
     p = target.getAParameter() and
     p.isParams() and
     numArgs = c.getNumberOfArguments() and
-    arg =
-      [
-        getImplicitArgument(c, [p.getPosition() .. numArgs - 1]),
-        getExplicitArgument(c, p.getName())
-      ]
+    arg = c.getArgumentForParameter(p)
   |
     numArgs > target.getNumberOfParameters()
     or

csharp/ql/lib/semmle/code/csharp/exprs/Call.qll

@@ -57,60 +57,29 @@ class Call extends DotNet::Call, Expr, @call {
    *
    * This takes into account both positional and named arguments, but does not
    * consider default arguments.
-   *
-   * An argument must always have a type that is convertible to the relevant
-   * parameter type. Therefore, `params` arguments are only taken into account
-   * when they are passed as explicit arrays. For example, in the call to `M1`
-   * on line 5, `o` is not an argument for `M1`'s `args` parameter, while
-   * `new object[] { o }` on line 6 is, in
-   *
-   * ```csharp
-   * class C {
-   *   void M1(params object[] args) { }
-   *
-   *   void M2(object o) {
-   *     M1(o);
-   *     M1(new object[] { o });
-   *   }
-   * }
-   * ```
    */
   cached
   override Expr getArgumentForParameter(DotNet::Parameter p) {
     this.getTarget().getAParameter() = p and
     (
       // Appears in the positional part of the call
-      result = this.getImplicitArgument(p.getPosition()) and
-      (
-        p.(Parameter).isParams()
-        implies
-        (
-          isValidExplicitParamsType(p, result.getType()) and
-          not this.hasMultipleParamsArguments()
-        )
-      )
+      result = this.getImplicitArgument(p)
       or
       // Appears in the named part of the call
-      result = this.getExplicitArgument(p.getName()) and
-      (p.(Parameter).isParams() implies isValidExplicitParamsType(p, result.getType()))
-    )
-  }
-
-  /**
-   * Holds if this call has multiple arguments for a `params` parameter
-   * of the targeted callable.
-   */
-  private predicate hasMultipleParamsArguments() {
-    exists(Parameter p | p = this.getTarget().getAParameter() |
-      p.isParams() and
-      exists(this.getArgument(any(int i | i > p.getPosition())))
+      result = this.getExplicitArgument(p.getName())
     )
   }
 
   pragma[noinline]
-  private Expr getImplicitArgument(int pos) {
-    result = this.getArgument(pos) and
-    not exists(result.getExplicitArgumentName())
+  private Expr getImplicitArgument(DotNet::Parameter p) {
+    not exists(result.getExplicitArgumentName()) and
+    (
+      p.(Parameter).isParams() and
+      result = this.getArgument(any(int i | i >= p.getPosition()))
+      or
+      not p.(Parameter).isParams() and
+      result = this.getArgument(p.getPosition())
+    )
   }
 
   pragma[nomagic]
@@ -254,28 +223,6 @@ class Call extends DotNet::Call, Expr, @call {
   override string toString() { result = "call" }
 }
 
-/**
- * Holds if the type `t` is a valid argument type for passing an explicit array
- * to the `params` parameter `p`. For example, the types `object[]` and `string[]`
- * of the arguments on lines 4 and 5, respectively, are valid for the parameter
- * `args` on line 1 in
- *
- * ```csharp
- * void M(params object[] args) { ... }
- *
- * void CallM(object[] os, string[] ss, string s) {
- *   M(os);
- *   M(ss);
- *   M(s);
- * }
- * ```
- */
-pragma[nomagic]
-private predicate isValidExplicitParamsType(Parameter p, Type t) {
-  p.isParams() and
-  t.isImplicitlyConvertibleTo(p.getType())
-}
-
 /**
  * A method call, for example `a.M()` on line 5 in
  *