From b1fc3110de1eb63d30a3934459e076e4a1bce7b6 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 6 Dec 2019 10:52:01 +0100 Subject: [PATCH 1/4] [java] StringInstatiation tests - remove unnecessary CDATA --- .../performance/xml/StringInstantiation.xml | 40 ++++++++----------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml index d905d16cd1..9735eb22f7 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml @@ -3,10 +3,9 @@ xmlns="http://pmd.sourceforge.net/rule-tests" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd"> + - + new 'new String's 2 + - + new String array 0 + - + using multiple parameter constructor 0 + - + using 4 parameter constructor 0 + - + byte array constructor is ok 0 + - + Method returning new String 1 + - + Not a new String 0 + - + Returns new String(str) 1 Date: Fri, 6 Dec 2019 11:18:27 +0100 Subject: [PATCH 2/4] [java] StringInstatiation: False negative with String-array access Fixes #2141 --- docs/pages/release_notes.md | 3 ++ .../performance/StringInstantiationRule.java | 44 ++++++++++++++++++- .../performance/xml/StringInstantiation.xml | 16 +++++++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b8f8783555..2c0d1c1b22 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -16,6 +16,9 @@ This is a {{ site.pmd.release_type }} release. ### Fixed Issues +* java-performance + * [#2141](https://github.com/pmd/pmd/issues/2141): \[java] StringInstatiation: False negative with String-array access + ### API Changes ### External Contributions diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java index 8f17a26fca..1ebee70f94 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java @@ -6,19 +6,30 @@ package net.sourceforge.pmd.lang.java.rule.performance; import java.util.List; +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.ASTArrayDimsAndInits; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; import net.sourceforge.pmd.lang.java.ast.ASTExpression; 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.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.symboltable.TypedNameDeclaration; import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper; +import net.sourceforge.pmd.lang.java.typeresolution.typedefinition.JavaTypeDefinition; import net.sourceforge.pmd.lang.symboltable.NameDeclaration; public class StringInstantiationRule extends AbstractJavaRule { + public StringInstantiationRule() { + addRuleChainVisit(ASTAllocationExpression.class); + } + @Override public Object visit(ASTAllocationExpression node, Object data) { if (!(node.jjtGetChild(0) instanceof ASTClassOrInterfaceType)) { @@ -29,8 +40,9 @@ public class StringInstantiationRule extends AbstractJavaRule { return data; } + boolean arrayAccess = isArrayAccess(node); List exp = node.findDescendantsOfType(ASTExpression.class); - if (exp.size() >= 2) { + if (exp.size() >= 2 && !arrayAccess) { return data; } @@ -50,9 +62,37 @@ public class StringInstantiationRule extends AbstractJavaRule { return data; } - if (nd instanceof TypedNameDeclaration && TypeHelper.isExactlyAny((TypedNameDeclaration) nd, String.class)) { + if (nd instanceof TypedNameDeclaration && TypeHelper.isExactlyAny((TypedNameDeclaration) nd, String.class) || arrayAccess) { addViolation(data, node); } return data; } + + private boolean isArrayAccess(ASTAllocationExpression node) { + ASTArguments arguments = node.getFirstChildOfType(ASTArguments.class); + if (arguments == null || arguments.getArgumentCount() != 1) { + return false; + } + + Node firstArg = arguments.getFirstChildOfType(ASTArgumentList.class).jjtGetChild(0); + ASTPrimaryExpression primary = firstArg.getFirstChildOfType(ASTPrimaryExpression.class); + if (primary == null) { + return false; + } + + ASTPrimaryPrefix prefix = primary.getFirstChildOfType(ASTPrimaryPrefix.class); + if (prefix == null || !isStringArrayTypeDefinition(prefix.getTypeDefinition())) { + return false; + } + + ASTPrimarySuffix suffix = primary.getFirstChildOfType(ASTPrimarySuffix.class); + return suffix != null && suffix.isArrayDereference(); + } + + private boolean isStringArrayTypeDefinition(JavaTypeDefinition typeDefinition) { + if (typeDefinition == null || !typeDefinition.isArrayType() || typeDefinition.getElementType() == null) { + return false; + } + return typeDefinition.getElementType().getType() == String.class; + } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml index 9735eb22f7..ff29bc24cd 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml @@ -99,4 +99,20 @@ public class Foo { } ]]> + + + #2141 [java] StringInstatiation: False negative with String-array access + 1 + 4 + + From 9616312de29ad0995828e8b9afa67273423c9a90 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 6 Dec 2019 12:13:04 +0100 Subject: [PATCH 3/4] [java] Support type resolution for array access expressions --- .../typeresolution/ClassTypeResolver.java | 22 +++++++++++++++++-- .../typeresolution/ClassTypeResolverTest.java | 17 ++++++++++++++ .../typeresolution/testdata/ArrayAccess.java | 21 ++++++++++++++++++ 3 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/testdata/ArrayAccess.java diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java index b58389e76c..7b9feb997b 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java @@ -69,6 +69,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTPreDecrementExpression; import net.sourceforge.pmd.lang.java.ast.ASTPreIncrementExpression; 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.ASTReferenceType; import net.sourceforge.pmd.lang.java.ast.ASTRelationalExpression; @@ -951,8 +952,10 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter { if (currentChild.getType() != null) { - // rollup type from the child: PrimaryPrefix -> PrimaryExpression - primaryNodeType = currentChild.getTypeDefinition(); + // rollup type from the child: PrimaryPrefix/PrimarySuffx -> PrimaryExpression + if (primaryNodeType == null || !primaryNodeType.isArrayType()) { + primaryNodeType = currentChild.getTypeDefinition(); + } // if this expression is a method call, then make sure, PrimaryPrefix has the type // on which the method is executed (type of the target reference) @@ -969,6 +972,13 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter { name.setTypeDefinition(primaryNodeType); } } + + // maybe array access? + if (primaryNodeType != null && primaryNodeType.isArrayType()) { + if (currentChild instanceof ASTPrimarySuffix && ((ASTPrimarySuffix) currentChild).isArrayDereference()) { + primaryNodeType = JavaTypeDefinition.forClass(primaryNodeType.getType().getComponentType()); + } + } } else { // avoid falsely passing tests primaryNodeType = null; @@ -1066,6 +1076,14 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter { return data; } + @Override + public Object visit(ASTPrimarySuffix node, Object data) { + super.visit(node, data); + rollupTypeUnary(node); + + return data; + } + @Override public Object visit(ASTTypeArgument node, Object data) { if (node.jjtGetNumChildren() == 0) { // if type argument is '?' diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java index 049c873d65..163917e813 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java @@ -77,6 +77,7 @@ import net.sourceforge.pmd.typeresolution.testdata.AbstractReturnTypeUseCase; import net.sourceforge.pmd.typeresolution.testdata.AnonymousClassFromInterface; import net.sourceforge.pmd.typeresolution.testdata.AnonymousInnerClass; import net.sourceforge.pmd.typeresolution.testdata.AnoymousExtendingObject; +import net.sourceforge.pmd.typeresolution.testdata.ArrayAccess; import net.sourceforge.pmd.typeresolution.testdata.ArrayListFound; import net.sourceforge.pmd.typeresolution.testdata.ArrayTypes; import net.sourceforge.pmd.typeresolution.testdata.ArrayVariableDeclaration; @@ -797,6 +798,22 @@ public class ClassTypeResolverTest { assertEquals("All expressions not tested", index, expressions.size()); } + @Test + public void testArrayAccess() throws JaxenException { + List expressions = selectNodes(ArrayAccess.class, ASTExpression.class, "//VariableInitializer/Expression"); + + int index = 1; + // int aElement = a[0]; + assertEquals(int.class, expressions.get(index).getType()); + + index += 2; + // Object bElement = b[0][0]; + assertEquals(Object.class, expressions.get(index).getType()); + + index += 3; + // ArrayAccess cElement = c[0][0][0]; + assertEquals(ArrayAccess.class, expressions.get(index).getType()); + } @Test public void testReferenceType() { diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/testdata/ArrayAccess.java b/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/testdata/ArrayAccess.java new file mode 100644 index 0000000000..07766e56ce --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/testdata/ArrayAccess.java @@ -0,0 +1,21 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.typeresolution.testdata; + +public class ArrayAccess { + + @SuppressWarnings("unused") + public void test() { + int[] a = new int[1]; + int aElement = a[0]; + + Object[][] b = new Object[1][0]; + Object bElement = b[0][0]; + + ArrayAccess[][][] c = new ArrayAccess[][][] { new ArrayAccess[1][2] }; + ArrayAccess cElement = c[0][0][0]; + } + +} From b7d61fca9495c5eb73e2f0392dd33bf6604d121e Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 6 Dec 2019 12:13:38 +0100 Subject: [PATCH 4/4] [java] StringInstantiation: fix false negative with multi-dim arr access --- .../performance/StringInstantiationRule.java | 26 ++++++------------- .../performance/xml/StringInstantiation.xml | 8 ++++-- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java index 1ebee70f94..ee7c593953 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java @@ -16,12 +16,10 @@ import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; import net.sourceforge.pmd.lang.java.ast.ASTExpression; 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.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.symboltable.TypedNameDeclaration; import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper; -import net.sourceforge.pmd.lang.java.typeresolution.typedefinition.JavaTypeDefinition; import net.sourceforge.pmd.lang.symboltable.NameDeclaration; public class StringInstantiationRule extends AbstractJavaRule { @@ -40,9 +38,13 @@ public class StringInstantiationRule extends AbstractJavaRule { return data; } - boolean arrayAccess = isArrayAccess(node); + if (isArrayAccess(node)) { + addViolation(data, node); + return data; + } + List exp = node.findDescendantsOfType(ASTExpression.class); - if (exp.size() >= 2 && !arrayAccess) { + if (exp.size() >= 2) { return data; } @@ -62,7 +64,7 @@ public class StringInstantiationRule extends AbstractJavaRule { return data; } - if (nd instanceof TypedNameDeclaration && TypeHelper.isExactlyAny((TypedNameDeclaration) nd, String.class) || arrayAccess) { + if (nd instanceof TypedNameDeclaration && TypeHelper.isExactlyAny((TypedNameDeclaration) nd, String.class)) { addViolation(data, node); } return data; @@ -76,23 +78,11 @@ public class StringInstantiationRule extends AbstractJavaRule { Node firstArg = arguments.getFirstChildOfType(ASTArgumentList.class).jjtGetChild(0); ASTPrimaryExpression primary = firstArg.getFirstChildOfType(ASTPrimaryExpression.class); - if (primary == null) { - return false; - } - - ASTPrimaryPrefix prefix = primary.getFirstChildOfType(ASTPrimaryPrefix.class); - if (prefix == null || !isStringArrayTypeDefinition(prefix.getTypeDefinition())) { + if (primary == null || primary.getType() != String.class) { return false; } ASTPrimarySuffix suffix = primary.getFirstChildOfType(ASTPrimarySuffix.class); return suffix != null && suffix.isArrayDereference(); } - - private boolean isStringArrayTypeDefinition(JavaTypeDefinition typeDefinition) { - if (typeDefinition == null || !typeDefinition.isArrayType() || typeDefinition.getElementType() == null) { - return false; - } - return typeDefinition.getElementType().getType() == String.class; - } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml index ff29bc24cd..3ad5d3ba96 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml @@ -102,8 +102,8 @@ public class Foo { #2141 [java] StringInstatiation: False negative with String-array access - 1 - 4 + 2 + 4,10