From a18b54ba145a9e6d5cdb160db0e508bf1a9b5c81 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Thu, 26 Oct 2017 12:04:43 +0700 Subject: [PATCH] SimplifyBooleanExpressionFix: ability to extract side effects Fixes IDEA-181074 Invalid suggested simplification for Enum.valueOf --- .../dataFlow/DataFlowInspectionBase.java | 25 ++----- .../SimplifyBooleanExpressionFix.java | 70 ++++++++++--------- .../dataFlow/DataFlowInspection.java | 16 +++++ .../dataFlow/fixture/SideEffectNoBrace.java | 13 ++++ .../fixture/SideEffectNoBrace_after.java | 15 ++++ .../dataFlow/fixture/SideEffectReturn.java | 15 ++++ .../fixture/SideEffectReturn_after.java | 16 +++++ .../dataFlow/fixture/SideEffectWhile.java | 13 ++++ .../fixture/SideEffectWhile_after.java | 11 +++ .../DataFlowInspectionTest.java | 15 ++++ 10 files changed, 157 insertions(+), 52 deletions(-) rename java/{java-analysis-impl => java-impl}/src/com/intellij/codeInsight/daemon/impl/quickfix/SimplifyBooleanExpressionFix.java (90%) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/SideEffectNoBrace.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/SideEffectNoBrace_after.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/SideEffectReturn.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/SideEffectReturn_after.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/SideEffectWhile.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/SideEffectWhile_after.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java index 2fe5b75c41c5..d282171cb3e6 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java @@ -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() { diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/SimplifyBooleanExpressionFix.java b/java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/SimplifyBooleanExpressionFix.java similarity index 90% rename from java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/SimplifyBooleanExpressionFix.java rename to java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/SimplifyBooleanExpressionFix.java index 2adbd7d1fd02..3809023ff7a7 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/SimplifyBooleanExpressionFix.java +++ b/java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/SimplifyBooleanExpressionFix.java @@ -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 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; diff --git a/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java b/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java index 4c29ded22283..16839d62c9c0 100644 --- a/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java @@ -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); diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectNoBrace.java b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectNoBrace.java new file mode 100644 index 000000000000..8a484f164415 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectNoBrace.java @@ -0,0 +1,13 @@ +class SideEffectReturn { + private boolean isValidValue(String value) { + if (!value.isEmpty()) + return Test.valueOf(value) != null; + return false; + } + + enum Test { + A, + B + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectNoBrace_after.java b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectNoBrace_after.java new file mode 100644 index 000000000000..b8988eaadf27 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectNoBrace_after.java @@ -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 + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectReturn.java b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectReturn.java new file mode 100644 index 000000000000..817235475a20 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectReturn.java @@ -0,0 +1,15 @@ +class SideEffectReturn { + private boolean isValidValue(String value) { + try { + return Test.valueOf(value) != null; + } catch (IllegalArgumentException e) { + return false; + } + } + + enum Test { + A, + B + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectReturn_after.java b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectReturn_after.java new file mode 100644 index 000000000000..6fbbd63f8f44 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectReturn_after.java @@ -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 + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectWhile.java b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectWhile.java new file mode 100644 index 000000000000..c7aaa8ee8b19 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectWhile.java @@ -0,0 +1,13 @@ +class SideEffectReturn { + private void testLoop(String value) { + while(Test.valueOf(value) == null) { + + } + } + + enum Test { + A, + B + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectWhile_after.java b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectWhile_after.java new file mode 100644 index 000000000000..385241c80330 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/SideEffectWhile_after.java @@ -0,0 +1,11 @@ +class SideEffectReturn { + private void testLoop(String value) { + Test.valueOf(value); + } + + enum Test { + A, + B + + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java index 61270d7f0835..807eb8738365 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java @@ -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