Fix #4813: [java] SwitchStmtsShouldHaveDefault false positive with pattern matching (#5252)

Merge pull request #5252 from adangel:issue-4813
2024-10-24 14:08:12 +02:00 · 2024-10-24 14:08:12 +02:00 · 4b23718aac
commit 4b23718aac
parent 3a501a0f6b 662759755d
5 changed files with 155 additions and 4 deletions
--- a/.all-contributorsrc
+++ b/.all-contributorsrc
@ -7453,7 +7453,8 @@
      "avatar_url": "https://avatars.githubusercontent.com/u/16755668?v=4",
      "profile": "https://github.com/emouty",
      "contributions": [
-        "code"
+        "code",
+        "bug"
      ]
    },
    {
--- a/docs/pages/pmd/projectdocs/credits.md
+++ b/docs/pages/pmd/projectdocs/credits.md
@ -918,7 +918,7 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
      <td align="center" valign="top" width="14.28%"><a href="https://github.com/eant60"><img src="https://avatars.githubusercontent.com/u/41472980?v=4?s=100" width="100px;" alt="eant60"/><br /><sub><b>eant60</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Aeant60" title="Bug reports">🐛</a></td>
      <td align="center" valign="top" width="14.28%"><a href="https://github.com/ekkirala"><img src="https://avatars.githubusercontent.com/u/44954455?v=4?s=100" width="100px;" alt="ekkirala"/><br /><sub><b>ekkirala</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Aekkirala" title="Bug reports">🐛</a></td>
      <td align="center" valign="top" width="14.28%"><a href="https://github.com/emersonmoura"><img src="https://avatars.githubusercontent.com/u/5419868?v=4?s=100" width="100px;" alt="emersonmoura"/><br /><sub><b>emersonmoura</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Aemersonmoura" title="Bug reports">🐛</a></td>
-      <td align="center" valign="top" width="14.28%"><a href="https://github.com/emouty"><img src="https://avatars.githubusercontent.com/u/16755668?v=4?s=100" width="100px;" alt="emouty"/><br /><sub><b>emouty</b></sub></a><br /><a href="https://github.com/pmd/pmd/commits?author=emouty" title="Code">💻</a></td>
+      <td align="center" valign="top" width="14.28%"><a href="https://github.com/emouty"><img src="https://avatars.githubusercontent.com/u/16755668?v=4?s=100" width="100px;" alt="emouty"/><br /><sub><b>emouty</b></sub></a><br /><a href="https://github.com/pmd/pmd/commits?author=emouty" title="Code">💻</a> <a href="https://github.com/pmd/pmd/issues?q=author%3Aemouty" title="Bug reports">🐛</a></td>
      <td align="center" valign="top" width="14.28%"><a href="https://github.com/eugenepugach"><img src="https://avatars.githubusercontent.com/u/133967768?v=4?s=100" width="100px;" alt="eugenepugach"/><br /><sub><b>eugenepugach</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Aeugenepugach" title="Bug reports">🐛</a></td>
      <td align="center" valign="top" width="14.28%"><a href="https://juejin.cn/user/1063982985642525"><img src="https://avatars.githubusercontent.com/u/24585054?v=4?s=100" width="100px;" alt="fairy"/><br /><sub><b>fairy</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Aguxiaonian" title="Bug reports">🐛</a></td>
      <td align="center" valign="top" width="14.28%"><a href="https://github.com/filiprafalowicz"><img src="https://avatars.githubusercontent.com/u/24355557?v=4?s=100" width="100px;" alt="filiprafalowicz"/><br /><sub><b>filiprafalowicz</b></sub></a><br /><a href="https://github.com/pmd/pmd/commits?author=filiprafalowicz" title="Code">💻</a></td>
--- a/docs/pages/release_notes.md
+++ b/docs/pages/release_notes.md
@ -24,10 +24,12 @@ See [PR #5040](https://github.com/pmd/pmd/pull/5040) for details.
 ### 🌟 Rule Changes

 #### Changed Rules
-* {% rule java/performance/TooFewBranchesForSwitch %} (Java Performance) doesn't report empty switches anymore.
+* {% rule java/bestpractices/SwitchStmtsShouldHaveDefault %} (Java Best Practices) doesn't report empty switch statements anymore.
  To detect these, use {% rule java/codestyle/EmptyControlStatement %}.
 * {% rule java/bestpractices/UnitTestShouldUseAfterAnnotation %} (Java Best Practices) now also considers JUnit 5 and TestNG tests.
 * {% rule java/bestpractices/UnitTestShouldUseBeforeAnnotation %} (Java Best Practices) now also considers JUnit 5 and TestNG tests.
+* {% rule java/performance/TooFewBranchesForSwitch %} (Java Performance) doesn't report empty switches anymore.
+  To detect these, use {% rule java/codestyle/EmptyControlStatement %}.

 #### Renamed Rules
 * Several rules for unit testing have been renamed to better reflect their actual scope. Lots of them were called
@ -47,6 +49,8 @@ The old rule names still work but are deprecated.
 * java
  * [#4532](https://github.com/pmd/pmd/issues/4532): \[java] Rule misnomer for JUnit* rules
  * [#5261](https://github.com/pmd/pmd/issues/5261): \[java] Record patterns with empty deconstructor lists lead to NPE
+* java-bestpractices
+  * [#4813](https://github.com/pmd/pmd/issues/4813): \[java] SwitchStmtsShouldHaveDefault false positive with pattern matching
 * java-codestyle
  * [#5253](https://github.com/pmd/pmd/issues/5253): \[java] BooleanGetMethodName: False-negatives with `Boolean` wrapper
 * java-design
@ -79,6 +83,7 @@ The old rule names still work but are deprecated.
 * [#5247](https://github.com/pmd/pmd/pull/5247): Fix #5030: \[java] SwitchDensity false positive with pattern matching - [Andreas Dangel](https://github.com/adangel) (@adangel)
 * [#5248](https://github.com/pmd/pmd/pull/5248): Fix #3362: \[java] ImplicitSwitchFallThrough should consider switch expressions - [Andreas Dangel](https://github.com/adangel) (@adangel)
 * [#5251](https://github.com/pmd/pmd/pull/5251): Fix #5249 and #5250: \[java] TooFewBranchesForSwitch ignore pattern matching and support switch expressions - [Andreas Dangel](https://github.com/adangel) (@adangel)
+* [#5252](https://github.com/pmd/pmd/pull/5252): Fix #4813: \[java] SwitchStmtsShouldHaveDefault false positive with pattern matching - [Andreas Dangel](https://github.com/adangel) (@adangel)
 * [#5258](https://github.com/pmd/pmd/pull/5258): Ignore generated antlr classes in coverage reports - [Juan Martín Sotuyo Dodero](https://github.com/jsotuyod) (@jsotuyod)
 * [#5264](https://github.com/pmd/pmd/pull/5264): Fix #5261: \[java] Fix NPE with empty pattern list - [Clément Fournier](https://github.com/oowekyala) (@oowekyala)
 * [#5269](https://github.com/pmd/pmd/pull/5269): Fix #5253: \[java] Support Boolean wrapper class for BooleanGetMethodName rule - [Aryant Tripathi](https://github.com/Aryant-Tripathi) (@Aryant-Tripathi)
--- a/pmd-java/src/main/resources/category/java/bestpractices.xml
+++ b/pmd-java/src/main/resources/category/java/bestpractices.xml
@ -1168,12 +1168,20 @@ class SomeTestClass {
            easier to follow. This can be achieved by adding a `default` case, or,
            if the switch is on an enum type, by ensuring there is one switch branch
            for each enum constant.
+
+            This rule doesn't consider Switch Statements, that use Pattern Matching, since for these the
+            compiler already ensures that all cases are covered. The same is true for Switch Expressions,
+            which are also not considered by this rule.
        </description>
        <priority>3</priority>
        <properties>
            <property name="xpath">
                <value><![CDATA[
-                //SwitchStatement[@DefaultCase = false() and @ExhaustiveEnumSwitch = false()]
+                //SwitchStatement
+                    [@DefaultCase = false()]
+                    [@ExhaustiveEnumSwitch = false()]
+                    (: exclude pattern tests - for these, the compiler will ensure exhaustiveness :)
+                    [*/SwitchLabel[@PatternLabel = false()]]
            ]]></value>
            </property>
        </properties>
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/SwitchStmtsShouldHaveDefault.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/SwitchStmtsShouldHaveDefault.xml
@ -19,6 +19,19 @@ public class Foo {
        ]]></code>
    </test-code>

+    <test-code>
+        <description>empty switch is ok</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    void bar() {
+        int x = 2;
+        switch (x) { } // this is ok. The empty switch is detected by EmptyControlStatement
+    }
+}
+        ]]></code>
+    </test-code>
+
    <test-code>
        <description>simple ok case</description>
        <expected-problems>0</expected-problems>
@ -240,4 +253,128 @@ public enum GradeSystem {
 }
        ]]></code>
    </test-code>
+
+    <test-code>
+        <description>[java] SwitchStmtsShouldHaveDefault false positive with pattern matching #4813</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public sealed interface AcceptableResult permits Success, Warning {
+    public String message();
+}
+public final class Success implements AcceptableResult {
+    @Override
+    public String message() {
+        return "Success!";
+    }
+}
+public abstract class Failure {
+    abstract public String message();
+}
+public non-sealed class Warning extends Failure implements AcceptableResult {
+    @Override
+    public String message() {
+        return "Oopsie";
+    }
+}
+public class ProviderWarning extends Warning {
+    @Override
+    public String message() {
+        return "Ohoh";
+    }
+}
+public class Example {
+    public void test(AcceptableResult result) {
+        switch (result) {
+            case Warning failure -> System.out.println("WARNING " + failure.message());
+            case Success success -> System.out.println(success.message());
+        }
+    }
+    public void test2(AcceptableResult result) {
+        switch (result) {
+            case ProviderWarning failure: System.out.println("Provider WARNING " + failure.message()); break;
+            case Warning failure: System.out.println("WARNING " + failure.message()); break;
+            case Success success: System.out.println(success.message()); break;
+        }
+    }
+    public void test3(AcceptableResult result) {
+        switch (result) {
+            case ProviderWarning failure -> System.out.println("Provider WARNING " + failure.message());
+            case Success success -> System.out.println(success.message());
+            default -> System.out.println("default case");
+        }
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>[java] SwitchStmtsShouldHaveDefault false positive with pattern matching #4813, example 2</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+sealed interface S permits A, B {}
+final class A implements S {}
+sealed abstract class B implements S permits C, D {}
+final class C extends B {}
+final class D extends B {}
+public class Example2 {
+    static int testSealedExhaustive(S s) {
+        switch(s) {
+            case A a -> { return 1; }
+            case C c -> { return 2; }
+            case D d -> { return 3; }
+            // case B b -> { return 4; } // not explicitly necessary, but possible
+        }
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>With Record Patterns #4813</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+record R(int i) {}
+
+public class SwitchWithRecordPattern {
+    public void check(R r) {
+        switch(r) {
+            case R(int a) -> System.out.println(a);
+        }
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Multiple Case Constants</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+enum MyEnum { A, B, C }
+
+public class SwitchMultipleCaseConstants {
+    public void switchLabels(MyEnum e) {
+        switch(e) {
+            case A,B: System.out.println("a or b"); break;
+            case C: System.out.println("c");
+        }
+        String s = switch(e) {
+            case A,B: yield "a or b";
+            case C: yield "c";
+        };
+        System.out.println(s);
+    }
+    public void switchRules(MyEnum e) {
+        switch(e) {
+            case A,B -> System.out.println("a or b");
+            case C -> System.out.println("c");
+        }
+        String s = switch(e) {
+            case A,B -> "a or b";
+            case C -> "c";
+        };
+        System.out.println(s);
+    }
+}
+]]></code>
+    </test-code>
 </test-data>