phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[java] Fix rules InefficientStringBuffering and refactor existing rules

Browse Source 
Rules affected additionally: AppendCharacterWithChar,
ConsecutiveLiteralAppends, InsufficientStringBufferDeclaration

Deprecate InefficientStringBuffering::isInStringBufferOperation
This commit is contained in:
Andreas Dangel
2020-06-14 12:50:02 +02:00
parent 67cca2a123
commit b8e9ff19b2
7 changed files with 167 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/AppendCharacterWithCharRule.java
View File

@ -4,6 +4,7 @@
package net.sourceforge.pmd.lang.java.rule.performance;
import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
@ -33,7 +34,7 @@ public class AppendCharacterWithCharRule extends AbstractJavaRule {
}
if (node.isSingleCharacterStringLiteral()) {
if (!InefficientStringBufferingRule.isInStringBufferOperation(node, 8, "append")) {
if (!InefficientStringBufferingRule.isInStringBufferOperationChain(node, "append")) {
return data;
}
@ -42,6 +43,10 @@ public class AppendCharacterWithCharRule extends AbstractJavaRule {
if (primaryExpression != null && primaryExpression.getFirstChildOfType(ASTPrimarySuffix.class) != null) {
return data;
}
// ignore, if this literal is part of a different expression, e.g. "X" + something else
if (primaryExpression != null && !(primaryExpression.getNthParent(2) instanceof ASTArgumentList)) {
return data;
}
addViolation(data, node);
}

4
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/ConsecutiveLiteralAppendsRule.java
View File

@ -21,6 +21,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTDoStatement;
import net.sourceforge.pmd.lang.java.ast.ASTFinallyStatement;
import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression;
import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTName;
@ -78,6 +79,7 @@ public class ConsecutiveLiteralAppendsRule extends AbstractJavaRule {
BLOCK_PARENTS.add(ASTMethodDeclaration.class);
BLOCK_PARENTS.add(ASTCatchStatement.class);
BLOCK_PARENTS.add(ASTFinallyStatement.class);
BLOCK_PARENTS.add(ASTLambdaExpression.class);
}
private static final PropertyDescriptor<Integer> THRESHOLD_DESCRIPTOR
@ -119,7 +121,7 @@ public class ConsecutiveLiteralAppendsRule extends AbstractJavaRule {
currentBlock = getFirstParentBlock(n);
if (InefficientStringBufferingRule.isInStringBufferOperation(n, 3, "append")) {
if (InefficientStringBufferingRule.isInStringBufferOperationChain(n, "append")) {
// append method call detected
ASTPrimaryExpression s = n.getFirstParentOfType(ASTPrimaryExpression.class);
int numChildren = s.getNumChildren();

110
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InefficientStringBufferingRule.java
View File

@ -4,6 +4,7 @@
package net.sourceforge.pmd.lang.java.rule.performance;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
@ -17,6 +18,9 @@ import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTName;
import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
import net.sourceforge.pmd.lang.java.ast.ASTPrimitiveType;
import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression;
import net.sourceforge.pmd.lang.java.ast.ASTType;
@ -58,17 +62,19 @@ public class InefficientStringBufferingRule extends AbstractJavaRule {
}
}
if (immediateLiterals > 1) {
boolean onlyStringLiterals = immediateLiterals == immediateStringLiterals
&& immediateLiterals == node.getNumChildren();
if (onlyStringLiterals) {
return data;
}
// if literal + public static final, return
// if literal + final, return
List<ASTName> nameNodes = node.findDescendantsOfType(ASTName.class);
for (ASTName name : nameNodes) {
if (name.getNameDeclaration() != null && name.getNameDeclaration() instanceof VariableNameDeclaration) {
VariableNameDeclaration vnd = (VariableNameDeclaration) name.getNameDeclaration();
AccessNode accessNodeParent = vnd.getAccessNodeParent();
if (accessNodeParent.isFinal() && accessNodeParent.isStatic()) {
if (accessNodeParent.isFinal()) {
return data;
}
}
@ -98,8 +104,10 @@ public class InefficientStringBufferingRule extends AbstractJavaRule {
if (isAllocatedStringBuffer(node)) {
addViolation(data, node);
} else if (isInStringBufferOperationChain(node, "append")) {
addViolation(data, node);
}
} else if (isInStringBufferOperation(node, 6, "append")) {
} else if (isInStringBufferOperationChain(node, "append")) {
addViolation(data, node);
}
return data;
@ -111,7 +119,7 @@ public class InefficientStringBufferingRule extends AbstractJavaRule {
List<ASTClassOrInterfaceType> types = type.findDescendantsOfType(ASTClassOrInterfaceType.class);
if (!types.isEmpty()) {
ASTClassOrInterfaceType typeDeclaration = types.get(0);
if (String.class == typeDeclaration.getType() || "String".equals(typeDeclaration.getImage())) {
if (TypeHelper.isA(typeDeclaration, String.class)) {
return true;
}
}
@ -125,19 +133,39 @@ public class InefficientStringBufferingRule extends AbstractJavaRule {
}
private ASTType getTypeNode(ASTName name) {
ASTType result = null;
if (name.getNameDeclaration() instanceof VariableNameDeclaration) {
VariableNameDeclaration vnd = (VariableNameDeclaration) name.getNameDeclaration();
if (vnd.getAccessNodeParent() instanceof ASTLocalVariableDeclaration) {
ASTLocalVariableDeclaration l = (ASTLocalVariableDeclaration) vnd.getAccessNodeParent();
return l.getTypeNode();
result = l.getTypeNode();
} else if (vnd.getAccessNodeParent() instanceof ASTFormalParameter) {
ASTFormalParameter p = (ASTFormalParameter) vnd.getAccessNodeParent();
return p.getTypeNode();
result = p.getTypeNode();
}
}
return null;
return result;
}
static boolean isInStringBufferOperationChain(Node node, String methodName) {
ASTPrimaryExpression expr = node.getFirstParentOfType(ASTPrimaryExpression.class);
MethodCallChain methodCalls = MethodCallChain.wrap(expr);
while (expr != null && methodCalls == null) {
expr = expr.getFirstParentOfType(ASTPrimaryExpression.class);
methodCalls = MethodCallChain.wrap(expr);
}
if (methodCalls != null && !methodCalls.isExactlyOfAnyType(StringBuffer.class, StringBuilder.class)) {
methodCalls = null;
}
return methodCalls != null && methodCalls.getMethodNames().contains(methodName);
}
/**
* @deprecated will be removed with PMD 7
*/
@Deprecated
protected static boolean isInStringBufferOperation(Node node, int length, String methodName) {
if (!(node.getNthParent(length) instanceof ASTStatementExpression)) {
return false;
@ -174,4 +202,70 @@ public class InefficientStringBufferingRule extends AbstractJavaRule {
ASTClassOrInterfaceType an = ao.getFirstChildOfType(ASTClassOrInterfaceType.class);
return an != null && TypeHelper.isEither(an, StringBuffer.class, StringBuilder.class);
}
private static class MethodCallChain {
private final ASTPrimaryExpression primary;
private MethodCallChain(ASTPrimaryExpression primary) {
this.primary = primary;
}
// Note: The impl here is technically not correct: The type of a method call
// chain is the result of the last method called, not the type of the
// first receiver object (== PrimaryPrefix).
boolean isExactlyOfAnyType(Class<?> clazz, Class<?> ... clazzes) {
ASTPrimaryPrefix typeNode = getTypeNode();
if (TypeHelper.isExactlyA(typeNode, clazz.getName())) {
return true;
}
if (clazzes != null) {
for (Class<?> c : clazzes) {
if (TypeHelper.isExactlyA(typeNode, c.getName())) {
return true;
}
}
}
return false;
}
ASTPrimaryPrefix getTypeNode() {
return primary.getFirstChildOfType(ASTPrimaryPrefix.class);
}
List<String> getMethodNames() {
List<String> methodNames = new ArrayList<>();
ASTPrimaryPrefix prefix = getTypeNode();
ASTName name = prefix.getFirstChildOfType(ASTName.class);
if (name != null) {
String firstMethod = name.getImage();
int dot = firstMethod.lastIndexOf('.');
if (dot != -1) {
firstMethod = firstMethod.substring(dot + 1);
}
methodNames.add(firstMethod);
}
for (ASTPrimarySuffix suffix : primary.findChildrenOfType(ASTPrimarySuffix.class)) {
if (suffix.getImage() != null) {
methodNames.add(suffix.getImage());
}
}
return methodNames;
}
static MethodCallChain wrap(ASTPrimaryExpression primary) {
if (primary != null && isMethodCall(primary)) {
return new MethodCallChain(primary);
}
return null;
}
private static boolean isMethodCall(ASTPrimaryExpression primary) {
return primary.getNumChildren() >= 2
&& primary.getChild(0) instanceof ASTPrimaryPrefix
&& primary.getChild(1) instanceof ASTPrimarySuffix;
}
}
}

4
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InsufficientStringBufferDeclarationRule.java
View File

@ -73,10 +73,10 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRule {
for (NameOccurrence no : usage) {
JavaNameOccurrence jno = (JavaNameOccurrence) no;
Node n = jno.getLocation();
if (!InefficientStringBufferingRule.isInStringBufferOperation(n, 3, "append")) {
if (!InefficientStringBufferingRule.isInStringBufferOperationChain(n, "append")) {
if (!jno.isOnLeftHandSide()
&& !InefficientStringBufferingRule.isInStringBufferOperation(n, 3, "setLength")) {
&& !InefficientStringBufferingRule.isInStringBufferOperationChain(n, "setLength")) {
continue;
}
if (constructorLength != -1 && anticipatedLength > constructorLength) {

14
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/AppendCharacterWithChar.xml
View File

@ -190,6 +190,20 @@ public class Foo {
public void bar(StringBuffer sb, int length) {
sb.append("a".repeat(length));
}
}
]]></code>
</test-code>
<test-code>
<description>append single character string in constructor chain</description>
<expected-problems>2</expected-problems>
<expected-linenumbers>3,4</expected-linenumbers>
<code><![CDATA[
public class Foo {
public void bar() {
StringBuilder sb = new StringBuilder().append("a");
sb = new StringBuilder().append("c");
}
}
]]></code>
</test-code>

18
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/ConsecutiveLiteralAppends.xml
View File

@ -1446,6 +1446,24 @@ public final class Test {
r2.run();
return sb.toString();
}
}
]]></code>
</test-code>
<test-code>
<description>StringBuilder chains</description>
<expected-problems>2</expected-problems>
<expected-linenumbers>3,6</expected-linenumbers>
<code><![CDATA[
public class Foo {
public void bar() {
StringBuilder sb1 = new StringBuilder().append("abc").append("def"); // bad
StringBuilder sb2 = new StringBuilder().append("abc"); // ok
StringBuilder sb3 = new StringBuilder();
sb3.append("abc").append("def"); // bad
StringBuilder sb4 = new StringBuilder();
sb4.append("abc"); // ok
}
}
]]></code>
</test-code>

28
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InefficientStringBuffering.xml
View File

@ -345,9 +345,9 @@ public class Foo {
</test-code>
<test-code>
<description>Violation: Avoid contact in append method invocations</description>
<expected-problems>3</expected-problems>
<expected-linenumbers>9,18,24</expected-linenumbers>
<description>Violation: Avoid concat in append method invocations</description>
<expected-problems>5</expected-problems>
<expected-linenumbers>9,13,18,27,33</expected-linenumbers>
<code><![CDATA[
public class Foo {
@ -360,9 +360,18 @@ public class Foo {
sb.append("arg='" + arg + "'"); // bad
}
private void concatNumericIsBad() {
private void concatIsBad2(String arg) {
StringBuilder sb = new StringBuilder().append("arg='" + arg + "'"); // bad
}
private void concatIsBad3(String arg) {
StringBuilder sb;
sb = new StringBuilder().append("arg='" + arg + "'"); // bad
}
private void concatNumeric() {
StringBuilder sb = new StringBuilder();
sb.append(1 + 2); // bad
sb.append(1 + 2);
}
private String testStaticBad() {
@ -379,7 +388,7 @@ public class Foo {
</test-code>
<test-code>
<description>No violation: Avoid contact in append method invocations</description>
<description>No violation: Avoid concat in append method invocations</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
@ -418,6 +427,13 @@ public class Foo {
final String local = "local"; // final assumed a constant expression
return new StringBuilder().append("fld:" + local).toString(); // good
}
private String testFinalLocalGood2() {
final String local = "local"; // final assumed a constant expression
StringBuilder sb = new StringBuilder();
sb.append("local:" + local); // good
return sb.toString;
}
}
]]></code>
</test-code>