Merge pull request github#1956 from hvitved/csharp/get-an-out-node

C#: Refactor `getAnOutNode()` predicate

Author: calumgrant

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

@@ -72,7 +72,7 @@ module Stages {
       or
       exists(any(DataFlow::Node n).toString())
       or
-      exists(any(OutNode n).getCall())
+      exists(any(OutNode n).getCall(_))
       or
       exists(CallContext cc)
       or

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

@@ -172,64 +172,16 @@ private module Cached {
     result = viableImplInCallContext(call, ctx) and
     reducedViableImplInReturn(result, call)
   }
-
-  /** A valid return type for a method that uses `yield return`. */
-  private class YieldReturnType extends Type {
-    YieldReturnType() {
-      exists(Type t | t = this.getSourceDeclaration() |
-        t instanceof SystemCollectionsIEnumerableInterface
-        or
-        t instanceof SystemCollectionsIEnumeratorInterface
-        or
-        t instanceof SystemCollectionsGenericIEnumerableTInterface
-        or
-        t instanceof SystemCollectionsGenericIEnumeratorInterface
-      )
-    }
-  }
-
-  /**
-   * Gets a node that can read the value returned from `call` with return kind
-   * `kind`.
-   */
-  cached
-  OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) {
-    // normal `return`
-    result = call.getNode() and
-    kind instanceof NormalReturnKind and
-    not call.getExpr().getType() instanceof VoidType
-    or
-    // `yield return`
-    result = call.getNode() and
-    kind instanceof YieldReturnKind and
-    call.getExpr().getType() instanceof YieldReturnType
-    or
-    // `out`/`ref` parameter
-    exists(Parameter p, AssignableDefinitions::OutRefDefinition def |
-      p.getSourceDeclaration().getPosition() = kind.(OutRefReturnKind).getPosition()
-    |
-      def = result.(SsaDefinitionNode).getDefinition().(Ssa::ExplicitDefinition).getADefinition() and
-      def.getTargetAccess() = call.getExpr().(Call).getArgumentForParameter(p)
-    )
-    or
-    // implicit captured variable return
-    exists(Ssa::ExplicitDefinition def, Ssa::ImplicitCallDefinition cdef, LocalScopeVariable v |
-      kind.(ImplicitCapturedReturnKind).getVariable() = v and
-      def.isCapturedVariableDefinitionFlowOut(cdef, _) and
-      cdef = result.(SsaDefinitionNode).getDefinition() and
-      v = def.getSourceVariable().getAssignable()
-    |
-      call.getControlFlowNode() = cdef.getControlFlowNode()
-      or
-      exists(DataFlowCall outer | call.(ImplicitDelegateDataFlowCall).isArgumentOf(outer, _) |
-        outer.getControlFlowNode() = cdef.getControlFlowNode()
-      )
-    )
-  }
 }
 
 import Cached
 
+/**
+ * Gets a node that can read the value returned from `call` with return kind
+ * `kind`.
+ */
+OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) { call = result.getCall(kind) }
+
 predicate viableCallable = viableImpl/1;
 
 /**
@@ -394,6 +346,9 @@ class ImplicitDelegateDataFlowCall extends DelegateDataFlowCall, TImplicitDelega
   /** Gets the number of parameters of the supplied delegate. */
   int getNumberOfDelegateParameters() { result = arg.getDelegateType().getNumberOfParameters() }
 
+  /** Gets the return type of the supplied delegate. */
+  Type getDelegateReturnType() { result = arg.getDelegateType().getReturnType() }
+
   override DotNet::Callable getARuntimeTarget(CallContext::CallContext cc) {
     result = cfn.getElement().(DelegateArgumentToLibraryCallable).getARuntimeTarget(cc)
   }

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

@@ -944,17 +944,35 @@ import ReturnNodes
 
 /** A data flow node that represents the output of a call. */
 abstract class OutNode extends Node {
-  /** Gets the underlying call. */
+  /** Gets the underlying call, where this node is a corresponding output of kind `kind`. */
   cached
-  abstract DataFlowCall getCall();
+  abstract DataFlowCall getCall(ReturnKind kind);
 }
 
 private module OutNodes {
+  private import semmle.code.csharp.frameworks.system.Collections
+  private import semmle.code.csharp.frameworks.system.collections.Generic
+
   private DataFlowCall csharpCall(Expr e, ControlFlow::Node cfn) {
     e = any(DispatchCall dc | result = TNonDelegateCall(cfn, dc)).getCall() or
     result = TExplicitDelegateCall(cfn, e)
   }
 
+  /** A valid return type for a method that uses `yield return`. */
+  private class YieldReturnType extends Type {
+    YieldReturnType() {
+      exists(Type t | t = this.getSourceDeclaration() |
+        t instanceof SystemCollectionsIEnumerableInterface
+        or
+        t instanceof SystemCollectionsIEnumeratorInterface
+        or
+        t instanceof SystemCollectionsGenericIEnumerableTInterface
+        or
+        t instanceof SystemCollectionsGenericIEnumeratorInterface
+      )
+    }
+  }
+
   /**
    * A data flow node that reads a value returned directly by a callable,
    * either via a C# call or a CIL call.
@@ -969,8 +987,16 @@ private module OutNodes {
       )
     }
 
-    override DataFlowCall getCall() {
-      Stages::DataFlowStage::forceCachingInSameStage() and result = call
+    override DataFlowCall getCall(ReturnKind kind) {
+      Stages::DataFlowStage::forceCachingInSameStage() and
+      result = call and
+      (
+        kind instanceof NormalReturnKind and
+        not call.getExpr().getType() instanceof VoidType
+        or
+        kind instanceof YieldReturnKind and
+        call.getExpr().getType() instanceof YieldReturnType
+      )
     }
   }
 
@@ -995,21 +1021,30 @@ private module OutNodes {
       )
     }
 
-    override DataFlowCall getCall() { result = call }
+    override DataFlowCall getCall(ReturnKind kind) {
+      result = call and
+      kind.(ImplicitCapturedReturnKind).getVariable() = this
+            .getDefinition()
+            .getSourceVariable()
+            .getAssignable()
+    }
   }
 
   /**
    * A data flow node that reads a value returned by a callable using an
    * `out` or `ref` parameter.
    */
   class ParamOutNode extends OutNode, SsaDefinitionNode {
-    ParamOutNode() {
-      this.getDefinition().(Ssa::ExplicitDefinition).getADefinition() instanceof
-        AssignableDefinitions::OutRefDefinition
-    }
+    private AssignableDefinitions::OutRefDefinition outRefDef;
 
-    override DataFlowCall getCall() {
-      result = csharpCall(_, this.getDefinition().getControlFlowNode())
+    ParamOutNode() { outRefDef = this.getDefinition().(Ssa::ExplicitDefinition).getADefinition() }
+
+    override DataFlowCall getCall(ReturnKind kind) {
+      result = csharpCall(_, this.getDefinition().getControlFlowNode()) and
+      exists(Parameter p |
+        p.getSourceDeclaration().getPosition() = kind.(OutRefReturnKind).getPosition() and
+        outRefDef.getTargetAccess() = result.getExpr().(Call).getArgumentForParameter(p)
+      )
     }
   }
 
@@ -1036,7 +1071,16 @@ private module OutNodes {
 
     override ControlFlow::Nodes::ElementNode getControlFlowNode() { result = cfn }
 
-    override ImplicitDelegateDataFlowCall getCall() { result.getNode() = this }
+    override ImplicitDelegateDataFlowCall getCall(ReturnKind kind) {
+      result.getNode() = this and
+      (
+        kind instanceof NormalReturnKind and
+        not result.getDelegateReturnType() instanceof VoidType
+        or
+        kind instanceof YieldReturnKind and
+        result.getDelegateReturnType() instanceof YieldReturnType
+      )
+    }
 
     override Callable getEnclosingCallable() { result = cfn.getEnclosingCallable() }
 

csharp/ql/test/library-tests/dataflow/global/GetAnOutNode.expected

@@ -4,22 +4,16 @@
 | Capture.cs:33:30:33:39 | [implicit call] access to local variable captureIn3 | return | Capture.cs:33:30:33:39 | [output] access to local variable captureIn3 |
 | Capture.cs:71:9:71:21 | call to local function CaptureOut1 | captured sink30 | Capture.cs:71:9:71:21 | SSA call def(sink30) |
 | Capture.cs:83:9:83:21 | [transitive] call to local function CaptureOut2 | captured sink31 | Capture.cs:83:9:83:21 | SSA call def(sink31) |
-| Capture.cs:83:9:83:21 | call to local function CaptureOut2 | captured sink31 | Capture.cs:83:9:83:21 | SSA call def(sink31) |
 | Capture.cs:92:9:92:41 | call to method Select | captured sink32 | Capture.cs:92:9:92:41 | SSA call def(sink32) |
 | Capture.cs:92:9:92:41 | call to method Select | return | Capture.cs:92:9:92:41 | call to method Select |
 | Capture.cs:92:9:92:41 | call to method Select | yield return | Capture.cs:92:9:92:41 | call to method Select |
 | Capture.cs:92:9:92:51 | call to method ToArray | return | Capture.cs:92:9:92:51 | call to method ToArray |
 | Capture.cs:92:30:92:40 | [implicit call] access to local variable captureOut3 | captured sink32 | Capture.cs:92:9:92:41 | SSA call def(sink32) |
 | Capture.cs:92:30:92:40 | [implicit call] access to local variable captureOut3 | return | Capture.cs:92:30:92:40 | [output] access to local variable captureOut3 |
 | Capture.cs:121:9:121:35 | [transitive] call to local function CaptureOutMultipleLambdas | captured nonSink0 | Capture.cs:121:9:121:35 | SSA call def(nonSink0) |
-| Capture.cs:121:9:121:35 | [transitive] call to local function CaptureOutMultipleLambdas | captured nonSink0 | Capture.cs:121:9:121:35 | SSA call def(nonSink0) |
-| Capture.cs:121:9:121:35 | [transitive] call to local function CaptureOutMultipleLambdas | captured sink40 | Capture.cs:121:9:121:35 | SSA call def(sink40) |
 | Capture.cs:121:9:121:35 | [transitive] call to local function CaptureOutMultipleLambdas | captured sink40 | Capture.cs:121:9:121:35 | SSA call def(sink40) |
-| Capture.cs:121:9:121:35 | call to local function CaptureOutMultipleLambdas | captured nonSink0 | Capture.cs:121:9:121:35 | SSA call def(nonSink0) |
-| Capture.cs:121:9:121:35 | call to local function CaptureOutMultipleLambdas | captured sink40 | Capture.cs:121:9:121:35 | SSA call def(sink40) |
 | Capture.cs:132:9:132:25 | call to local function CaptureThrough1 | captured sink33 | Capture.cs:132:9:132:25 | SSA call def(sink33) |
 | Capture.cs:144:9:144:25 | [transitive] call to local function CaptureThrough2 | captured sink34 | Capture.cs:144:9:144:25 | SSA call def(sink34) |
-| Capture.cs:144:9:144:25 | call to local function CaptureThrough2 | captured sink34 | Capture.cs:144:9:144:25 | SSA call def(sink34) |
 | Capture.cs:153:9:153:45 | call to method Select | captured sink35 | Capture.cs:153:9:153:45 | SSA call def(sink35) |
 | Capture.cs:153:9:153:45 | call to method Select | return | Capture.cs:153:9:153:45 | call to method Select |
 | Capture.cs:153:9:153:45 | call to method Select | yield return | Capture.cs:153:9:153:45 | call to method Select |