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 b2607c073f..251b4aa302 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 @@ -13,10 +13,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 { @@ -56,13 +59,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 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> declarations = scope + .getDeclarations(VariableNameDeclaration.class); + for (final Map.Entry> 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); 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 4ebff8d465..94b05c57f9 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 @@ -143,6 +143,27 @@ public class UnnecessaryLocalBeforeReturnCase { T result = (T) criteria.uniqueResult(); return result; } +} + ]]> + + + + #282 UnnecessaryLocalBeforeReturn false positive when cloning Maps + 0 + customerErrors = new ConcurrentHashMap<>(); + + public void error(String customerNr, String errorMsg) { + customerErrors.put(customerNr, errorMsg); + } + + public Map getAndReset() { + final Map copy = new HashMap<>(customerErrors); + customerErrors.clear(); + return copy; // PMD complains that variable could be avoided + } } ]]> diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 067ab6a92f..3570cf1a63 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -19,6 +19,7 @@ The PMD team is pleased to announce PMD 5.5.5. * java-design * [#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 ### API Changes