Merge pull request #10790 from hvitved/csharp/avoid-get-a-reachable-read

C#: Deprecate `AssignableRead::getAReachableRead`

csharp/ql/lib/semmle/code/csharp/Assignable.qll

@@ -111,6 +111,7 @@ class AssignableRead extends AssignableAccess {
    * - The reads of `i` on lines 7 and 8 are next to the read on line 6.
    * - The read of `this.Field` on line 11 is next to the read on line 10.
    */
+  pragma[nomagic]
   AssignableRead getANextRead() {
     forex(ControlFlow::Node cfn | cfn = result.getAControlFlowNode() |
       cfn = this.getAnAdjacentReadSameVar()
@@ -124,7 +125,7 @@ class AssignableRead extends AssignableAccess {
    *
    * This is the transitive closure of `getANextRead()`.
    */
-  AssignableRead getAReachableRead() { result = this.getANextRead+() }
+  deprecated AssignableRead getAReachableRead() { result = this.getANextRead+() }
 }
 
 /**
@@ -479,6 +480,7 @@ class AssignableDefinition extends TAssignableDefinition {
    * Subsequent reads can be found by following the steps defined by
    * `AssignableRead.getANextRead()`.
    */
+  pragma[nomagic]
   AssignableRead getAFirstRead() {
     forex(ControlFlow::Node cfn | cfn = result.getAControlFlowNode() |
       exists(Ssa::ExplicitDefinition def | result = def.getAFirstReadAtNode(cfn) |
@@ -494,7 +496,7 @@ class AssignableDefinition extends TAssignableDefinition {
    *
    * This is the equivalent with `getAFirstRead().getANextRead*()`.
    */
-  AssignableRead getAReachableRead() { result = this.getAFirstRead().getANextRead*() }
+  deprecated AssignableRead getAReachableRead() { result = this.getAFirstRead().getANextRead*() }
 
   /** Gets a textual representation of this assignable definition. */
   string toString() { none() }

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

@@ -174,7 +174,9 @@ class VariableAccess extends AssignableAccess, @variable_access_expr {
 class VariableRead extends VariableAccess, AssignableRead {
   override VariableRead getANextRead() { result = AssignableRead.super.getANextRead() }
 
-  override VariableRead getAReachableRead() { result = AssignableRead.super.getAReachableRead() }
+  deprecated override VariableRead getAReachableRead() {
+    result = AssignableRead.super.getAReachableRead()
+  }
 }
 
 /**
@@ -200,7 +202,7 @@ class LocalScopeVariableAccess extends VariableAccess, @local_scope_variable_acc
 class LocalScopeVariableRead extends LocalScopeVariableAccess, VariableRead {
   override LocalScopeVariableRead getANextRead() { result = VariableRead.super.getANextRead() }
 
-  override LocalScopeVariableRead getAReachableRead() {
+  deprecated override LocalScopeVariableRead getAReachableRead() {
     result = VariableRead.super.getAReachableRead()
   }
 }
@@ -242,7 +244,7 @@ class ParameterAccess extends LocalScopeVariableAccess, @parameter_access_expr {
 class ParameterRead extends ParameterAccess, LocalScopeVariableRead {
   override ParameterRead getANextRead() { result = LocalScopeVariableRead.super.getANextRead() }
 
-  override ParameterRead getAReachableRead() {
+  deprecated override ParameterRead getAReachableRead() {
     result = LocalScopeVariableRead.super.getAReachableRead()
   }
 }
@@ -297,7 +299,7 @@ class LocalVariableAccess extends LocalScopeVariableAccess, @local_variable_acce
 class LocalVariableRead extends LocalVariableAccess, LocalScopeVariableRead {
   override LocalVariableRead getANextRead() { result = LocalScopeVariableRead.super.getANextRead() }
 
-  override LocalVariableRead getAReachableRead() {
+  deprecated override LocalVariableRead getAReachableRead() {
     result = LocalScopeVariableRead.super.getAReachableRead()
   }
 }
@@ -442,7 +444,9 @@ class PropertyAccess extends AssignableMemberAccess, PropertyAccessExpr {
 class PropertyRead extends PropertyAccess, AssignableRead {
   override PropertyRead getANextRead() { result = AssignableRead.super.getANextRead() }
 
-  override PropertyRead getAReachableRead() { result = AssignableRead.super.getAReachableRead() }
+  deprecated override PropertyRead getAReachableRead() {
+    result = AssignableRead.super.getAReachableRead()
+  }
 }
 
 /**
@@ -581,7 +585,9 @@ class IndexerAccess extends AssignableMemberAccess, ElementAccess, IndexerAccess
 class IndexerRead extends IndexerAccess, ElementRead {
   override IndexerRead getANextRead() { result = ElementRead.super.getANextRead() }
 
-  override IndexerRead getAReachableRead() { result = ElementRead.super.getAReachableRead() }
+  deprecated override IndexerRead getAReachableRead() {
+    result = ElementRead.super.getAReachableRead()
+  }
 }
 
 /**

csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll

@@ -71,6 +71,20 @@ class FormatMethod extends Method {
   }
 }
 
+pragma[nomagic]
+private predicate parameterReadPostDominatesEntry(ParameterRead pr) {
+  pr.getAControlFlowNode().postDominates(pr.getEnclosingCallable().getEntryPoint()) and
+  getParameterType(pr.getTarget()) instanceof ObjectType
+}
+
+pragma[nomagic]
+private predicate alwaysPassedToFormatItemParameter(ParameterRead pr) {
+  pr = any(StringFormatItemParameter other).getAnAssignedArgument() and
+  parameterReadPostDominatesEntry(pr)
+  or
+  alwaysPassedToFormatItemParameter(pr.getANextRead())
+}
+
 /**
  * A parameter that is used as a format item for `string.Format()`. Either a
  * format item parameter of `string.Format()`, or a parameter of a method that
@@ -85,15 +99,9 @@ class StringFormatItemParameter extends Parameter {
     )
     or
     // Parameter of a source method that forwards to `string.Format()`
-    exists(
-      AssignableDefinitions::ImplicitParameterDefinition def, ParameterRead pr,
-      StringFormatItemParameter other
-    |
+    exists(AssignableDefinitions::ImplicitParameterDefinition def |
       def.getParameter() = this and
-      pr = def.getAReachableRead() and
-      pr.getAControlFlowNode().postDominates(this.getCallable().getEntryPoint()) and
-      other.getAnAssignedArgument() = pr and
-      getParameterType(this) instanceof ObjectType
+      alwaysPassedToFormatItemParameter(def.getAFirstRead())
     )
   }
 }

csharp/ql/src/API Abuse/DisposeNotCalledOnException.ql

@@ -22,12 +22,22 @@ private class DisposeCall extends MethodCall {
   DisposeCall() { this.getTarget() instanceof DisposeMethod }
 }
 
-private predicate localFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-  DataFlow::localFlowStep(nodeFrom, nodeTo) and
-  not exists(AssignableDefinition def, UsingStmt us |
-    nodeTo.asExpr() = def.getAReachableRead() and
+pragma[nomagic]
+private predicate isDisposedAccess(AssignableRead ar) {
+  exists(AssignableDefinition def, UsingStmt us |
+    ar = def.getAFirstRead() and
     def.getTargetAccess() = us.getAVariableDeclExpr().getAccess()
   )
+  or
+  exists(AssignableRead mid |
+    isDisposedAccess(mid) and
+    ar = mid.getANextRead()
+  )
+}
+
+private predicate localFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+  DataFlow::localFlowStep(nodeFrom, nodeTo) and
+  not isDisposedAccess(nodeTo.asExpr())
 }
 
 private predicate reachesDisposeCall(DisposeCall disposeCall, DataFlow::Node node) {

csharp/ql/src/Useless code/DefaultToStringQuery.qll

@@ -6,6 +6,7 @@ import semmle.code.csharp.frameworks.System
  * Holds if expression `e`, of type `t`, invokes `ToString()` either explicitly
  * or implicitly.
  */
+pragma[nomagic]
 predicate invokesToString(Expr e, ValueOrRefType t) {
   // Explicit invocation
   exists(MethodCall mc | mc.getQualifier() = e |
@@ -20,20 +21,24 @@ predicate invokesToString(Expr e, ValueOrRefType t) {
   // Implicit invocation via forwarder method
   t = e.stripCasts().getType() and
   not t instanceof StringType and
-  exists(Parameter p |
-    alwaysInvokesToStringOnParameter(p) and
+  exists(AssignableDefinitions::ImplicitParameterDefinition def, Parameter p |
+    def.getParameter() = p and
+    alwaysInvokesToString(def.getAFirstRead()) and
     e = p.getAnAssignedArgument()
   )
 }
 
-pragma[noinline]
-private predicate alwaysInvokesToStringOnParameter(Parameter p) {
-  exists(AssignableDefinitions::ImplicitParameterDefinition def, ParameterRead pr |
-    def.getParameter() = p and
-    pr = def.getAReachableRead() and
-    pr.getAControlFlowNode().postDominates(p.getCallable().getEntryPoint()) and
-    invokesToString(pr, _)
-  )
+pragma[nomagic]
+private predicate parameterReadPostDominatesEntry(ParameterRead pr) {
+  pr.getAControlFlowNode().postDominates(pr.getEnclosingCallable().getEntryPoint())
+}
+
+pragma[nomagic]
+private predicate alwaysInvokesToString(ParameterRead pr) {
+  parameterReadPostDominatesEntry(pr) and
+  invokesToString(pr, _)
+  or
+  alwaysInvokesToString(pr.getANextRead())
 }
 
 /**