From 60c7ac63372337ef844a981a746cbde8d4b8c22d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 00:09:30 -0300 Subject: [PATCH 1/9] Add test case for #3845 --- .../xml/InsufficientStringBufferDeclaration.xml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) 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 7fe8a4366c..7c207c7938 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 @@ -1357,6 +1357,21 @@ public class InsufficientStringBufferDeclarationNPE { msg = sb.toString(); } } +} + ]]> + + + + [java] InsufficientStringBufferDeclaration should consider literal expression #3845 + 1 + From 6420c14fc5a4c69f650018b249d242c10a05a36b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 00:09:41 -0300 Subject: [PATCH 2/9] Properly handle more int binary ops --- ...sufficientStringBufferDeclarationRule.java | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) 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 14d7f5ba64..b9c45f227b 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 @@ -250,17 +250,26 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha public Void visit(ASTInfixExpression node, MutableInt data) { MutableInt temp = new MutableInt(-1); - if (BinaryOp.ADD.equals(node.getOperator())) { - data.setValue(0); - node.getLeftOperand().acceptVisitor(this, temp); - data.add(temp.getValue()); - node.getRightOperand().acceptVisitor(this, temp); - data.add(temp.getValue()); - } else if (BinaryOp.MUL.equals(node.getOperator())) { - node.getLeftOperand().acceptVisitor(this, temp); - data.setValue(temp.getValue()); - node.getRightOperand().acceptVisitor(this, temp); - data.setValue(data.getValue() * temp.getValue()); + node.getLeftOperand().acceptVisitor(this, temp); + data.setValue(temp.getValue()); + node.getRightOperand().acceptVisitor(this, temp); + + switch (node.getOperator()) { + case ADD: + data.add(temp.getValue()); + break; + + case SUB: + data.subtract(temp.getValue()); + break; + + case MUL: + data.setValue(data.getValue() * temp.getValue()); + break; + + case DIV: + data.setValue(data.getValue() / temp.getValue()); + break; } return null; From ff263e29a007813cfb758a12c38516050251eff5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 00:11:19 -0300 Subject: [PATCH 3/9] Update changelog, refs #3845 --- docs/pages/release_notes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 58fe6e34a8..6bbd0f14dd 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -18,6 +18,7 @@ This is a {{ site.pmd.release_type }} release. * java-codestyle * [#4881](https://github.com/pmd/pmd/issues/4881): \[java] ClassNamingConventions: interfaces are identified as abstract classes (regression in 7.0.0) * java-performance + * [#3845](https://github.com/pmd/pmd/issues/3845): \[java] InsufficientStringBufferDeclaration should consider literal expression * [#4874](https://github.com/pmd/pmd/issues/4874): \[java] StringInstantiation: False-positive when using `new String(charArray)` ### 🚨 API Changes From f20a51e5ae4fdbae57110dffa7d69a9f084ffd56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 00:34:16 -0300 Subject: [PATCH 4/9] Add defauilt case --- .../performance/InsufficientStringBufferDeclarationRule.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 b9c45f227b..b4166953a7 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 @@ -44,7 +44,6 @@ import net.sourceforge.pmd.lang.java.types.TypeTestUtil; */ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulechainRule { - private static final int DEFAULT_BUFFER_SIZE = 16; public InsufficientStringBufferDeclarationRule() { @@ -270,6 +269,10 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha case DIV: data.setValue(data.getValue() / temp.getValue()); break; + + default: + data.setValue(0); + break; } return null; From d1b5e3ef7cf8a751dda9b44355d50f48f1590131 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 09:12:02 -0300 Subject: [PATCH 5/9] Use -1 for unknown values --- .../performance/InsufficientStringBufferDeclarationRule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b4166953a7..12de31ad07 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 @@ -271,7 +271,7 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha break; default: - data.setValue(0); + data.setValue(-1); break; } From cd5ce3255f348f5807bce470db8b6376301f0c79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 11:14:51 -0300 Subject: [PATCH 6/9] Rework to allow for -1 as a valid value --- ...sufficientStringBufferDeclarationRule.java | 50 ++++++++++++------- 1 file changed, 31 insertions(+), 19 deletions(-) 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 12de31ad07..5c90d9b0da 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 @@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.java.rule.performance; import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.OptionalInt; import java.util.Set; import org.apache.commons.lang3.mutable.MutableInt; @@ -51,6 +52,7 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha } private static class State { + static final int UNKNOWN_CAPACITY = -1; ASTVariableId variable; TypeNode rootNode; int capacity; @@ -196,7 +198,7 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha } } else if ("setLength".equals(methodCall.getMethodName())) { int newLength = calculateExpression(methodCall.getArguments().get(0)); - if (state.capacity != -1 && newLength > state.capacity) { + if (state.capacity != State.UNKNOWN_CAPACITY && newLength > state.capacity) { state.capacity = newLength; // a bigger setLength increases capacity state.rootNode = methodCall; } @@ -213,7 +215,7 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha } private State getConstructorCapacity(ASTVariableId variable, ASTExpression node) { - State state = new State(variable, null, -1, 0); + State state = new State(variable, null, State.UNKNOWN_CAPACITY, 0); JavaNode possibleConstructorCall = node; @@ -239,39 +241,48 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha return new State(variable, constructorCall, DEFAULT_BUFFER_SIZE, state.anticipatedLength); } + private static class MutableOptionalInt { + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") + OptionalInt val = OptionalInt.empty(); + } private int calculateExpression(ASTExpression expression) { - class ExpressionVisitor extends JavaVisitorBase { - + class ExpressionVisitor extends JavaVisitorBase { @Override - public Void visit(ASTInfixExpression node, MutableInt data) { - MutableInt temp = new MutableInt(-1); + public Void visit(ASTInfixExpression node, MutableOptionalInt data) { + MutableOptionalInt left = new MutableOptionalInt(); + MutableOptionalInt right = new MutableOptionalInt(); - node.getLeftOperand().acceptVisitor(this, temp); - data.setValue(temp.getValue()); - node.getRightOperand().acceptVisitor(this, temp); + node.getLeftOperand().acceptVisitor(this, left); + node.getRightOperand().acceptVisitor(this, right); + if (!left.val.isPresent() || !right.val.isPresent()) { + data.val = OptionalInt.empty(); + return null; + } + + OptionalInt ret; switch (node.getOperator()) { case ADD: - data.add(temp.getValue()); + data.val = OptionalInt.of(left.val.getAsInt() + right.val.getAsInt()); break; case SUB: - data.subtract(temp.getValue()); + data.val = OptionalInt.of(left.val.getAsInt() - right.val.getAsInt()); break; case MUL: - data.setValue(data.getValue() * temp.getValue()); + data.val = OptionalInt.of(left.val.getAsInt() * right.val.getAsInt()); break; case DIV: - data.setValue(data.getValue() / temp.getValue()); + data.val = OptionalInt.of(left.val.getAsInt() / right.val.getAsInt()); break; default: - data.setValue(-1); + data.val = OptionalInt.empty(); break; } @@ -279,14 +290,15 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha } @Override - public Void visit(ASTNumericLiteral node, MutableInt data) { - data.setValue(node.getValueAsInt()); + public Void visit(ASTNumericLiteral node, MutableOptionalInt data) { + data.val = OptionalInt.of(node.getValueAsInt()); return null; } } - MutableInt result = new MutableInt(-1); - expression.acceptVisitor(new ExpressionVisitor(), result); - return result.getValue(); + // TODO : use expression.getConstValue() ? it would fail on conditionals, we need to somehow say "get me the min / max"… + MutableOptionalInt size = new MutableOptionalInt(); + expression.acceptVisitor(new ExpressionVisitor(), size); + return size.val.isPresent() ? size.val.getAsInt() : State.UNKNOWN_CAPACITY; } } From c9a2dea7dae031cbb4ebfdb7a5b0fef97b4aac20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 11:41:49 -0300 Subject: [PATCH 7/9] Cleanup --- .../performance/InsufficientStringBufferDeclarationRule.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 5c90d9b0da..6daa4616e3 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 @@ -241,7 +241,7 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha return new State(variable, constructorCall, DEFAULT_BUFFER_SIZE, state.anticipatedLength); } - private static class MutableOptionalInt { + private final static class MutableOptionalInt { @SuppressWarnings("OptionalUsedAsFieldOrParameterType") OptionalInt val = OptionalInt.empty(); } @@ -263,7 +263,6 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha return null; } - OptionalInt ret; switch (node.getOperator()) { case ADD: data.val = OptionalInt.of(left.val.getAsInt() + right.val.getAsInt()); From 2cb1641121f83f1286e3ad5013d7cc276faa1d5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 13:14:56 -0300 Subject: [PATCH 8/9] Just use getConstValue --- ...sufficientStringBufferDeclarationRule.java | 65 +------------------ .../InsufficientStringBufferDeclaration.xml | 16 +++++ 2 files changed, 18 insertions(+), 63 deletions(-) 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 6daa4616e3..ae4dd2c3e6 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 @@ -7,11 +7,8 @@ package net.sourceforge.pmd.lang.java.rule.performance; import java.util.HashMap; import java.util.HashSet; import java.util.Map; -import java.util.OptionalInt; 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.ASTAssignableExpr.ASTNamedReferenceExpr; import net.sourceforge.pmd.lang.java.ast.ASTAssignmentExpression; @@ -20,7 +17,6 @@ import net.sourceforge.pmd.lang.java.ast.ASTCharLiteral; import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall; import net.sourceforge.pmd.lang.java.ast.ASTExpression; import net.sourceforge.pmd.lang.java.ast.ASTIfStatement; -import net.sourceforge.pmd.lang.java.ast.ASTInfixExpression; import net.sourceforge.pmd.lang.java.ast.ASTLiteral; import net.sourceforge.pmd.lang.java.ast.ASTMethodCall; import net.sourceforge.pmd.lang.java.ast.ASTNumericLiteral; @@ -28,9 +24,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTStringLiteral; import net.sourceforge.pmd.lang.java.ast.ASTSwitchBranch; import net.sourceforge.pmd.lang.java.ast.ASTSwitchStatement; import net.sourceforge.pmd.lang.java.ast.ASTVariableId; -import net.sourceforge.pmd.lang.java.ast.BinaryOp; import net.sourceforge.pmd.lang.java.ast.JavaNode; -import net.sourceforge.pmd.lang.java.ast.JavaVisitorBase; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule; import net.sourceforge.pmd.lang.java.types.TypeTestUtil; @@ -241,63 +235,8 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha return new State(variable, constructorCall, DEFAULT_BUFFER_SIZE, state.anticipatedLength); } - private final static class MutableOptionalInt { - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - OptionalInt val = OptionalInt.empty(); - } - private int calculateExpression(ASTExpression expression) { - - class ExpressionVisitor extends JavaVisitorBase { - - @Override - public Void visit(ASTInfixExpression node, MutableOptionalInt data) { - MutableOptionalInt left = new MutableOptionalInt(); - MutableOptionalInt right = new MutableOptionalInt(); - - node.getLeftOperand().acceptVisitor(this, left); - node.getRightOperand().acceptVisitor(this, right); - - if (!left.val.isPresent() || !right.val.isPresent()) { - data.val = OptionalInt.empty(); - return null; - } - - switch (node.getOperator()) { - case ADD: - data.val = OptionalInt.of(left.val.getAsInt() + right.val.getAsInt()); - break; - - case SUB: - data.val = OptionalInt.of(left.val.getAsInt() - right.val.getAsInt()); - break; - - case MUL: - data.val = OptionalInt.of(left.val.getAsInt() * right.val.getAsInt()); - break; - - case DIV: - data.val = OptionalInt.of(left.val.getAsInt() / right.val.getAsInt()); - break; - - default: - data.val = OptionalInt.empty(); - break; - } - - return null; - } - - @Override - public Void visit(ASTNumericLiteral node, MutableOptionalInt data) { - data.val = OptionalInt.of(node.getValueAsInt()); - return null; - } - } - - // TODO : use expression.getConstValue() ? it would fail on conditionals, we need to somehow say "get me the min / max"… - MutableOptionalInt size = new MutableOptionalInt(); - expression.acceptVisitor(new ExpressionVisitor(), size); - return size.val.isPresent() ? size.val.getAsInt() : State.UNKNOWN_CAPACITY; + Object value = expression.getConstValue(); + return value == null ? State.UNKNOWN_CAPACITY : (Integer) value; } } 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 7c207c7938..2f7b4d8f9e 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 @@ -1372,6 +1372,22 @@ public class LiteralExpression { sb.append('a'); sb.append('a'); } +} + ]]> + + + + Properly handle constant variable references + 1 + From a3dccb0940d89c6cd13e0306810f5ca0ba7cf463 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 25 Mar 2024 14:29:57 -0300 Subject: [PATCH 9/9] Fix handling of constant String expressions --- ...sufficientStringBufferDeclarationRule.java | 7 +++-- .../InsufficientStringBufferDeclaration.xml | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) 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 ae4dd2c3e6..7f1905577f 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 @@ -225,8 +225,11 @@ public class InsufficientStringBufferDeclarationRule extends AbstractJavaRulecha ASTConstructorCall constructorCall = (ASTConstructorCall) possibleConstructorCall; if (constructorCall.getArguments().size() == 1) { ASTExpression argument = constructorCall.getArguments().get(0); - if (argument instanceof ASTStringLiteral) { - int stringLength = ((ASTStringLiteral) argument).length(); + if (TypeTestUtil.isA(String.class, argument)) { + if (argument.getConstValue() == null) { + return state; + } + int stringLength = ((String) argument.getConstValue()).length(); return new State(variable, constructorCall, DEFAULT_BUFFER_SIZE + stringLength, stringLength + state.anticipatedLength); } else { return new State(variable, constructorCall, calculateExpression(argument), state.anticipatedLength); 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 2f7b4d8f9e..ec267fcfcc 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 @@ -1388,6 +1388,34 @@ public class LiteralExpression { sb.append('a'); sb.append('a'); } +} + ]]> + + + + Properly handle constant string expressions + 1 + \n" + + "\n" + + " \n" + + " \n" + + "

Don't panic!

\n" + + " " + ); + + sb.append("this string is longer than 16 characters"); + return sb.toString(); + } } ]]>