From 4137df4309187da0fa0e7f15dfbe1dca4da07f67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= <jmsotuyo@monits.com>
Date: Wed, 8 Feb 2017 12:01:59 -0300
Subject: [PATCH] [java] UnnecessaryLocalBeforeReturn checks usages

 - We now check symbol table usages to detect violations.
 - We drop the consecutive lines requirement.
 - Resolves #240
---
 .../UnnecessaryLocalBeforeReturnRule.java     | 54 ++++---------------
 .../xml/UnnecessaryLocalBeforeReturn.xml      | 32 +++++++++++
 2 files changed, 41 insertions(+), 45 deletions(-)

diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UnnecessaryLocalBeforeReturnRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UnnecessaryLocalBeforeReturnRule.java
index 855e19ff0b..087e281d06 100644
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UnnecessaryLocalBeforeReturnRule.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UnnecessaryLocalBeforeReturnRule.java
@@ -6,9 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.design;
 import java.util.List;
 import java.util.Map;
 
-import net.sourceforge.pmd.lang.ast.Node;
-import net.sourceforge.pmd.lang.java.ast.ASTAssertStatement;
-import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
@@ -43,26 +40,20 @@ public class UnnecessaryLocalBeforeReturnRule extends AbstractJavaRule {
             return data;
         }
 
-        Map<VariableNameDeclaration, List<NameOccurrence>> vars = name.getScope().getDeclarations(VariableNameDeclaration.class);
-        for (Map.Entry<VariableNameDeclaration, List<NameOccurrence>> entry: vars.entrySet()) {
-            VariableNameDeclaration key = entry.getKey();
+        Map<VariableNameDeclaration, List<NameOccurrence>> vars = name.getScope()
+                .getDeclarations(VariableNameDeclaration.class);
+        for (Map.Entry<VariableNameDeclaration, List<NameOccurrence>> entry : vars.entrySet()) {
             List<NameOccurrence> usages = entry.getValue();
 
-            // skip, if there is an assert between declaration and return
-            if (hasAssertStatement(key, rtn)) {
-                continue;
-            }
+            if (usages.size() == 1) { // If there is more than 1 usage, then it's not only returned
+                NameOccurrence occ = usages.get(0);
 
-            for (NameOccurrence occ: usages) {
                 if (occ.getLocation().equals(name)) {
-                    // only check declarations that occur one line earlier
-                    if (key.getNode().getBeginLine() == name.getBeginLine() - 1) {
-                        String var = name.getImage();
-                        if (var.indexOf('.') != -1) {
-                            var = var.substring(0, var.indexOf('.'));
-                        }
-                        addViolation(data, rtn, var);
+                    String var = name.getImage();
+                    if (var.indexOf('.') != -1) {
+                        var = var.substring(0, var.indexOf('.'));
                     }
+                    addViolation(data, rtn, var);
                 }
             }
         }
@@ -85,31 +76,4 @@ public class UnnecessaryLocalBeforeReturnRule extends AbstractJavaRule {
       }
       return false;
     }
-
-    /**
-     * Checks whether there is an assert statement between the variable declaration
-     * and the return statement, that uses the variable.
-     * @param variableDeclaration
-     * @param rtn
-     * @return
-     */
-    private boolean hasAssertStatement(VariableNameDeclaration variableDeclaration, ASTReturnStatement rtn) {
-        ASTBlockStatement blockStatement = variableDeclaration.getAccessNodeParent().getFirstParentOfType(ASTBlockStatement.class);
-        int startIndex = blockStatement.jjtGetChildIndex() + 1;
-        int endIndex = rtn.getFirstParentOfType(ASTBlockStatement.class).jjtGetChildIndex();
-        Node block = blockStatement.jjtGetParent();
-        for (int i = startIndex; i < endIndex; i++) {
-            List<ASTAssertStatement> asserts = block.jjtGetChild(i).findDescendantsOfType(ASTAssertStatement.class);
-            for (ASTAssertStatement assertStatement : asserts) {
-                List<ASTName> names = assertStatement.findDescendantsOfType(ASTName.class);
-                for (ASTName n : names) {
-                    if (n.hasImageEqualTo(variableDeclaration.getName())) {
-                        return true;
-                    }
-                }
-            }
-        }
-
-        return false;
-    }
 }
diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UnnecessaryLocalBeforeReturn.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UnnecessaryLocalBeforeReturn.xml
index cd473e0759..d76d1eb0c7 100644
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UnnecessaryLocalBeforeReturn.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UnnecessaryLocalBeforeReturn.xml
@@ -91,6 +91,38 @@ public class Foo {
                 return i;
         }
     }
+}
+        ]]></code>
+    </test-code>
+    
+    <test-code>
+        <description>Detect violation even if not on consecutive lines</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    public int bar() {
+        int res = 2;
+        doSomething();
+        return res;
+    }
+    
+    public void doSomething() { }
+}
+        ]]></code>
+    </test-code>
+    
+    <test-code>
+        <description>No violations on multiple uses of the variable</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    public int bar() {
+        int res = 2;
+        doSomething(res);
+        return res;
+    }
+    
+    public void doSomething(int x) { }
 }
         ]]></code>
     </test-code>