Java/C++/C#: Address review comments and fix test.

Author: aschackmull

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImplCommon.qll

@@ -34,6 +34,16 @@ private module ImplCommon {
       )
     }
 
+    /*
+     * The `FlowThrough_*` modules take a `step` relation as input and provide
+     * an `argumentValueFlowsThrough` relation as output.
+     *
+     * `FlowThrough_v1` includes just `simpleLocalFlowStep`, which is then used
+     * to detect getters and setters.
+     * `FlowThrough_v2` then includes a little bit of local field flow on top
+     * of `simpleLocalFlowStep`.
+     */
+
     private module FlowThrough_v1 {
       private predicate step = simpleLocalFlowStep/2;
 
@@ -233,6 +243,11 @@ private module ImplCommon {
       FlowThrough_v1::argumentValueFlowsThrough(node1, node2, _)
     }
 
+    /**
+     * Holds if `p` can flow to `node` in the same callable allowing local flow
+     * steps and value flow through methods. Call contexts are only accounted
+     * for in the nested calls.
+     */
     private predicate parameterValueFlowNoCtx(ParameterNode p, Node node) {
       p = node
       or

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImplCommon.qll

@@ -34,6 +34,16 @@ private module ImplCommon {
       )
     }
 
+    /*
+     * The `FlowThrough_*` modules take a `step` relation as input and provide
+     * an `argumentValueFlowsThrough` relation as output.
+     *
+     * `FlowThrough_v1` includes just `simpleLocalFlowStep`, which is then used
+     * to detect getters and setters.
+     * `FlowThrough_v2` then includes a little bit of local field flow on top
+     * of `simpleLocalFlowStep`.
+     */
+
     private module FlowThrough_v1 {
       private predicate step = simpleLocalFlowStep/2;
 
@@ -233,6 +243,11 @@ private module ImplCommon {
       FlowThrough_v1::argumentValueFlowsThrough(node1, node2, _)
     }
 
+    /**
+     * Holds if `p` can flow to `node` in the same callable allowing local flow
+     * steps and value flow through methods. Call contexts are only accounted
+     * for in the nested calls.
+     */
     private predicate parameterValueFlowNoCtx(ParameterNode p, Node node) {
       p = node
       or

csharp/ql/src/semmle/code/csharp/Caching.qll

@@ -50,7 +50,7 @@ module Stages {
   cached
   module DataFlowStage {
     private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
-    private import semmle.code.csharp.dataflow.internal.DataFlowImplCommon
+    private import semmle.code.csharp.dataflow.internal.DataFlowImplCommon::Public
     private import semmle.code.csharp.dataflow.internal.TaintTrackingPrivate
 
     cached

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImplCommon.qll

@@ -34,6 +34,16 @@ private module ImplCommon {
       )
     }
 
+    /*
+     * The `FlowThrough_*` modules take a `step` relation as input and provide
+     * an `argumentValueFlowsThrough` relation as output.
+     *
+     * `FlowThrough_v1` includes just `simpleLocalFlowStep`, which is then used
+     * to detect getters and setters.
+     * `FlowThrough_v2` then includes a little bit of local field flow on top
+     * of `simpleLocalFlowStep`.
+     */
+
     private module FlowThrough_v1 {
       private predicate step = simpleLocalFlowStep/2;
 
@@ -233,6 +243,11 @@ private module ImplCommon {
       FlowThrough_v1::argumentValueFlowsThrough(node1, node2, _)
     }
 
+    /**
+     * Holds if `p` can flow to `node` in the same callable allowing local flow
+     * steps and value flow through methods. Call contexts are only accounted
+     * for in the nested calls.
+     */
     private predicate parameterValueFlowNoCtx(ParameterNode p, Node node) {
       p = node
       or

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

@@ -3,7 +3,6 @@ private import cil
 private import dotnet
 private import DataFlowPublic
 private import DataFlowDispatch
-private import DataFlowImplCommon
 private import ControlFlowReachability
 private import DelegateDataFlow
 private import semmle.code.csharp.Caching

csharp/ql/src/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll

@@ -1,6 +1,5 @@
 private import csharp
 private import TaintTrackingPublic
-private import semmle.code.csharp.dataflow.internal.DataFlowImplCommon
 private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
 private import semmle.code.csharp.dataflow.internal.ControlFlowReachability
 private import semmle.code.csharp.dataflow.LibraryTypeDataFlow

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImplCommon.qll

@@ -34,6 +34,16 @@ private module ImplCommon {
       )
     }
 
+    /*
+     * The `FlowThrough_*` modules take a `step` relation as input and provide
+     * an `argumentValueFlowsThrough` relation as output.
+     *
+     * `FlowThrough_v1` includes just `simpleLocalFlowStep`, which is then used
+     * to detect getters and setters.
+     * `FlowThrough_v2` then includes a little bit of local field flow on top
+     * of `simpleLocalFlowStep`.
+     */
+
     private module FlowThrough_v1 {
       private predicate step = simpleLocalFlowStep/2;
 
@@ -233,6 +243,11 @@ private module ImplCommon {
       FlowThrough_v1::argumentValueFlowsThrough(node1, node2, _)
     }
 
+    /**
+     * Holds if `p` can flow to `node` in the same callable allowing local flow
+     * steps and value flow through methods. Call contexts are only accounted
+     * for in the nested calls.
+     */
     private predicate parameterValueFlowNoCtx(ParameterNode p, Node node) {
       p = node
       or

java/ql/test/library-tests/dataflow/gettersetter/A.java

@@ -0,0 +1,51 @@
+public class A {
+  int foo;
+
+  int getFoo() {
+    return this.foo;
+  }
+
+  void setFoo(int x) {
+    this.foo = x;
+  }
+
+  static A withFoo(int x) {
+    A a = new A();
+    a.foo = x;
+    return a;
+  }
+
+  static void run() {
+    A a = new A();
+    a.setFoo(1);
+    int x = a.getFoo();
+    A a2 = withFoo(2);
+    x = a.aGetter();
+    x = a2.notAGetter();
+  }
+
+  static class C1 {
+    A maybeId(A a) {
+      return a;
+    }
+  }
+
+  static class C2 extends C1 {
+    @Override
+    A maybeId(A a) {
+      return new A();
+    }
+  }
+
+  static A maybeIdWrap(A a, C1 c) {
+    return c.maybeId(a);
+  }
+
+  int aGetter() {
+    return maybeIdWrap(this, new C1()).foo;
+  }
+
+  int notAGetter() {
+    return maybeIdWrap(this, new C2()).foo;
+  }
+}

java/ql/test/library-tests/dataflow/gettersetter/gettersetter.expected

@@ -0,0 +1,9 @@
+| Read | A.java:5:12:5:15 | this | A.java:5:12:5:19 | this.foo | A.java:2:7:2:9 | foo |
+| Read | A.java:21:13:21:13 | a | A.java:21:13:21:22 | getFoo(...) | A.java:2:7:2:9 | foo |
+| Read | A.java:23:9:23:9 | a | A.java:23:9:23:19 | aGetter(...) | A.java:2:7:2:9 | foo |
+| Read | A.java:45:12:45:38 | maybeIdWrap(...) | A.java:45:12:45:42 | maybeIdWrap(...).foo | A.java:2:7:2:9 | foo |
+| Read | A.java:49:12:49:38 | maybeIdWrap(...) | A.java:49:12:49:42 | maybeIdWrap(...).foo | A.java:2:7:2:9 | foo |
+| Store | A.java:9:16:9:16 | x | A.java:9:5:9:8 | this [post update] | A.java:2:7:2:9 | foo |
+| Store | A.java:14:13:14:13 | x | A.java:14:5:14:5 | a [post update] | A.java:2:7:2:9 | foo |
+| Store | A.java:20:14:20:14 | 1 | A.java:20:5:20:5 | a [post update] | A.java:2:7:2:9 | foo |
+| Store | A.java:22:20:22:20 | 2 | A.java:22:12:22:21 | withFoo(...) | A.java:2:7:2:9 | foo |

java/ql/test/library-tests/dataflow/gettersetter/gettersetter.ql

@@ -0,0 +1,11 @@
+import java
+import semmle.code.java.dataflow.internal.DataFlowImplCommon::Public
+import semmle.code.java.dataflow.internal.DataFlowImplSpecific::Public
+import semmle.code.java.dataflow.internal.DataFlowImplSpecific::Private
+
+from Node n1, Content f, Node n2, string k
+where
+  read(n1, f, n2) and k = "Read"
+  or
+  store(n1, f, n2) and k = "Store"
+select k, n1, n2, f