Skip to content

Commit

Permalink
Fix pmd#5249 and pmd#5250: [java] TooFewBranchesForSwitch ignore patt…
Browse files Browse the repository at this point in the history
…ern matching and support switch expressions (pmd#5251)

Merge pull request pmd#5251 from adangel:issue-5249-5250
  • Loading branch information
adangel committed Oct 24, 2024
2 parents 93f6279 + 45b1217 commit 3a501a0
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 77 deletions.
10 changes: 10 additions & 0 deletions docs/pages/release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ 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.
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.

Expand All @@ -36,6 +38,8 @@ See [PR #5040](https://github.com/pmd/pmd/pull/5040) for details.
* {% rule java/bestpractices/UnitTestShouldUseAfterAnnotation %} (Java Best Practices) has been renamed from `JUnit4TestShouldUseAfterAnnotation`.
* {% rule java/bestpractices/UnitTestShouldUseBeforeAnnotation %} (Java Best Practices) has been renamed from `JUnit4TestShouldUseBeforeAnnotation`.
* {% rule java/bestpractices/UnitTestShouldUseTestAnnotation %} (Java Best Practices) has been renamed from `JUnit4TestShouldUseTestAnnotation`.
* {% rule java/performance/TooFewBranchesForSwitch %} (Java Performance) has been renamed from `TooFewBranchesForASwitchStatement`,
as it now applies to Switch Expressions as well.

The old rule names still work but are deprecated.

Expand All @@ -50,6 +54,9 @@ The old rule names still work but are deprecated.
* java-errorprone
* [#3362](https://github.com/pmd/pmd/issues/3362): \[java] ImplicitSwitchFallThrough should consider switch expressions
* [#5067](https://github.com/pmd/pmd/issues/5067): \[java] CloseResource: False positive for FileSystems.getDefault()
* java-performance
* [#5249](https://github.com/pmd/pmd/issues/5249): \[java] TooFewBranchesForASwitchStatement false positive for Pattern Matching
* [#5250](https://github.com/pmd/pmd/issues/5250): \[java] TooFewBranchesForASwitchStatement should consider Switch Expressions

### 🚨 API Changes
* java-bestpractices
Expand All @@ -59,6 +66,8 @@ The old rule names still work but are deprecated.
* The old rule name `JUnitAssertionsShouldIncludeMessage` has been deprecated. Use the new name {% rule java/bestpractices/UnitTestAssertionsShouldIncludeMessage %} instead.
* The old rule name `JUnitTestContainsTooManyAsserts` has been deprecated. Use the new name {% rule java/bestpractices/UnitTestContainsTooManyAsserts %} instead.
* The old rule name `JUnitTestsShouldIncludeAssert` has been deprecated. Use the new name {% rule java/bestpractices/UnitTestShouldIncludeAssert %} instead.
* java-performance
* The old rule name `TooFewBranchesForASwitchStatement` has been deprecated. Use the new name {% rule java/performance/TooFewBranchesForSwitch %} instead.


### ✨ Merged pull requests
Expand All @@ -69,6 +78,7 @@ The old rule names still work but are deprecated.
* [#5245](https://github.com/pmd/pmd/pull/5245): \[java] Improve UnitTestShouldUse{After,Before}Annotation rules to support JUnit5 and TestNG - [Andreas Dangel](https://github.com/adangel) (@adangel)
* [#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)
* [#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)
Expand Down
13 changes: 10 additions & 3 deletions pmd-java/src/main/resources/category/java/performance.xml
Original file line number Diff line number Diff line change
Expand Up @@ -607,24 +607,31 @@ private String baz() {
</example>
</rule>

<rule name="TooFewBranchesForASwitchStatement"
<rule name="TooFewBranchesForASwitchStatement" deprecated="true" ref="TooFewBranchesForSwitch"/>

<rule name="TooFewBranchesForSwitch"
language="java"
since="4.2"
class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"
message="A switch with less than three branches is inefficient, use a 'if statement' instead."
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_performance.html#toofewbranchesforaswitchstatement">
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_performance.html#toofewbranchesforswitch">
<description>
Switch statements are intended to be used to support complex branching behaviour. Using a switch for only a few
cases is ill-advised, since switches are not as easy to understand as if-else statements. In these cases use the
if-else statement to increase code readability.

Note: This rule was named TooFewBranchesForASwitchStatement before PMD 7.7.0.
</description>
<priority>3</priority>
<properties>
<property name="minimumNumberCaseForASwitch" type="Integer" description="Minimum number of branches for a switch" min="1" max="100" value="3"/>
<property name="xpath">
<value>
<![CDATA[
//SwitchStatement[ (count(*) - 1) < $minimumNumberCaseForASwitch ]
//(SwitchStatement | SwitchExpression)
[ (count(*) - 1) < $minimumNumberCaseForASwitch ]
(: only consider if no pattern matching is used :)
[*/SwitchLabel[@PatternLabel = false()]]
]]>
</value>
</property>
Expand Down
2 changes: 1 addition & 1 deletion pmd-java/src/main/resources/rulesets/java/quickstart.xml
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@
<!--<rule ref="category/java/performance.xml/RedundantFieldInitializer"/>-->
<!-- <rule ref="category/java/performance.xml/StringInstantiation" /> -->
<!-- <rule ref="category/java/performance.xml/StringToString" /> -->
<!--<rule ref="category/java/performance.xml/TooFewBranchesForASwitchStatement"/>-->
<!-- <rule ref="category/java/performance.xml/TooFewBranchesForSwitch"/> -->
<!-- <rule ref="category/java/performance.xml/UseArrayListInsteadOfVector" /> -->
<!-- <rule ref="category/java/performance.xml/UseArraysAsList" /> -->
<!-- <rule ref="category/java/performance.xml/UseIndexOfChar" /> -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@

import net.sourceforge.pmd.test.PmdRuleTst;

class TooFewBranchesForASwitchStatementTest extends PmdRuleTst {
class TooFewBranchesForSwitchTest extends PmdRuleTst {
// no additional unit tests
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
<?xml version="1.0" encoding="UTF-8"?>
<test-data
xmlns="http://pmd.sourceforge.net/rule-tests"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">

<test-code>
<description>Switch Statement with no case, ok</description>
<rule-property name="minimumNumberCaseForASwitch">3</rule-property>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class DumbSwitch {
public void foo(int i) {
switch (i) { } // This is detected by EmptyControlStatement
}
}
]]></code>
</test-code>

<test-code>
<description>Switch Statement with only one case, not ok</description>
<rule-property name="minimumNumberCaseForASwitch">3</rule-property>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class DumbSwitch {
public void foo(int i) {
switch (i) {
case 0:
{
System.err.println("I am a fish.");
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>Switch Expression with only one case, not ok #5250</description>
<rule-property name="minimumNumberCaseForASwitch">3</rule-property>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class DumbSwitch {
public String foo(int i) {
return switch (i) {
case 0:
{
yield "I am a fish.";
}
};
}
}
]]></code>
</test-code>

<test-code>
<description>Even two branches is not enough for a switch statement</description>
<rule-property name="minimumNumberCaseForASwitch">3</rule-property>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class DumbSwitch {
public void foo(int i) {
switch (i) {
case 0:
{
System.err.println("I am a fish.");
}
case 1:
{
System.exit();
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>Three branches in a switch statement is ok.</description>
<rule-property name="minimumNumberCaseForASwitch">3</rule-property>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class ValidSwitch {
public void foo(int i) {
switch (i) {
case 0:
{
System.err.println("I am a fish.");
}
case 1:
{
System.exit();
}
case 2:
{
// ...
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>[java] TooFewBranchesForASwitchStatement false positive for Pattern Matching #5249</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
sealed interface S permits A {}
final class A implements S {}
public class Sample {
public void simpleSwitchStatment(S s) {
switch(s) {
case A a -> System.out.println("a");
}
}
public void simpleSwitchExpression(S s) {
String result = switch(s) {
case A a -> "a";
};
}
}
]]></code>
</test-code>

<test-code>
<description>Record patterns are ignored, too #5249</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-data>

0 comments on commit 3a501a0

Please sign in to comment.