Skip to content

Commit c51adb9

Browse files
authored
Fix Standard Comparison clean-up false positive for enablement (#2228)
- fix StandardComparisonFixCore.StandardComparisonFinder to look for known comparisons (Comparable.compareTo, Comparator.compare, and String.compareToIgnoreCase) - add new test to CleanUpTest - fixes #2221
1 parent 09c1afd commit c51adb9

File tree

2 files changed

+68
-18
lines changed

2 files changed

+68
-18
lines changed

org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/fix/StandardComparisonFixCore.java

Lines changed: 54 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2021 Fabrice TIERCELIN and others.
2+
* Copyright (c) 2021, 2025 Fabrice TIERCELIN and others.
33
*
44
* This program and the accompanying materials
55
* are made available under the terms of the Eclipse Public License 2.0
@@ -26,6 +26,8 @@
2626
import org.eclipse.jdt.core.dom.ASTVisitor;
2727
import org.eclipse.jdt.core.dom.CompilationUnit;
2828
import org.eclipse.jdt.core.dom.Expression;
29+
import org.eclipse.jdt.core.dom.IMethodBinding;
30+
import org.eclipse.jdt.core.dom.ITypeBinding;
2931
import org.eclipse.jdt.core.dom.InfixExpression;
3032
import org.eclipse.jdt.core.dom.MethodInvocation;
3133
import org.eclipse.jdt.core.dom.ThisExpression;
@@ -55,30 +57,64 @@ public boolean visit(final InfixExpression visited) {
5557
if (orderedCondition != null
5658
&& Arrays.asList(InfixExpression.Operator.EQUALS, InfixExpression.Operator.NOT_EQUALS).contains(orderedCondition.getOperator())) {
5759
MethodInvocation comparisonMI= orderedCondition.getFirstOperand();
58-
Long literalValue= ASTNodes.getIntegerLiteral(orderedCondition.getSecondOperand());
59-
60-
if (literalValue != null
61-
&& literalValue.compareTo(0L) != 0
62-
&& comparisonMI.getExpression() != null
63-
&& !ASTNodes.is(comparisonMI.getExpression(), ThisExpression.class)) {
64-
if (literalValue.compareTo(0L) < 0) {
65-
if (InfixExpression.Operator.EQUALS.equals(orderedCondition.getOperator())) {
66-
fResult.add(new StandardComparisonFixOperation(visited, comparisonMI, InfixExpression.Operator.LESS));
67-
} else {
68-
fResult.add(new StandardComparisonFixOperation(visited, comparisonMI, InfixExpression.Operator.GREATER_EQUALS));
60+
if (comparisonMI != null) {
61+
IMethodBinding comparisonMethodBinding= comparisonMI.resolveMethodBinding();
62+
if (comparisonMethodBinding != null) {
63+
IMethodBinding methodDeclaration= comparisonMethodBinding.getMethodDeclaration();
64+
ITypeBinding declaringClass= methodDeclaration.getDeclaringClass();
65+
if (declaringClass != null) {
66+
boolean knownComparison= false;
67+
if (comparisonMethodBinding.getName().equals("compareTo") && isFromClass(declaringClass, "java.lang.Comparable") //$NON-NLS-1$ //$NON-NLS-2$
68+
|| comparisonMethodBinding.getName().equals("compareToIgnoreCase") && isFromClass(declaringClass, "java.lang.String") //$NON-NLS-1$ //$NON-NLS-2$
69+
|| comparisonMethodBinding.getName().equals("compare") && isFromClass(declaringClass, "java.util.Comparator")) { //$NON-NLS-1$ //$NON-NLS-2$
70+
knownComparison= true;
71+
}
72+
if (knownComparison) {
73+
Long literalValue= ASTNodes.getIntegerLiteral(orderedCondition.getSecondOperand());
74+
75+
if (literalValue != null
76+
&& literalValue.compareTo(0L) != 0
77+
&& comparisonMI.getExpression() != null
78+
&& !ASTNodes.is(comparisonMI.getExpression(), ThisExpression.class)) {
79+
if (literalValue.compareTo(0L) < 0) {
80+
if (InfixExpression.Operator.EQUALS.equals(orderedCondition.getOperator())) {
81+
fResult.add(new StandardComparisonFixOperation(visited, comparisonMI, InfixExpression.Operator.LESS));
82+
} else {
83+
fResult.add(new StandardComparisonFixOperation(visited, comparisonMI, InfixExpression.Operator.GREATER_EQUALS));
84+
}
85+
} else if (InfixExpression.Operator.EQUALS.equals(orderedCondition.getOperator())) {
86+
fResult.add(new StandardComparisonFixOperation(visited, comparisonMI, InfixExpression.Operator.GREATER));
87+
} else {
88+
fResult.add(new StandardComparisonFixOperation(visited, comparisonMI, InfixExpression.Operator.LESS_EQUALS));
89+
}
90+
91+
return false;
92+
}
93+
}
6994
}
70-
} else if (InfixExpression.Operator.EQUALS.equals(orderedCondition.getOperator())) {
71-
fResult.add(new StandardComparisonFixOperation(visited, comparisonMI, InfixExpression.Operator.GREATER));
72-
} else {
73-
fResult.add(new StandardComparisonFixOperation(visited, comparisonMI, InfixExpression.Operator.LESS_EQUALS));
7495
}
75-
76-
return false;
7796
}
7897
}
7998

8099
return true;
81100
}
101+
102+
private boolean isFromClass(ITypeBinding declaringClass, String name) {
103+
if (declaringClass == null) {
104+
return false;
105+
}
106+
if (declaringClass.getErasure().getQualifiedName().equals(name)) {
107+
return true;
108+
}
109+
ITypeBinding[] interfaces= declaringClass.getInterfaces();
110+
for (ITypeBinding anInterface : interfaces) {
111+
if (isFromClass(anInterface, name)) {
112+
return true;
113+
}
114+
}
115+
ITypeBinding superClass= declaringClass.getSuperclass();
116+
return isFromClass(superClass, name);
117+
}
82118
}
83119

84120
public static class StandardComparisonFixOperation extends CompilationUnitRewriteOperation {

org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29182,6 +29182,7 @@ public void testDoNotUseStandardComparison() throws Exception {
2918229182
package test1;
2918329183

2918429184
import java.util.Comparator;
29185+
import java.util.Random;
2918529186

2918629187
public class E implements Comparator<Double> {
2918729188
public boolean doNotRefactorValidCases() {
@@ -29240,6 +29241,19 @@ public boolean doNotRefactorLocalComparingToZero() {
2924029241
public int compare(Double o1, Double o2) {
2924129242
return Double.compare(o1, o2) + 100;
2924229243
}
29244+
29245+
public void doNotRefactorIssue2221() {
29246+
if (getVal().intValue() == 1)
29247+
System.out.println("1");
29248+
if (getVal().intValue() == 2)
29249+
System.out.println("2");
29250+
if (getVal().intValue() == -1)
29251+
System.out.println("-1");
29252+
}
29253+
29254+
private Integer getVal() {
29255+
return new Random().nextInt();
29256+
}
2924329257
}
2924429258
""";
2924529259
ICompilationUnit cu= pack.createCompilationUnit("E.java", sample, false, null);

0 commit comments

Comments
 (0)