Merge branch 'issue-282'

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UnnecessaryLocalBeforeReturnRule.java

@@ -14,10 +14,13 @@ import net.sourceforge.pmd.lang.java.ast.ASTName;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
 import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer;
 import net.sourceforge.pmd.lang.java.ast.AccessNode;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration;
 import net.sourceforge.pmd.lang.symboltable.NameOccurrence;
+import net.sourceforge.pmd.lang.symboltable.Scope;
+import net.sourceforge.pmd.lang.symboltable.ScopedNode;
 
 public class UnnecessaryLocalBeforeReturnRule extends AbstractJavaRule {
 
@@ -58,13 +61,56 @@ public class UnnecessaryLocalBeforeReturnRule extends AbstractJavaRule {
                     if (var.indexOf('.') != -1) {
                         var = var.substring(0, var.indexOf('.'));
                     }
-                    addViolation(data, rtn, var);
+                    // Is the variable initialized with another member that is later used?
+                    if (!isInitDataModifiedAfterInit(variableDeclaration, rtn)) {
+                        addViolation(data, rtn, var);
+                    }
                 }
             }
         }
         return data;
     }
 
+    private boolean isInitDataModifiedAfterInit(final VariableNameDeclaration variableDeclaration,
+            final ASTReturnStatement rtn) {
+        final ASTVariableInitializer initializer = variableDeclaration.getAccessNodeParent()
+                .getFirstDescendantOfType(ASTVariableInitializer.class);
+        if (initializer != null) {
+            final List<ASTName> referencedNames = initializer.findDescendantsOfType(ASTName.class);
+            for (final ASTName refName : referencedNames) {
+                // TODO : Shouldn't the scope allow us to search for a var name occurrences directly, moving up through parent scopes?
+                Scope scope = refName.getScope();
+                do {
+                    final Map<VariableNameDeclaration, List<NameOccurrence>> declarations = scope
+                            .getDeclarations(VariableNameDeclaration.class);
+                    for (final Map.Entry<VariableNameDeclaration, List<NameOccurrence>> entry : declarations
+                            .entrySet()) {
+                        if (entry.getKey().getName().equals(refName.getImage())) {
+                            // Variable found! Check usage locations
+                            for (final NameOccurrence occ : entry.getValue()) {
+                                final ScopedNode location = occ.getLocation();
+                                // Is it used after initializing our "unnecessary" local but before the return statement?
+                                // TODO : should node define isAfter / isBefore helper methods?
+                                if ((location.getBeginLine() > initializer.getEndLine()
+                                        || (location.getBeginLine() == initializer.getEndLine() && location.getBeginColumn() >= initializer.getEndColumn()))
+                                        && (location.getEndLine() < rtn.getBeginLine()
+                                                || (location.getEndLine() == rtn.getBeginLine()
+                                                        && location.getEndColumn() <= rtn.getEndColumn()))) {
+                                    return true;
+                                }
+                            }
+
+                            return false;
+                        }
+                    }
+                    scope = scope.getParent();
+                } while (scope != null);
+            }
+        }
+
+        return false;
+    }
+
     private boolean isNotAnnotated(VariableNameDeclaration variableDeclaration) {
         AccessNode accessNodeParent = variableDeclaration.getAccessNodeParent();
         return !accessNodeParent.hasDescendantOfType(ASTAnnotation.class);

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UnnecessaryLocalBeforeReturn.xml

@@ -143,6 +143,27 @@ public class UnnecessaryLocalBeforeReturnCase {
         T result = (T) criteria.uniqueResult();
         return result;
     }
+}
+        ]]></code>
+    </test-code>
+    
+    <test-code>
+        <description>#282 UnnecessaryLocalBeforeReturn false positive when cloning Maps</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class CustomerErrorCollector {
+
+    private final ConcurrentHashMap<String, String> customerErrors = new ConcurrentHashMap<>();
+
+    public void error(String customerNr, String errorMsg) {
+        customerErrors.put(customerNr, errorMsg);
+    }
+
+    public Map<String, String> getAndReset() {
+        final Map<String, String> copy = new HashMap<>(customerErrors);
+        customerErrors.clear();
+        return copy; // PMD complains that variable could be avoided
+    }
 }
         ]]></code>
     </test-code>

src/site/markdown/overview/changelog.md

@@ -450,6 +450,7 @@ For example:
     *   [#240](https://github.com/pmd/pmd/issues/240): \[java] UnnecessaryLocalBeforeReturn: Enhance by checking usages
     *   [#274](https://github.com/pmd/pmd/issues/274): \[java] AccessorMethodGeneration: Method inside static inner class incorrectly reported
     *   [#275](https://github.com/pmd/pmd/issues/275): \[java] FinalFieldCouldBeStatic: Constant in @interface incorrectly reported as "could be made static"
+    *   [#282](https://github.com/pmd/pmd/issues/282): \[java] UnnecessaryLocalBeforeReturn false positive when cloning Maps
     *   [#291](https://github.com/pmd/pmd/issues/291): \[java] Improve quality of AccessorClassGeneration
 *   java-imports
     *   [#1546](https://sourceforge.net/p/pmd/bugs/1546/): \[java] UnnecessaryFullyQualifiedNameRule doesn't take into consideration conflict resolution