diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 8bc04ca735..317f5e2205 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -121,6 +121,8 @@ This release ships with 3 new Java rules. * [#3248](https://github.com/pmd/pmd/issues/3248): \[java] Documentation is wrong for SingletonClassReturningNewInstance rule * [#3249](https://github.com/pmd/pmd/pull/3249): \[java] AvoidFieldNameMatchingTypeName: False negative with interfaces * [#3268](https://github.com/pmd/pmd/pull/3268): \[java] ConstructorCallsOverridableMethod: IndexOutOfBoundsException with annotations +* java-performance + * [#1438](https://github.com/pmd/pmd/issues/1438): \[java] InsufficientStringBufferDeclaration false positive for initial calculated StringBuilder size * javascript * [#699](https://github.com/pmd/pmd/issues/699): \[javascript] Update Rhino library to 1.7.13 * [#2081](https://github.com/pmd/pmd/issues/2081): \[javascript] Failing with OutOfMemoryError parsing a Javascript file diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InsufficientStringBufferDeclarationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InsufficientStringBufferDeclarationRule.java index e2fe4e7bd5..821cd700eb 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InsufficientStringBufferDeclarationRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InsufficientStringBufferDeclarationRule.java @@ -10,11 +10,16 @@ import java.util.List; import java.util.Map; import java.util.Set; +import org.apache.commons.lang3.mutable.MutableInt; + import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.java.ast.ASTAdditiveExpression; import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; +import net.sourceforge.pmd.lang.java.ast.ASTArgumentList; +import net.sourceforge.pmd.lang.java.ast.ASTArguments; import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.java.ast.ASTCastExpression; +import net.sourceforge.pmd.lang.java.ast.ASTExpression; import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter; import net.sourceforge.pmd.lang.java.ast.ASTIfStatement; @@ -28,9 +33,9 @@ import net.sourceforge.pmd.lang.java.ast.ASTSwitchLabel; import net.sourceforge.pmd.lang.java.ast.ASTSwitchLabeledBlock; import net.sourceforge.pmd.lang.java.ast.ASTSwitchLabeledExpression; import net.sourceforge.pmd.lang.java.ast.ASTSwitchStatement; -import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; -import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer; +import net.sourceforge.pmd.lang.java.ast.JavaNode; +import net.sourceforge.pmd.lang.java.ast.JavaParserVisitorAdapter; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.symboltable.JavaNameOccurrence; import net.sourceforge.pmd.lang.symboltable.NameOccurrence; @@ -72,7 +77,9 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRule { constructorLength = getConstructorLength(node, constructorLength); anticipatedLength = getInitialLength(node); - + if (anticipatedLength > 0) { + constructorLength = anticipatedLength + DEFAULT_BUFFER_SIZE; + } anticipatedLength += getConstructorAppendsLength(node); List usage = node.getUsages(); @@ -83,47 +90,85 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRule { if (!InefficientStringBufferingRule.isInStringBufferOperationChain(n, "append")) { if (!jno.isOnLeftHandSide() - && !InefficientStringBufferingRule.isInStringBufferOperationChain(n, "setLength")) { + && !(InefficientStringBufferingRule.isInStringBufferOperationChain(n, "setLength") + || InefficientStringBufferingRule.isInStringBufferOperationChain(n, "ensureCapacity"))) { continue; } + + if (n.getImage().endsWith("setLength")) { + int newLength = getConstructorLength(n, 0); + if (newLength > constructorLength) { + constructorLength = newLength; // a bigger setLength increases capacity + rootNode = n; + } + anticipatedLength = newLength; // setLength fills the string builder, any new append adds to this + } else if (n.getImage().endsWith("ensureCapacity")) { + int newCapacity = getConstructorLength(n, 0); + if (newCapacity > constructorLength) { // only a bigger new capacity changes the capacity + constructorLength = newCapacity; + rootNode = n; + } + } else { + // this is a constructor call. report possible violation for the old instance now + if (constructorLength != -1 && anticipatedLength > constructorLength) { + anticipatedLength += processBlocks(blocks); + reportViolation(data, node, rootNode, constructorLength, anticipatedLength); + } + + // new initial capacity + constructorLength = getConstructorLength(n, DEFAULT_BUFFER_SIZE); + rootNode = n; + anticipatedLength = getInitialLength(n); + if (anticipatedLength > 0) { + constructorLength = anticipatedLength + DEFAULT_BUFFER_SIZE; + } + anticipatedLength += getConstructorAppendsLength(n); + } + if (constructorLength != -1 && anticipatedLength > constructorLength) { anticipatedLength += processBlocks(blocks); - String[] param = { String.valueOf(constructorLength), String.valueOf(anticipatedLength) }; - addViolation(data, rootNode, param); + reportViolation(data, node, rootNode, constructorLength, anticipatedLength); } - constructorLength = getConstructorLength(n, constructorLength); - rootNode = n; - anticipatedLength = getInitialLength(node); - } - ASTPrimaryExpression s = n.getFirstParentOfType(ASTPrimaryExpression.class); - int numChildren = s.getNumChildren(); - for (int jx = 0; jx < numChildren; jx++) { - Node sn = s.getChild(jx); - if (!(sn instanceof ASTPrimarySuffix) || sn.getImage() != null) { - continue; - } - int thisSize = 0; - Node block = getFirstParentBlock(sn); - if (isAdditive(sn)) { - thisSize = processAdditive(sn); - } else { - thisSize = processNode(sn); - } - if (block != null) { - storeBlockStatistics(blocks, thisSize, block); - } else { - anticipatedLength += thisSize; + } else { + ASTPrimaryExpression s = n.getFirstParentOfType(ASTPrimaryExpression.class); + int numChildren = s.getNumChildren(); + for (int jx = 0; jx < numChildren; jx++) { + Node sn = s.getChild(jx); + if (!(sn instanceof ASTPrimarySuffix) || sn.getImage() != null) { + continue; + } + int thisSize = 0; + Node block = getFirstParentBlock(sn); + if (isAdditive(sn)) { + thisSize = processAdditive(sn); + } else { + thisSize = processNode(sn); + } + if (block != null) { + storeBlockStatistics(blocks, thisSize, block); + } else { + anticipatedLength += thisSize; + } } } } anticipatedLength += processBlocks(blocks); if (constructorLength != -1 && anticipatedLength > constructorLength) { - String[] param = { String.valueOf(constructorLength), String.valueOf(anticipatedLength) }; - addViolation(data, rootNode, param); + reportViolation(data, node, rootNode, constructorLength, anticipatedLength); } return data; } + private void reportViolation(Object data, ASTVariableDeclaratorId instance, Node reportNode, int capacity, + int anticipatedLength) { + String typeName = "StringBuilder"; + if (instance.getType() != null) { + typeName = instance.getType().getSimpleName(); + } + + addViolation(data, reportNode, new Object[] {typeName, capacity, anticipatedLength}); + } + /** * This rule is concerned with IF and Switch blocks. Process the block into * a local Map, from which we can later determine which is the longest block @@ -219,6 +264,8 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRule { // base 10 integer string: 3735928559 anticipatedLength += String.valueOf(literal.getValueAsLong()).length(); } + } else if (literal.isLongLiteral()) { + anticipatedLength += String.valueOf(literal.getValueAsLong()).length(); } else { anticipatedLength += str.length(); } @@ -245,48 +292,16 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRule { } } - // if there is any addition/subtraction going on then just use the - // default. - ASTAdditiveExpression exp = block.getFirstDescendantOfType(ASTAdditiveExpression.class); - if (exp != null) { - return DEFAULT_BUFFER_SIZE; - } - ASTMultiplicativeExpression mult = block.getFirstDescendantOfType(ASTMultiplicativeExpression.class); - if (mult != null) { - return DEFAULT_BUFFER_SIZE; - } - - List literals; - ASTAllocationExpression constructorCall = block.getFirstDescendantOfType(ASTAllocationExpression.class); - if (constructorCall != null) { - // if this is a constructor call, only consider the literals within - // it. - literals = constructorCall.findDescendantsOfType(ASTLiteral.class); - } else { - // otherwise it might be a setLength call... - literals = block.findDescendantsOfType(ASTLiteral.class); - } - if (literals.isEmpty()) { - List name = block.findDescendantsOfType(ASTName.class); + // argumentList can be from constructor call or setLength call + ASTArgumentList argumentList = block.getFirstDescendantOfType(ASTArgumentList.class); + if (argumentList != null) { + // if there are any method calls involved, we can't calculate the initial size + List name = argumentList.findDescendantsOfType(ASTName.class); if (!name.isEmpty()) { iConstructorLength = -1; + } else { + iConstructorLength = calculateExpression(argumentList); } - } else if (literals.size() == 1) { - ASTLiteral literal = literals.get(0); - String str = literal.getImage(); - if (str == null) { - iConstructorLength = 0; - } else if (isStringOrCharLiteral(literal)) { - // since it's not taken into account - // anywhere. only count the extra 16 - // characters - // don't add the constructor's length - iConstructorLength = 14 + str.length(); - } else if (literal.isIntLiteral()) { - iConstructorLength = literal.getValueAsInt(); - } - } else { - iConstructorLength = -1; } if (iConstructorLength == 0) { @@ -300,17 +315,74 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRule { return iConstructorLength; } + private int calculateExpression(ASTArgumentList argumentList) { + if (argumentList == null) { + return -1; + } + + ASTExpression expr = argumentList.getFirstChildOfType(ASTExpression.class); + if (expr == null) { + return -1; + } + + class ExpressionVisitor extends JavaParserVisitorAdapter { + @Override + public Object visit(ASTExpression node, Object data) { + node.getChild(0).jjtAccept(this, data); + return data; + } + + @Override + public Object visit(ASTAdditiveExpression node, Object data) { + MutableInt partSum = new MutableInt(0); + for (JavaNode child : node.children()) { + MutableInt part = new MutableInt(); + child.jjtAccept(this, part); + partSum.add(part.getValue()); + } + ((MutableInt) data).setValue(partSum.getValue()); + return data; + } + + @Override + public Object visit(ASTMultiplicativeExpression node, Object data) { + MutableInt partResult = new MutableInt(1); + for (JavaNode child : node.children()) { + MutableInt part = new MutableInt(0); + child.jjtAccept(this, part); + partResult.setValue(partResult.getValue() * part.getValue()); + } + ((MutableInt) data).setValue(partResult.getValue()); + return data; + } + + @Override + public Object visit(ASTLiteral node, Object data) { + ((MutableInt) data).setValue(node.getValueAsInt()); + return data; + } + } + + MutableInt result = new MutableInt(0); + expr.jjtAccept(new ExpressionVisitor(), result); + return result.getValue(); + } + private int getInitialLength(Node node) { - Node block = node.getFirstParentOfType(ASTBlockStatement.class); - if (block == null) { block = node.getFirstParentOfType(ASTFieldDeclaration.class); if (block == null) { block = node.getFirstParentOfType(ASTFormalParameter.class); } } - List literals = block.findDescendantsOfType(ASTLiteral.class); + + ASTAllocationExpression allocation = block.getFirstDescendantOfType(ASTAllocationExpression.class); + if (allocation == null) { + return 0; + } + + List literals = allocation.findDescendantsOfType(ASTLiteral.class); if (literals.size() == 1) { ASTLiteral literal = literals.get(0); String str = literal.getImage(); @@ -323,23 +395,24 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRule { } private int getConstructorAppendsLength(final Node node) { - final Node parent = node.getFirstParentOfType(ASTVariableDeclarator.class); - int size = 0; - if (parent != null) { - final Node initializer = parent.getFirstChildOfType(ASTVariableInitializer.class); - if (initializer != null) { - final Node primExp = initializer.getFirstDescendantOfType(ASTPrimaryExpression.class); - if (primExp != null) { - for (int i = 0; i < primExp.getNumChildren(); i++) { - final Node sn = primExp.getChild(i); - if (!(sn instanceof ASTPrimarySuffix) || sn.getImage() != null) { - continue; - } - size += processNode(sn); - } - } + Node block = node.getFirstParentOfType(ASTBlockStatement.class); + if (block == null) { + block = node.getFirstParentOfType(ASTFieldDeclaration.class); + if (block == null) { + block = node.getFirstParentOfType(ASTFormalParameter.class); } } + + int size = 0; + // these are constructor arguments and method arguments from all method calls + // but we want here only method calls, that are chained + List arguments = block.findDescendantsOfType(ASTArguments.class); + for (ASTArguments arg : arguments) { + if (arg.getParent() instanceof ASTAllocationExpression) { + continue; + } + size += processNode(arg); + } return size; } diff --git a/pmd-java/src/main/resources/category/java/performance.xml b/pmd-java/src/main/resources/category/java/performance.xml index f200437198..ce03878a6e 100644 --- a/pmd-java/src/main/resources/category/java/performance.xml +++ b/pmd-java/src/main/resources/category/java/performance.xml @@ -517,7 +517,7 @@ sb.append(System.getProperty("java.io.tmpdir")); @@ -530,10 +530,10 @@ is assumed if the length of the constructor can not be determined. 3 diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InsufficientStringBufferDeclaration.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InsufficientStringBufferDeclaration.xml index 39335c141a..c9a6913e37 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InsufficientStringBufferDeclaration.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InsufficientStringBufferDeclaration.xml @@ -9,7 +9,6 @@ 0 2, StringBuffer not allocated with enough space 2 + 3,10 + + StringBuffer has been initialized with size 16, but has at least 33 characters appended. + StringBuilder has been initialized with size 16, but has at least 33 characters appended. + 3, StringBuffer allocated with space 0 4, StringBuffer allocated from variable 0 5, creating a new StringBuffer 0 6, Initialize with a specific String 2 + 4,11 + + StringBuffer has been initialized with size 40, but has at least 52 characters appended. + StringBuilder has been initialized with size 40, but has at least 52 characters appended. + 0 9, Field level variable 2 + 2,10 + + StringBuffer has been initialized with size 16, but has at least 28 characters appended. + StringBuilder has been initialized with size 16, but has at least 28 characters appended. + 10, Appending non-literals 0 11, Initialized to null 0 13, compound append 2 + 3,7 + + StringBuffer has been initialized with size 16, but has at least 25 characters appended. + StringBuilder has been initialized with size 16, but has at least 25 characters appended. + 0 15, Append long, incorrect presize 2 + 3,7 + + StringBuffer has been initialized with size 16, but has at least 19 characters appended. + StringBuilder has been initialized with size 16, but has at least 19 characters appended. + @@ -319,7 +345,6 @@ public class Foo { 0 17, Append char, incorrect presize 2 + 3,9 + + StringBuffer has been initialized with size 2, but has at least 3 characters appended. + StringBuilder has been initialized with size 2, but has at least 3 characters appended. + 0 19, String concatenation, incorrect presize 2 + 3,7 + + StringBuffer has been initialized with size 16, but has at least 33 characters appended. + StringBuilder has been initialized with size 16, but has at least 33 characters appended. + 20, String concatenation with non-literal, incorrect presize 2 + 3,7 + + StringBuffer has been initialized with size 16, but has at least 32 characters appended. + StringBuilder has been initialized with size 16, but has at least 32 characters appended. + 21, Incorrectly presized twice 4 + 4,6,11,13 + + StringBuffer has been initialized with size 2, but has at least 5 characters appended. + StringBuffer has been initialized with size 5, but has at least 23 characters appended. + StringBuilder has been initialized with size 2, but has at least 5 characters appended. + StringBuilder has been initialized with size 5, but has at least 23 characters appended. + 22, appends inside if/else if/else statements 0 23, appends inside if/else if/else statements 0 24, appends inside if/else if/else statements 2 + 4,14 + + StringBuffer has been initialized with size 16, but has at least 46 characters appended. + StringBuilder has been initialized with size 16, but has at least 46 characters appended. + 25, Compound ifs 0 26, Compound if, pushed over the edge 2 + 4,19 + + StringBuffer has been initialized with size 16, but has at least 17 characters appended. + StringBuilder has been initialized with size 16, but has at least 17 characters appended. + 28, Compound if, pushed over the edge 2 + 3,13 + + StringBuffer has been initialized with size 16, but has at least 53 characters appended. + StringBuilder has been initialized with size 16, but has at least 53 characters appended. + 34, Uses setLength incorrectly 2 + 3,10 + + StringBuffer has been initialized with size 16, but has at least 17 characters appended. + StringBuilder has been initialized with size 16, but has at least 17 characters appended. + Append a hex int 2 + 5,9 + + StringBuffer has been initialized with size 16, but has at least 20 characters appended. + StringBuilder has been initialized with size 16, but has at least 20 characters appended. + #1371 InsufficientStringBufferDeclaration not detected properly on StringBuffer - 1 + 2 + 3,7 + + StringBuffer has been initialized with size 16, but has at least 41 characters appended. + StringBuffer has been initialized with size 16, but has at least 52 characters appended. + @@ -1034,8 +1116,9 @@ public class Test { [java] StringBuilder/Buffer false negatives with typeres #2881 1 + 6 - StringBuffer constructor is initialized with size 16, but has at least 17 characters appended. + StringBuffer has been initialized with size 16, but has at least 17 characters appended. [java] StringBuilder/Buffer false negatives with typeres #2881 (countertest, no classpath) 1 + 3 - StringBuffer constructor is initialized with size 16, but has at least 17 characters appended. + StringBuffer has been initialized with size 16, but has at least 17 characters appended. + + + + [java] InsufficientStringBufferDeclaration false positive for initial calculated StringBuilder size #1438 + 0 + + + + + Calculated initial size in constructor + 4 + 10,34,44,61 + + StringBuilder has been initialized with size 4, but has at least 5 characters appended. + StringBuilder has been initialized with size 5, but has at least 6 characters appended. + StringBuilder has been initialized with size 8, but has at least 10 characters appended. + StringBuilder has been initialized with size 8, but has at least 9 characters appended. + + insufficient capacity + return sb.toString(); + } + + public String case6_insufficient_setLength() { + StringBuilder sb = new StringBuilder(5); + sb.append("xx"); + sb.setLength(2 + 2 * 3); // line 44 - new length is 8, new capacity now 8 -> violation here + sb.append("aa"); // appending 2 chars -> insufficient capacity + return sb.toString(); + } + + public String case7_sufficient_ensureCapacity() { + StringBuilder sb = new StringBuilder(5); + sb.append("xx"); + sb.ensureCapacity(2 + 2 * 3); // length is still 2, new capacity now at least 8 + sb.append("aa"); // length is 4 + sb.append("bb"); // length is 6 + return sb.toString(); + } + + public String case8_insufficient_ensureCapacity() { + StringBuilder sb = new StringBuilder(5); + sb.append("xx"); + sb.ensureCapacity(2 + 2 * 3); // line 61 - length is still 2, new capacity now at least 8 -> violation here + sb.append("aa"); // length is 4 + sb.append("bb"); // length is 6 + sb.append("cc"); // length is 8 + sb.append('d'); // length is now 9 + return sb.toString(); + } +} + ]]> + + + + False positive with method formal parameter + 0 +