IG: care about side effects in pointless compound assignment

plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/CommentTracker.java

@@ -23,7 +23,7 @@ import java.util.function.Predicate;
  * @author Tagir Valeev
  */
 public class CommentTracker {
-  private Set<PsiElement> ignoredParents = new HashSet<>();
+  private final Set<PsiElement> ignoredParents = new HashSet<>();
   private List<PsiComment> comments = new ArrayList<>();
 
   /**
@@ -174,6 +174,26 @@ public class CommentTracker {
     return result;
   }
 
+  public static @NotNull PsiElement replaceWithSubexpressionAndRestoreComments(@NotNull PsiExpression expression,
+                                                                               @NotNull PsiExpression replacement) {
+    if (!PsiTreeUtil.isAncestor(expression, replacement, true)) throw new IllegalArgumentException("replacement is not a subexpression");
+    final CommentTracker tracker = new CommentTracker();
+    tracker.markUnchanged(replacement);
+    tracker.grabComments(expression);
+    final PsiElement parent = expression.getParent();
+    final int limit = replacement.getTextOffset();
+    PsiElement anchor = expression;
+    for (PsiComment comment : tracker.comments) {
+      if (comment.getTextOffset() < limit) {
+        parent.addBefore(comment, anchor);
+      }
+      else {
+        anchor = parent.addAfter(comment, anchor);
+      }
+    }
+    return expression.replace(replacement);
+  }
+
   /**
    * Creates a replacement element from the text and replaces given element,
    * collecting all the comments inside it and restores comments putting them
@@ -198,26 +218,24 @@ public class CommentTracker {
 
   private static @NotNull PsiElement createElement(@NotNull PsiElement element, @NotNull String text) {
     PsiElementFactory factory = JavaPsiFacade.getElementFactory(element.getProject());
-    PsiElement replacement;
     if (element instanceof PsiExpression) {
-      replacement = factory.createExpressionFromText(text, element);
+      return factory.createExpressionFromText(text, element);
     }
     else if (element instanceof PsiStatement) {
-      replacement = factory.createStatementFromText(text, element);
+      return factory.createStatementFromText(text, element);
     }
     else if (element instanceof PsiTypeElement) {
-      replacement = factory.createTypeElementFromText(text, element);
+      return factory.createTypeElementFromText(text, element);
     }
     else if (element instanceof PsiIdentifier) {
-      replacement = factory.createIdentifier(text);
+      return factory.createIdentifier(text);
     }
     else if (element instanceof PsiComment) {
-      replacement = factory.createCommentFromText(text, element);
+      return factory.createCommentFromText(text, element);
     }
     else {
       throw new IllegalArgumentException("Unsupported element type: " + element);
     }
-    return replacement;
   }
 
   /**

plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java

@@ -21,6 +21,7 @@ import com.intellij.psi.*;
 import com.intellij.psi.tree.IElementType;
 import com.intellij.psi.util.PropertyUtil;
 import com.intellij.psi.util.PsiTreeUtil;
+import com.intellij.util.SmartList;
 import gnu.trove.THashSet;
 import one.util.streamex.StreamEx;
 import org.jetbrains.annotations.NotNull;
@@ -79,7 +80,7 @@ public class SideEffectChecker {
   }
 
   public static List<PsiExpression> extractSideEffectExpressions(@NotNull PsiExpression element) {
-    List<PsiElement> list = new ArrayList<>();
+    List<PsiElement> list = new SmartList<>();
     element.accept(new SideEffectsVisitor(list, element));
     return StreamEx.of(list).select(PsiExpression.class).toList();
   }

plugins/InspectionGadgets/src/com/siyeh/ig/controlflow/PointlessBooleanExpressionInspection.java

@@ -36,6 +36,7 @@ import org.jetbrains.annotations.Nullable;
 
 import javax.swing.*;
 import java.util.*;
+import java.util.function.Supplier;
 
 public class PointlessBooleanExpressionInspection extends BaseInspection {
   private enum BooleanExpressionKind {
@@ -78,9 +79,9 @@ public class PointlessBooleanExpressionInspection extends BaseInspection {
   @NotNull
   public String buildErrorString(Object... infos) {
     final String replacement = (String)infos[1];
-    if (replacement.isEmpty()) {
-      final PsiAssignmentExpression expression = (PsiAssignmentExpression)infos[0];
-      return InspectionGadgetsBundle.message("boolean.expression.does.not.modify.problem.descriptor", expression);
+    final PsiExpression expression = (PsiExpression)infos[0];
+    if (replacement.isEmpty() && expression instanceof PsiAssignmentExpression) {
+      return InspectionGadgetsBundle.message("boolean.expression.does.not.modify.problem.descriptor", expression.getText());
     }
     return InspectionGadgetsBundle.message("boolean.expression.can.be.simplified.problem.descriptor", replacement);
   }
@@ -305,7 +306,7 @@ public class PointlessBooleanExpressionInspection extends BaseInspection {
   @Override
   public InspectionGadgetsFix buildFix(Object... infos) {
     final String replacement = (String)infos[1];
-    if (replacement.isEmpty()) {
+    if (replacement.isEmpty() && infos[0] instanceof PsiAssignmentExpression) {
       return new RemovePointlessBooleanExpressionFix();
     }
     boolean hasSideEffect = (boolean)infos[2];
@@ -323,7 +324,19 @@ public class PointlessBooleanExpressionInspection extends BaseInspection {
 
     @Override
     protected void doFix(Project project, ProblemDescriptor descriptor) {
-      new CommentTracker().deleteAndRestoreComments(descriptor.getPsiElement().getParent());
+      final PsiElement element = descriptor.getPsiElement();
+      if (!(element instanceof PsiAssignmentExpression)) {
+        return;
+      }
+      final PsiAssignmentExpression assignmentExpression = (PsiAssignmentExpression)element;
+      final List<PsiExpression> sideEffects = SideEffectChecker.extractSideEffectExpressions(assignmentExpression.getLExpression());
+      assert sideEffects.size() < 2;
+      if (!sideEffects.isEmpty()) {
+        CommentTracker.replaceWithSubexpressionAndRestoreComments(assignmentExpression, sideEffects.get(0));
+      }
+      else {
+        new CommentTracker().deleteAndRestoreComments(element.getParent());
+      }
     }
   }
 
@@ -430,11 +443,10 @@ public class PointlessBooleanExpressionInspection extends BaseInspection {
         return;
       }
 
-      String replacement = buildSimplifiedExpression(expression, new StringBuilder(), new CommentTracker()).toString();
-
-      if (parent instanceof PsiLambdaExpression &&
-          !LambdaUtil.isSafeLambdaBodyReplacement((PsiLambdaExpression)parent,
-                                                  () -> JavaPsiFacade.getElementFactory(expression.getProject()).createExpressionFromText(replacement, expression))) {
+      final String replacement = buildSimplifiedExpression(expression, new StringBuilder(), new CommentTracker()).toString();
+      final Supplier<PsiElement> newBodySupplier =
+        () -> JavaPsiFacade.getElementFactory(expression.getProject()).createExpressionFromText(replacement, expression);
+      if (parent instanceof PsiLambdaExpression && !LambdaUtil.isSafeLambdaBodyReplacement((PsiLambdaExpression)parent, newBodySupplier)) {
         return;
       }
 

plugins/InspectionGadgets/test/com/siyeh/igfixes/pointlessboolean/CompoundAssignmentSideEffect.after.java

@@ -0,0 +1,16 @@
+class CompoundAssignmentSideEffect {
+
+  void m() {
+    createSomeObject(/*1*/)// 2
+    /*3*//*4*/
+    ;
+  }
+
+  X createSomeObject() {
+    return new X();
+  }
+
+  class X {
+    boolean b = false;
+  }
+}

plugins/InspectionGadgets/test/com/siyeh/igfixes/pointlessboolean/CompoundAssignmentSideEffect.java

@@ -0,0 +1,15 @@
+class CompoundAssignmentSideEffect {
+
+  void m() {
+    createSomeObject(/*1*/).b // 2
+      &= /*3*//*4*/ true<caret>;
+  }
+
+  X createSomeObject() {
+    return new X();
+  }
+
+  class X {
+    boolean b = false;
+  }
+}

plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/PointlessBooleanExpressionFixTest.java

@@ -20,9 +20,10 @@ public class PointlessBooleanExpressionFixTest extends IGQuickFixesTestCase {
   public void testNegation() { doTest(); }
   public void testPolyadic() { doTest(); }
   public void testBoxed() { doTest(); }
-  public void testSideEffects() { doTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix.sideEffect"));}
-  public void testSideEffectsField() { doTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix.sideEffect"));}
+  public void testSideEffects() { doTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix.sideEffect")); }
+  public void testSideEffectsField() { doTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix.sideEffect")); }
   public void testCompoundAssignment1() { doTest(InspectionGadgetsBundle.message("boolean.expression.remove.compound.assignment.quickfix")); }
   public void testCompoundAssignment2() { doTest(); }
   public void testCompoundAssignment3() { doTest(); }
+  public void testCompoundAssignmentSideEffect() { doTest(InspectionGadgetsBundle.message("boolean.expression.remove.compound.assignment.quickfix")); }
 }