Merge pull request #3805 from oowekyala:issue3605-default-case

[java] Fix #3605 bug with switch arrows #3805 * pr-3805: Add @karel1980 as a contributor [doc] Update release notes (#3605 #3805) Update reference files Fix bug with switch arrows
pmd · Feb 25, 2022 · 47901b7 · 47901b7
2 parents 36134ed + f369455
commit 47901b7
Show file tree

Hide file tree

Showing 11 changed files with 259 additions and 75 deletions.
diff --git a/.all-contributorsrc b/.all-contributorsrc
@@ -6548,6 +6548,15 @@
         "code",
         "doc"
       ]
+    },
+    {
+      "login": "karel1980",
+      "name": "Karel Vervaeke",
+      "avatar_url": "https://avatars.githubusercontent.com/u/153021?v=4",
+      "profile": "https://github.com/karel1980",
+      "contributions": [
+        "bug"
+      ]
     }
   ],
   "contributorsPerLine": 7,

diff --git a/docs/pages/pmd/projectdocs/credits.md b/docs/pages/pmd/projectdocs/credits.md
diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md
@@ -22,6 +22,8 @@ This is a {{ site.pmd.release_type }} release.
     *   [#2502](https://github.com/pmd/pmd/issues/2502): \[doc] Add floating table-of-contents (toc) on the right
 *   java
     *   [#3698](https://github.com/pmd/pmd/issues/3697): \[java] Parsing error with try-with-resources and qualified resource
+*   java-bestpractices
+    *   [#3605](https://github.com/pmd/pmd/issues/3605): \[java] SwitchStmtsShouldHaveDefault triggered when default case is present
 *   java-codestyle
     *   [#278](https://github.com/pmd/pmd/issues/278): \[java] ConfusingTernary should treat `!= null` as positive condition
 *   java-performance

diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatement.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatement.java
@@ -4,7 +4,9 @@
 
 package net.sourceforge.pmd.lang.java.ast;
 
+import java.util.ArrayList;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Set;
 
 import org.apache.commons.lang3.EnumUtils;
@@ -99,9 +101,28 @@ public boolean isExhaustiveEnumSwitch() {
         return false;
     }
 
+    /**
+     * Returns true if this a switch which uses fallthrough branches
+     * (old school {@code case label: break;}) and not arrow branches.
+     * If the switch has no branches, returns false.
+     */
+    public boolean isFallthroughSwitch() {
+        return getFirstChildOfType(ASTSwitchLabel.class) != null
+            && getNumChildren() != 1;
+    }
 
     @Override
     public Iterator<ASTSwitchLabel> iterator() {
-        return new NodeChildrenIterator<>(this, ASTSwitchLabel.class);
+        List<ASTSwitchLabel> result = new ArrayList<>(findChildrenOfType(ASTSwitchLabel.class));
+        for (ASTSwitchLabeledBlock labeled : findChildrenOfType(ASTSwitchLabeledBlock.class)) {
+            result.add((ASTSwitchLabel) labeled.getChild(0));
+        }
+        for (ASTSwitchLabeledExpression labeled : findChildrenOfType(ASTSwitchLabeledExpression.class)) {
+            result.add((ASTSwitchLabel) labeled.getChild(0));
+        }
+        for (ASTSwitchLabeledThrowStatement labeled : findChildrenOfType(ASTSwitchLabeledThrowStatement.class)) {
+            result.add((ASTSwitchLabel) labeled.getChild(0));
+        }
+        return result.iterator();
     }
 }
diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatementTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatementTest.java
@@ -17,5 +17,52 @@ public void exhaustiveEnumSwitchWithDefault() {
                 .get(0);
         Assert.assertFalse(switchStatement.isExhaustiveEnumSwitch()); // this should not throw a NPE...
         Assert.assertTrue(switchStatement.hasDefaultCase());
+        Assert.assertTrue(switchStatement.isFallthroughSwitch());
+    }
+
+    @Test
+    public void defaultCaseWithArrowBlock() {
+        ASTSwitchStatement switchStatement = java.parse(
+                "class Foo { void bar(int x) {"
+                + "switch (x) { default -> { } } } }")
+                .getFirstDescendantOfType(ASTSwitchStatement.class);
+        Assert.assertFalse(switchStatement.isExhaustiveEnumSwitch());
+        Assert.assertTrue(switchStatement.iterator().hasNext());
+        Assert.assertTrue(switchStatement.hasDefaultCase());
+        Assert.assertFalse(switchStatement.isFallthroughSwitch());
+    }
+
+    @Test
+    public void emptySwitch() {
+        ASTSwitchStatement switchStatement = java.parse(
+                "class Foo { void bar(int x) {"
+                + "switch (x) { } } }")
+                .getFirstDescendantOfType(ASTSwitchStatement.class);
+        Assert.assertFalse(switchStatement.isExhaustiveEnumSwitch());
+        Assert.assertFalse(switchStatement.iterator().hasNext());
+        Assert.assertFalse(switchStatement.hasDefaultCase());
+        Assert.assertFalse(switchStatement.isFallthroughSwitch());
+    }
+
+    @Test
+    public void defaultCaseWithArrowExprs() {
+        ASTSwitchStatement switchStatement =
+            java.parse(
+                    "import net.sourceforge.pmd.lang.java.rule.bestpractices.switchstmtsshouldhavedefault.SimpleEnum;\n"
+                        + "\n"
+                        + "            public class Foo {\n"
+                        + "                void bar(SimpleEnum x) {\n"
+                        + "                    switch (x) {\n"
+                        + "                    case FOO -> System.out.println(\"it is on\");\n"
+                        + "                    case BAR -> System.out.println(\"it is off\");\n"
+                        + "                    default -> System.out.println(\"it is neither on nor off - should not happen? maybe null?\");\n"
+                        + "                    }\n"
+                        + "                }\n"
+                        + "            }")
+                .getFirstDescendantOfType(ASTSwitchStatement.class);
+        Assert.assertFalse(switchStatement.isExhaustiveEnumSwitch());
+        Assert.assertTrue(switchStatement.iterator().hasNext());
+        Assert.assertFalse(switchStatement.isFallthroughSwitch());
+        Assert.assertTrue(switchStatement.hasDefaultCase());
     }
 }
diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/AllJavaAstTreeDumpTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/AllJavaAstTreeDumpTest.java
@@ -0,0 +1,22 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.ast;
+
+import org.junit.runner.RunWith;
+import org.junit.runners.Suite;
+import org.junit.runners.Suite.SuiteClasses;
+
+@RunWith(Suite.class)
+@SuiteClasses({
+    ParserCornersTest.class,
+    Java15TreeDumpTest.class,
+    Java16PreviewTreeDumpTest.class,
+    Java16TreeDumpTest.class,
+    Java17PreviewTreeDumpTest.class,
+    Java17TreeDumpTest.class
+})
+public class AllJavaAstTreeDumpTest {
+
+}
diff --git a/...t/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java17p/DealingWithNull.txt b/...t/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java17p/DealingWithNull.txt
@@ -15,7 +15,7 @@
             |     +- Block[@containsComment = false]
             |        +- BlockStatement[@Allocation = false]
             |           +- Statement[]
-            |              +- SwitchStatement[@DefaultCase = false, @ExhaustiveEnumSwitch = false]
+            |              +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = false]
             |                 +- Expression[@StandAlonePrimitive = false]
             |                 |  +- PrimaryExpression[]
             |                 |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]
@@ -82,7 +82,7 @@
             |     +- Block[@containsComment = false]
             |        +- BlockStatement[@Allocation = true]
             |           +- Statement[]
-            |              +- SwitchStatement[@DefaultCase = false, @ExhaustiveEnumSwitch = false]
+            |              +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = false]
             |                 +- Expression[@StandAlonePrimitive = false]
             |                 |  +- PrimaryExpression[]
             |                 |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]
@@ -167,7 +167,7 @@
             |     +- Block[@containsComment = false]
             |        +- BlockStatement[@Allocation = false]
             |        |  +- Statement[]
-            |        |     +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false]
+            |        |     +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = true]
             |        |        +- Expression[@StandAlonePrimitive = false]
             |        |        |  +- PrimaryExpression[]
             |        |        |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]
@@ -219,7 +219,7 @@
             |        |              +- BreakStatement[]
             |        +- BlockStatement[@Allocation = false]
             |        |  +- Statement[]
-            |        |     +- SwitchStatement[@DefaultCase = false, @ExhaustiveEnumSwitch = false]
+            |        |     +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = false]
             |        |        +- Expression[@StandAlonePrimitive = false]
             |        |        |  +- PrimaryExpression[]
             |        |        |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]
@@ -262,7 +262,7 @@
             |        |                                   +- Literal[@CharLiteral = false, @DoubleLiteral = false, @EscapedStringLiteral = ""default case"", @FloatLiteral = false, @Image = ""default case"", @IntLiteral = false, @LongLiteral = false, @SingleCharacterStringLiteral = false, @StringLiteral = true, @TextBlock = false, @TextBlockContent = ""default case"", @ValueAsDouble = NaN, @ValueAsFloat = NaN, @ValueAsInt = 0, @ValueAsLong = 0]
             |        +- BlockStatement[@Allocation = false]
             |        |  +- Statement[]
-            |        |     +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false]
+            |        |     +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = true]
             |        |        +- Expression[@StandAlonePrimitive = false]
             |        |        |  +- PrimaryExpression[]
             |        |        |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]
@@ -289,7 +289,7 @@
             |        |                                      +- Literal[@CharLiteral = false, @DoubleLiteral = false, @EscapedStringLiteral = ""The rest (including null)"", @FloatLiteral = false, @Image = ""The rest (including null)"", @IntLiteral = false, @LongLiteral = false, @SingleCharacterStringLiteral = false, @StringLiteral = true, @TextBlock = false, @TextBlockContent = ""The rest (including null)"", @ValueAsDouble = NaN, @ValueAsFloat = NaN, @ValueAsInt = 0, @ValueAsLong = 0]
             |        +- BlockStatement[@Allocation = false]
             |           +- Statement[]
-            |              +- SwitchStatement[@DefaultCase = false, @ExhaustiveEnumSwitch = false]
+            |              +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = false]
             |                 +- Expression[@StandAlonePrimitive = false]
             |                 |  +- PrimaryExpression[]
             |                 |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]

diff --git a/.../net/sourceforge/pmd/lang/java/ast/jdkversiontests/java17p/EnhancedTypeCheckingSwitch.txt b/.../net/sourceforge/pmd/lang/java/ast/jdkversiontests/java17p/EnhancedTypeCheckingSwitch.txt
@@ -15,7 +15,7 @@
    |        |     +- Block[@containsComment = false]
    |        |        +- BlockStatement[@Allocation = false]
    |        |           +- Statement[]
-   |        |              +- SwitchStatement[@DefaultCase = false, @ExhaustiveEnumSwitch = false]
+   |        |              +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = false]
    |        |                 +- Expression[@StandAlonePrimitive = false]
    |        |                 |  +- PrimaryExpression[]
    |        |                 |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]

diff --git a/...sourceforge/pmd/lang/java/ast/jdkversiontests/java17p/GuardedAndParenthesizedPatterns.txt b/...sourceforge/pmd/lang/java/ast/jdkversiontests/java17p/GuardedAndParenthesizedPatterns.txt
@@ -15,7 +15,7 @@
             |     +- Block[@containsComment = false]
             |        +- BlockStatement[@Allocation = false]
             |           +- Statement[]
-            |              +- SwitchStatement[@DefaultCase = false, @ExhaustiveEnumSwitch = false]
+            |              +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = false]
             |                 +- Expression[@StandAlonePrimitive = false]
             |                 |  +- PrimaryExpression[]
             |                 |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]

diff --git a/...rceforge/pmd/lang/java/ast/jdkversiontests/java17p/ScopeOfPatternVariableDeclarations.txt b/...rceforge/pmd/lang/java/ast/jdkversiontests/java17p/ScopeOfPatternVariableDeclarations.txt
@@ -15,7 +15,7 @@
             |     +- Block[@containsComment = false]
             |        +- BlockStatement[@Allocation = true]
             |           +- Statement[]
-            |              +- SwitchStatement[@DefaultCase = false, @ExhaustiveEnumSwitch = false]
+            |              +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = false]
             |                 +- Expression[@StandAlonePrimitive = false]
             |                 |  +- PrimaryExpression[]
             |                 |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]
@@ -113,7 +113,7 @@
             |     +- Block[@containsComment = false]
             |        +- BlockStatement[@Allocation = false]
             |           +- Statement[]
-            |              +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false]
+            |              +- SwitchStatement[@DefaultCase = true, @ExhaustiveEnumSwitch = false, @FallthroughSwitch = true]
             |                 +- Expression[@StandAlonePrimitive = false]
             |                 |  +- PrimaryExpression[]
             |                 |     +- PrimaryPrefix[@SuperModifier = false, @ThisModifier = false]

diff --git a/...ces/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/SwitchStmtsShouldHaveDefault.xml b/...ces/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/SwitchStmtsShouldHaveDefault.xml
@@ -79,6 +79,88 @@ public class Foo {
         }
     }
 }
-        ]]></code>
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>#3605 switch on enum with default</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            import net.sourceforge.pmd.lang.java.rule.bestpractices.switchstmtsshouldhavedefault.SimpleEnum;
+
+            public class Foo {
+
+                void bar(SimpleEnum x) {
+                    switch (x) {
+                    case BZAZ:
+                        int y = 8;
+                        break;
+                    case FOO:
+                        break;
+                    case BAR:
+                        int w = 8;
+                        break;
+                    default:
+                        break;
+                    }
+                }
+            }
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>#3605 switch on enum with default, nonexhaustive</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            import net.sourceforge.pmd.lang.java.rule.bestpractices.switchstmtsshouldhavedefault.SimpleEnum;
+
+            public class Foo {
+
+                void bar(SimpleEnum x) {
+                    switch (x) {
+                    case BZAZ:
+                        int y = 8;
+                        break;
+                    case FOO:
+                        break;
+                    default:
+                        break;
+                    }
+                }
+            }
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>#3605 switch on enum with default, nonexhaustive, arrow</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            import net.sourceforge.pmd.lang.java.rule.bestpractices.switchstmtsshouldhavedefault.SimpleEnum;
+
+            public class Foo {
+                void bar(SimpleEnum x) {
+                    switch (x) {
+                    case FOO -> System.out.println("it is on");
+                    case BAR -> System.out.println("it is off");
+                    default -> System.out.println("it is neither on nor off - should not happen? maybe null?");
+                    }
+                }
+            }
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>#3605 switch on enum with default, exhaustive, arrow</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            import net.sourceforge.pmd.lang.java.rule.bestpractices.switchstmtsshouldhavedefault.SimpleEnum;
+
+            public class Foo {
+                void bar(SimpleEnum x) {
+                    switch (x) {
+                    case FOO -> System.out.println("it is on");
+                    case BAR -> System.out.println("it is off");
+                    case BZAZ -> System.out.println("it is bzaz");
+                    default -> System.out.println("it is neither on nor off - should not happen? maybe null?");
+                    }
+                }
+            }
+            ]]></code>
     </test-code>
 </test-data>