Update tests to inline expectations

Author: joefarebrother

Diff for: python/ql/src/Variables/LoopVariableCapture/LoopVariableCapture.ql

@@ -10,80 +10,10 @@
  */
 
 import python
+import LoopVariableCaptureQuery
 import semmle.python.dataflow.new.DataFlow
-
-abstract class Loop extends AstNode {
-  abstract Variable getALoopVariable();
-}
-
-class ForLoop extends Loop, For {
-  override Variable getALoopVariable() {
-    this.getTarget() = result.getAnAccess().getParentNode*() and
-    result.getScope() = this.getScope()
-  }
-}
-
-predicate capturesLoopVariable(CallableExpr capturing, Loop loop, Variable var) {
-  var.getAnAccess().getScope() = capturing.getInnerScope() and
-  capturing.getParentNode+() = loop and
-  var = loop.getALoopVariable()
-}
-
-module EscapingCaptureFlowConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node node) { capturesLoopVariable(node.asExpr(), _, _) }
-
-  predicate isSink(DataFlow::Node node) {
-    // Stored in a field.
-    // This appeared to lead to FPs through wrapper classes.
-    // exists(DataFlow::AttrWrite aw | aw.getObject() = node)
-    // or
-    // Stored in a dict/list.
-    exists(Assign assign, Subscript sub |
-      sub = assign.getATarget() and node.asExpr() = assign.getValue()
-    )
-    or
-    // Stored in a list.
-    exists(DataFlow::MethodCallNode mc | mc.calls(_, "append") and node = mc.getArg(0))
-    or
-    // Used in a yield statement, likely included in a collection.
-    // The element of comprehension expressions desugar to involve a yield statement internally.
-    exists(Yield y | node.asExpr() = y.getValue())
-  }
-
-  predicate isBarrierOut(DataFlow::Node node) { isSink(node) }
-
-  predicate isBarrier(DataFlow::Node node) {
-    // Incorrect virtual dispatch to __call__ methods is a source of FPs.
-    exists(Function call |
-      call.getName() = "__call__" and
-      call.getArg(0) = node.(DataFlow::ParameterNode).getParameter()
-    )
-  }
-
-  predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet cs) {
-    isSink(node) and
-    (
-      cs instanceof DataFlow::TupleElementContent or
-      cs instanceof DataFlow::ListElementContent or
-      cs instanceof DataFlow::SetElementContent or
-      cs instanceof DataFlow::DictionaryElementAnyContent
-    )
-  }
-}
-
-module EscapingCaptureFlow = DataFlow::Global<EscapingCaptureFlowConfig>;
-
 import EscapingCaptureFlow::PathGraph
 
-predicate escapingCapture(
-  CallableExpr capturing, Loop loop, Variable var, EscapingCaptureFlow::PathNode source,
-  EscapingCaptureFlow::PathNode sink
-) {
-  capturesLoopVariable(capturing, loop, var) and
-  capturing = source.getNode().asExpr() and
-  EscapingCaptureFlow::flowPath(source, sink)
-}
-
 from
   CallableExpr capturing, AstNode loop, Variable var, string descr,
   EscapingCaptureFlow::PathNode source, EscapingCaptureFlow::PathNode sink

Diff for: python/ql/src/Variables/LoopVariableCapture/LoopVariableCaptureQuery.qll

@@ -0,0 +1,81 @@
+/** Definitions for reasoning about loop variable capture issues. */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+
+/** A looping construct. */
+abstract class Loop extends AstNode {
+  /**
+   * Gets a loop variable of this loop.
+   * For example, `x` and `y` in `for x,y in pairs: print(x+y)`
+   */
+  abstract Variable getALoopVariable();
+}
+
+/** A `for` loop. */
+private class ForLoop extends Loop, For {
+  override Variable getALoopVariable() {
+    this.getTarget() = result.getAnAccess().getParentNode*() and
+    result.getScope() = this.getScope()
+  }
+}
+
+/** Holds if the callable `capturing` captures the variable `var` from the loop `loop`. */
+predicate capturesLoopVariable(CallableExpr capturing, Loop loop, Variable var) {
+  var.getAnAccess().getScope() = capturing.getInnerScope() and
+  capturing.getParentNode+() = loop and
+  var = loop.getALoopVariable()
+}
+
+/** Dataflow configuration for reasoning about callables that capture a loop variable and then may escape from the loop. */
+module EscapingCaptureFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { capturesLoopVariable(node.asExpr(), _, _) }
+
+  predicate isSink(DataFlow::Node node) {
+    // Stored in a dict/list.
+    exists(Assign assign, Subscript sub |
+      sub = assign.getATarget() and node.asExpr() = assign.getValue()
+    )
+    or
+    // Stored in a list.
+    exists(DataFlow::MethodCallNode mc | mc.calls(_, "append") and node = mc.getArg(0))
+    or
+    // Used in a yield statement, likely included in a collection.
+    // The element of comprehension expressions desugar to involve a yield statement internally.
+    exists(Yield y | node.asExpr() = y.getValue())
+    // Checks for storing in a field leads to false positives, so are omitted.
+  }
+
+  predicate isBarrierOut(DataFlow::Node node) { isSink(node) }
+
+  predicate isBarrier(DataFlow::Node node) {
+    // Incorrect virtual dispatch to __call__ methods is a source of FPs.
+    exists(Function call |
+      call.getName() = "__call__" and
+      call.getArg(0) = node.(DataFlow::ParameterNode).getParameter()
+    )
+  }
+
+  predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet cs) {
+    isSink(node) and
+    (
+      cs instanceof DataFlow::TupleElementContent or
+      cs instanceof DataFlow::ListElementContent or
+      cs instanceof DataFlow::SetElementContent or
+      cs instanceof DataFlow::DictionaryElementAnyContent
+    )
+  }
+}
+
+/** Dataflow for reasoning about callables that capture a loop variable and then escape from the loop. */
+module EscapingCaptureFlow = DataFlow::Global<EscapingCaptureFlowConfig>;
+
+/** Holds if `capturing` is a callable that captures the variable `var` of the loop `loop`, and then may escape the loop via a flow path from `source` to `sink`. */
+predicate escapingCapture(
+  CallableExpr capturing, Loop loop, Variable var, EscapingCaptureFlow::PathNode source,
+  EscapingCaptureFlow::PathNode sink
+) {
+  capturesLoopVariable(capturing, loop, var) and
+  capturing = source.getNode().asExpr() and
+  EscapingCaptureFlow::flowPath(source, sink)
+}

Diff for: python/ql/test/query-tests/Variables/capture/LoopVariableCapture.expected

@@ -0,0 +1,19 @@
+import python
+import Variables.LoopVariableCapture.LoopVariableCaptureQuery
+import utils.test.InlineExpectationsTest
+
+module MethodArgTest implements TestSig {
+  string getARelevantTag() { result = ["capturedVar"] }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(CallableExpr capturing, AstNode loop, Variable var |
+      escapingCapture(capturing, loop, var, _, _) and
+      element = capturing.toString() and
+      location = capturing.getLocation() and
+      tag = "capturedVar" and
+      value = var.getId()
+    )
+  }
+}
+
+import MakeTest<MethodArgTest>

Diff for: python/ql/test/query-tests/Variables/capture/LoopVariableCapture.qlref

@@ -4,10 +4,10 @@ def bad1():
     for x in range(10):
         def inner(): # $capturedVar=x
             return x
-    results.append(inner)
+        results.append(inner)
     return results
 
-a = [lambda: i for i in range(1, 4)] # $capturedVar=a
+a = [lambda: i for i in range(1, 4)] # $capturedVar=i
 for f in a:
     print(f())
 
@@ -18,7 +18,14 @@ def good1():
     for y in range(10):
         def inner(y=y):
             return y
-    results.append(inner)
+        results.append(inner)
+    return results
+
+# OK: Using default argument.
+def good2():
+    results = []
+    for y in range(10):
+        results.append(lambda y=y: y)
     return results
 
 #Factory function
@@ -46,7 +53,7 @@ def inner():
 for f in s:
     print(f())
 
-d = {i:lambda: i for i in range(1, 4)} # $capturedVar=d
+d = {i:lambda: i for i in range(1, 4)} # $capturedVar=i
 for k, f in d.items():
     print(k, f())