Merge pull request #3674 from adangel:issue-3639-usestringbufferlength

[java] UseStringBufferLength: false negative with empty string variable
#3674

* pr-3674:
  [java] UseStringBufferLength: false negative with empty string
variable

docs/pages/release_notes.md

@@ -54,6 +54,7 @@ not support all features of the latest EcmaScript standard.
     *   [#3701](https://github.com/pmd/pmd/issues/3701): \[java] MissingStaticMethodInNonInstantiatableClass false positive with method inner classes
 *   java-performance
     *   [#3492](https://github.com/pmd/pmd/issues/3492): \[java] UselessStringValueOf: False positive when there is no initial String to append to
+    *   [#3639](https://github.com/pmd/pmd/issues/3639): \[java] UseStringBufferLength: false negative with empty string variable
     *   [#3712](https://github.com/pmd/pmd/issues/3712): \[java] InsufficientStringBufferDeclaration false positive with StringBuilder.setLength(0)
 *   javascript
     *   [#3703](https://github.com/pmd/pmd/issues/3703): \[javascript] Error - no Node adapter class registered for XmlPropRef

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/UseStringBufferLengthRule.java

@@ -4,15 +4,19 @@
 
 package net.sourceforge.pmd.lang.java.rule.performance;
 
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
 import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
+import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration;
 import net.sourceforge.pmd.lang.symboltable.NameDeclaration;
@@ -56,15 +60,15 @@ public class UseStringBufferLengthRule extends AbstractJavaRule {
     @Override
     public Object visit(ASTName decl, Object data) {
         if (!decl.getImage().endsWith("toString")) {
-            return data;
+            return super.visit(decl, data);
         }
         NameDeclaration nd = decl.getNameDeclaration();
         if (nd == null) {
-            return data;
+            return super.visit(decl, data);
         }
         if (alreadySeen.contains(nd) || !(nd instanceof VariableNameDeclaration)
                 || !ConsecutiveLiteralAppendsRule.isStringBuilderOrBuffer(((VariableNameDeclaration) nd).getDeclaratorId())) {
-            return data;
+            return super.visit(decl, data);
         }
         alreadySeen.add(nd);
 
@@ -72,7 +76,7 @@ public class UseStringBufferLengthRule extends AbstractJavaRule {
             addViolation(data, decl);
         }
 
-        return data;
+        return super.visit(decl, data);
     }
 
     /**
@@ -112,16 +116,39 @@ public class UseStringBufferLengthRule extends AbstractJavaRule {
         // 3. child: equals
         if (parent.getChild(2).hasImageEqualTo("equals")) {
             // 4. child: the arguments of equals, there must be exactly one and
-            // it must be ""
+            // it must be "" or a final variable initialized with ""
-            List<ASTArgumentList> argList = parent.getChild(3).findDescendantsOfType(ASTArgumentList.class);
+            Node primarySuffix = parent.getChild(3);
-            if (argList.size() == 1) {
+            List<ASTArgumentList> methodCalls = primarySuffix.findDescendantsOfType(ASTArgumentList.class);
-                List<ASTLiteral> literals = argList.get(0).findDescendantsOfType(ASTLiteral.class);
+            if (methodCalls.size() == 1 && methodCalls.get(0).size() == 1) {
+                ASTExpression firstArgument = primarySuffix.getChild(0).getFirstChildOfType(ASTArgumentList.class)
+                        .findChildrenOfType(ASTExpression.class).get(0);
+                List<ASTLiteral> literals = firstArgument.findDescendantsOfType(ASTLiteral.class);
+                if (literals.isEmpty()) {
+                    literals = findLiteralsInVariableInitializer(firstArgument);
+                }
                 return literals.size() == 1 && literals.get(0).hasImageEqualTo("\"\"");
             }
         }
         return false;
     }
 
+    private List<ASTLiteral> findLiteralsInVariableInitializer(ASTExpression firstArgument) {
+        List<ASTName> varAccess = firstArgument.findDescendantsOfType(ASTName.class);
+        if (varAccess.size() == 1) {
+            NameDeclaration nameDeclaration = varAccess.get(0).getNameDeclaration();
+            if (nameDeclaration != null && nameDeclaration.getNode() instanceof ASTVariableDeclaratorId) {
+                ASTVariableDeclaratorId varId = (ASTVariableDeclaratorId) nameDeclaration.getNode();
+                if (varId.isFinal() && varId.getParent() instanceof ASTVariableDeclarator) {
+                    ASTVariableDeclarator declarator = (ASTVariableDeclarator) varId.getParent();
+                    if (declarator.getInitializer() != null) {
+                        return declarator.getInitializer().findDescendantsOfType(ASTLiteral.class);
+                    }
+                }
+            }
+        }
+        return Collections.emptyList();
+    }
+
     private boolean isLengthViolation(Node parent) {
         // 3. child: length
         return parent.getChild(2).hasImageEqualTo("length");

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/UseStringBufferLength.xml

@@ -188,6 +188,28 @@ public class Ineffecient
     {
         return "Ineffecient";
     }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>[java] UseStringBufferLength: false negative with empty string variable #3639</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>5</expected-linenumbers>
+        <code><![CDATA[
+public class Foo {
+    public void foo() {
+        StringBuffer sb = new StringBuffer("any_string");
+        final String nullStr = "";
+        if (sb.toString().equals(nullStr)) { // PMD should report a warning here
+            System.out.println("Buffer is empty");
+        }
+
+        // the preferred way
+        if (sb.length() == 0) {
+            System.out.println("Buffer is empty");
+        }
+    }
 }
         ]]></code>
     </test-code>