diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index c8bf47f07d..790342c5fc 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -36,6 +36,8 @@ This is a {{ site.pmd.release_type }} release. * [#3321](https://github.com/pmd/pmd/issues/3321): \[apex] New rule to detect inaccessible AuraEnabled getters (summer '21 security update) * core * [#3323](https://github.com/pmd/pmd/pull/3323): \[core] Adds fullDescription and tags in SARIF report +* java-bestpractices + * [#3315](https://github.com/pmd/pmd/issues/3315): \[java] LiteralsFirstInComparisons false positive with two constants * java-codestyle * [#3317](https://github.com/pmd/pmd/pull/3317): \[java] Update UnnecessaryImport to recognize usage of imported types in javadoc's `@exception` tag * java-errorprone diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java index c26a8473e8..240b11456b 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java @@ -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 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 imports) { + Map> importedTypes = new HashMap<>(); + Set 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); } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml index 31a83b3547..e120121a3d 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml @@ -409,4 +409,36 @@ public class Foo { } ]]> + + + [java] LiteralsFirstInComparisons with two constants #3315 + 0 + + + + + [java] LiteralsFirstInComparisons with two constants #3315 - with on demand import + 0 + +