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

[java] LiteralsFirstInComparisons false positive with two constants

Browse Source 
Fixes #3315
This commit is contained in:
Andreas Dangel
2021-06-11 20:13:04 +02:00
parent 0b6c0594f1
commit b739c0ef04
3 changed files with 122 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

89
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java
View File

@ -4,6 +4,16 @@
package net.sourceforge.pmd.lang.java.rule.bestpractices;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.apache.commons.lang3.StringUtils;
import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
import net.sourceforge.pmd.lang.java.ast.ASTArguments;
import net.sourceforge.pmd.lang.java.ast.ASTConditionalAndExpression;
@ -11,6 +21,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTConditionalOrExpression;
import net.sourceforge.pmd.lang.java.ast.ASTEqualityExpression;
import net.sourceforge.pmd.lang.java.ast.ASTExpression;
import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTName;
import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral;
@ -21,6 +32,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
import net.sourceforge.pmd.lang.java.ast.JavaNode;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration;
import net.sourceforge.pmd.lang.java.typeresolution.ClassTypeResolver;
import net.sourceforge.pmd.lang.symboltable.NameDeclaration;
public class LiteralsFirstInComparisonsRule extends AbstractJavaRule {
@ -51,10 +63,12 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule {
private boolean isNullableComparisonWithStringLiteral(ASTPrimaryExpression expression) {
String opName = getOperationName(expression);
ASTName opTarget = getOperationTarget(expression);
ASTPrimarySuffix argsSuffix = getSuffixOfArguments(expression);
return opName != null && argsSuffix != null
&& isStringLiteralComparison(opName, argsSuffix)
&& isNotWithinNullComparison(expression);
&& isNotWithinNullComparison(expression)
&& !isConstantString(opTarget);
}
private String getOperationName(ASTPrimaryExpression primaryExpression) {
@ -67,6 +81,12 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule {
return primaryExpression.getNumChildren() > 2;
}
private ASTName getOperationTarget(ASTPrimaryExpression primaryExpression) {
return isMethodsChain(primaryExpression)
? getOperationTargetBySuffix(primaryExpression)
: getOperationTargetByPrefix(primaryExpression);
}
private String getOperationNameBySuffix(ASTPrimaryExpression primaryExpression) {
ASTPrimarySuffix opAsSuffix = getPrimarySuffixAtIndexFromEnd(primaryExpression, 1);
if (opAsSuffix != null) {
@ -76,6 +96,14 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule {
return null;
}
private ASTName getOperationTargetBySuffix(ASTPrimaryExpression primaryExpression) {
ASTPrimarySuffix opAsSuffix = getPrimarySuffixAtIndexFromEnd(primaryExpression, 1);
if (opAsSuffix != null) {
return opAsSuffix.getFirstChildOfType(ASTName.class);
}
return null;
}
private String getOperationNameByPrefix(ASTPrimaryExpression primaryExpression) {
ASTPrimaryPrefix opAsPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class);
if (opAsPrefix != null) {
@ -85,6 +113,14 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule {
return null;
}
private ASTName getOperationTargetByPrefix(ASTPrimaryExpression primaryExpression) {
ASTPrimaryPrefix opAsPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class);
if (opAsPrefix != null) {
return opAsPrefix.getFirstChildOfType(ASTName.class); // name of pattern "*.operation"
}
return null;
}
private ASTPrimarySuffix getSuffixOfArguments(ASTPrimaryExpression primaryExpression) {
return getPrimarySuffixAtIndexFromEnd(primaryExpression, 0);
}
@ -166,11 +202,62 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule {
return resolvedNode.isFinal()
&& resolvedNode.isField()
&& resolvedNode.getFirstParentOfType(ASTFieldDeclaration.class).isStatic();
} else if (resolved == null) {
// try to resolve a referenced static field
List<ASTImportDeclaration> imports = node.getRoot().findChildrenOfType(ASTImportDeclaration.class);
Field field = tryResolve(name.getImage(), node.getRoot().getClassTypeResolver(), imports);
if (field != null) {
return Modifier.isStatic(field.getModifiers()) && Modifier.isFinal(field.getModifiers());
}
}
}
return false;
}
private Field tryResolve(String fullPossibleClassName, ClassTypeResolver resolver, List<ASTImportDeclaration> imports) {
Map<String, Class<?>> importedTypes = new HashMap<>();
Set<String> onDemandImports = new HashSet<>();
for (ASTImportDeclaration importDecl : imports) {
if (importDecl.getType() != null) {
importedTypes.put(importDecl.getType().getSimpleName(), importDecl.getType());
} else if (importDecl.isImportOnDemand()) {
onDemandImports.add(importDecl.getImportedName());
}
}
String[] splitName = fullPossibleClassName.split("\\.");
for (int i = splitName.length; i > 0; i--) {
String possibleClassName = StringUtils.join(splitName, ".", 0, i);
if (importedTypes.containsKey(possibleClassName)) {
String possibleFieldName = splitName[i];
Class<?> type = importedTypes.get(possibleClassName);
try {
return type.getDeclaredField(possibleFieldName);
} catch (ReflectiveOperationException ignored) {
// skip, try next
}
}
}
// try on-demand
for (int i = splitName.length; i > 0; i--) {
String possibleClassName = StringUtils.join(splitName, ".", 0, i);
for (String prefix : onDemandImports) {
Class<?> type = resolver.loadClassOrNull(prefix + "." + possibleClassName);
if (type == null) {
continue;
}
String possibleFieldName = splitName[i];
try {
return type.getDeclaredField(possibleFieldName);
} catch (ReflectiveOperationException ignored) {
// skip, try next
}
}
}
return null;
}
private boolean isNotWithinNullComparison(ASTPrimaryExpression node) {
return !isWithinNullComparison(node);
}

32
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml
View File

@ -409,4 +409,36 @@ public class Foo {
}
]]></code>
</test-code>
<test-code>
<description>[java] LiteralsFirstInComparisons with two constants #3315</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import net.sourceforge.pmd.PMDVersion;
public class LiteralsFirstInComparisonCase {
private static final String S1 = "s1";
private static final String S2 = "s2";
public static boolean compare() {
return S1.equals(S2);
}
public static boolean isUnkown() {
return PMDVersion.VERSION.equals(S2);
}
}
]]></code>
</test-code>
<test-code>
<description>[java] LiteralsFirstInComparisons with two constants #3315 - with on demand import</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import net.sourceforge.pmd.*;
public class LiteralsFirstInComparisonCase {
private static final String S2 = "s2";
public static boolean isUnkown() {
return PMDVersion.VERSION.equals(S2);
}
}
]]></code>
</test-code>
</test-data>