diff --git a/java/java-analysis-impl/resources/messages/InspectionGadgetsBundle.properties b/java/java-analysis-impl/resources/messages/InspectionGadgetsBundle.properties index b558c1ba5d29..281c89123070 100644 --- a/java/java-analysis-impl/resources/messages/InspectionGadgetsBundle.properties +++ b/java/java-analysis-impl/resources/messages/InspectionGadgetsBundle.properties @@ -854,7 +854,8 @@ loop.statements.that.dont.loop.problem.descriptor=#ref statement do conditional.expression.with.identical.branches.problem.descriptor=Conditional expression #ref with identical branches #loc conditional.can.be.pushed.inside.expression.problem.descriptor=Conditional expression can be pushed inside branch #loc duplicate.condition.problem.descriptor=Duplicate condition #ref #loc -duplicate.condition.ignore.method.calls.option=Ignore conditions with side effects +duplicate.condition.ignore.method.calls.option=Ignore conditions with possible side effects +duplicate.condition.ignore.method.calls.option.description=If checked, conditions with potential side effects (for example, unknown method calls) will not be reported. Methods that are known to produce side effects will not be reported in any case. iterator.next.does.not.throw.nosuchelementexception.problem.descriptor=Iterator.#ref() which can't throw 'NoSuchElementException' #loc infinite.loop.statement.problem.descriptor=#ref statement cannot complete without throwing an exception #loc confusing.floating.point.literal.problem.descriptor=Confusing floating-point literal #ref #loc diff --git a/java/java-analysis-impl/src/com/siyeh/ig/controlflow/DuplicateConditionInspection.java b/java/java-analysis-impl/src/com/siyeh/ig/controlflow/DuplicateConditionInspection.java index 84fffa05fedb..400c25175174 100644 --- a/java/java-analysis-impl/src/com/siyeh/ig/controlflow/DuplicateConditionInspection.java +++ b/java/java-analysis-impl/src/com/siyeh/ig/controlflow/DuplicateConditionInspection.java @@ -20,6 +20,7 @@ import com.intellij.psi.*; import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; +import com.intellij.util.ThreeState; import com.siyeh.InspectionGadgetsBundle; import com.siyeh.ig.BaseInspection; import com.siyeh.ig.BaseInspectionVisitor; @@ -54,7 +55,8 @@ public final class DuplicateConditionInspection extends BaseInspection { @Override public @NotNull OptPane getOptionsPane() { return pane( - checkbox("ignoreSideEffectConditions", InspectionGadgetsBundle.message("duplicate.condition.ignore.method.calls.option"))); + checkbox("ignoreSideEffectConditions", InspectionGadgetsBundle.message("duplicate.condition.ignore.method.calls.option")) + .description(InspectionGadgetsBundle.message("duplicate.condition.ignore.method.calls.option.description"))); } @Override @@ -141,20 +143,16 @@ public final class DuplicateConditionInspection extends BaseInspection { private void findDuplicatesAccordingToSideEffects(Set conditions) { final List conditionList = new ArrayList<>(conditions); - if (ignoreSideEffectConditions) { - conditionList.replaceAll(cond -> SideEffectChecker.mayHaveSideEffects(cond) ? null : cond); - // Every condition having side-effect separates non-side-effect conditions into independent groups - // like: - // if(!readToken() || token == X || token == Y) ... - // else if(!readToken() || token == X || token == Y) ... - // here we analyze independently first ['token == X', 'token == Y'] and second ['token == X', 'token == Y'] - // thus no warning is issued. Such constructs often appear in parsers. - StreamEx.of(conditionList).groupRuns((a, b) -> a != null && b != null) - .filter(list -> list.size() >= 2).forEach(this::findDuplicates); - } - else { - findDuplicates(conditionList); - } + ThreeState wantedStatus = ignoreSideEffectConditions ? ThreeState.UNSURE : ThreeState.YES; + conditionList.replaceAll(cond -> SideEffectChecker.getSideEffectStatus(cond).isAtLeast(wantedStatus) ? null : cond); + // Every condition having side-effect separates non-side-effect conditions into independent groups + // like: + // if(!readToken() || token == X || token == Y) ... + // else if(!readToken() || token == X || token == Y) ... + // here we analyze independently first ['token == X', 'token == Y'] and second ['token == X', 'token == Y'] + // thus no warning is issued. Such constructs often appear in parsers. + StreamEx.of(conditionList).groupRuns((a, b) -> a != null && b != null) + .filter(list -> list.size() >= 2).forEach(this::findDuplicates); } private void findDuplicates(List conditions) { diff --git a/java/java-analysis-impl/src/com/siyeh/ig/psiutils/SideEffectChecker.java b/java/java-analysis-impl/src/com/siyeh/ig/psiutils/SideEffectChecker.java index 8464097d9977..7335de442cef 100644 --- a/java/java-analysis-impl/src/com/siyeh/ig/psiutils/SideEffectChecker.java +++ b/java/java-analysis-impl/src/com/siyeh/ig/psiutils/SideEffectChecker.java @@ -17,11 +17,13 @@ package com.siyeh.ig.psiutils; import com.intellij.codeInspection.dataFlow.ContractValue; import com.intellij.codeInspection.dataFlow.JavaMethodContractUtil; +import com.intellij.codeInspection.dataFlow.MutationSignature; import com.intellij.psi.*; import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.InheritanceUtil; import com.intellij.psi.util.*; import com.intellij.util.SmartList; +import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -68,6 +70,17 @@ public final class SideEffectChecker { return visitor.mayHaveSideEffects(); } + /** + * @param exp expression to test + * @return whether expression produces side effect. {@link ThreeState#UNSURE} means + * that unannotated method is called, which may or may not produce side effect. + */ + public static @NotNull ThreeState getSideEffectStatus(@NotNull PsiExpression exp) { + final SideEffectsVisitor visitor = new SideEffectsVisitor(null, exp); + exp.accept(visitor); + return visitor.getSideEffectStatus(); + } + public static boolean mayHaveSideEffects(@NotNull PsiElement element, @NotNull Predicate shouldIgnoreElement) { final SideEffectsVisitor visitor = new SideEffectsVisitor(null, element, shouldIgnoreElement); element.accept(visitor); @@ -139,7 +152,7 @@ public final class SideEffectChecker { private final @Nullable List mySideEffects; private final @NotNull PsiElement myStartElement; private final @NotNull Predicate myIgnorePredicate; - boolean found; + private @NotNull ThreeState found = ThreeState.NO; SideEffectsVisitor(@Nullable List sideEffects, @NotNull PsiElement startElement) { this(sideEffects, startElement, call -> false); @@ -152,11 +165,17 @@ public final class SideEffectChecker { } private boolean addSideEffect(PsiElement element) { - if (myIgnorePredicate.test(element)) return false; - found = true; + return addSideEffect(element, ThreeState.YES); + } + + private boolean addSideEffect(PsiElement element, ThreeState state) { + if (state == ThreeState.NO || myIgnorePredicate.test(element)) return false; + if (state == ThreeState.YES || state == ThreeState.UNSURE && found == ThreeState.NO) { + found = state; + } if(mySideEffects != null) { mySideEffects.add(element); - } else { + } else if (found == ThreeState.YES) { stopWalking(); } return true; @@ -171,24 +190,35 @@ public final class SideEffectChecker { @Override public void visitMethodCallExpression(@NotNull PsiMethodCallExpression expression) { final PsiMethod method = expression.resolveMethod(); - if (!isPure(method)) { - if (addSideEffect(expression)) return; - } + ThreeState sideEffect = getMethodSideEffect(method); + if (addSideEffect(expression, sideEffect)) return; super.visitMethodCallExpression(expression); } - protected static boolean isPure(PsiMethod method) { - if (method == null) return false; + private static @NotNull ThreeState getMethodSideEffect(PsiMethod method) { + if (method == null) { + return ThreeState.UNSURE; + } PsiField field = PropertyUtil.getFieldOfGetter(method); - if (field != null) return !field.hasModifierProperty(PsiModifier.VOLATILE); - return JavaMethodContractUtil.isPure(method) && !mayHaveExceptionalSideEffect(method); + if (field != null) { + return ThreeState.fromBoolean(field.hasModifierProperty(PsiModifier.VOLATILE)); + } + if (mayHaveExceptionalSideEffect(method)) { + return ThreeState.UNSURE; + } + MutationSignature mutationSignature = MutationSignature.fromMethod(method); + if (mutationSignature.isPure()) { + return ThreeState.NO; + } + if (mutationSignature.mutatesAnything() || PropertyUtil.getFieldOfSetter(method) != null) { + return ThreeState.YES; + } + return ThreeState.UNSURE; } @Override public void visitNewExpression(@NotNull PsiNewExpression expression) { - if (!expression.isArrayCreation() && !isSideEffectFreeConstructor(expression)) { - if (addSideEffect(expression)) return; - } + if (addSideEffect(expression, getConstructorSideEffect(expression))) return; super.visitNewExpression(expression); } @@ -269,6 +299,10 @@ public final class SideEffectChecker { } public boolean mayHaveSideEffects() { + return found != ThreeState.NO; + } + + public @NotNull ThreeState getSideEffectStatus() { return found; } } @@ -293,21 +327,30 @@ public final class SideEffectChecker { && mc.getReturnValue().isFail()); } - private static boolean isSideEffectFreeConstructor(@NotNull PsiNewExpression newExpression) { + private static @NotNull ThreeState getConstructorSideEffect(@NotNull PsiNewExpression newExpression) { + if (newExpression.isArrayCreation()) return ThreeState.NO; PsiAnonymousClass anonymousClass = newExpression.getAnonymousClass(); if (anonymousClass != null && anonymousClass.getInitializers().length == 0) { PsiClass baseClass = anonymousClass.getBaseClassType().resolve(); if (baseClass != null && baseClass.isInterface()) { - return true; + return ThreeState.NO; } } PsiJavaCodeReferenceElement classReference = newExpression.getClassReference(); PsiClass aClass = classReference == null ? null : (PsiClass)classReference.resolve(); String qualifiedName = aClass == null ? null : aClass.getQualifiedName(); - if (qualifiedName == null) return false; - if (ourSideEffectFreeClasses.contains(qualifiedName)) return true; + if (qualifiedName == null) return ThreeState.UNSURE; + if (ourSideEffectFreeClasses.contains(qualifiedName)) return ThreeState.NO; PsiMethod method = newExpression.resolveConstructor(); - if (method != null && JavaMethodContractUtil.isPure(method)) return true; + if (method != null) { + MutationSignature signature = MutationSignature.fromMethod(method); + if (signature.isPure()) { + return ThreeState.NO; + } + if (signature.mutatesAnything()) { + return ThreeState.YES; + } + } PsiFile file = aClass.getContainingFile(); PsiDirectory directory = file.getContainingDirectory(); @@ -318,21 +361,21 @@ public final class SideEffectChecker { if (CommonClassNames.DEFAULT_PACKAGE.equals(packageName) || "java.io".equals(packageName)) { PsiClass throwableClass = JavaPsiFacade.getInstance(aClass.getProject()).findClass(CommonClassNames.JAVA_LANG_THROWABLE, aClass.getResolveScope()); if (throwableClass != null && com.intellij.psi.util.InheritanceUtil.isInheritorOrSelf(aClass, throwableClass, true)) { - return true; + return ThreeState.NO; } } if (method == null) { PsiClass superClass = aClass.getSuperClass(); if (superClass != null && CommonClassNames.JAVA_LANG_OBJECT.equals(superClass.getQualifiedName())) { for (PsiClassInitializer initializer : aClass.getInitializers()) { - if (!initializer.hasModifierProperty(PsiModifier.STATIC)) return false; + if (!initializer.hasModifierProperty(PsiModifier.STATIC)) return ThreeState.UNSURE; } for (PsiField field : aClass.getFields()) { - if (!field.hasModifierProperty(PsiModifier.STATIC) && field.hasInitializer()) return false; + if (!field.hasModifierProperty(PsiModifier.STATIC) && field.hasInitializer()) return ThreeState.UNSURE; } - return true; + return ThreeState.NO; } } - return false; + return ThreeState.UNSURE; } } diff --git a/java/java-tests/testData/ig/com/siyeh/igtest/controlflow/duplicate_condition/DuplicateCondition.java b/java/java-tests/testData/ig/com/siyeh/igtest/controlflow/duplicate_condition/DuplicateCondition.java index fa8f25623374..cc14a4b8eda4 100644 --- a/java/java-tests/testData/ig/com/siyeh/igtest/controlflow/duplicate_condition/DuplicateCondition.java +++ b/java/java-tests/testData/ig/com/siyeh/igtest/controlflow/duplicate_condition/DuplicateCondition.java @@ -1,5 +1,7 @@ package com.siyeh.igtest.controlflow.duplicate_condition; +import java.util.*; + public class DuplicateCondition { void x(boolean b) { @@ -73,4 +75,8 @@ public class DuplicateCondition { System.out.println("two"); } } + + void testCollection(Set set) { + if (set.add("foo") || set.remove("bar") || set.add("foo") || set.remove("bar")) {} + } } \ No newline at end of file diff --git a/java/java-tests/testData/ig/com/siyeh/igtest/controlflow/duplicate_condition/DuplicateConditionNoSideEffect.java b/java/java-tests/testData/ig/com/siyeh/igtest/controlflow/duplicate_condition/DuplicateConditionNoSideEffect.java index 6e228bff9770..136382c39b4d 100644 --- a/java/java-tests/testData/ig/com/siyeh/igtest/controlflow/duplicate_condition/DuplicateConditionNoSideEffect.java +++ b/java/java-tests/testData/ig/com/siyeh/igtest/controlflow/duplicate_condition/DuplicateConditionNoSideEffect.java @@ -1,20 +1,7 @@ -/* - * 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.siyeh.igtest.controlflow.duplicate_condition; +import java.util.*; + public class DuplicateConditionNoSideEffect { public void foo() { @@ -76,4 +63,8 @@ public class DuplicateConditionNoSideEffect { { return true; } + + void testCollection(Set set) { + if (set.add("foo") || set.remove("bar") || set.add("foo") || set.remove("bar")) {} + } } \ No newline at end of file diff --git a/java/java-tests/testSrc/com/siyeh/ig/controlflow/DuplicateConditionInspectionTest.java b/java/java-tests/testSrc/com/siyeh/ig/controlflow/DuplicateConditionInspectionTest.java index c86798e0df5e..342b0c971aa1 100644 --- a/java/java-tests/testSrc/com/siyeh/ig/controlflow/DuplicateConditionInspectionTest.java +++ b/java/java-tests/testSrc/com/siyeh/ig/controlflow/DuplicateConditionInspectionTest.java @@ -1,10 +1,16 @@ package com.siyeh.ig.controlflow; import com.intellij.codeInspection.InspectionProfileEntry; +import com.intellij.testFramework.LightProjectDescriptor; import com.siyeh.ig.LightJavaInspectionTestCase; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; public class DuplicateConditionInspectionTest extends LightJavaInspectionTestCase { + @Override + protected @NotNull LightProjectDescriptor getProjectDescriptor() { + return JAVA_21_ANNOTATED; + } public void testDuplicateCondition() { doTest(); diff --git a/platform/util/src/com/intellij/util/ThreeState.java b/platform/util/src/com/intellij/util/ThreeState.java index 9c8dc961e5de..ed89df45585f 100644 --- a/platform/util/src/com/intellij/util/ThreeState.java +++ b/platform/util/src/com/intellij/util/ThreeState.java @@ -28,6 +28,23 @@ public enum ThreeState { return this == YES; } + /** + * @param other state to compare with + * @return true if the state is at least the same positive as the supplied one + */ + public boolean isAtLeast(@NotNull ThreeState other) { + switch (other) { + case YES: + return this == YES; + case UNSURE: + return this != NO; + case NO: + return true; + default: + throw new IllegalStateException("Unexpected value: " + other); + } + } + /** * @return {@code YES} if the given states contain {@code YES}, otherwise {@code UNSURE} if the given states contain {@code UNSURE}, otherwise {@code NO} */