From b8e9ff19b2ef1f8fbc949832dbdb0636a6c7ac9e Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sun, 14 Jun 2020 12:50:02 +0200 Subject: [PATCH] [java] Fix rules InefficientStringBuffering and refactor existing rules Rules affected additionally: AppendCharacterWithChar, ConsecutiveLiteralAppends, InsufficientStringBufferDeclaration Deprecate InefficientStringBuffering::isInStringBufferOperation --- .../AppendCharacterWithCharRule.java | 7 +- .../ConsecutiveLiteralAppendsRule.java | 4 +- .../InefficientStringBufferingRule.java | 110 ++++++++++++++++-- ...sufficientStringBufferDeclarationRule.java | 4 +- .../xml/AppendCharacterWithChar.xml | 14 +++ .../xml/ConsecutiveLiteralAppends.xml | 18 +++ .../xml/InefficientStringBuffering.xml | 28 ++++- 7 files changed, 167 insertions(+), 18 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/AppendCharacterWithCharRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/AppendCharacterWithCharRule.java index 6f725ef954..e448ab1746 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/AppendCharacterWithCharRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/AppendCharacterWithCharRule.java @@ -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); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/ConsecutiveLiteralAppendsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/ConsecutiveLiteralAppendsRule.java index 065df94419..bad078608e 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/ConsecutiveLiteralAppendsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/ConsecutiveLiteralAppendsRule.java @@ -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 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(); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InefficientStringBufferingRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InefficientStringBufferingRule.java index b6151fd576..88b25e23da 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InefficientStringBufferingRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/InefficientStringBufferingRule.java @@ -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 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 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 getMethodNames() { + List 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; + } + } } 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 53c1bc4098..e384b66d42 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 @@ -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) { diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/AppendCharacterWithChar.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/AppendCharacterWithChar.xml index 869fde46d4..1772f5ee86 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/AppendCharacterWithChar.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/AppendCharacterWithChar.xml @@ -190,6 +190,20 @@ public class Foo { public void bar(StringBuffer sb, int length) { sb.append("a".repeat(length)); } +} + ]]> + + + + append single character string in constructor chain + 2 + 3,4 + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/ConsecutiveLiteralAppends.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/ConsecutiveLiteralAppends.xml index 6a7e4ceb80..abe85b048e 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/ConsecutiveLiteralAppends.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/ConsecutiveLiteralAppends.xml @@ -1446,6 +1446,24 @@ public final class Test { r2.run(); return sb.toString(); } +} + ]]> + + + + StringBuilder chains + 2 + 3,6 + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InefficientStringBuffering.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InefficientStringBuffering.xml index 48afdcefcd..4e48e6ba71 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InefficientStringBuffering.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/InefficientStringBuffering.xml @@ -345,9 +345,9 @@ public class Foo { - Violation: Avoid contact in append method invocations - 3 - 9,18,24 + Violation: Avoid concat in append method invocations + 5 + 9,13,18,27,33 - No violation: Avoid contact in append method invocations + No violation: Avoid concat in append method invocations 0