Merge pull request #8391 from michaelnebel/csharp/gvn-interface

C#: Deprecate the StructuralComparisonConfiguration interface and use sameGvn instead.

csharp/ql/lib/semmle/code/csharp/commons/Constants.qll

@@ -105,31 +105,12 @@ private module ConstantComparisonOperation {
   }
 }
 
-private class StructuralComparisonConfig extends StructuralComparison::StructuralComparisonConfiguration {
-  StructuralComparisonConfig() { this = "CompareIdenticalValues" }
-
-  override predicate candidate(ControlFlowElement x, ControlFlowElement y) {
-    exists(ComparisonTest ct |
-      x = ct.getFirstArgument() and
-      y = ct.getSecondArgument()
-    )
-  }
-
-  ComparisonTest getComparisonTest() {
-    exists(Element x, Element y |
-      result.getFirstArgument() = x and
-      result.getSecondArgument() = y and
-      same(x, y)
-    )
-  }
-}
-
 /**
  * Holds if comparison test `ct` compares two structurally identical
  * expressions.
  */
 predicate comparesIdenticalValues(ComparisonTest ct) {
-  ct = any(StructuralComparisonConfig c).getComparisonTest()
+  StructuralComparison::sameGvn(ct.getFirstArgument(), ct.getSecondArgument())
 }
 
 /**

csharp/ql/lib/semmle/code/csharp/commons/StructuralComparison.qll

@@ -192,13 +192,18 @@ private import Cached
 
 predicate toGvn = toGvnCached/1;
 
+/**
+ * Holds if the control flow elements `x` and `y` are structurally equal.
+ */
 pragma[inline]
-private predicate sameGvn(ControlFlowElement x, ControlFlowElement y) {
+predicate sameGvn(ControlFlowElement x, ControlFlowElement y) {
   pragma[only_bind_into](toGvn(pragma[only_bind_out](x))) =
     pragma[only_bind_into](toGvn(pragma[only_bind_out](y)))
 }
 
 /**
+ * DEPRECATED: Use `sameGvn` instead.
+ *
  * A configuration for performing structural comparisons of program elements
  * (expressions and statements).
  *
@@ -207,7 +212,7 @@ private predicate sameGvn(ControlFlowElement x, ControlFlowElement y) {
  *
  * Each use of the library is identified by a unique string value.
  */
-abstract class StructuralComparisonConfiguration extends string {
+abstract deprecated class StructuralComparisonConfiguration extends string {
   bindingset[this]
   StructuralComparisonConfiguration() { any() }
 
@@ -235,55 +240,3 @@ abstract class StructuralComparisonConfiguration extends string {
    */
   predicate same(ControlFlowElement x, ControlFlowElement y) { candidate(x, y) and sameGvn(x, y) }
 }
-
-/**
- * INTERNAL: Do not use.
- *
- * A verbatim copy of the class `StructuralComparisonConfiguration` for internal
- * use.
- *
- * A copy is needed in order to use structural comparison within the standard
- * library without running into caching issues.
- */
-module Internal {
-  // Import all uses of the internal library to make sure caching works
-  private import semmle.code.csharp.controlflow.Guards as G
-
-  /**
-   * A configuration for performing structural comparisons of program elements
-   * (expressions and statements).
-   *
-   * The predicate `candidate()` must be overridden, in order to identify the
-   * elements for which to perform structural comparison.
-   *
-   * Each use of the library is identified by a unique string value.
-   */
-  abstract class InternalStructuralComparisonConfiguration extends string {
-    bindingset[this]
-    InternalStructuralComparisonConfiguration() { any() }
-
-    /**
-     * Holds if elements `x` and `y` are candidates for testing structural
-     * equality.
-     *
-     * Subclasses are expected to override this predicate to identify the
-     * top-level elements which they want to compare. Care should be
-     * taken to avoid identifying too many pairs of elements, as in general
-     * there are very many structurally equal subtrees in a program, and
-     * in order to keep the computation feasible we must focus attention.
-     *
-     * Note that this relation is not expected to be symmetric -- it's
-     * fine to include a pair `(x, y)` but not `(y, x)`.
-     * In fact, not including the symmetrically implied fact will save
-     * half the computation time on the structural comparison.
-     */
-    abstract predicate candidate(ControlFlowElement x, ControlFlowElement y);
-
-    /**
-     * Holds if elements `x` and `y` structurally equal. `x` and `y` must be
-     * flagged as candidates for structural equality, that is,
-     * `candidate(x, y)` must hold.
-     */
-    predicate same(ControlFlowElement x, ControlFlowElement y) { candidate(x, y) and sameGvn(x, y) }
-  }
-}

csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll

@@ -8,7 +8,7 @@ private import dotnet
 private import ControlFlow::SuccessorTypes
 private import semmle.code.csharp.commons.Assertions
 private import semmle.code.csharp.commons.ComparisonTest
-private import semmle.code.csharp.commons.StructuralComparison::Internal
+private import semmle.code.csharp.commons.StructuralComparison as SC
 private import semmle.code.csharp.controlflow.BasicBlocks
 private import semmle.code.csharp.controlflow.internal.Completion
 private import semmle.code.csharp.frameworks.System
@@ -1798,32 +1798,30 @@ module Internal {
   }
 
   /**
-   * A helper class for calculating structurally equal access/call expressions.
+   * Holds if access/call expression `e` (targeting declaration `target`)
+   * is a sub expression of a guard that controls whether basic block
+   * `bb` is reached.
    */
-  private class ConditionOnExprComparisonConfig extends InternalStructuralComparisonConfiguration {
-    ConditionOnExprComparisonConfig() { this = "ConditionOnExprComparisonConfig" }
+  pragma[noinline]
+  private predicate candidateAux(AccessOrCallExpr e, Declaration target, BasicBlock bb) {
+    target = e.getTarget() and
+    guardControlsSub(_, bb, e)
+  }
 
-    override predicate candidate(ControlFlowElement x, ControlFlowElement y) {
-      exists(BasicBlock bb, Declaration d |
-        this.candidateAux(x, d, bb) and
-        y =
-          any(AccessOrCallExpr e |
-            e.getAControlFlowNode().getBasicBlock() = bb and
-            e.getTarget() = d
-          )
-      )
-    }
+  private predicate candidate(AccessOrCallExpr x, AccessOrCallExpr y) {
+    exists(BasicBlock bb, Declaration d |
+      candidateAux(x, d, bb) and
+      y =
+        any(AccessOrCallExpr e |
+          e.getAControlFlowNode().getBasicBlock() = bb and
+          e.getTarget() = d
+        )
+    )
+  }
 
-    /**
-     * Holds if access/call expression `e` (targeting declaration `target`)
-     * is a sub expression of a guard that controls whether basic block
-     * `bb` is reached.
-     */
-    pragma[noinline]
-    private predicate candidateAux(AccessOrCallExpr e, Declaration target, BasicBlock bb) {
-      target = e.getTarget() and
-      guardControlsSub(_, bb, e)
-    }
+  private predicate same(AccessOrCallExpr x, AccessOrCallExpr y) {
+    candidate(x, y) and
+    SC::sameGvn(x, y)
   }
 
   cached
@@ -1849,7 +1847,7 @@ module Internal {
     pragma[nomagic]
     private predicate guardControlsSubSame(Guard g, BasicBlock bb, ControlGuardDescendant sub) {
       guardControlsSub(g, bb, sub) and
-      any(ConditionOnExprComparisonConfig c).same(sub, _)
+      same(sub, _)
     }
 
     pragma[nomagic]
@@ -1862,7 +1860,7 @@ module Internal {
       guardedBB = guardedCfn.getBasicBlock() and
       guardControls(g, guardedBB, v) and
       guardControlsSubSame(g, guardedBB, sub) and
-      any(ConditionOnExprComparisonConfig c).same(sub, guarded)
+      same(sub, guarded)
     }
 
     pragma[nomagic]

csharp/ql/src/Concurrency/UnsafeLazyInitialization.ql

@@ -14,24 +14,12 @@
 import csharp
 import semmle.code.csharp.commons.StructuralComparison
 
-class DoubleCheckedLock extends StructuralComparisonConfiguration {
-  DoubleCheckedLock() { this = "double checked lock" }
-
-  override predicate candidate(ControlFlowElement x, ControlFlowElement y) {
-    exists(IfStmt unlockedIf, IfStmt lockedIf, LockStmt lock |
-      x = unlockedIf.getCondition() and
-      y = lockedIf.getCondition() and
-      lock = unlockedIf.getThen().stripSingletonBlocks() and
-      lockedIf.getParent*() = lock.getBlock()
-    )
-  }
-}
-
-predicate doubleCheckedLock(Field field, IfStmt ifs) {
-  exists(DoubleCheckedLock config, LockStmt lock, Expr eq1, Expr eq2 | ifs.getCondition() = eq1 |
-    lock = ifs.getThen().stripSingletonBlocks() and
-    config.same(eq1, eq2) and
-    field.getAnAccess() = eq1.getAChildExpr*()
+predicate doubleCheckedLock(Field field, IfStmt unlockedIf) {
+  exists(LockStmt lock, IfStmt lockedIf |
+    lock = unlockedIf.getThen().stripSingletonBlocks() and
+    lockedIf.getParent*() = lock.getBlock() and
+    sameGvn(unlockedIf.getCondition(), lockedIf.getCondition()) and
+    field.getAnAccess() = unlockedIf.getCondition().getAChildExpr*()
   )
 }
 

csharp/ql/src/Language Abuse/MissedTernaryOpportunity.ql

@@ -12,25 +12,8 @@
 import csharp
 import semmle.code.csharp.commons.StructuralComparison
 
-class StructuralComparisonConfig extends StructuralComparisonConfiguration {
-  StructuralComparisonConfig() { this = "MissedTernaryOpportunity" }
-
-  override predicate candidate(ControlFlowElement x, ControlFlowElement y) {
-    exists(IfStmt is, AssignExpr ae1 |
-      ae1 = is.getThen().stripSingletonBlocks().(ExprStmt).getExpr()
-    |
-      x = ae1.getLValue() and
-      exists(AssignExpr ae2 | ae2 = is.getElse().stripSingletonBlocks().(ExprStmt).getExpr() |
-        y = ae2.getLValue()
-      )
-    )
-  }
-
-  IfStmt getIfStmt() {
-    exists(AssignExpr ae | ae = result.getThen().stripSingletonBlocks().(ExprStmt).getExpr() |
-      same(ae.getLValue(), _)
-    )
-  }
+private Expr getAssignedExpr(Stmt stmt) {
+  result = stmt.stripSingletonBlocks().(ExprStmt).getExpr().(AssignExpr).getLValue()
 }
 
 from IfStmt is, string what
@@ -40,10 +23,8 @@ where
     is.getElse().stripSingletonBlocks() instanceof ReturnStmt and
     what = "return"
     or
-    exists(StructuralComparisonConfig c |
-      is = c.getIfStmt() and
-      what = "write to the same variable"
-    )
+    sameGvn(getAssignedExpr(is.getThen()), getAssignedExpr(is.getElse())) and
+    what = "write to the same variable"
   ) and
   not exists(IfStmt other | is = other.getElse())
 select is,

csharp/ql/src/Language Abuse/UselessIsBeforeAs.ql

@@ -13,35 +13,26 @@
 import csharp
 import semmle.code.csharp.commons.StructuralComparison
 
-class StructuralComparisonConfig extends StructuralComparisonConfiguration {
-  StructuralComparisonConfig() { this = "UselessIsBeforeAs" }
+private predicate candidate(AsExpr ae, IsExpr ie) {
+  exists(IfStmt is, TypeAccessPatternExpr tape |
+    ie = is.getCondition().getAChild*() and
+    tape = ie.getPattern() and
+    ae.getTargetType() = tape.getTarget()
+  |
+    ae = is.getThen().getAChild*()
+    or
+    ae = is.getElse().getAChild*()
+  )
+}
 
-  override predicate candidate(ControlFlowElement x, ControlFlowElement y) {
-    exists(IfStmt is, AsExpr ae, IsExpr ie, TypeAccessPatternExpr tape |
-      ie = is.getCondition().getAChild*() and
-      tape = ie.getPattern() and
-      ae.getTargetType() = tape.getTarget() and
-      x = ie.getExpr() and
-      y = ae.getExpr()
-    |
-      ae = is.getThen().getAChild*()
-      or
-      ae = is.getElse().getAChild*()
-    )
-  }
-
-  predicate uselessIsBeforeAs(AsExpr ae, IsExpr ie) {
-    exists(Expr x, Expr y |
-      same(x, y) and
-      ie.getExpr() = x and
-      ae.getExpr() = y
-    )
-  }
+private predicate uselessIsBeforeAs(AsExpr ae, IsExpr ie) {
+  candidate(ae, ie) and
+  sameGvn(ie.getExpr(), ae.getExpr())
 }
 
 from AsExpr ae, IsExpr ie
 where
-  exists(StructuralComparisonConfig c | c.uselessIsBeforeAs(ae, ie)) and
+  uselessIsBeforeAs(ae, ie) and
   not exists(MethodCall mc | ae = mc.getAnArgument().getAChildExpr*())
 select ae,
   "This 'as' expression performs a type test - it should be directly compared against null, rendering the 'is' $@ potentially redundant.",

csharp/ql/src/Language Abuse/UselessNullCoalescingExpression.ql

@@ -14,24 +14,22 @@
 import csharp
 import semmle.code.csharp.commons.StructuralComparison
 
-class StructuralComparisonConfig extends StructuralComparisonConfiguration {
-  StructuralComparisonConfig() { this = "UselessNullCoalescingExpression" }
-
-  override predicate candidate(ControlFlowElement x, ControlFlowElement y) {
-    exists(NullCoalescingExpr nce |
-      x.(Access) = nce.getLeftOperand() and
-      y.(Access) = nce.getRightOperand().getAChildExpr*()
-    )
-  }
-
-  NullCoalescingExpr getUselessNullCoalescingExpr() {
-    exists(AssignableAccess x |
-      result.getLeftOperand() = x and
-      forex(AssignableAccess y | same(x, y) | y instanceof AssignableRead and not y.isRefArgument())
-    )
-  }
+pragma[noinline]
+private predicate same(AssignableAccess x, AssignableAccess y) {
+  exists(NullCoalescingExpr nce |
+    x = nce.getLeftOperand() and
+    y = nce.getRightOperand().getAChildExpr*()
+  ) and
+  sameGvn(x, y)
 }
 
-from StructuralComparisonConfig c, NullCoalescingExpr nce
-where nce = c.getUselessNullCoalescingExpr()
+private predicate uselessNullCoalescingExpr(NullCoalescingExpr nce) {
+  exists(AssignableAccess x |
+    nce.getLeftOperand() = x and
+    forex(AssignableAccess y | same(x, y) | y instanceof AssignableRead and not y.isRefArgument())
+  )
+}
+
+from NullCoalescingExpr nce
+where uselessNullCoalescingExpr(nce)
 select nce, "Both operands of this null-coalescing expression access the same variable or property."

csharp/ql/src/Likely Bugs/NestedLoopsSameVariable.ql

@@ -15,18 +15,6 @@ import csharp
 import semmle.code.csharp.commons.ComparisonTest
 import semmle.code.csharp.commons.StructuralComparison as SC
 
-/** A structural comparison configuration for comparing the conditions of nested `for` loops. */
-class NestedForConditions extends SC::StructuralComparisonConfiguration {
-  NestedForConditions() { this = "Compare nested for conditions" }
-
-  override predicate candidate(ControlFlowElement e1, ControlFlowElement e2) {
-    exists(NestedForLoopSameVariable nested |
-      e1 = nested.getInnerForStmt().getCondition() and
-      e2 = nested.getOuterForStmt().getCondition()
-    )
-  }
-}
-
 private predicate hasChild(Stmt outer, Element child) {
   outer = child.getParent() and
   (outer instanceof ForStmt or outer = any(ForStmt f).getBody())
@@ -61,9 +49,7 @@ class NestedForLoopSameVariable extends ForStmt {
   }
 
   private predicate haveSameCondition() {
-    exists(NestedForConditions config |
-      config.same(this.getInnerForStmt().getCondition(), this.getOuterForStmt().getCondition())
-    )
+    SC::sameGvn(this.getInnerForStmt().getCondition(), this.getOuterForStmt().getCondition())
   }
 
   private predicate haveSameUpdate() {

csharp/ql/src/Likely Bugs/SelfAssignment.ql

@@ -13,37 +13,25 @@
 import csharp
 import semmle.code.csharp.commons.StructuralComparison
 
-class StructuralComparisonConfig extends StructuralComparisonConfiguration {
-  StructuralComparisonConfig() { this = "SelfAssignment" }
-
-  override predicate candidate(ControlFlowElement x, ControlFlowElement y) {
-    exists(AssignExpr ae |
-      // Member initializers are never self-assignments, in particular
-      // not initializers such as `new C { F = F };`
-      not ae instanceof MemberInitializer and
-      // Enum field initializers are never self assignments. `enum E { A = 42 }`
-      not ae.getParent().(Field).getDeclaringType() instanceof Enum
-    |
-      ae.getLValue() = x and
-      ae.getRValue() = y
-    ) and
-    forall(Expr e | e = x.(Expr).getAChildExpr*() |
-      // Non-trivial property accesses may have side-effects,
-      // so these are not considered
-      e instanceof PropertyAccess implies e instanceof TrivialPropertyAccess
-    )
-  }
-
-  AssignExpr getSelfAssignExpr() {
-    exists(Expr x, Expr y |
-      same(x, y) and
-      result.getLValue() = x and
-      result.getRValue() = y
-    )
-  }
+private predicate candidate(AssignExpr ae) {
+  // Member initializers are never self-assignments, in particular
+  // not initializers such as `new C { F = F };`
+  not ae instanceof MemberInitializer and
+  // Enum field initializers are never self assignments. `enum E { A = 42 }`
+  not ae.getParent().(Field).getDeclaringType() instanceof Enum and
+  forall(Expr e | e = ae.getLValue().getAChildExpr*() |
+    // Non-trivial property accesses may have side-effects,
+    // so these are not considered
+    e instanceof PropertyAccess implies e instanceof TrivialPropertyAccess
+  )
 }
 
-Declaration getDeclaration(Expr e) {
+private predicate selfAssignExpr(AssignExpr ae) {
+  candidate(ae) and
+  sameGvn(ae.getLValue(), ae.getRValue())
+}
+
+private Declaration getDeclaration(Expr e) {
   result = e.(VariableAccess).getTarget()
   or
   result = e.(MemberAccess).getTarget()
@@ -51,6 +39,6 @@ Declaration getDeclaration(Expr e) {
   result = getDeclaration(e.(ArrayAccess).getQualifier())
 }
 
-from StructuralComparisonConfig c, AssignExpr ae, Declaration target
-where ae = c.getSelfAssignExpr() and target = getDeclaration(ae.getLValue())
+from AssignExpr ae, Declaration target
+where selfAssignExpr(ae) and target = getDeclaration(ae.getLValue())
 select ae, "This assignment assigns $@ to itself.", target, target.getName()

csharp/ql/src/Performance/UseTryGetValue.ql

@@ -13,19 +13,14 @@ import csharp
 import semmle.code.csharp.commons.StructuralComparison
 import semmle.code.csharp.controlflow.Guards as G
 
-class SameElement extends StructuralComparisonConfiguration {
-  SameElement() { this = "Same element" }
-
-  override predicate candidate(ControlFlowElement e1, ControlFlowElement e2) {
-    exists(MethodCall mc, IndexerRead access |
-      mc.getTarget().hasName("ContainsKey") and
-      access.getQualifier().(G::GuardedExpr).isGuardedBy(mc, mc.getQualifier(), _) and
-      e1 = mc.getArgument(0) and
-      e2 = access.getIndex(0)
-    )
-  }
+pragma[noinline]
+private predicate candidate(MethodCall mc, IndexerRead access) {
+  mc.getTarget().hasName("ContainsKey") and
+  access.getQualifier().(G::GuardedExpr).isGuardedBy(mc, mc.getQualifier(), _)
 }
 
-from SameElement element, MethodCall call, IndexerAccess index
-where element.same(call.getArgument(0), index.getIndex(0))
+from MethodCall call, IndexerRead index
+where
+  candidate(call, index) and
+  sameGvn(call.getArgument(0), index.getIndex(0))
 select call, "Inefficient use of 'ContainsKey' and $@.", index, "indexer"

csharp/ql/test/library-tests/structuralcomparison/structuralComparison.ql

@@ -1,28 +1,25 @@
 import csharp
 import semmle.code.csharp.commons.StructuralComparison
 
-private class StructuralComparisonTest extends StructuralComparisonConfiguration {
-  StructuralComparisonTest() { this = "StructuralComparisonTest" }
-
-  /**
-   * All pairs of controls flow elements found in the source and within the same
-   * enclosing callable excluding all instances of `ThisAccess` to reduce the size
-   * of the output.
-   */
-  override predicate candidate(ControlFlowElement e1, ControlFlowElement e2) {
-    e1.fromSource() and
-    e2.fromSource() and
-    e1 != e2 and
-    e1.getEnclosingCallable() = e2.getEnclosingCallable() and
-    not e1 instanceof ThisAccess
-  }
+/**
+ * All pairs of controls flow elements found in the source and within the same
+ * enclosing callable excluding all instances of `ThisAccess` to reduce the size
+ * of the output.
+ */
+private predicate candidate(ControlFlowElement x, ControlFlowElement y) {
+  x.fromSource() and
+  y.fromSource() and
+  x != y and
+  x.getEnclosingCallable() = y.getEnclosingCallable() and
+  not x instanceof ThisAccess
 }
 
-query predicate same(ControlFlowElement e1, ControlFlowElement e2) {
-  exists(StructuralComparisonTest sct, Location l1, Location l2 |
-    sct.same(e1, e2) and
-    l1 = e1.getLocation() and
-    l2 = e2.getLocation() and
+query predicate same(ControlFlowElement x, ControlFlowElement y) {
+  exists(Location l1, Location l2 |
+    candidate(x, y) and
+    sameGvn(x, y) and
+    l1 = x.getLocation() and
+    l2 = y.getLocation() and
     (
       l1.getStartLine() < l2.getStartLine()
       or
@@ -31,4 +28,4 @@ query predicate same(ControlFlowElement e1, ControlFlowElement e2) {
   )
 }
 
-query predicate gvn(ControlFlowElement e, Gvn gvn) { gvn = toGvn(e) and e.fromSource() }
+query predicate gvn(ControlFlowElement x, Gvn gvn) { gvn = toGvn(x) and x.fromSource() }