[java-inspections] IDEA-345238 Duplicate conditions: respect mutation signature

GitOrigin-RevId: b9271196151e985b5147b9264ea73d8e3118b59a

java/java-analysis-impl/resources/messages/InspectionGadgetsBundle.properties

@@ -854,7 +854,8 @@ loop.statements.that.dont.loop.problem.descriptor=<code>#ref</code> statement do
 conditional.expression.with.identical.branches.problem.descriptor=Conditional expression <code>#ref</code> 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 <code>#ref</code> #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=<code>Iterator.#ref()</code> which can't throw 'NoSuchElementException' #loc
 infinite.loop.statement.problem.descriptor=<code>#ref</code> statement cannot complete without throwing an exception #loc
 confusing.floating.point.literal.problem.descriptor=Confusing floating-point literal <code>#ref</code> #loc

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<PsiExpression> conditions) {
       final List<PsiExpression> 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<PsiExpression> conditions) {

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<? super PsiElement> shouldIgnoreElement) {
     final SideEffectsVisitor visitor = new SideEffectsVisitor(null, element, shouldIgnoreElement);
     element.accept(visitor);
@@ -139,7 +152,7 @@ public final class SideEffectChecker {
     private final @Nullable List<? super PsiElement> mySideEffects;
     private final @NotNull PsiElement myStartElement;
     private final @NotNull Predicate<? super PsiElement> myIgnorePredicate;
-    boolean found;
+    private @NotNull ThreeState found = ThreeState.NO;
 
     SideEffectsVisitor(@Nullable List<? super PsiElement> 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;
   }
 }

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<String> set) {
+    if (set.add("foo") || set.remove("bar") || set.add("foo") || set.remove("bar")) {}
+  }
 }

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<String> set) {
+    if (set.add("foo") || set.remove("bar") || set.add("foo") || set.remove("bar")) {}
+  }
 }

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();

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}
    */