Merge pull request #426 from hvitved/csharp/accessor-argument

C#: Improve `AccessorCall::getArgument()`
2026-05-03 04:39:29 +02:00 · 2018-11-08 16:03:48 +00:00
parent 3c49bc6e67 f7db845331
commit 6fdb0aab41
8 changed files with 225 additions and 120 deletions
--- a/change-notes/1.19/analysis-csharp.md
+++ b/change-notes/1.19/analysis-csharp.md
@@ -3,7 +3,7 @@
 ## General improvements

 * Control flow graph improvements:
-* The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead.
+  * The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead.
  * Code that is only reachable from a constant failing assertion, such as `Debug.Assert(false)`, is considered to be unreachable.

 ## New queries
@@ -21,3 +21,5 @@


 ## Changes to QL libraries
+
+* `getArgument()` on `AccessorCall` has been improved so it now takes tuple assignments into account. For example, the argument for the implicit `value` parameter in the setter of property `P` is `0` in `(P, x) = (0, 1)`. Additionally, the argument for the `value` parameter in compound assignments is now only the expanded value, for example, in `P += 7` the argument is `P + 7` and not `7`.
--- a/csharp/ql/src/semmle/code/csharp/Assignable.qll
+++ b/csharp/ql/src/semmle/code/csharp/Assignable.qll
@@ -183,54 +183,16 @@ class AssignableWrite extends AssignableAccess {
  }
 }

-private cached module AssignableDefinitionImpl {
-  cached newtype TAssignableDefinition =
-    TAssignmentDefinition(Assignment a) {
-      not a.getLValue() instanceof TupleExpr
-    }
-    or
-    TTupleAssignmentDefinition(AssignExpr ae, Expr leaf) {
-      exists(TupleExpr te |
-        ae.getLValue() = te and
-        te.getAnArgument+() = leaf and
-        // `leaf` is either an assignable access or a local variable declaration
-        not leaf instanceof TupleExpr
-      )
-    }
-    or
-    TOutRefDefinition(AssignableAccess aa) {
-      aa.isOutArgument()
-      or
-      isRelevantRefCall(_, aa)
-    }
-    or
-    TMutationDefinition(MutatorOperation mo)
-    or
-    TLocalVariableDefinition(LocalVariableDeclExpr lvde) {
-      not lvde.hasInitializer() and
-      not exists(getTupleSource(TTupleAssignmentDefinition(_, lvde))) and
-      not lvde = any(IsPatternExpr ipe).getVariableDeclExpr() and
-      not lvde = any(TypeCase tc).getVariableDeclExpr()
-    }
-    or
-    TImplicitParameterDefinition(Parameter p) {
-      exists(Callable c |
-        p = c.getAParameter() |
-        c.hasBody() or
-        c.(Constructor).hasInitializer()
-      )
-    }
-    or
-    TAddressOfDefinition(AddressOfExpr aoe)
-    or
-    TIsPatternDefinition(IsPatternExpr ipe)
-    or
-    TTypeCasePatternDefinition(TypeCase tc)
-    or
-    TInitializer(Assignable a, Expr e) {
-      e = a.(Field).getInitializer() or
-      e = a.(Property).getInitializer()
-    }
+/** INTERNAL: Do not use. */
+module AssignableInternal {
+  private predicate tupleAssignmentDefinition(AssignExpr ae, Expr leaf) {
+    exists(TupleExpr te |
+      ae.getLValue() = te and
+      te.getAnArgument+() = leaf and
+      // `leaf` is either an assignable access or a local variable declaration
+      not leaf instanceof TupleExpr
+    )
+  }

  /**
   * Holds if `ae` is a tuple assignment, and `left` is a sub expression
@@ -238,7 +200,7 @@ private cached module AssignableDefinitionImpl {
   * right-hand side `right`.
   */
  private predicate tupleAssignmentPair(AssignExpr ae, Expr left, Expr right) {
-    exists(TTupleAssignmentDefinition(ae, _)) and
+    tupleAssignmentDefinition(ae, _) and
    left = ae.getLValue() and
    right = ae.getRValue()
    or
@@ -249,16 +211,6 @@ private cached module AssignableDefinitionImpl {
    )
  }

-  /**
-   * Gets the source expression assigned in tuple definition `def`, if any.
-   */
-  cached Expr getTupleSource(TTupleAssignmentDefinition def) {
-    exists(AssignExpr ae, Expr leaf |
-      def = TTupleAssignmentDefinition(ae, leaf) |
-      tupleAssignmentPair(ae, leaf, result)
-    )
-  }
-
  /**
   * Holds if the `ref` assignment to `aa` via call `c` is relevant.
   */
@@ -341,19 +293,6 @@ private cached module AssignableDefinitionImpl {
    not result = TImplicitParameterDefinition(_)
  }

-  /**
-   * Holds if the `ref` assignment to `aa` via call `c` is uncertain.
-   */
-  cached predicate isUncertainRefCall(Call c, AssignableAccess aa) {
-    isRelevantRefCall(c, aa)
-    and
-    exists(ControlFlow::BasicBlock bb, Parameter p |
-      isAnalyzableRefCall(c, aa, p) |
-      parameterReachesWithoutDef(p, bb) and
-      bb.getLastNode() = p.getCallable().getExitPoint()
-    )
-  }
-
  /**
   * Holds if `p` is an analyzable `ref` parameter and there is a path from the
   * entry point of `p`'s callable to basic block `bb` without passing through
@@ -378,51 +317,137 @@ private cached module AssignableDefinitionImpl {
    bb.getANode() = getAnAnalyzableRefDef(_, _, p).getAControlFlowNode()
  }

-  // Not defined by dispatch in order to avoid too conservative negative recursion error
-  cached Assignable getTarget(AssignableDefinition def) {
-    result = def.getTargetAccess().getTarget()
-    or
-    exists(Expr leaf |
-      def = TTupleAssignmentDefinition(_, leaf) |
-      result = leaf.(LocalVariableDeclExpr).getVariable()
-    )
-    or
-    def = any(AssignableDefinitions::ImplicitParameterDefinition p |
-      result = p.getParameter()
-    )
-    or
-    def = any(AssignableDefinitions::LocalVariableDefinition decl |
-      result = decl.getDeclaration().getVariable()
-    )
-    or
-    def = any(AssignableDefinitions::IsPatternDefinition is |
-      result = is.getDeclaration().getVariable()
-    )
-    or
-    def = any(AssignableDefinitions::TypeCasePatternDefinition case |
-      result = case.getDeclaration().getVariable()
-    )
-    or
-    def = any(AssignableDefinitions::InitializerDefinition init |
-      result = init.getAssignable()
-    )
-  }
+  private cached module Cached {
+    cached newtype TAssignableDefinition =
+      TAssignmentDefinition(Assignment a) {
+        not a.getLValue() instanceof TupleExpr
+      }
+      or
+      TTupleAssignmentDefinition(AssignExpr ae, Expr leaf) {
+        tupleAssignmentDefinition(ae, leaf)
+      }
+      or
+      TOutRefDefinition(AssignableAccess aa) {
+        aa.isOutArgument()
+        or
+        isRelevantRefCall(_, aa)
+      }
+      or
+      TMutationDefinition(MutatorOperation mo)
+      or
+      TLocalVariableDefinition(LocalVariableDeclExpr lvde) {
+        not lvde.hasInitializer() and
+        not exists(getTupleSource(TTupleAssignmentDefinition(_, lvde))) and
+        not lvde = any(IsPatternExpr ipe).getVariableDeclExpr() and
+        not lvde = any(TypeCase tc).getVariableDeclExpr()
+      }
+      or
+      TImplicitParameterDefinition(Parameter p) {
+        exists(Callable c |
+          p = c.getAParameter() |
+          c.hasBody() or
+          c.(Constructor).hasInitializer()
+        )
+      }
+      or
+      TAddressOfDefinition(AddressOfExpr aoe)
+      or
+      TIsPatternDefinition(IsPatternExpr ipe)
+      or
+      TTypeCasePatternDefinition(TypeCase tc)
+      or
+      TInitializer(Assignable a, Expr e) {
+        e = a.(Field).getInitializer() or
+        e = a.(Property).getInitializer()
+      }

-  // Not defined by dispatch in order to avoid too conservative negative recursion error
-  cached AssignableAccess getTargetAccess(AssignableDefinition def) {
-    def = TAssignmentDefinition(any(Assignment a | a.getLValue() = result))
-    or
-    def = TTupleAssignmentDefinition(_, result)
-    or
-    def = TOutRefDefinition(result)
-    or
-    def = TMutationDefinition(any(MutatorOperation mo | mo.getOperand() = result))
-    or
-    def = TAddressOfDefinition(any(AddressOfExpr aoe | aoe.getOperand() = result))
+    /**
+     * Gets the source expression assigned in tuple definition `def`, if any.
+     */
+    cached Expr getTupleSource(TTupleAssignmentDefinition def) {
+      exists(AssignExpr ae, Expr leaf |
+        def = TTupleAssignmentDefinition(ae, leaf) |
+        tupleAssignmentPair(ae, leaf, result)
+      )
+    }
+
+    /**
+     * Holds if the `ref` assignment to `aa` via call `c` is uncertain.
+     */
+    cached predicate isUncertainRefCall(Call c, AssignableAccess aa) {
+      isRelevantRefCall(c, aa)
+      and
+      exists(ControlFlow::BasicBlock bb, Parameter p |
+        isAnalyzableRefCall(c, aa, p) |
+        parameterReachesWithoutDef(p, bb) and
+        bb.getLastNode() = p.getCallable().getExitPoint()
+      )
+    }
+
+    // Not defined by dispatch in order to avoid too conservative negative recursion error
+    cached Assignable getTarget(AssignableDefinition def) {
+      result = def.getTargetAccess().getTarget()
+      or
+      exists(Expr leaf |
+        def = TTupleAssignmentDefinition(_, leaf) |
+        result = leaf.(LocalVariableDeclExpr).getVariable()
+      )
+      or
+      def = any(AssignableDefinitions::ImplicitParameterDefinition p |
+        result = p.getParameter()
+      )
+      or
+      def = any(AssignableDefinitions::LocalVariableDefinition decl |
+        result = decl.getDeclaration().getVariable()
+      )
+      or
+      def = any(AssignableDefinitions::IsPatternDefinition is |
+        result = is.getDeclaration().getVariable()
+      )
+      or
+      def = any(AssignableDefinitions::TypeCasePatternDefinition case |
+        result = case.getDeclaration().getVariable()
+      )
+      or
+      def = any(AssignableDefinitions::InitializerDefinition init |
+        result = init.getAssignable()
+      )
+    }
+
+    // Not defined by dispatch in order to avoid too conservative negative recursion error
+    cached AssignableAccess getTargetAccess(AssignableDefinition def) {
+      def = TAssignmentDefinition(any(Assignment a | a.getLValue() = result))
+      or
+      def = TTupleAssignmentDefinition(_, result)
+      or
+      def = TOutRefDefinition(result)
+      or
+      def = TMutationDefinition(any(MutatorOperation mo | mo.getOperand() = result))
+      or
+      def = TAddressOfDefinition(any(AddressOfExpr aoe | aoe.getOperand() = result))
+    }
+
+    /**
+     * Gets the argument for the implicit `value` parameter in the accessor call
+     * `ac`, if any.
+     */
+    cached Expr getAccessorCallValueArgument(AccessorCall ac) {
+      exists(AssignExpr ae |
+        tupleAssignmentDefinition(ae, ac) |
+        tupleAssignmentPair(ae, ac, result)
+      )
+      or
+      exists(Assignment a |
+        ac = a.getLValue() |
+        result = a.getRValue() and
+        not a.(AssignOperation).hasExpandedAssignment()
+      )
+    }
  }
+  import Cached
 }

-private import AssignableDefinitionImpl
+private import AssignableInternal

 /**
 * An assignable definition.
--- a/csharp/ql/src/semmle/code/csharp/exprs/Call.qll
+++ b/csharp/ql/src/semmle/code/csharp/exprs/Call.qll
@@ -643,8 +643,7 @@ class PropertyCall extends AccessorCall, PropertyAccessExpr {

  override Expr getArgument(int i) {
    i = 0 and
-    this instanceof AssignableWrite and
-    exists(Assignment a | a.getLValue() = this and result = a.getRValue())
+    result = AssignableInternal::getAccessorCallValueArgument(this)
  }

  override string toString() {
@@ -681,9 +680,8 @@ class IndexerCall extends AccessorCall, IndexerAccessExpr {
  override Expr getArgument(int i) {
    result = this.(ElementAccess).getIndex(i)
    or
-    this instanceof AssignableWrite and
    i = count(this.(ElementAccess).getAnIndex()) and
-    exists(Assignment a | a.getLValue() = this and result = a.getRValue())
+    result = AssignableInternal::getAccessorCallValueArgument(this)
  }

  override string toString() { result = IndexerAccessExpr.super.toString() }
--- a/csharp/ql/src/semmle/code/csharp/frameworks/system/Xml.qll
+++ b/csharp/ql/src/semmle/code/csharp/frameworks/system/Xml.qll
@@ -138,6 +138,10 @@ class SystemXmlSchemaXmlSchemaValidationFlags extends EnumConstant {
  }
 }

+private Expr getBitwiseOrOperand(Expr e) {
+  result = e.(BitwiseOrExpr).getAnOperand()
+}
+
 /** A creation of an instance of `System.Xml.XmlReaderSettings`. */
 class XmlReaderSettingsCreation extends ObjectCreation {
  XmlReaderSettingsCreation() {
@@ -157,10 +161,11 @@ class XmlReaderSettingsCreation extends ObjectCreation {
  /** Gets a value set for the given property in this local context. */
  private Expr getPropertyValue(Property p) {
    p = this.getType().(RefType).getAProperty() and
-    exists(PropertyCall set |
+    exists(PropertyCall set, Expr arg |
      set.getTarget() = p.getSetter() and
      DataFlow::localFlow(DataFlow::exprNode(this), DataFlow::exprNode(set.getQualifier())) and
-      result = set.getAnArgument()
+      arg = set.getAnArgument() and
+      result = getBitwiseOrOperand*(arg)
    )
  }
 }
--- a/csharp/ql/test/library-tests/arguments/argumentByName.expected
+++ b/csharp/ql/test/library-tests/arguments/argumentByName.expected
@@ -17,3 +17,21 @@
 | arguments.cs:39:9:39:28 | call to method f3 | arguments.cs:39:27:39:27 | 0 | o |
 | arguments.cs:40:9:40:42 | call to method f3 | arguments.cs:40:18:40:35 | array creation of type Int32[] | args |
 | arguments.cs:40:9:40:42 | call to method f3 | arguments.cs:40:41:40:41 | 0 | o |
+| arguments.cs:54:9:54:12 | access to property Prop | arguments.cs:54:16:54:16 | 0 | value |
+| arguments.cs:55:9:55:12 | access to property Prop | arguments.cs:55:16:55:25 | access to indexer | value |
+| arguments.cs:55:16:55:25 | access to indexer | arguments.cs:55:21:55:21 | 1 | a |
+| arguments.cs:55:16:55:25 | access to indexer | arguments.cs:55:24:55:24 | 2 | b |
+| arguments.cs:56:10:56:13 | access to property Prop | arguments.cs:56:31:56:31 | 5 | value |
+| arguments.cs:56:16:56:25 | access to indexer | arguments.cs:56:21:56:21 | 3 | a |
+| arguments.cs:56:16:56:25 | access to indexer | arguments.cs:56:24:56:24 | 4 | b |
+| arguments.cs:56:16:56:25 | access to indexer | arguments.cs:56:34:56:34 | 6 | value |
+| arguments.cs:58:9:58:12 | access to property Prop | arguments.cs:58:9:58:17 | ... + ... | value |
+| arguments.cs:59:9:59:18 | access to indexer | arguments.cs:59:14:59:14 | 8 | a |
+| arguments.cs:59:9:59:18 | access to indexer | arguments.cs:59:17:59:17 | 9 | b |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:9:60:26 | ... + ... | value |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:14:60:15 | 10 | a |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:14:60:15 | 10 | a |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:18:60:19 | 11 | b |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:18:60:19 | 11 | b |
+| arguments.cs:62:16:62:27 | access to indexer | arguments.cs:62:21:62:22 | 15 | a |
+| arguments.cs:62:16:62:27 | access to indexer | arguments.cs:62:25:62:26 | 16 | b |
--- a/csharp/ql/test/library-tests/arguments/argumentByParameter.expected
+++ b/csharp/ql/test/library-tests/arguments/argumentByParameter.expected
@@ -17,3 +17,23 @@
 | arguments.cs:39:9:39:28 | call to method f3 | arguments.cs:39:27:39:27 | 0 | arguments.cs:33:17:33:17 | o |
 | arguments.cs:40:9:40:42 | call to method f3 | arguments.cs:40:18:40:35 | array creation of type Int32[] | arguments.cs:33:33:33:36 | args |
 | arguments.cs:40:9:40:42 | call to method f3 | arguments.cs:40:41:40:41 | 0 | arguments.cs:33:17:33:17 | o |
+| arguments.cs:54:9:54:12 | access to property Prop | arguments.cs:54:16:54:16 | 0 | arguments.cs:48:21:48:23 | value |
+| arguments.cs:55:9:55:12 | access to property Prop | arguments.cs:55:16:55:25 | access to indexer | arguments.cs:48:21:48:23 | value |
+| arguments.cs:55:16:55:25 | access to indexer | arguments.cs:55:21:55:21 | 1 | arguments.cs:50:18:50:18 | a |
+| arguments.cs:55:16:55:25 | access to indexer | arguments.cs:55:24:55:24 | 2 | arguments.cs:50:25:50:25 | b |
+| arguments.cs:56:10:56:13 | access to property Prop | arguments.cs:56:31:56:31 | 5 | arguments.cs:48:21:48:23 | value |
+| arguments.cs:56:16:56:25 | access to indexer | arguments.cs:56:21:56:21 | 3 | arguments.cs:50:18:50:18 | a |
+| arguments.cs:56:16:56:25 | access to indexer | arguments.cs:56:24:56:24 | 4 | arguments.cs:50:25:50:25 | b |
+| arguments.cs:56:16:56:25 | access to indexer | arguments.cs:56:34:56:34 | 6 | arguments.cs:50:44:50:46 | value |
+| arguments.cs:58:9:58:12 | access to property Prop | arguments.cs:58:9:58:17 | ... + ... | arguments.cs:48:21:48:23 | value |
+| arguments.cs:59:9:59:18 | access to indexer | arguments.cs:59:14:59:14 | 8 | arguments.cs:50:18:50:18 | a |
+| arguments.cs:59:9:59:18 | access to indexer | arguments.cs:59:14:59:14 | 8 | arguments.cs:50:18:50:18 | a |
+| arguments.cs:59:9:59:18 | access to indexer | arguments.cs:59:17:59:17 | 9 | arguments.cs:50:25:50:25 | b |
+| arguments.cs:59:9:59:18 | access to indexer | arguments.cs:59:17:59:17 | 9 | arguments.cs:50:25:50:25 | b |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:9:60:26 | ... + ... | arguments.cs:50:44:50:46 | value |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:14:60:15 | 10 | arguments.cs:50:18:50:18 | a |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:14:60:15 | 10 | arguments.cs:50:18:50:18 | a |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:18:60:19 | 11 | arguments.cs:50:25:50:25 | b |
+| arguments.cs:60:9:60:20 | access to indexer | arguments.cs:60:18:60:19 | 11 | arguments.cs:50:25:50:25 | b |
+| arguments.cs:62:16:62:27 | access to indexer | arguments.cs:62:21:62:22 | 15 | arguments.cs:50:18:50:18 | a |
+| arguments.cs:62:16:62:27 | access to indexer | arguments.cs:62:25:62:26 | 16 | arguments.cs:50:25:50:25 | b |
--- a/csharp/ql/test/library-tests/arguments/arguments.cs
+++ b/csharp/ql/test/library-tests/arguments/arguments.cs
@@ -44,4 +44,21 @@ class ArgumentsTest
    {
        f4(new object[] { null }, null);
    }
+
+    int Prop { get; set; }
+
+    int this[int a, int b] { get => a + b; set { } }
+
+    void f5()
+    {
+        Prop = 0;
+        Prop = this[1, 2];
+        (Prop, this[3, 4]) = (5, 6);
+        Prop++;
+        Prop += 7;
+        this[8, 9]++;
+        this[10, 11] += 12;
+        var tuple = (13, 14);
+        (Prop, this[15, 16]) = tuple;
+    }
 }
--- a/csharp/ql/test/library-tests/arguments/parameterGetArguments.expected
+++ b/csharp/ql/test/library-tests/arguments/parameterGetArguments.expected
@@ -17,3 +17,23 @@
 | arguments.cs:33:33:33:36 | args | arguments.cs:38:15:38:18 | access to parameter args |
 | arguments.cs:33:33:33:36 | args | arguments.cs:39:18:39:21 | access to parameter args |
 | arguments.cs:33:33:33:36 | args | arguments.cs:40:18:40:35 | array creation of type Int32[] |
+| arguments.cs:48:21:48:23 | value | arguments.cs:54:16:54:16 | 0 |
+| arguments.cs:48:21:48:23 | value | arguments.cs:55:16:55:25 | access to indexer |
+| arguments.cs:48:21:48:23 | value | arguments.cs:56:31:56:31 | 5 |
+| arguments.cs:48:21:48:23 | value | arguments.cs:58:9:58:17 | ... + ... |
+| arguments.cs:50:18:50:18 | a | arguments.cs:55:21:55:21 | 1 |
+| arguments.cs:50:18:50:18 | a | arguments.cs:56:21:56:21 | 3 |
+| arguments.cs:50:18:50:18 | a | arguments.cs:59:14:59:14 | 8 |
+| arguments.cs:50:18:50:18 | a | arguments.cs:59:14:59:14 | 8 |
+| arguments.cs:50:18:50:18 | a | arguments.cs:60:14:60:15 | 10 |
+| arguments.cs:50:18:50:18 | a | arguments.cs:60:14:60:15 | 10 |
+| arguments.cs:50:18:50:18 | a | arguments.cs:62:21:62:22 | 15 |
+| arguments.cs:50:25:50:25 | b | arguments.cs:55:24:55:24 | 2 |
+| arguments.cs:50:25:50:25 | b | arguments.cs:56:24:56:24 | 4 |
+| arguments.cs:50:25:50:25 | b | arguments.cs:59:17:59:17 | 9 |
+| arguments.cs:50:25:50:25 | b | arguments.cs:59:17:59:17 | 9 |
+| arguments.cs:50:25:50:25 | b | arguments.cs:60:18:60:19 | 11 |
+| arguments.cs:50:25:50:25 | b | arguments.cs:60:18:60:19 | 11 |
+| arguments.cs:50:25:50:25 | b | arguments.cs:62:25:62:26 | 16 |
+| arguments.cs:50:44:50:46 | value | arguments.cs:56:34:56:34 | 6 |
+| arguments.cs:50:44:50:46 | value | arguments.cs:60:9:60:26 | ... + ... |