Java: Add initial version of empty method query

Author: tamasvajk

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

@@ -0,0 +1,39 @@
+# J-D-002: An empty method serves no purpose and may indicate programmer error
+
+An empty method serves no purpose and may indicate programmer error.
+
+## Overview
+
+An empty method may indicate that an implmentation was intended to be provided but was accidentally omitted. When using the method, it will not be clear that it does not provide an implentation and with dynamic dispatch, resolving to a blank method may result in unexpected program behaviour.
+
+## 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, or 4) throw an `UnsupportedOperationException` in the method (like is done in `java.util.Collections.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 annotated with `org.springframework.aop.Pointcut` or `org.aspectj.lang.annotation.Pointcut`. Such annotations are commonly paired with empty methods and are considered an acceptable use case for an empty method.
+
+## References
+- [java.util.Collections.add](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/util/Collection.html#add(E))
+- [Template pattern is a valid empty method use](https://en.wikipedia.org/wiki/Template_method_pattern)

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

@@ -0,0 +1,99 @@
+/**
+ * @id java/empty-method
+ * @name J-D-002: An empty method serves no purpose and may indicate programmer error
+ * @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 very-high
+ * @problem.severity warning
+ * @tags correctness
+ *       maintainability
+ *       readability
+ */
+
+import java
+
+/**
+ * Represents a likely a test method, which is either a method that is already
+ * recognized as a `TestMethod` or something that is likely a JUnit test or
+ * something in the expected test path for Java tests.
+ */
+class LikelyTestMethod extends Method {
+  LikelyTestMethod() {
+    this.getDeclaringType() instanceof TestClass
+    or
+    this instanceof TestMethod
+    or
+    this instanceof LikelyJunitTest
+    or
+    //standard Maven/Gradle test file discovery filepath
+    this.getFile().getAbsolutePath().matches("%src/test/java%")
+    or
+    this.getDeclaringType() instanceof SurefireTest
+  }
+}
+
+/**
+ * Classes that are likely part of junit tests (more general than `TestMethod` from `UnitTest.qll`)
+ * 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().toString().matches("%Test")
+    )
+  }
+}
+
+/**
+ * Maven surefire patterns to consider which files are testcases:
+ * https://maven.apache.org/surefire/maven-surefire-plugin/examples/inclusion-exclusion.html
+ */
+class SurefireTest extends Class {
+  SurefireTest() {
+    this.getFile().getAbsolutePath().matches("%src/test/java%") and
+    this.getFile()
+        .getBaseName()
+        .matches(["Test%.java", "%Test.java", "%Tests.java", "%TestCase.java"])
+  }
+}
+
+/**
+ * Frameworks that provide `PointCuts`
+ * which commonly intentionally annotate empty methods
+ */
+class PointCutAnnotation extends Annotation {
+  PointCutAnnotation() {
+    this.getType().hasQualifiedName("org.aspectj.lang.annotation", "Pointcut")
+  }
+}
+
+/**
+ * A `Method` from source that is not abstract
+ */
+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(Javadoc jd | m.getDoc().getJavadoc() = jd) and
+  //methods annotated this way are usually intentionally empty
+  not exists(PointCutAnnotation a | a = m.getAnAnnotation())
+select m, "Empty method found."

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

@@ -0,0 +1 @@
+| Test.java:16:17:16:18 | f2 | Empty method found. |

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

@@ -0,0 +1 @@
+Language Abuse/EmptyMethod.ql

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

@@ -0,0 +1,29 @@
+import org.aspectj.lang.annotation.Pointcut;
+
+public class Test {
+
+    // COMPLIANT
+    public void f() {
+        int i = 0;
+    }
+
+    // COMPLIANT
+    public void f1() {
+        // intentionally empty
+    }
+
+    // NON_COMPLIANT
+    public void f2() {}
+
+    // COMPLIANT - exception
+    @Pointcut()
+    public void f4() {}
+
+    public abstract class TestInner {
+
+        public abstract void f(); // COMPLIANT - intentionally empty
+    }
+
+}
+
+

java/ql/test/query-tests/EmptyMethod/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../stubs/aspectj