diff --git a/java/java-impl/src/com/intellij/codeInspection/streamToLoop/StreamToLoopInspection.java b/java/java-impl/src/com/intellij/codeInspection/streamToLoop/StreamToLoopInspection.java index 00ef683a8b2b..95ad4ccab841 100644 --- a/java/java-impl/src/com/intellij/codeInspection/streamToLoop/StreamToLoopInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/streamToLoop/StreamToLoopInspection.java @@ -85,7 +85,7 @@ public class StreamToLoopInspection extends BaseJavaBatchLocalInspectionTool { super.visitMethodCallExpression(call); PsiReferenceExpression expression = call.getMethodExpression(); PsiElement nameElement = expression.getReferenceNameElement(); - if (nameElement == null || !SUPPORTED_TERMINALS.contains(nameElement.getText()) || !isSupportedCodeLocation(call)) return; + if (nameElement == null || !SUPPORTED_TERMINALS.contains(nameElement.getText()) || !ControlFlowUtils.canExtractStatement(call)) return; PsiMethod method = call.resolveMethod(); if(method == null) return; PsiClass aClass = method.getContainingClass(); @@ -112,45 +112,6 @@ public class StreamToLoopInspection extends BaseJavaBatchLocalInspectionTool { }; } - private static boolean isSupportedCodeLocation(PsiMethodCallExpression call) { - PsiElement cur = call; - PsiElement parent = cur.getParent(); - while(parent instanceof PsiExpression || parent instanceof PsiExpressionList) { - if(parent instanceof PsiLambdaExpression) { - return true; - } - if(parent instanceof PsiPolyadicExpression) { - PsiPolyadicExpression polyadicExpression = (PsiPolyadicExpression)parent; - IElementType type = polyadicExpression.getOperationTokenType(); - if ((type.equals(JavaTokenType.ANDAND) || type.equals(JavaTokenType.OROR)) && polyadicExpression.getOperands()[0] != cur) { - // not the first in the &&/|| chain: we cannot properly generate code which would short-circuit as well - return false; - } - } - if(parent instanceof PsiConditionalExpression && ((PsiConditionalExpression)parent).getCondition() != cur) { - return false; - } - if(parent instanceof PsiMethodCallExpression) { - PsiReferenceExpression methodExpression = ((PsiMethodCallExpression)parent).getMethodExpression(); - if(methodExpression.textMatches("this") || methodExpression.textMatches("super")) { - return false; - } - } - cur = parent; - parent = cur.getParent(); - } - if(parent instanceof PsiReturnStatement || parent instanceof PsiExpressionStatement) return true; - if(parent instanceof PsiLocalVariable) { - PsiElement grandParent = parent.getParent(); - if(grandParent instanceof PsiDeclarationStatement && ((PsiDeclarationStatement)grandParent).getDeclaredElements().length == 1) { - return true; - } - } - if(parent instanceof PsiForeachStatement && ((PsiForeachStatement)parent).getIteratedValue() == cur) return true; - if(parent instanceof PsiIfStatement && ((PsiIfStatement)parent).getCondition() == cur) return true; - return false; - } - @Nullable static Operation createOperationFromCall(StreamVariable outVar, PsiMethodCallExpression call, boolean supportUnknownSources) { PsiMethod method = call.resolveMethod(); @@ -304,7 +265,7 @@ public class StreamToLoopInspection extends BaseJavaBatchLocalInspectionTool { PsiElement element = descriptor.getStartElement(); if(!(element instanceof PsiMethodCallExpression)) return; PsiMethodCallExpression terminalCall = (PsiMethodCallExpression)element; - if(!isSupportedCodeLocation(terminalCall)) return; + if(!ControlFlowUtils.canExtractStatement(terminalCall)) return; PsiElementFactory factory = JavaPsiFacade.getElementFactory(project); terminalCall = RefactoringUtil.ensureCodeBlock(terminalCall); if (terminalCall == null) return; diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties index a7a67d29712a..921d21624c10 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties @@ -1138,6 +1138,7 @@ conditional.expression.with.identical.branches.collapse.quickfix=Collapse condit redundant.else.unwrap.quickfix=Remove redundant 'else' constant.conditional.expression.problem.descriptor=#ref can be simplified to ''{0}'' #loc constant.conditional.expression.simplify.quickfix=Simplify +constant.conditional.expression.simplify.quickfix.sideEffect=Extract side effects and simplify enum.switch.statement.which.misses.cases.problem.descriptor=#ref statement on enumerated type ''{0}'' misses cases #loc for.loop.replaceable.by.while.ignore.option=Ignore 'infinite' for loops without conditions for.loop.replaceable.by.while.replace.quickfix=Replace with 'while' diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java index 3159d81a3e3e..90568bb4ac0b 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java @@ -18,6 +18,7 @@ package com.siyeh.ig.psiutils; import com.intellij.psi.*; import com.intellij.psi.controlFlow.*; import com.intellij.psi.search.searches.ReferencesSearch; +import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; import com.intellij.util.ArrayUtil; @@ -868,6 +869,49 @@ public class ControlFlowUtils { return false; } + /** + * @param expression expression to check + * @return true if given expression can be converted to statement without semantics change + */ + public static boolean canExtractStatement(PsiExpression expression) { + PsiElement cur = expression; + PsiElement parent = cur.getParent(); + while(parent instanceof PsiExpression || parent instanceof PsiExpressionList) { + if(parent instanceof PsiLambdaExpression) { + return true; + } + if(parent instanceof PsiPolyadicExpression) { + PsiPolyadicExpression polyadicExpression = (PsiPolyadicExpression)parent; + IElementType type = polyadicExpression.getOperationTokenType(); + if ((type.equals(JavaTokenType.ANDAND) || type.equals(JavaTokenType.OROR)) && polyadicExpression.getOperands()[0] != cur) { + // not the first in the &&/|| chain: we cannot properly generate code which would short-circuit as well + return false; + } + } + if(parent instanceof PsiConditionalExpression && ((PsiConditionalExpression)parent).getCondition() != cur) { + return false; + } + if(parent instanceof PsiMethodCallExpression) { + PsiReferenceExpression methodExpression = ((PsiMethodCallExpression)parent).getMethodExpression(); + if(methodExpression.textMatches("this") || methodExpression.textMatches("super")) { + return false; + } + } + cur = parent; + parent = cur.getParent(); + } + if(parent instanceof PsiReturnStatement || parent instanceof PsiExpressionStatement) return true; + if(parent instanceof PsiLocalVariable) { + PsiElement grandParent = parent.getParent(); + if(grandParent instanceof PsiDeclarationStatement && ((PsiDeclarationStatement)grandParent).getDeclaredElements().length == 1) { + return true; + } + } + if(parent instanceof PsiForeachStatement && ((PsiForeachStatement)parent).getIteratedValue() == cur) return true; + if(parent instanceof PsiIfStatement && ((PsiIfStatement)parent).getCondition() == cur) return true; + return false; + } + public enum InitializerUsageStatus { // Variable is declared just before the wanted place DECLARED_JUST_BEFORE, diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/PointlessBooleanExpressionInspection.java b/plugins/InspectionGadgets/src/com/siyeh/ig/controlflow/PointlessBooleanExpressionInspection.java similarity index 74% rename from plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/PointlessBooleanExpressionInspection.java rename to plugins/InspectionGadgets/src/com/siyeh/ig/controlflow/PointlessBooleanExpressionInspection.java index b0788d06e057..de312be55a67 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/PointlessBooleanExpressionInspection.java +++ b/plugins/InspectionGadgets/src/com/siyeh/ig/controlflow/PointlessBooleanExpressionInspection.java @@ -21,25 +21,26 @@ import com.intellij.openapi.project.Project; import com.intellij.psi.*; import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.ConstantExpressionUtil; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.refactoring.util.RefactoringUtil; import com.intellij.util.IncorrectOperationException; import com.siyeh.InspectionGadgetsBundle; import com.siyeh.ig.BaseInspection; import com.siyeh.ig.BaseInspectionVisitor; import com.siyeh.ig.InspectionGadgetsFix; import com.siyeh.ig.PsiReplacementUtil; -import com.siyeh.ig.psiutils.ComparisonUtils; -import com.siyeh.ig.psiutils.ExpressionUtils; -import com.siyeh.ig.psiutils.ParenthesesUtils; +import com.siyeh.ig.psiutils.*; +import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import javax.swing.*; -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; -import java.util.Set; +import java.util.*; public class PointlessBooleanExpressionInspection extends BaseInspection { + private enum BooleanExpressionKind { + USELESS, USELESS_WITH_SIDE_EFFECTS, UNKNOWN + } private static final Set booleanTokens = new HashSet<>(); static { @@ -261,10 +262,25 @@ public class PointlessBooleanExpressionInspection extends BaseInspection { @Override public InspectionGadgetsFix buildFix(Object... infos) { - return new PointlessBooleanExpressionFix(); + boolean hasSideEffect = (boolean)infos[1]; + return new PointlessBooleanExpressionFix(hasSideEffect); } private class PointlessBooleanExpressionFix extends InspectionGadgetsFix { + private boolean myHasSideEffect; + + public PointlessBooleanExpressionFix(boolean hasSideEffect) { + myHasSideEffect = hasSideEffect; + } + + @Nls + @NotNull + @Override + public String getName() { + return myHasSideEffect + ? InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix.sideEffect") + : InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix"); + } @Override @NotNull @@ -278,8 +294,39 @@ public class PointlessBooleanExpressionInspection extends BaseInspection { if (!(element instanceof PsiExpression)) { return; } - final PsiExpression expression = (PsiExpression)element; - PsiReplacementUtil.replaceExpression(expression, buildSimplifiedExpression(expression, new StringBuilder()).toString()); + PsiExpression expression = (PsiExpression)element; + String simplifiedExpression = buildSimplifiedExpression(expression, new StringBuilder()).toString(); + boolean isConstant = simplifiedExpression.equals("true") || simplifiedExpression.equals("false"); + if (isConstant) { + expression = RefactoringUtil.ensureCodeBlock(expression); + if (expression == null) return; + PsiStatement anchor = PsiTreeUtil.getParentOfType(expression, PsiStatement.class); + if (anchor == null) return; + List sideEffects = extractSideEffects(expression); + PsiStatement[] statements = StatementExtractor.generateStatements(sideEffects, expression); + if (statements.length > 0) { + BlockUtils.addBefore(anchor, statements); + } + } + PsiReplacementUtil.replaceExpression(expression, simplifiedExpression); + } + + private List extractSideEffects(PsiExpression expression) { + if (!(expression instanceof PsiPolyadicExpression)) { + return Collections.emptyList(); + } + PsiPolyadicExpression polyadicExpression = (PsiPolyadicExpression)expression; + IElementType sign = polyadicExpression.getOperationTokenType(); + Boolean stopper = sign == JavaTokenType.ANDAND ? Boolean.FALSE : sign == JavaTokenType.OROR ? Boolean.TRUE : null; + PsiExpression[] operands = polyadicExpression.getOperands(); + List sideEffects = new ArrayList<>(); + for (PsiExpression operand : operands) { + if (stopper != null && stopper.equals(evaluate(operand))) { + break; + } + sideEffects.addAll(SideEffectChecker.extractSideEffectExpressions(operand)); + } + return sideEffects; } } @@ -303,41 +350,66 @@ public class PointlessBooleanExpressionInspection extends BaseInspection { } private void checkExpression(PsiExpression expression) { - if (!isPointlessBooleanExpression(expression)) { + BooleanExpressionKind kind = getExpressionKind(expression); + if (kind == BooleanExpressionKind.UNKNOWN) { return; } final PsiElement parent = ParenthesesUtils.getParentSkipParentheses(expression); - if (parent instanceof PsiExpression && isPointlessBooleanExpression((PsiExpression)parent)) { + if (parent instanceof PsiExpression && getExpressionKind((PsiExpression)parent) != BooleanExpressionKind.UNKNOWN) { return; } - registerError(expression, expression); + registerError(expression, expression, kind == BooleanExpressionKind.USELESS_WITH_SIDE_EFFECTS); } - private boolean isPointlessBooleanExpression(PsiExpression expression) { + @NotNull + private BooleanExpressionKind getExpressionKind(PsiExpression expression) { if (expression instanceof PsiPrefixExpression) { - return evaluate(expression) != null; + return evaluate(expression) != null ? BooleanExpressionKind.USELESS : BooleanExpressionKind.UNKNOWN; } if (expression instanceof PsiPolyadicExpression) { final PsiPolyadicExpression polyadicExpression = (PsiPolyadicExpression)expression; final IElementType sign = polyadicExpression.getOperationTokenType(); if (!booleanTokens.contains(sign)) { - return false; + return BooleanExpressionKind.UNKNOWN; } final PsiExpression[] operands = polyadicExpression.getOperands(); boolean containsConstant = false; + boolean stopCheckingSideEffects = false; + boolean sideEffectMayBeRemoved = false; + boolean reducedToConstant = false; for (PsiExpression operand : operands) { if (operand == null) { - return false; + return BooleanExpressionKind.UNKNOWN; } final PsiType type = operand.getType(); if (type == null || !type.equals(PsiType.BOOLEAN) && !type.equalsToText(CommonClassNames.JAVA_LANG_BOOLEAN)) { - return false; + return BooleanExpressionKind.UNKNOWN; + } + if (!stopCheckingSideEffects && SideEffectChecker.mayHaveSideEffects(operand)) { + sideEffectMayBeRemoved = true; + } + Boolean value = evaluate(operand); + if (value != null) { + containsConstant = true; + if ((JavaTokenType.ANDAND.equals(sign) && !value) || (JavaTokenType.OROR.equals(sign) && value)) { + stopCheckingSideEffects = true; + reducedToConstant = true; + } + if ((JavaTokenType.AND.equals(sign) && !value) || (JavaTokenType.OR.equals(sign) && value)) { + reducedToConstant = true; + } } - containsConstant |= evaluate(operand) != null; } - return containsConstant; + if (containsConstant) { + if (sideEffectMayBeRemoved && reducedToConstant) { + return ControlFlowUtils.canExtractStatement(expression) + ? BooleanExpressionKind.USELESS_WITH_SIDE_EFFECTS + : BooleanExpressionKind.UNKNOWN; + } + return BooleanExpressionKind.USELESS; + } } - return false; + return BooleanExpressionKind.UNKNOWN; } } @@ -356,6 +428,9 @@ public class PointlessBooleanExpressionInspection extends BaseInspection { if (tokenType.equals(JavaTokenType.OROR)) { final PsiExpression[] operands = polyadicExpression.getOperands(); for (PsiExpression operand : operands) { + if (SideEffectChecker.mayHaveSideEffects(operand)) { + return null; + } if (evaluate(operand) == Boolean.TRUE) { return Boolean.TRUE; } @@ -364,6 +439,9 @@ public class PointlessBooleanExpressionInspection extends BaseInspection { else if (tokenType.equals(JavaTokenType.ANDAND)) { final PsiExpression[] operands = polyadicExpression.getOperands(); for (PsiExpression operand : operands) { + if (SideEffectChecker.mayHaveSideEffects(operand)) { + return null; + } if (evaluate(operand) == Boolean.FALSE) { return Boolean.FALSE; } @@ -383,8 +461,7 @@ public class PointlessBooleanExpressionInspection extends BaseInspection { } } } - final Boolean value = (Boolean)ConstantExpressionUtil.computeCastTo(expression, PsiType.BOOLEAN); - return value != null ? value : null; + return (Boolean)ConstantExpressionUtil.computeCastTo(expression, PsiType.BOOLEAN); } private static boolean containsReference(@Nullable PsiExpression expression) { diff --git a/plugins/InspectionGadgets/test/com/siyeh/igfixes/pointlessboolean/SideEffects.after.java b/plugins/InspectionGadgets/test/com/siyeh/igfixes/pointlessboolean/SideEffects.after.java new file mode 100644 index 000000000000..d47495fd437c --- /dev/null +++ b/plugins/InspectionGadgets/test/com/siyeh/igfixes/pointlessboolean/SideEffects.after.java @@ -0,0 +1,34 @@ +/* + * 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. + */ +class C { + double sideEffect(int x) { + System.out.println("Side effect"+x); + return Math.random(); + } + + void m() { + if (sideEffect(1) > 0.5 && sideEffect(2) < 0.1) { + if (sideEffect(3) / sideEffect(4) < 1) { + sideEffect(5); + } else { + sideEffect(6); + } + } + if(false) { + System.out.println("oops"); + } + } +} diff --git a/plugins/InspectionGadgets/test/com/siyeh/igfixes/pointlessboolean/SideEffects.java b/plugins/InspectionGadgets/test/com/siyeh/igfixes/pointlessboolean/SideEffects.java new file mode 100644 index 000000000000..e31dee24a4d0 --- /dev/null +++ b/plugins/InspectionGadgets/test/com/siyeh/igfixes/pointlessboolean/SideEffects.java @@ -0,0 +1,29 @@ +/* + * 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. + */ +class C { + double sideEffect(int x) { + System.out.println("Side effect"+x); + return Math.random(); + } + + void m() { + if(sideEffect(1) > 0.5 && sideEffect(2) < 0.1 + && (sideEffect(3)/sideEffect(4) < 1 ? sideEffect(5) > 0.5 : sideEffect(6) < 0.6) + && false && sideEffect(7) * sideEffect(8) < 0.2) { + System.out.println("oops"); + } + } +} diff --git a/plugins/InspectionGadgets/test/com/siyeh/igtest/controlflow/pointless_boolean_expression/PointlessBooleanExpression.java b/plugins/InspectionGadgets/test/com/siyeh/igtest/controlflow/pointless_boolean_expression/PointlessBooleanExpression.java index 11b1bb6c8157..13414778edd8 100644 --- a/plugins/InspectionGadgets/test/com/siyeh/igtest/controlflow/pointless_boolean_expression/PointlessBooleanExpression.java +++ b/plugins/InspectionGadgets/test/com/siyeh/igtest/controlflow/pointless_boolean_expression/PointlessBooleanExpression.java @@ -28,6 +28,36 @@ class PointlessBooleanExpression { boolean y = false || c; boolean z = b != true; } + + boolean sideEffect() { + System.out.println("hello"); + return Math.random() > 0.5; + } + + // side-effect cannot be extracted from field declaration + boolean field = sideEffect() && false; + boolean field1 = false & sideEffect(); + // no side-effect extraction necessary + boolean field2 = sideEffect() && true; + boolean field3 = false && sideEffect(); + + void method() { + if(sideEffect() && false) { + System.out.println("ok"); + } + if(sideEffect() && true) { + System.out.println("ooh"); + } + // Do not warn as we cannot simplify w/o reordering calls which could be undesired + // this code is warned by DFA inspection (w/o quick-fix) + if(Math.random() > 0.5 && (sideEffect() || true)) { + System.out.println("well"); + } + // Here side-effect can be extracted before loop + if((sideEffect() || true) && Math.random() > 0.5) { + System.out.println("well"); + } + } } class Presley { void elvis(Object king) { diff --git a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/PointlessBooleanExpressionFixTest.java b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/PointlessBooleanExpressionFixTest.java index ca7ed5f335c3..98e970976c23 100644 --- a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/PointlessBooleanExpressionFixTest.java +++ b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/PointlessBooleanExpressionFixTest.java @@ -26,10 +26,13 @@ public class PointlessBooleanExpressionFixTest extends IGQuickFixesTestCase { super.setUp(); myFixture.enableInspections(new PointlessBooleanExpressionInspection()); myRelativePath = "pointlessboolean"; - myDefaultHint = InspectionGadgetsBundle.message("pointless.bitwise.expression.simplify.quickfix"); + myDefaultHint = InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix"); } public void testNegation() { doTest(); } public void testPolyadic() { doTest(); } public void testBoxed() { doTest(); } + public void testSideEffects() { + doTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix.sideEffect")); + } }