From 59dbf739973daedc360f648bfea94399021a4314 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 30 Sep 2021 14:45:18 +0200 Subject: [PATCH] [java] Update rule UnnecessaryCaseChange --- .ci/files/all-java.xml | 2 +- .../errorprone/UnnecessaryCaseChangeRule.java | 121 ++++-------------- .../errorprone/UnnecessaryCaseChangeTest.java | 1 - .../errorprone/xml/UnnecessaryCaseChange.xml | 23 ++-- 4 files changed, 39 insertions(+), 108 deletions(-) diff --git a/.ci/files/all-java.xml b/.ci/files/all-java.xml index 0a86a06f54..402980110a 100644 --- a/.ci/files/all-java.xml +++ b/.ci/files/all-java.xml @@ -257,7 +257,7 @@ - + diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnnecessaryCaseChangeRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnnecessaryCaseChangeRule.java index 62512bedc8..510a0a1142 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnnecessaryCaseChangeRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnnecessaryCaseChangeRule.java @@ -8,115 +8,40 @@ import static java.util.Arrays.asList; import java.util.List; -import net.sourceforge.pmd.lang.java.ast.ASTArguments; -import net.sourceforge.pmd.lang.java.ast.ASTName; -import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; -import net.sourceforge.pmd.lang.java.ast.JavaNode; -import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; +import net.sourceforge.pmd.lang.java.ast.ASTExpression; +import net.sourceforge.pmd.lang.java.ast.ASTMethodCall; +import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule; -public class UnnecessaryCaseChangeRule extends AbstractJavaRule { +public class UnnecessaryCaseChangeRule extends AbstractJavaRulechainRule { private static final List CASE_CHANGING_METHODS = asList("toLowerCase", "toUpperCase"); private static final List EQUALITY_METHODS = asList("equals", "equalsIgnoreCase"); + public UnnecessaryCaseChangeRule() { + super(ASTMethodCall.class); + } + @Override - public Object visit(ASTPrimaryExpression expr, Object data) { - if (hasUnnecessaryCaseChange(expr)) { - addViolation(data, expr); - } - return super.visit(expr, data); - } - - private boolean hasUnnecessaryCaseChange(ASTPrimaryExpression expr) { - int equalsMethodCallIndex = getEqualsMethodCallIndex(expr); - if (equalsMethodCallIndex != -1) { - int equalsMethodCallArgsIndex = equalsMethodCallIndex + 1; - ASTPrimaryExpression equalsCallArgs = getMethodCallArgsAtPosition(expr, equalsMethodCallArgsIndex); - return anyHasCaseChangingMethodCall(expr, equalsCallArgs); - } - return false; - } - - private int getEqualsMethodCallIndex(ASTPrimaryExpression expr) { - for (int callIndex = 0; callIndex < expr.getNumChildren(); callIndex++) { - JavaNode methodCall = expr.getChild(callIndex); - if (isEqualsMethodCall(methodCall)) { - return callIndex; + public Object visit(ASTMethodCall node, Object data) { + if (EQUALITY_METHODS.contains(node.getMethodName()) && node.getArguments().size() == 1) { + if (isCaseChangingMethodCall(node.getQualifier()) + || isCaseChangingMethodCall(node.getArguments().get(0))) { + addViolation(data, node); } } - return -1; + return data; } - private boolean isEqualsMethodCall(JavaNode methodCall) { - return calledMethodHasNameFromList(methodCall, EQUALITY_METHODS); - } - - private ASTPrimaryExpression getMethodCallArgsAtPosition(ASTPrimaryExpression expr, int argsPos) { - if (hasChildAtPosition(expr, argsPos)) { - JavaNode methodCallArgs = expr.getChild(argsPos); - return methodCallArgs.getFirstDescendantOfType(ASTPrimaryExpression.class); - } - return null; - } - - private boolean hasChildAtPosition(ASTPrimaryExpression expr, int pos) { - return expr.getNumChildren() > pos; - } - - private boolean anyHasCaseChangingMethodCall(ASTPrimaryExpression ... exprs) { - for (ASTPrimaryExpression expr : exprs) { - if (expr != null && hasCaseChangingMethodCall(expr)) { - return true; - } + /** + * Checks for toLower/UpperCase method calls without arguments. + * These method take an optional Locale as an argument - in that case, + * these case conversions are considered deliberate. + */ + private boolean isCaseChangingMethodCall(ASTExpression expr) { + if (expr instanceof ASTMethodCall) { + ASTMethodCall call = (ASTMethodCall) expr; + return CASE_CHANGING_METHODS.contains(call.getMethodName()) && call.getArguments().size() == 0; } return false; } - - private boolean hasCaseChangingMethodCall(ASTPrimaryExpression expr) { - for (int callArgsIndex = 1; callArgsIndex < expr.getNumChildren(); callArgsIndex++) { - JavaNode methodCall = expr.getChild(callArgsIndex - 1); - JavaNode methodCallArgs = expr.getChild(callArgsIndex); - if (isCaseChangingMethodCall(methodCall, methodCallArgs)) { - return true; - } - } - return false; - } - - private boolean isCaseChangingMethodCall(JavaNode methodCall, JavaNode methodCallArgs) { - if (calledMethodHasNameFromList(methodCall, CASE_CHANGING_METHODS)) { - ASTArguments args = methodCallArgs.getFirstDescendantOfType(ASTArguments.class); - return args != null && args.size() == 0; - } - return false; - } - - private boolean calledMethodHasNameFromList(JavaNode methodCall, List nameList) { - String methodName = getCalledMethodName(methodCall); - if (methodName != null) { - for (String nameFromList : nameList) { - if (methodName.endsWith(nameFromList)) { - return true; - } - } - } - return false; - } - - private String getCalledMethodName(JavaNode methodCall) { - String methodName = methodCall.getImage(); - if (methodName == null) { - ASTName name = methodCall.getFirstDescendantOfType(ASTName.class); - return name != null ? methodNameFromCallImage(name.getImage()) : null; - } - return methodName; - } - - private String methodNameFromCallImage(String methodCallImage) { - if (methodCallImage.contains(".")) { - String[] callParts = methodCallImage.split("\\."); - return callParts[1]; - } - return methodCallImage; - } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnnecessaryCaseChangeTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnnecessaryCaseChangeTest.java index 5bd4304f20..4cbb6ea7e3 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnnecessaryCaseChangeTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnnecessaryCaseChangeTest.java @@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.errorprone; import net.sourceforge.pmd.testframework.PmdRuleTst; -@org.junit.Ignore("Rule has not been updated yet") public class UnnecessaryCaseChangeTest extends PmdRuleTst { // no additional unit tests } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UnnecessaryCaseChange.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UnnecessaryCaseChange.xml index d3daf483ca..8bbb435a24 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UnnecessaryCaseChange.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UnnecessaryCaseChange.xml @@ -10,7 +10,7 @@ @@ -22,7 +22,7 @@ public class Foo { @@ -34,7 +34,7 @@ public class Foo { @@ -46,7 +46,7 @@ public class Foo { @@ -57,13 +57,17 @@ public class Foo { 1 clazz) { return null; } + public String getVariableName() { return null; } + public String getImage() { return null; } } ]]> @@ -86,8 +90,11 @@ public class Foo { NullPointerException if in constructor test 0