Extend assertion-arguments-switched.ql to cover more constant expressions

Author: Marcono1234

codeql-custom-queries-java/queries/unit-tests/assertion-arguments-constant.ql

@@ -16,6 +16,6 @@ from MethodAccess assertCall, AssertMethod assertMethod
 where
     assertMethod = assertCall.getMethod()
     and forex(int inputParamIndex | inputParamIndex = assertMethod.getAnInputParamIndex() |
-        assertCall.getArgument(inputParamIndex) instanceof CompileTimeConstantOrLiteral
+        assertCall.getArgument(inputParamIndex) instanceof ConstantExpr
     )
 select assertCall, "Assertion will always have the same outcome"

codeql-custom-queries-java/queries/unit-tests/assertion-arguments-switched.ql

@@ -9,15 +9,60 @@
  * assertEquals(actual, "expected");
  * ```
  * SonarSource rule: [RSPEC-3415](https://rules.sonarsource.com/java/RSPEC-3415)
+ *
+ * @kind problem
+ * @id TODO
  */
 
- import java
- import lib.AssertLib
+import java
+import lib.AssertLib
 
- from MethodAccess assertCall, AssertTwoArgumentsMethod assertMethod
+/**
+ * Expression which seems to be intended as 'expected' argument for an assertion.
+ */
+class ExpectedValueExpr extends Expr {
+  ExpectedValueExpr() {
+    // Note: Can cause false positives if the value of a constant (= 'actual') defined in the same
+    // project is being checked, but on the other hand there are also cases where the constant is
+    // is intended as 'expected' value
+    this instanceof ConstantExpr
+    or
+    exists(MethodAccess call, Method m |
+      call = this and
+      m = call.getMethod() and
+      m.isStatic() and
+      forall(Expr arg | arg = call.getAnArgument() | arg instanceof ExpectedValueExpr) and
+      // Only consider methods from third-party libraries or JDK, otherwise intention might be to
+      // test result of method
+      not m.getSourceDeclaration().fromSource()
+    )
+    or
+    exists(ClassInstanceExpr newExpr |
+      newExpr = this and
+      forall(Expr arg | arg = newExpr.getAnArgument() | arg instanceof ExpectedValueExpr)
+      // Do not exclude types declared in same project, assume that `newExpr` is always the 'expected' value
+      // If `newExpr` is intended as 'actual' then this might be misuse of `assertEquals` for `equals`
+      // implementation check (which is discouraged)
+    )
+    or
+    exists(ArrayCreationExpr newArray | newArray = this |
+      // Either creates array with constant dimensions
+      forex(Expr dimExpr | dimExpr = newArray.getADimension() |
+        dimExpr instanceof ExpectedValueExpr
+      )
+      or
+      // Or with init containing constants (checked transitively)
+      forex(Expr initValue | initValue = newArray.getInit().getAnInit+() |
+        initValue instanceof ArrayInit or initValue instanceof ExpectedValueExpr
+      )
+    )
+  }
+}
+
+from MethodAccess assertCall, AssertTwoArgumentsMethod assertMethod
 where
-    assertMethod = assertCall.getMethod()
-    and assertCall.getArgument(assertMethod.getAssertionParamIndex()) instanceof CompileTimeConstantOrLiteral
-    // Ignore if both arguments are constant, that is already detected by separate query
-    and not assertCall.getArgument(assertMethod.getFixedParamIndex()) instanceof CompileTimeConstantOrLiteral
+  assertMethod = assertCall.getMethod() and
+  assertCall.getArgument(assertMethod.getAssertionParamIndex()) instanceof ExpectedValueExpr and
+  // Ignore if both arguments are constant, that is already detected by separate query
+  not assertCall.getArgument(assertMethod.getFixedParamIndex()) instanceof ExpectedValueExpr
 select assertCall, "Assertion arguments are switched"

codeql-custom-queries-java/queries/unit-tests/lib/AssertLib.qll

@@ -200,13 +200,26 @@ abstract class AssertThrowsMethod extends AssertMethod {
 }
 
 /**
- * A compile time constant expression or any literal.
+ * An expression with constant value.
  */
-class CompileTimeConstantOrLiteral extends Expr {
-  CompileTimeConstantOrLiteral() {
+class ConstantExpr extends Expr {
+  ConstantExpr() {
     // CompileTimeConstantExpr does not include NullLiteral and TypeLiteral
     this instanceof CompileTimeConstantExpr
-    or this instanceof NullLiteral
-    or this instanceof TypeLiteral
+    or
+    this instanceof NullLiteral
+    or
+    this instanceof TypeLiteral
+    or
+    this.(FieldRead).getField() instanceof EnumConstant
+    or
+    exists(Field f |
+      f = this.(FieldRead).getField() and
+      f.isStatic() and
+      f.isFinal() and
+      // And field is declared in third-party library or JDK, otherwise test might intentionally
+      // use constant as 'actual' argument to check its value
+      not f.fromSource()
+    )
   }
 }