From 1db8b83dfff4427515b5032655ca6e5696494879 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 24 May 2017 14:12:51 +0700 Subject: [PATCH] IDEA-173177 Inspection: Comparing 'compareTo()` or 'Comparator.compare()' result with 1 / -1 --- .../afterComparableDirect.java | 10 ++ .../afterComparableVar.java | 17 +++ .../afterComparator.java | 10 ++ .../beforeComparableDirect.java | 10 ++ .../beforeComparableVar.java | 17 +++ .../beforeComparator.java | 10 ++ ...paratorResultComparisonInspectionTest.java | 39 ++++++ .../src/messages/InspectionsBundle.properties | 5 +- .../ComparatorResultComparisonInspection.java | 131 ++++++++++++++++++ .../ComparatorResultComparison.html | 10 ++ resources/src/META-INF/IdeaPlugin.xml | 4 + 11 files changed, 262 insertions(+), 1 deletion(-) create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparableDirect.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparableVar.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparator.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparableDirect.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparableVar.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparator.java create mode 100644 java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/ComparatorResultComparisonInspectionTest.java create mode 100644 plugins/InspectionGadgets/src/com/intellij/codeInspection/ComparatorResultComparisonInspection.java create mode 100644 resources-en/src/inspectionDescriptions/ComparatorResultComparison.html diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparableDirect.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparableDirect.java new file mode 100644 index 000000000000..7e801fd4566c --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparableDirect.java @@ -0,0 +1,10 @@ +// "Replace with '> 0'" "true" +import java.util.*; + +class Test { + void test(String str) { + if(str.compareTo("xyz") > 0) { + System.out.println("Oops"); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparableVar.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparableVar.java new file mode 100644 index 000000000000..06183fdd5091 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparableVar.java @@ -0,0 +1,17 @@ +// "Fix all 'Comparison of compare method result with specific constant' problems in file" "true" +import java.util.*; + +class Test { + String test(Comparable c1, Comparable c2) { + int result = c1.compareTo(c2); + if(0 == result) { + return "equal"; + } else if(0 > result) { + return "less"; + } else if(0 < result) { + return "greater"; + } else { + return "impossible"; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparator.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparator.java new file mode 100644 index 000000000000..62fcc74b3239 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/afterComparator.java @@ -0,0 +1,10 @@ +// "Replace with '>= 0'" "true" +import java.util.*; + +class Test { + void test(Comparator cmp) { + if(cmp.compare("a", "b") >= 0) { + System.out.println("Oops"); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparableDirect.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparableDirect.java new file mode 100644 index 000000000000..9488a8318b07 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparableDirect.java @@ -0,0 +1,10 @@ +// "Replace with '> 0'" "true" +import java.util.*; + +class Test { + void test(String str) { + if(str.compareTo("xyz") == 1) { + System.out.println("Oops"); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparableVar.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparableVar.java new file mode 100644 index 000000000000..e6b4e5360bc9 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparableVar.java @@ -0,0 +1,17 @@ +// "Fix all 'Comparison of compare method result with specific constant' problems in file" "true" +import java.util.*; + +class Test { + String test(Comparable c1, Comparable c2) { + int result = c1.compareTo(c2); + if(0 == result) { + return "equal"; + } else if(-1 == result) { + return "less"; + } else if(1 == result) { + return "greater"; + } else { + return "impossible"; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparator.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparator.java new file mode 100644 index 000000000000..4e6e4a6d6dbe --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison/beforeComparator.java @@ -0,0 +1,10 @@ +// "Replace with '>= 0'" "true" +import java.util.*; + +class Test { + void test(Comparator cmp) { + if(cmp.compare("a", "b") != -1) { + System.out.println("Oops"); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/ComparatorResultComparisonInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/ComparatorResultComparisonInspectionTest.java new file mode 100644 index 000000000000..eaf8d367b038 --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/ComparatorResultComparisonInspectionTest.java @@ -0,0 +1,39 @@ +/* + * Copyright 2000-2017 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.intellij.java.codeInsight.daemon.quickFix; + +import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase; +import com.intellij.codeInspection.ComparatorResultComparisonInspection; +import com.intellij.codeInspection.LocalInspectionTool; +import org.jetbrains.annotations.NotNull; + + +public class ComparatorResultComparisonInspectionTest extends LightQuickFixParameterizedTestCase { + @NotNull + @Override + protected LocalInspectionTool[] configureLocalInspectionTools() { + return new LocalInspectionTool[]{ + new ComparatorResultComparisonInspection() + }; + } + + public void test() throws Exception { doAllTests(); } + + @Override + protected String getBasePath() { + return "/codeInsight/daemonCodeAnalyzer/quickFix/comparatorResultComparison"; + } +} \ No newline at end of file diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 9f822ead32c4..3168fe550df9 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -868,4 +868,7 @@ inspection.replace.with.trivial.lambda.fix.family.name=Replace with trivial lamb inspection.replace.with.trivial.lambda.fix.name=Replace with lambda returning ''{0}'' inspection.useless.null.check.message=Useless null-check: {0} is never null -inspection.useless.null.check.fix.family.name=Remove useless null-check \ No newline at end of file +inspection.useless.null.check.fix.family.name=Remove useless null-check + +inspection.comparator.result.comparison.display.name=Comparison of compare method result with specific constant +inspection.comparator.result.comparison.fix.family.name=Fix comparator result comparison diff --git a/plugins/InspectionGadgets/src/com/intellij/codeInspection/ComparatorResultComparisonInspection.java b/plugins/InspectionGadgets/src/com/intellij/codeInspection/ComparatorResultComparisonInspection.java new file mode 100644 index 000000000000..b5199c053703 --- /dev/null +++ b/plugins/InspectionGadgets/src/com/intellij/codeInspection/ComparatorResultComparisonInspection.java @@ -0,0 +1,131 @@ +/* + * Copyright 2000-2017 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.intellij.codeInspection; + +import com.intellij.codeInspection.dataFlow.value.DfaRelationValue.RelationType; +import com.intellij.openapi.project.Project; +import com.intellij.psi.*; +import com.intellij.psi.controlFlow.DefUseUtil; +import com.intellij.psi.tree.IElementType; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.PsiUtil; +import com.intellij.util.ObjectUtils; +import com.siyeh.InspectionGadgetsBundle; +import com.siyeh.ig.callMatcher.CallMatcher; +import com.siyeh.ig.psiutils.CommentTracker; +import com.siyeh.ig.psiutils.ExpressionUtils; +import org.jetbrains.annotations.Nls; +import org.jetbrains.annotations.NotNull; + +public class ComparatorResultComparisonInspection extends BaseJavaBatchLocalInspectionTool { + private static final CallMatcher COMPARE_METHOD = CallMatcher.anyOf( + CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_COMPARATOR, "compare").parameterCount(2), + CallMatcher.instanceCall(CommonClassNames.JAVA_LANG_COMPARABLE, "compareTo").parameterCount(1) + ); + + @NotNull + @Override + public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) { + return new JavaElementVisitor() { + @Override + public void visitMethodCallExpression(PsiMethodCallExpression call) { + if (!COMPARE_METHOD.test(call)) return; + checkComparison(call); + PsiElement parent = PsiUtil.skipParenthesizedExprUp(call.getParent()); + if (parent instanceof PsiLocalVariable) { + PsiLocalVariable var = (PsiLocalVariable)parent; + PsiCodeBlock block = PsiTreeUtil.getParentOfType(var, PsiCodeBlock.class); + if (block != null) { + for (PsiElement element : DefUseUtil.getRefs(block, var, var.getInitializer())) { + checkComparison(element); + } + } + } + } + + private void checkComparison(PsiElement compareExpression) { + PsiBinaryExpression binOp = + ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprUp(compareExpression.getParent()), PsiBinaryExpression.class); + if (binOp == null) return; + PsiJavaToken sign = binOp.getOperationSign(); + IElementType tokenType = sign.getTokenType(); + if (!tokenType.equals(JavaTokenType.EQEQ) && !tokenType.equals(JavaTokenType.NE)) return; + PsiExpression constOperand = + PsiTreeUtil.isAncestor(binOp.getLOperand(), compareExpression, false) ? binOp.getROperand() : binOp.getLOperand(); + if (constOperand == null) return; + Object constantExpression = ExpressionUtils.computeConstantExpression(constOperand); + if (!(constantExpression instanceof Integer)) return; + int value = ((Integer)constantExpression).intValue(); + if (value == 0) return; + RelationType relationType = value < 0 ? RelationType.LT : RelationType.GT; + if (tokenType.equals(JavaTokenType.NE)) { + relationType = relationType.getNegated(); + } + boolean jodaCondition = constOperand == binOp.getLOperand(); + if (jodaCondition) { + relationType = relationType.getFlipped(); + } + holder.registerProblem(sign, InspectionsBundle.message("inspection.comparator.result.comparison.display.name"), + new ComparatorComparisonFix(jodaCondition, relationType)); + } + }; + } + + private static class ComparatorComparisonFix implements LocalQuickFix { + private final boolean myJodaCondition; + private final RelationType myType; + + public ComparatorComparisonFix(boolean jodaCondition, RelationType type) { + myJodaCondition = jodaCondition; + myType = type; + } + + @Nls + @NotNull + @Override + public String getName() { + return InspectionGadgetsBundle.message("replace.with", getReplacement()); + } + + @NotNull + private String getReplacement() { + return myJodaCondition ? "0 "+myType : myType + " 0"; + } + + @Nls + @NotNull + @Override + public String getFamilyName() { + return InspectionsBundle.message("inspection.comparator.result.comparison.fix.family.name"); + } + + @Override + public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) { + PsiBinaryExpression binOp = PsiTreeUtil.getParentOfType(descriptor.getStartElement(), PsiBinaryExpression.class); + if (binOp == null) return; + CommentTracker ct = new CommentTracker(); + String replacement; + if(myJodaCondition) { + PsiExpression operand = binOp.getROperand(); + if (operand == null) return; + replacement = getReplacement() + ct.text(operand); + } else { + replacement = ct.text(binOp.getLOperand()) + getReplacement(); + } + ct.replaceAndRestoreComments(binOp, replacement); + } + } +} diff --git a/resources-en/src/inspectionDescriptions/ComparatorResultComparison.html b/resources-en/src/inspectionDescriptions/ComparatorResultComparison.html new file mode 100644 index 000000000000..8e8faa79cc9f --- /dev/null +++ b/resources-en/src/inspectionDescriptions/ComparatorResultComparison.html @@ -0,0 +1,10 @@ + + +This inspection warns when result of Comparator.compare or Comparable.compareTo is compared with + specific non-zero constant (like if(a.compareTo(b) == -1)). By contract, + these methods can return any positive number (not just 1) or any negative number (not just -1), so comparing against + particular numbers is a bad practice. + +

New in 2017.2

+ + \ No newline at end of file diff --git a/resources/src/META-INF/IdeaPlugin.xml b/resources/src/META-INF/IdeaPlugin.xml index e35af4a6dbbd..69a5a730f6e7 100644 --- a/resources/src/META-INF/IdeaPlugin.xml +++ b/resources/src/META-INF/IdeaPlugin.xml @@ -772,6 +772,10 @@ +