Merge pull request #19998 from tamasvajk/quality/label-in-switch

Java: Add query to detect non-case labels in switch statements

Author: tamasvajk

java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected

@@ -6,6 +6,7 @@ ql/java/ql/src/Compatibility/JDK9/UnderscoreIdentifier.ql
 ql/java/ql/src/DeadCode/UselessParameter.ql
 ql/java/ql/src/Language Abuse/EmptyMethod.ql
 ql/java/ql/src/Language Abuse/IterableIterator.ql
+ql/java/ql/src/Language Abuse/LabelInSwitch.ql
 ql/java/ql/src/Language Abuse/TypeVariableHidesType.ql
 ql/java/ql/src/Language Abuse/UselessNullCheck.ql
 ql/java/ql/src/Language Abuse/UselessTypeTest.ql

java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected

@@ -5,6 +5,7 @@ ql/java/ql/src/Compatibility/JDK9/JdkInternalAccess.ql
 ql/java/ql/src/Compatibility/JDK9/UnderscoreIdentifier.ql
 ql/java/ql/src/DeadCode/UselessParameter.ql
 ql/java/ql/src/Language Abuse/IterableIterator.ql
+ql/java/ql/src/Language Abuse/LabelInSwitch.ql
 ql/java/ql/src/Language Abuse/UselessNullCheck.ql
 ql/java/ql/src/Language Abuse/UselessTypeTest.ql
 ql/java/ql/src/Language Abuse/WrappedIterator.ql

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

@@ -0,0 +1,33 @@
+## Overview
+
+Java allows to freely mix `case` labels and ordinary statement labels in the body of
+a `switch` statement. However, this is confusing to read and may be the result of a typo.
+
+## Recommendation
+
+Examine the non-`case` labels to see whether they were meant to be `case` labels. If not, consider placing the non-`case` label headed code into a function, and use a function call inline in the `switch` body instead.
+
+## Example
+
+```java
+public class Test {
+    void test_noncase_label_in_switch(int p) {
+        switch (p) {
+            case 1: // Compliant
+            case2:  // Non-compliant, likely a typo
+            break;
+            case 3:
+                notcaselabel: // Non-compliant, confusing to read
+                for (;;) {
+                    break notcaselabel;
+                }
+        }
+    }
+}
+```
+
+In the example, `case2` is most likely a typo and should be fixed. For the intentional `notcaselabel`, placing the labelled code into a function and then calling that function is more readable.
+
+## References
+
+CodeQL query help for JavaScript and TypeScript - [Non-case label in switch statement](https://codeql.github.com/codeql-query-help/javascript/js-label-in-switch/).

java/ql/src/Language Abuse/LabelInSwitch.ql

@@ -0,0 +1,25 @@
+/**
+ * @id java/label-in-switch
+ * @name Non-case label in switch statement
+ * @description A non-case label appearing in a switch statement
+ *              is confusing to read or may even indicate a bug.
+ * @previous-id java/label-in-case
+ * @kind problem
+ * @precision very-high
+ * @problem.severity recommendation
+ * @tags quality
+ *       maintainability
+ *       readability
+ */
+
+import java
+
+from LabeledStmt l, SwitchStmt s, string alert
+where
+  l = s.getAStmt+() and
+  if exists(JumpStmt jump | jump.getTargetLabel() = l)
+  then alert = "Confusing non-case label in switch statement."
+  else
+    alert =
+      "Possibly erroneous non-case label in switch statement. The case keyword might be missing."
+select l, alert

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

@@ -0,0 +1,2 @@
+| Test.java:14:17:14:31 | <Label>: ... | Possibly erroneous non-case label in switch statement. The case keyword might be missing. |
+| Test.java:17:17:17:39 | <Label>: ... | Confusing non-case label in switch statement. |

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

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

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

@@ -0,0 +1,21 @@
+public class Test {
+    void test_case_label_only_in_switch(int p) {
+        switch (p) {
+            case 1:
+            case 2:
+                break;
+        }
+        notcaselabelnotinswitch: for (;;) {}
+    }
+
+    void test_noncase_label_in_switch(int p) {
+        switch (p) {
+            case 1:
+                notcaselabel1:; // $ Alert | Possibly erroneous non-case label in switch statement. The case keyword might be missing.
+                break;
+            case 2:
+                notcaselabel2: for (;;) { break notcaselabel2; } // $ Alert | Confusing non-case label in switch statement.
+                break;
+        }
+    }
+}