From 6b91f55eb7cb525fb706d7a5f283f864172a50fd Mon Sep 17 00:00:00 2001 From: peter Date: Fri, 28 Mar 2014 19:30:28 +0100 Subject: [PATCH] IDEA-122414 Boolean expression simplification changes behavior of the code --- .../dataFlow/DataFlowInspectionBase.java | 34 ++++++++----------- .../defUse/DefUseInspectionBase.java | 5 ++- .../dataFlow/DataFlowInspection.java | 15 ++++++++ .../defUse/DefUseInspection.java | 2 +- .../dataFlow/boxingBoolean/expected.xml | 2 +- .../fixture/TrueOrEqualsSomething.java | 9 +++++ .../fixture/TrueOrEqualsSomething_after.java | 8 +++++ .../DataFlowInspectionTest.java | 6 ++++ .../src/messages/InspectionsBundle.properties | 2 +- 9 files changed, 60 insertions(+), 23 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/TrueOrEqualsSomething.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/TrueOrEqualsSomething_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 2e15ac48f7f3..996d8c0a15f6 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 @@ -173,21 +173,21 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { return super.shouldCheckTimeLimit(); } }; - analyzeDfaWithNestedClosures(scope, holder, dfaRunner, Arrays.asList(dfaRunner.createMemoryState())); + analyzeDfaWithNestedClosures(scope, holder, dfaRunner, Arrays.asList(dfaRunner.createMemoryState()), onTheFly); } private void analyzeDfaWithNestedClosures(PsiElement scope, ProblemsHolder holder, StandardDataFlowRunner dfaRunner, - Collection initialStates) { + Collection initialStates, final boolean onTheFly) { final DataFlowInstructionVisitor visitor = new DataFlowInstructionVisitor(dfaRunner); final RunnerResult rc = dfaRunner.analyzeMethod(scope, visitor, IGNORE_ASSERT_STATEMENTS, initialStates); if (rc == RunnerResult.OK) { - createDescription(dfaRunner, holder, visitor); + createDescription(dfaRunner, holder, visitor, onTheFly); MultiMap nestedClosures = dfaRunner.getNestedClosures(); for (PsiElement closure : nestedClosures.keySet()) { - analyzeDfaWithNestedClosures(closure, holder, dfaRunner, nestedClosures.get(closure)); + analyzeDfaWithNestedClosures(closure, holder, dfaRunner, nestedClosures.get(closure), onTheFly); } } else if (rc == RunnerResult.TOO_COMPLEX) { @@ -234,7 +234,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { protected void addSurroundWithIfFix(PsiExpression qualifier, List fixes, boolean onTheFly) { } - private void createDescription(StandardDataFlowRunner runner, ProblemsHolder holder, DataFlowInstructionVisitor visitor) { + private void createDescription(StandardDataFlowRunner runner, ProblemsHolder holder, DataFlowInstructionVisitor visitor, final boolean onTheFly) { Pair, Set> constConditions = runner.getConstConditionalExpressions(); Set trueSet = constConditions.getFirst(); Set falseSet = constConditions.getSecond(); @@ -265,7 +265,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { reportCastMayFail(holder, (TypeCastInstruction)instruction); } else if (instruction instanceof BranchingInstruction) { - handleBranchingInstruction(holder, visitor, trueSet, falseSet, reportedAnchors, (BranchingInstruction)instruction); + handleBranchingInstruction(holder, visitor, trueSet, falseSet, reportedAnchors, (BranchingInstruction)instruction, onTheFly); } } @@ -410,7 +410,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { private void handleBranchingInstruction(ProblemsHolder holder, StandardInstructionVisitor visitor, Set trueSet, - Set falseSet, HashSet reportedAnchors, BranchingInstruction instruction) { + Set falseSet, HashSet reportedAnchors, BranchingInstruction instruction, final boolean onTheFly) { PsiElement psiAnchor = instruction.getPsiAnchor(); boolean underBinary = isAtRHSOfBooleanAnd(psiAnchor); if (instruction instanceof InstanceofInstruction && visitor.isInstanceofRedundant((InstanceofInstruction)instruction)) { @@ -434,11 +434,12 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { } else if (psiAnchor != null && !reportedAnchors.contains(psiAnchor) && !isCompileConstantInIfCondition(psiAnchor)) { boolean evaluatesToTrue = trueSet.contains(instruction); - if (onTheLeftSideOfConditionalAssignment(psiAnchor)) { + final PsiElement parent = psiAnchor.getParent(); + if (parent instanceof PsiAssignmentExpression && ((PsiAssignmentExpression)parent).getLExpression() == psiAnchor) { holder.registerProblem( psiAnchor, InspectionsBundle.message("dataflow.message.pointless.assignment.expression", Boolean.toString(evaluatesToTrue)), - createSimplifyToAssignmentFix() + createConditionalAssignmentFixes(evaluatesToTrue, (PsiAssignmentExpression)parent, onTheFly) ); } else if (!skipReportingConstantCondition(visitor, psiAnchor, evaluatesToTrue)) { @@ -452,6 +453,10 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { } } + protected LocalQuickFix[] createConditionalAssignmentFixes(boolean evaluatesToTrue, PsiAssignmentExpression parent, final boolean onTheFly) { + return LocalQuickFix.EMPTY_ARRAY; + } + private boolean skipReportingConstantCondition(StandardInstructionVisitor visitor, PsiElement psiAnchor, boolean evaluatesToTrue) { return DONT_REPORT_TRUE_ASSERT_STATEMENTS && isAssertionEffectively(psiAnchor, evaluatesToTrue) || visitor.silenceConstantCondition(psiAnchor); @@ -581,15 +586,6 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { return false; } - private static boolean onTheLeftSideOfConditionalAssignment(final PsiElement psiAnchor) { - final PsiElement parent = psiAnchor.getParent(); - if (parent instanceof PsiAssignmentExpression) { - final PsiAssignmentExpression expression = (PsiAssignmentExpression)parent; - if (expression.getLExpression() == psiAnchor) return true; - } - return false; - } - @Nullable private static LocalQuickFix createSimplifyBooleanExpressionFix(PsiElement element, final boolean value) { SimplifyBooleanExpressionFix fix = createIntention(element, value); @@ -626,7 +622,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { } @NotNull - private static LocalQuickFix createSimplifyToAssignmentFix() { + protected static LocalQuickFix createSimplifyToAssignmentFix() { return new LocalQuickFix() { @NotNull @Override diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/defUse/DefUseInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/defUse/DefUseInspectionBase.java index fa0a0e3f8bf9..47d21e4eca12 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/defUse/DefUseInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/defUse/DefUseInspectionBase.java @@ -19,6 +19,7 @@ import com.intellij.codeInsight.daemon.GroupNames; import com.intellij.codeInspection.*; import com.intellij.psi.*; import com.intellij.psi.controlFlow.DefUseUtil; +import com.intellij.util.containers.ContainerUtil; import gnu.trove.THashSet; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; @@ -99,10 +100,12 @@ public class DefUseInspectionBase extends BaseJavaBatchLocalInspectionTool { } else if (context instanceof PsiAssignmentExpression) { final PsiAssignmentExpression assignment = (PsiAssignmentExpression)context; + List fixes = ContainerUtil.createMaybeSingletonList(isOnTheFly ? createRemoveAssignmentFix() : null); holder.registerProblem(assignment.getLExpression(), InspectionsBundle.message("inspection.unused.assignment.problem.descriptor3", assignment.getRExpression().getText(), "#ref" + " #loc"), - ProblemHighlightType.LIKE_UNUSED_SYMBOL, createRemoveAssignmentFix()); + ProblemHighlightType.LIKE_UNUSED_SYMBOL, fixes.toArray(new LocalQuickFix[fixes.size()]) + ); } else { if (context instanceof PsiPrefixExpression && REPORT_PREFIX_EXPRESSIONS || 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 b1c04213a8b6..934479aa2e00 100644 --- a/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java @@ -19,11 +19,15 @@ import com.intellij.codeInsight.NullableNotNullDialog; import com.intellij.codeInspection.InspectionsBundle; import com.intellij.codeInspection.LocalQuickFix; import com.intellij.codeInspection.SurroundWithIfFix; +import com.intellij.codeInspection.defUse.DefUseInspection; import com.intellij.ide.DataManager; import com.intellij.openapi.actionSystem.CommonDataKeys; import com.intellij.openapi.project.Project; import com.intellij.openapi.project.ProjectManager; +import com.intellij.psi.JavaTokenType; +import com.intellij.psi.PsiAssignmentExpression; import com.intellij.psi.PsiExpression; +import com.intellij.psi.tree.IElementType; import javax.swing.*; import javax.swing.event.ChangeEvent; @@ -40,6 +44,17 @@ public class DataFlowInspection extends DataFlowInspectionBase { fixes.add(new SurroundWithIfFix(qualifier)); } } + + @Override + protected LocalQuickFix[] createConditionalAssignmentFixes(boolean evaluatesToTrue, PsiAssignmentExpression assignment, final boolean onTheFly) { + IElementType op = assignment.getOperationTokenType(); + boolean toRemove = op == JavaTokenType.ANDEQ && !evaluatesToTrue || op == JavaTokenType.OREQ && evaluatesToTrue; + if (toRemove && !onTheFly) { + return LocalQuickFix.EMPTY_ARRAY; + } + return new LocalQuickFix[]{toRemove ? new DefUseInspection.RemoveAssignmentFix() : createSimplifyToAssignmentFix()}; + } + @Override public JComponent createOptionsPanel() { return new OptionsPanel(); diff --git a/java/java-impl/src/com/intellij/codeInspection/defUse/DefUseInspection.java b/java/java-impl/src/com/intellij/codeInspection/defUse/DefUseInspection.java index 1c4b8a7a8012..a017bd1d11c5 100644 --- a/java/java-impl/src/com/intellij/codeInspection/defUse/DefUseInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/defUse/DefUseInspection.java @@ -55,7 +55,7 @@ public class DefUseInspection extends DefUseInspectionBase { return new RemoveAssignmentFix(); } - private static class RemoveAssignmentFix extends RemoveInitializerFix { + public static class RemoveAssignmentFix extends RemoveInitializerFix { @NotNull @Override public String getName() { diff --git a/java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml b/java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml index d04ea2815d43..e04ae9dabc10 100644 --- a/java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml +++ b/java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml @@ -33,7 +33,7 @@ Test.java 51 - Condition <code>o</code> at the left side of assignment expression is always <code>false</code>. Can be simplified to normal assignment. + Condition <code>o</code> at the left side of assignment expression is always <code>false</code>. Can be simplified. diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/TrueOrEqualsSomething.java b/java/java-tests/testData/inspection/dataFlow/fixture/TrueOrEqualsSomething.java new file mode 100644 index 000000000000..a41172b8838e --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/TrueOrEqualsSomething.java @@ -0,0 +1,9 @@ +class Contracts { + + private boolean method(boolean a) { + boolean b = true; + b |= a; + return b; + } + +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/TrueOrEqualsSomething_after.java b/java/java-tests/testData/inspection/dataFlow/fixture/TrueOrEqualsSomething_after.java new file mode 100644 index 000000000000..8e7695f4cee4 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/TrueOrEqualsSomething_after.java @@ -0,0 +1,8 @@ +class Contracts { + + private boolean method(boolean a) { + boolean b = true; + return b; + } + +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java index 80b6e028db00..ae6da7ca3ceb 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java @@ -323,6 +323,12 @@ public class DataFlowInspectionTest extends LightCodeInsightFixtureTestCase { doTest(); } + + public void testTrueOrEqualsSomething() { + doTest(); + myFixture.launchAction(myFixture.findSingleIntention("Remove redundant assignment")); + myFixture.checkResultByFile(getTestName(false) + "_after.java"); + } public void _testNullCheckBeforeInstanceof() { doTest(); } // http://youtrack.jetbrains.com/issue/IDEA-113220 } diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index eff9ee33d6f8..191cc7fafb28 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -77,7 +77,7 @@ dataflow.message.redundant.instanceof=Condition #ref #loc is redund dataflow.message.constant.condition=Condition #ref #loc is always {0} dataflow.message.constant.condition.when.reached=Condition #ref #loc is always {0} when reached dataflow.message.unreachable.switch.label=Switch label#ref #loc is unreachable -dataflow.message.pointless.assignment.expression=Condition #ref #loc at the left side of assignment expression is always {0}. Can be simplified to normal assignment +dataflow.message.pointless.assignment.expression=Condition #ref #loc at the left side of assignment expression is always {0}. Can be simplified dataflow.message.passing.null.argument=Passing null argument to parameter annotated as @NotNull dataflow.message.initializing.field.with.null=Field annotated as @NotNull is implicitly initialized with null dataflow.message.passing.nullable.argument=Argument #ref #loc might be null