Merge pull request #3195 from oowekyala:update-UnnecessaryReturn

[java] Improve rule UnnecessaryReturn to detect more cases #3195

.ci/files/all-java.xml

@@ -110,7 +110,7 @@
     <rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>
     <rule ref="category/java/codestyle.xml/UnnecessaryLocalBeforeReturn"/>
     <rule ref="category/java/codestyle.xml/UnnecessaryModifier"/>
-    <!-- <rule ref="category/java/codestyle.xml/UnnecessaryReturn"/> -->
+    <rule ref="category/java/codestyle.xml/UnnecessaryReturn"/>
     <!-- <rule ref="category/java/codestyle.xml/UseDiamondOperator"/> -->
     <rule ref="category/java/codestyle.xml/UseShortArrayInitializer"/>
     <rule ref="category/java/codestyle.xml/UseUnderscoresInNumericLiterals"/>

docs/pages/7_0_0_release_notes.md

@@ -151,6 +151,7 @@ The following previously deprecated rules have been finally removed:
     * [#2299](https://github.com/pmd/pmd/issues/2299): \[java] UnnecessaryFullyQualifiedName false positive with similar package name
     * [#2528](https://github.com/pmd/pmd/issues/2528): \[java] MethodNamingConventions - JUnit 5 method naming not support ParameterizedTest
     * [#2739](https://github.com/pmd/pmd/issues/2739): \[java] UselessParentheses false positive for string concatenation
+    * [#3195](https://github.com/pmd/pmd/pull/3195): \[java] Improve rule UnnecessaryReturn to detect more cases
 * java-errorprone
     * [#1005](https://github.com/pmd/pmd/issues/1005): \[java] CloneMethodMustImplementCloneable triggers for interfaces
     * [#2532](https://github.com/pmd/pmd/issues/2532): \[java] AvoidDecimalLiteralsInBigDecimalConstructor can not detect the case new BigDecimal(Expression)

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryReturnRule.java

@@ -1,34 +1,86 @@
-/**
+/*
  * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
  */
 
 package net.sourceforge.pmd.lang.java.rule.codestyle;
 
-import net.sourceforge.pmd.lang.java.ast.ASTBlock;
-import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
-import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.ast.NodeStream;
+import net.sourceforge.pmd.lang.java.ast.ASTCompactConstructorDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
+import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTLoopStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTStatement;
-import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+import net.sourceforge.pmd.lang.java.ast.ASTSwitchArrowBranch;
+import net.sourceforge.pmd.lang.java.ast.ASTSwitchBranch;
+import net.sourceforge.pmd.lang.java.ast.ASTSwitchExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTSwitchFallthroughBranch;
+import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
+import net.sourceforge.pmd.lang.java.ast.JavaNode;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
+import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 
-public class UnnecessaryReturnRule extends AbstractJavaRule {
+public class UnnecessaryReturnRule extends AbstractJavaRulechainRule {
 
-    @Override
-    public Object visit(ASTMethodDeclaration node, Object data) {
-
-        if (node.isVoid()) {
-            super.visit(node, data);
-        }
-        return data;
+    public UnnecessaryReturnRule() {
+        super(ASTReturnStatement.class);
     }
 
     @Override
     public Object visit(ASTReturnStatement node, Object data) {
-        if (node.getParent() instanceof ASTStatement && node.getNthParent(2) instanceof ASTBlockStatement
-                && node.getNthParent(3) instanceof ASTBlock && node.getNthParent(4) instanceof ASTMethodDeclaration) {
+        if (node.getNumChildren() > 0) {
+            return null;
+        }
+        NodeStream<ASTStatement> enclosingStatements =
+            node.ancestorsOrSelf()
+                .takeWhile(it -> !isCfgLimit(it))
+                .filterIs(ASTStatement.class);
+
+        if (enclosingStatements.all(UnnecessaryReturnRule::isLastStatementOfParent)) {
             addViolation(data, node);
         }
-        return data;
+        return null;
+    }
+
+    private boolean isCfgLimit(JavaNode it) {
+        return it instanceof ASTMethodOrConstructorDeclaration
+            || it instanceof ASTCompactConstructorDeclaration
+            || it instanceof ASTInitializer
+            || it instanceof ASTLambdaExpression;
+    }
+
+    /**
+     * Returns true if this is the last statement of the parent node,
+     * ie the next statement to be executed is after the parent in the
+     * CFG.
+     */
+    private static boolean isLastStatementOfParent(ASTStatement it) {
+        // Note that local class declaration statements could be ignored
+        // because they don't contribute anything to control flow. But this
+        // is rare enough that this has not been implemented. A corresponding
+        // test is in the test file.
+
+        JavaNode parent = it.getParent();
+        if (JavaRuleUtil.isLastChild(it)) {
+            if (parent instanceof ASTSwitchArrowBranch) {
+                return !isBranchOfSwitchExpr((ASTSwitchBranch) parent);
+            } else if (parent instanceof ASTSwitchFallthroughBranch) {
+                return JavaRuleUtil.isLastChild(parent) && !isBranchOfSwitchExpr((ASTSwitchBranch) parent);
+            } else {
+                return !(parent instanceof ASTLoopStatement); // returns break the loop so are not unnecessary (though it could be replaced by break)
+            }
+        }
+
+        // so we're not the last child...
+        return parent instanceof ASTIfStatement  // maybe we're before the else clause
+            || parent instanceof ASTTryStatement; // maybe we're the body of a try
+            // also maybe we're the body of a do/while, but that is a loop, so it's necessary
+    }
+
+    private static boolean isBranchOfSwitchExpr(ASTSwitchBranch branch) {
+        return branch.getParent() instanceof ASTSwitchExpression;
     }
 
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java

@@ -23,6 +23,7 @@ import org.checkerframework.checker.nullness.qual.NonNull;
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 import net.sourceforge.pmd.lang.ast.GenericToken;
+import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.ast.NodeStream;
 import net.sourceforge.pmd.lang.ast.impl.javacc.JavaccToken;
 import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration;
@@ -737,4 +738,12 @@ public final class JavaRuleUtil {
     private static boolean isStringConcatExpression(ASTExpression e) {
         return BinaryOp.isInfixExprWithOperator(e, BinaryOp.ADD) && TypeTestUtil.isA(String.class, e);
     }
+
+    /**
+     * Returns true if the node is the last child of its parent (or is the root node).
+     */
+    public static boolean isLastChild(Node it) {
+        Node parent = it.getParent();
+        return parent == null || it.getIndexInParent() == parent.getNumChildren() - 1;
+    }
 }

pmd-java/src/main/resources/category/java/codestyle.xml

@@ -1624,11 +1624,12 @@ public class Bar {
     <rule name="UnnecessaryReturn"
           language="java"
           since="1.3"
-          message="Avoid unnecessary return statements"
+          message="Unnecessary return statement"
           class="net.sourceforge.pmd.lang.java.rule.codestyle.UnnecessaryReturnRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_codestyle.html#unnecessaryreturn">
         <description>
-Avoid the use of unnecessary return statements.
+Avoid the use of unnecessary return statements. A return is unnecessary when no
+instructions follow anyway.
         </description>
         <priority>3</priority>
         <example>

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryReturnTest.java

@@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.codestyle;
 
 import net.sourceforge.pmd.testframework.PmdRuleTst;
 
-@org.junit.Ignore("Rule has not been updated yet")
 public class UnnecessaryReturnTest extends PmdRuleTst {
     // no additional unit tests
 }