SimplifyBooleanExpressionFix: ability to extract side effects

Fixes IDEA-181074 Invalid suggested simplification for Enum.valueOf
2026-09-27 10:03:11 +07:00 · 2017-10-26 12:07:49 +07:00
parent 5233c4c799
commit a18b54ba14
10 changed files with 157 additions and 52 deletions
@@ -6,7 +6,6 @@ import com.intellij.codeInsight.AnnotationUtil;
 import com.intellij.codeInsight.NullableNotNullManager;
 import com.intellij.codeInsight.PsiEquivalenceUtil;
 import com.intellij.codeInsight.daemon.GroupNames;
-import com.intellij.codeInsight.daemon.impl.quickfix.SimplifyBooleanExpressionFix;
 import com.intellij.codeInsight.intention.impl.AddNotNullAnnotationFix;
 import com.intellij.codeInsight.intention.impl.AddNullableAnnotationFix;
 import com.intellij.codeInspection.*;
@@ -841,8 +840,8 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool
  }

  @Nullable
-  private static LocalQuickFix createSimplifyBooleanExpressionFix(PsiElement element, final boolean value) {
-    SimplifyBooleanExpressionFix fix = createIntention(element, value);
+  private LocalQuickFix createSimplifyBooleanExpressionFix(PsiElement element, final boolean value) {
+    LocalQuickFixOnPsiElement fix = createSimplifyBooleanFix(element, value);
    if (fix == null) return null;
    final String text = fix.getText();
    return new LocalQuickFix() {
@@ -856,7 +855,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool
      public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
        final PsiElement psiElement = descriptor.getPsiElement();
        if (psiElement == null) return;
-        final SimplifyBooleanExpressionFix fix = createIntention(psiElement, value);
+        final LocalQuickFixOnPsiElement fix = createSimplifyBooleanFix(psiElement, value);
        if (fix == null) return;
        try {
          LOG.assertTrue(psiElement.isValid());
@@ -880,24 +879,10 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool
    return new SimplifyToAssignmentFix();
  }

-  private static SimplifyBooleanExpressionFix createIntention(PsiElement element, boolean value) {
-    if (!(element instanceof PsiExpression)) return null;
-    if (PsiTreeUtil.findChildOfType(element, PsiAssignmentExpression.class) != null) return null;
-
-    final PsiExpression expression = (PsiExpression)element;
-    while (element.getParent() instanceof PsiExpression) {
-      element = element.getParent();
-    }
-    final SimplifyBooleanExpressionFix fix = new SimplifyBooleanExpressionFix(expression, value);
-    // simplify intention already active
-    if (!fix.isAvailable() ||
-        SimplifyBooleanExpressionFix.canBeSimplified((PsiExpression)element)) {
-      return null;
-    }
-    return fix;
+  protected LocalQuickFixOnPsiElement createSimplifyBooleanFix(PsiElement element, boolean value) {
+    return null;
  }

-
  @Override
  @NotNull
  public String getDisplayName() {
@@ -1,18 +1,4 @@
-/*
- * Copyright 2000-2016 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.
- */
+// Copyright 2000-2017 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.

 package com.intellij.codeInsight.daemon.impl.quickfix;

@@ -30,11 +16,11 @@ import com.intellij.psi.controlFlow.ControlFlowUtil;
 import com.intellij.psi.tree.IElementType;
 import com.intellij.psi.util.PsiTreeUtil;
 import com.intellij.psi.util.PsiUtil;
+import com.intellij.refactoring.util.RefactoringUtil;
 import com.intellij.util.IncorrectOperationException;
+import com.intellij.util.ObjectUtils;
 import com.intellij.util.containers.ContainerUtil;
-import com.siyeh.ig.psiutils.DeclarationSearchUtils;
-import com.siyeh.ig.psiutils.ParenthesesUtils;
-import com.siyeh.ig.psiutils.SideEffectChecker;
+import com.siyeh.ig.psiutils.*;
 import org.jetbrains.annotations.NonNls;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -60,7 +46,23 @@ public class SimplifyBooleanExpressionFix extends LocalQuickFixOnPsiElement {
  @NotNull
  public String getText() {
    PsiExpression subExpression = getSubExpression();
-    return subExpression == null ? getFamilyName() : getIntentionText(subExpression, mySubExpressionValue);
+    if (subExpression == null) {
+      return getFamilyName();
+    }
+    return getIntentionText(subExpression, mySubExpressionValue) + (shouldExtractSideEffect() ? " extracting side effects" : "");
+  }
+
+  private boolean shouldExtractSideEffect() {
+    PsiExpression subExpression = getSubExpression();
+    if (subExpression != null &&
+        SideEffectChecker.mayHaveSideEffects(subExpression)) {
+      if (ControlFlowUtils.canExtractStatement(subExpression)) return true;
+      if (!mySubExpressionValue) {
+        PsiElement parent = PsiUtil.skipParenthesizedExprUp(subExpression.getParent());
+        if (parent instanceof PsiWhileStatement || parent instanceof PsiForStatement) return true;
+      }
+    }
+    return false;
  }

  @NotNull
@@ -113,26 +115,30 @@ public class SimplifyBooleanExpressionFix extends LocalQuickFixOnPsiElement {
  @Override
  public void invoke(@NotNull final Project project, @NotNull PsiFile file, @NotNull PsiElement startElement, @NotNull PsiElement endElement) {
    if (!isAvailable()) return;
-    simplifyExpression(project, getSubExpression(), mySubExpressionValue);
-  }
-
-  public static void simplifyExpression(Project project, final PsiExpression subExpression, final Boolean subExpressionValue) {
-    PsiExpression expression;
-    if (subExpressionValue == null) {
-      expression = subExpression;
-    }
-    else {
-      final PsiElementFactory factory = JavaPsiFacade.getElementFactory(project);
-      final PsiExpression constExpression = factory.createExpressionFromText(Boolean.toString(subExpressionValue.booleanValue()), subExpression);
-      expression = (PsiExpression)subExpression.replace(constExpression);
+    PsiExpression subExpression = getSubExpression();
+    if (subExpression == null) return;
+    if (shouldExtractSideEffect()) {
+      subExpression = RefactoringUtil.ensureCodeBlock(subExpression);
+      LOG.assertTrue(subExpression != null);
+      PsiStatement anchor = ObjectUtils.tryCast(RefactoringUtil.getParentStatement(subExpression, false), PsiStatement.class);
+      LOG.assertTrue(anchor != null);
+      List<PsiExpression> sideEffects = SideEffectChecker.extractSideEffectExpressions(subExpression);
+      PsiStatement[] statements = StatementExtractor.generateStatements(sideEffects, subExpression);
+      if (statements.length > 0) {
+        BlockUtils.addBefore(anchor, statements);
+      }
+      LOG.assertTrue(subExpression.isValid());
    }
+    final PsiElementFactory factory = JavaPsiFacade.getElementFactory(project);
+    final PsiExpression constExpression = factory.createExpressionFromText(Boolean.toString(mySubExpressionValue), subExpression);
+    PsiExpression expression = (PsiExpression)subExpression.replace(constExpression);
    while (expression.getParent() instanceof PsiExpression) {
      expression = (PsiExpression)expression.getParent();
    }
    simplifyExpression(expression);
  }

-  public static boolean simplifyIfOrLoopStatement(final PsiExpression expression) throws IncorrectOperationException {
+  private static boolean simplifyIfOrLoopStatement(final PsiExpression expression) throws IncorrectOperationException {
    boolean condition = Boolean.parseBoolean(expression.getText());
    if (!(expression instanceof PsiLiteralExpression) || !PsiType.BOOLEAN.equals(expression.getType())) return false;

@@ -16,6 +16,7 @@
 package com.intellij.codeInspection.dataFlow;

 import com.intellij.codeInsight.NullableNotNullDialog;
+import com.intellij.codeInsight.daemon.impl.quickfix.SimplifyBooleanExpressionFix;
 import com.intellij.codeInspection.*;
 import com.intellij.codeInspection.dataFlow.fix.SurroundWithRequireNonNullFix;
 import com.intellij.codeInspection.nullable.NullableStuffInspection;
@@ -23,6 +24,7 @@ import com.intellij.openapi.project.Project;
 import com.intellij.pom.java.LanguageLevel;
 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.refactoring.util.RefactoringUtil;
 import com.intellij.util.IncorrectOperationException;
@@ -71,6 +73,20 @@ public class DataFlowInspection extends DataFlowInspectionBase {
    return new IntroduceVariableFix(true);
  }

+  protected LocalQuickFixOnPsiElement createSimplifyBooleanFix(PsiElement element, boolean value) {
+    if (!(element instanceof PsiExpression)) return null;
+    if (PsiTreeUtil.findChildOfType(element, PsiAssignmentExpression.class) != null) return null;
+
+    final PsiExpression expression = (PsiExpression)element;
+    while (element.getParent() instanceof PsiExpression) {
+      element = element.getParent();
+    }
+    final SimplifyBooleanExpressionFix fix = new SimplifyBooleanExpressionFix(expression, value);
+    // simplify intention already active
+    if (!fix.isAvailable() || SimplifyBooleanExpressionFix.canBeSimplified((PsiExpression)element)) return null;
+    return fix;
+  }
+
  private static boolean isVolatileFieldReference(PsiExpression qualifier) {
    PsiElement target = qualifier instanceof PsiReferenceExpression ? ((PsiReferenceExpression)qualifier).resolve() : null;
    return target instanceof PsiField && ((PsiField)target).hasModifierProperty(PsiModifier.VOLATILE);
@@ -0,0 +1,13 @@
+class SideEffectReturn {
+  private boolean isValidValue(String value) {
+    if (!value.isEmpty())
+      return <warning descr="Condition 'Test.valueOf(value) != null' is always 'true'">Test.valueOf<caret>(value) != null</warning>;
+    return false;
+  }
+
+  enum Test {
+    A,
+    B
+
+  }
+}
@@ -0,0 +1,15 @@
+class SideEffectReturn {
+  private boolean isValidValue(String value) {
+    if (!value.isEmpty()) {
+        Test.valueOf(value);
+        return true;
+    }
+    return false;
+  }
+
+  enum Test {
+    A,
+    B
+
+  }
+}
@@ -0,0 +1,15 @@
+class SideEffectReturn {
+  private boolean isValidValue(String value) {
+    try {
+      return <warning descr="Condition 'Test.valueOf(value) != null' is always 'true'">Test.valueOf(value) <caret>!= null</warning>;
+    } catch (IllegalArgumentException e) {
+      return false;
+    }
+  }
+
+  enum Test {
+    A,
+    B
+
+  }
+}
@@ -0,0 +1,16 @@
+class SideEffectReturn {
+  private boolean isValidValue(String value) {
+    try {
+        Test.valueOf(value);
+        return true;
+    } catch (IllegalArgumentException e) {
+      return false;
+    }
+  }
+
+  enum Test {
+    A,
+    B
+
+  }
+}
@@ -0,0 +1,13 @@
+class SideEffectReturn {
+  private void testLoop(String value) {
+    while(<warning descr="Condition 'Test.valueOf(value) == null' is always 'false'">Test.v<caret>alueOf(value) == null</warning>) {
+
+    }
+  }
+
+  enum Test {
+    A,
+    B
+
+  }
+}
@@ -0,0 +1,11 @@
+class SideEffectReturn {
+  private void testLoop(String value) {
+      Test.valueOf(value);
+  }
+
+  enum Test {
+    A,
+    B
+
+  }
+}
@@ -485,6 +485,21 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase {
    checkIntentionResult("Remove 'for' statement");
  }

+  public void testSideEffectReturn() {
+    doTest();
+    checkIntentionResult("Simplify 'Test.valueOf(value) != null' to true extracting side effects");
+  }
+
+  public void testSideEffectNoBrace() {
+    doTest();
+    checkIntentionResult("Simplify 'Test.valueOf(value) != null' to true extracting side effects");
+  }
+
+  public void testSideEffectWhile() {
+    doTest();
+    checkIntentionResult("Remove 'while' statement extracting side effects");
+  }
+
  public void testUsingInterfaceConstant() { doTest();}

  //https://youtrack.jetbrains.com/issue/IDEA-162184