Merge branch 'main' into workflow/coverage/update

Author: owen-mc

.github/workflows/build-ripunzip.yml

@@ -17,7 +17,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        os: [ubuntu-20.04, macos-13, windows-2019]
+        os: [ubuntu-22.04, macos-13, windows-2019]
     runs-on: ${{ matrix.os }}
     steps:
       - uses: actions/checkout@v4

java/ql/lib/semmle/code/java/UnitTests.qll

@@ -66,6 +66,19 @@ class TestClass extends Class {
   }
 }
 
+/**
+ * A class that is likely a test class. That is either a definite test class, or
+ * a class whose name, package, or location suggests that it might be a test class.
+ */
+class LikelyTestClass extends Class {
+  LikelyTestClass() {
+    this instanceof TestClass or
+    this.getName().toLowerCase().matches("%test%") or
+    this.getPackage().getName().toLowerCase().matches("%test%") or
+    this.getLocation().getFile().getAbsolutePath().matches("%/src/test/java%")
+  }
+}
+
 /**
  * A test method declared within a JUnit 3.8 test class.
  */
@@ -185,6 +198,37 @@ class TestMethod extends Method {
   }
 }
 
+/**
+ * A method that is likely a test method.
+ */
+class LikelyTestMethod extends Method {
+  LikelyTestMethod() {
+    this.getDeclaringType() instanceof LikelyTestClass
+    or
+    this instanceof TestMethod
+    or
+    this instanceof LikelyJunitTest
+  }
+}
+
+/**
+ * A `Method` that is public, has no parameters,
+ * has a "void" return type, AND either has a name that starts with "test" OR
+ * has an annotation that ends with "Test"
+ */
+class LikelyJunitTest extends Method {
+  LikelyJunitTest() {
+    this.isPublic() and
+    this.getReturnType().hasName("void") and
+    this.hasNoParameters() and
+    (
+      this.getName().matches("JUnit%") or
+      this.getName().matches("test%") or
+      this.getAnAnnotation().getType().getName().matches("%Test")
+    )
+  }
+}
+
 /**
  * A TestNG annotation used to mark a method that runs "before".
  */

java/ql/src/Language Abuse/EmptyMethod.md

@@ -0,0 +1,40 @@
+## Overview
+
+An empty method may indicate that an implementation was intended to be provided but was accidentally omitted. When using the method, it will not be clear that it does not provide an implementation and with dynamic dispatch, resolving to a blank method may result in unexpected program behavior.
+
+## Recommendation
+
+If a method is intended to be left empty, do one of the following to indicate that it is intentionally empty: 
+1. Mark it abstract in an abstract class
+2. Place it in an interface (then it can be implicitly abstract)
+3. Place a comment in that method that lets others know that the implementation was intentionally omitted
+4. Add `UnsupportedOperationException` to the method (as in `java.util.Collection.add`).
+
+## Example
+
+```java
+public class Test {
+    public void f1() { // COMPLIANT
+        // intentionally empty
+    }
+
+    public void f2() {} // NON_COMPLIANT
+
+    public void f3(){ throw new UnsupportedOperationException(); } // COMPLIANT
+
+    public abstract class TestInner {
+
+        public abstract void f(); // COMPLIANT - intentionally empty
+    }
+
+}
+```
+
+## Implementation Notes
+
+The rule excludes reporting methods that are annotated.
+
+## References
+- Java SE Documentation: [java.util.Collection.add](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/util/Collection.html#add(E)).
+- Wikipedia: [Template method pattern](https://en.wikipedia.org/wiki/Template_method_pattern).
+- Common Weakness Enumeration: [CWE-1071](https://cwe.mitre.org/data/definitions/1071.html).

java/ql/src/Language Abuse/EmptyMethod.ql

@@ -0,0 +1,41 @@
+/**
+ * @id java/empty-method
+ * @name Empty method
+ * @description An empty method serves no purpose and makes code less readable. An empty method may
+ *              indicate an error on the part of the developer.
+ * @kind problem
+ * @precision medium
+ * @problem.severity recommendation
+ * @tags correctness
+ *       maintainability
+ *       readability
+ *       quality
+ *       external/cwe/cwe-1071
+ */
+
+import java
+
+/**
+ * A `Method` from source that is not abstract, and likely not a test method
+ */
+class NonAbstractSource extends Method {
+  NonAbstractSource() {
+    this.fromSource() and
+    not this.isAbstract() and
+    not this instanceof LikelyTestMethod
+  }
+}
+
+from NonAbstractSource m
+where
+  //empty
+  not exists(m.getBody().getAChild()) and
+  //permit comment lines explaining why this is empty
+  m.getNumberOfCommentLines() = 0 and
+  //permit a javadoc above as well as sufficient reason to leave empty
+  not exists(m.getDoc().getJavadoc()) and
+  //annotated methods are considered compliant
+  not exists(m.getAnAnnotation()) and
+  //native methods have no body
+  not m.isNative()
+select m, "Empty method found."

java/ql/src/change-notes/2025-03-10-empty-method.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new quality query, `java/empty-method`, to detect empty methods.

java/ql/src/experimental/Security/CWE/CWE-489/TestLib.qll

@@ -10,9 +10,4 @@ import java
  *    c) in a test class whose name has the word `test`
  *    d) in a test class implementing a test framework such as JUnit or TestNG
  */
-predicate isTestMethod(Method m) {
-  m.getDeclaringType().getName().toLowerCase().matches("%test%") or // Simple check to exclude test classes to reduce FPs
-  m.getDeclaringType().getPackage().getName().toLowerCase().matches("%test%") or // Simple check to exclude classes in test packages to reduce FPs
-  exists(m.getLocation().getFile().getAbsolutePath().indexOf("/src/test/java")) or //  Match test directory structure of build tools like maven
-  m instanceof TestMethod // Test method of a test case implementing a test framework such as JUnit or TestNG
-}
+predicate isTestMethod(LikelyTestMethod m) { any() }

java/ql/test/query-tests/EmptyMethod/Class1.java

@@ -0,0 +1,54 @@
+import org.aspectj.lang.annotation.Pointcut;
+
+public class Class1 {
+
+  // COMPLIANT
+  public void f() {
+    int i = 0;
+  }
+
+  // COMPLIANT
+  public void f1() {
+    // intentionally empty
+  }
+
+  // NON_COMPLIANT
+  public void f2() { } // $ Alert
+
+  // COMPLIANT - exception
+  @Pointcut()
+  public void f4() {
+  }
+
+  /**
+   * COMPLIANT - empty method with javadoc
+   */
+  public void f5() {
+  }
+
+  public abstract class TestInner {
+
+    public abstract void f(); // COMPLIANT - intentionally empty
+
+  }
+
+  public class Derived extends TestInner {
+
+    // COMPLIANT: with annotation
+    @Override
+    public void f() {
+    }
+
+    // COMPLIANT: native
+    public native int nativeMethod();
+  }
+
+  public interface TestInterface {
+
+    // NON_COMPLIANT
+    default void method() { } // $ Alert
+
+    void method2(); // COMPLIANT
+  }
+
+}

java/ql/test/query-tests/EmptyMethod/EmptyMethod.expected

@@ -0,0 +1,2 @@
+| Class1.java:16:15:16:16 | f2 | Empty method found. |
+| Class1.java:49:18:49:23 | method | Empty method found. |

java/ql/test/query-tests/EmptyMethod/EmptyMethod.qlref

@@ -0,0 +1,2 @@
+query: Language Abuse/EmptyMethod.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

java/ql/test/query-tests/EmptyMethod/Test.java

@@ -0,0 +1,5 @@
+public class Test {
+  // COMPLIANT: allow empty method in test class
+  public void f() {
+  }
+}