From 839d461ced9e89d6fc39f1db644b15ec2f1c5cb2 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 22 Sep 2017 15:36:37 +0700 Subject: [PATCH] TrivialIfInspection: support cases when implicit return is after enclosing 'if' --- .../BaseStreamApiMigration.java | 13 ------ .../streamMigration/FindFirstMigration.java | 4 +- .../streamMigration/MatchMigration.java | 4 +- .../StreamApiMigrationInspection.java | 18 +------- .../ig/controlflow/TrivialIfInspection.java | 22 +++++---- .../siyeh/ig/psiutils/ControlFlowUtils.java | 46 +++++++++++++++++++ .../controlflow/trivialIf/Nested.after.java | 9 ++++ .../igfixes/controlflow/trivialIf/Nested.java | 11 +++++ .../controlflow/TrivialIfInspectionTest.java | 12 +++++ .../fixes/controlflow/TrivialIfFixTest.java | 1 + 10 files changed, 96 insertions(+), 44 deletions(-) create mode 100644 plugins/InspectionGadgets/test/com/siyeh/igfixes/controlflow/trivialIf/Nested.after.java create mode 100644 plugins/InspectionGadgets/test/com/siyeh/igfixes/controlflow/trivialIf/Nested.java diff --git a/java/java-impl/src/com/intellij/codeInspection/streamMigration/BaseStreamApiMigration.java b/java/java-impl/src/com/intellij/codeInspection/streamMigration/BaseStreamApiMigration.java index 4f439f5c7bfa..abf8e3d9c470 100644 --- a/java/java-impl/src/com/intellij/codeInspection/streamMigration/BaseStreamApiMigration.java +++ b/java/java-impl/src/com/intellij/codeInspection/streamMigration/BaseStreamApiMigration.java @@ -17,7 +17,6 @@ package com.intellij.codeInspection.streamMigration; import com.intellij.openapi.project.Project; import com.intellij.psi.*; -import com.intellij.psi.controlFlow.*; import com.intellij.psi.util.PsiTreeUtil; import com.siyeh.ig.psiutils.ControlFlowUtils; import com.siyeh.ig.psiutils.ControlFlowUtils.InitializerUsageStatus; @@ -124,16 +123,4 @@ abstract class BaseStreamApiMigration { statement.delete(); } } - - static boolean isReachable(PsiReturnStatement target) { - ControlFlow flow; - try { - flow = ControlFlowFactory.getInstance(target.getProject()) - .getControlFlow(target.getParent(), LocalsOrMyInstanceFieldsControlFlowPolicy.getInstance()); - } - catch (AnalysisCanceledException e) { - return true; - } - return ControlFlowUtil.isInstructionReachable(flow, flow.getStartOffset(target), 0); - } } diff --git a/java/java-impl/src/com/intellij/codeInspection/streamMigration/FindFirstMigration.java b/java/java-impl/src/com/intellij/codeInspection/streamMigration/FindFirstMigration.java index fe0cec4c4070..b719531ebd5e 100644 --- a/java/java-impl/src/com/intellij/codeInspection/streamMigration/FindFirstMigration.java +++ b/java/java-impl/src/com/intellij/codeInspection/streamMigration/FindFirstMigration.java @@ -42,7 +42,7 @@ class FindFirstMigration extends BaseStreamApiMigration { PsiReturnStatement returnStatement = (PsiReturnStatement)statement; PsiExpression value = returnStatement.getReturnValue(); if (value == null) return null; - PsiReturnStatement nextReturnStatement = StreamApiMigrationInspection.getNextReturnStatement(loopStatement); + PsiReturnStatement nextReturnStatement = ControlFlowUtils.getNextReturnStatement(loopStatement); if (nextReturnStatement == null) return null; PsiExpression orElseExpression = nextReturnStatement.getReturnValue(); if (!ExpressionUtils.isSimpleExpression(orElseExpression)) return null; @@ -50,7 +50,7 @@ class FindFirstMigration extends BaseStreamApiMigration { restoreComments(loopStatement, body); boolean sibling = nextReturnStatement.getParent() == loopStatement.getParent(); PsiElement replacement = loopStatement.replace(elementFactory.createStatementFromText("return " + stream + ";", loopStatement)); - if(sibling || !isReachable(nextReturnStatement)) { + if(sibling || !ControlFlowUtils.isReachable(nextReturnStatement)) { nextReturnStatement.delete(); } return replacement; diff --git a/java/java-impl/src/com/intellij/codeInspection/streamMigration/MatchMigration.java b/java/java-impl/src/com/intellij/codeInspection/streamMigration/MatchMigration.java index 01fb925c3630..a7dbf268f4ee 100644 --- a/java/java-impl/src/com/intellij/codeInspection/streamMigration/MatchMigration.java +++ b/java/java-impl/src/com/intellij/codeInspection/streamMigration/MatchMigration.java @@ -44,7 +44,7 @@ class MatchMigration extends BaseStreamApiMigration { PsiExpression value = returnStatement.getReturnValue(); if (ExpressionUtils.isLiteral(value, Boolean.TRUE) || ExpressionUtils.isLiteral(value, Boolean.FALSE)) { boolean foundResult = (boolean)((PsiLiteralExpression)value).getValue(); - PsiReturnStatement nextReturnStatement = StreamApiMigrationInspection.getNextReturnStatement(sourceStatement); + PsiReturnStatement nextReturnStatement = ControlFlowUtils.getNextReturnStatement(sourceStatement); if (nextReturnStatement != null) { PsiExpression returnValue = nextReturnStatement.getReturnValue(); if(returnValue == null) return null; @@ -59,7 +59,7 @@ class MatchMigration extends BaseStreamApiMigration { return returnValue.replace(elementFactory.createExpressionFromText(streamText, nextReturnStatement)); } PsiElement result = sourceStatement.replace(elementFactory.createStatementFromText("return " + streamText + ";", sourceStatement)); - if(!isReachable(nextReturnStatement)) { + if(!ControlFlowUtils.isReachable(nextReturnStatement)) { nextReturnStatement.delete(); } return result; diff --git a/java/java-impl/src/com/intellij/codeInspection/streamMigration/StreamApiMigrationInspection.java b/java/java-impl/src/com/intellij/codeInspection/streamMigration/StreamApiMigrationInspection.java index 0832595fa00f..8b2e9e41e5c2 100644 --- a/java/java-impl/src/com/intellij/codeInspection/streamMigration/StreamApiMigrationInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/streamMigration/StreamApiMigrationInspection.java @@ -111,22 +111,6 @@ public class StreamApiMigrationInspection extends BaseJavaBatchLocalInspectionTo return new StreamApiMigrationVisitor(holder, isOnTheFly); } - @Nullable - static PsiReturnStatement getNextReturnStatement(PsiStatement statement) { - PsiElement nextStatement = PsiTreeUtil.skipWhitespacesAndCommentsForward(statement); - if (nextStatement instanceof PsiReturnStatement) return (PsiReturnStatement)nextStatement; - PsiElement parent = statement.getParent(); - if (parent instanceof PsiCodeBlock) { - PsiStatement[] statements = ((PsiCodeBlock)parent).getStatements(); - if (statements.length == 0 || statements[statements.length - 1] != statement) return null; - parent = parent.getParent(); - if (!(parent instanceof PsiBlockStatement)) return null; - parent = parent.getParent(); - } - if (parent instanceof PsiIfStatement) return getNextReturnStatement((PsiStatement)parent); - return null; - } - @Contract("null, null -> true; null, !null -> false") private static boolean sameReference(PsiExpression expr1, PsiExpression expr2) { if (expr1 == null && expr2 == null) return true; @@ -570,7 +554,7 @@ public class StreamApiMigrationInspection extends BaseJavaBatchLocalInspectionTo boolean shouldWarn = replaceTrivialForEach || tb.hasOperations(); PsiReturnStatement returnStatement = (PsiReturnStatement)tb.getSingleStatement(); PsiExpression value = returnStatement.getReturnValue(); - PsiReturnStatement nextReturnStatement = getNextReturnStatement(statement); + PsiReturnStatement nextReturnStatement = ControlFlowUtils.getNextReturnStatement(statement); if (nextReturnStatement != null && (ExpressionUtils.isLiteral(value, Boolean.TRUE) || ExpressionUtils.isLiteral(value, Boolean.FALSE))) { boolean foundResult = (boolean)((PsiLiteralExpression)value).getValue(); diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/TrivialIfInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/TrivialIfInspection.java index 323e4c1026d1..fb4b7ea53d8c 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/TrivialIfInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/TrivialIfInspection.java @@ -32,6 +32,7 @@ import com.siyeh.ig.psiutils.BoolUtils; import com.siyeh.ig.psiutils.ControlFlowUtils; import com.siyeh.ig.psiutils.EquivalenceChecker; import com.siyeh.ig.psiutils.ParenthesesUtils; +import org.intellij.lang.annotations.Pattern; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; @@ -39,6 +40,7 @@ import java.util.Collection; public class TrivialIfInspection extends BaseInspection implements CleanupLocalInspectionTool { + @Pattern(VALID_ID_PATTERN) @Override @NotNull public String getID() { @@ -144,11 +146,13 @@ public class TrivialIfInspection extends BaseInspection implements CleanupLocalI return; } final String conditionText = condition.getText(); - final PsiElement nextStatement = PsiTreeUtil.skipWhitespacesForward(statement); + PsiReturnStatement nextStatement = ControlFlowUtils.getNextReturnStatement(statement); @NonNls final String newStatement = "return " + conditionText + ';'; PsiReplacementUtil.replaceStatement(statement, newStatement); assert nextStatement != null; - nextStatement.delete(); + if (!ControlFlowUtils.isReachable(nextStatement)) { + nextStatement.delete(); + } } private static void replaceSimplifiableReturn(PsiIfStatement statement) { @@ -243,17 +247,19 @@ public class TrivialIfInspection extends BaseInspection implements CleanupLocalI return; } final String conditionText = BoolUtils.getNegatedExpressionText(condition); - final PsiElement nextStatement = PsiTreeUtil.skipWhitespacesForward(statement); + final PsiReturnStatement nextStatement = ControlFlowUtils.getNextReturnStatement(statement); if (nextStatement == null) { return; } final PsiElement nextSibling = statement.getNextSibling(); - if (nextSibling != nextStatement) { + if (nextSibling != nextStatement && nextStatement.getParent() == statement.getParent()) { statement.getParent().deleteChildRange(nextSibling, nextStatement.getPrevSibling()); } @NonNls final String newStatement = "return " + conditionText + ';'; PsiReplacementUtil.replaceStatement(statement, newStatement); - nextStatement.delete(); + if (!ControlFlowUtils.isReachable(nextStatement)) { + nextStatement.delete(); + } } private static void replaceSimplifiableReturnNegated(PsiIfStatement statement) { @@ -334,11 +340,7 @@ public class TrivialIfInspection extends BaseInspection implements CleanupLocalI } PsiStatement thenBranch = ifStatement.getThenBranch(); thenBranch = ControlFlowUtils.stripBraces(thenBranch); - final PsiElement nextStatement = PsiTreeUtil.skipWhitespacesForward(ifStatement); - if (!(nextStatement instanceof PsiStatement)) { - return false; - } - final PsiStatement elseBranch = (PsiStatement)nextStatement; + PsiReturnStatement elseBranch = ControlFlowUtils.getNextReturnStatement(ifStatement); return isReturn(thenBranch, thenReturn) && isReturn(elseBranch, elseReturn); } 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 09c7fe706daf..f914e76af2e0 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java @@ -937,6 +937,52 @@ public class ControlFlowUtils { return false; } + /** + * Finds the return statement which will be always executed after the supplied statement. It supports constructs like this: + * 
{@code
+   * if(condition) {
+   *   statement(); // this statement is supplied as a parameter
+   * }
+   * return true; // this return statement will be returned
+   * }
+ * + * @param statement statement to find the return after + * @return the found return statement or null. + */ + @Nullable + public static PsiReturnStatement getNextReturnStatement(PsiStatement statement) { + PsiElement nextStatement = PsiTreeUtil.skipWhitespacesAndCommentsForward(statement); + if (nextStatement instanceof PsiReturnStatement) return (PsiReturnStatement)nextStatement; + PsiElement parent = statement.getParent(); + if (parent instanceof PsiCodeBlock) { + PsiStatement[] statements = ((PsiCodeBlock)parent).getStatements(); + if (statements.length == 0 || statements[statements.length - 1] != statement) return null; + parent = parent.getParent(); + if (!(parent instanceof PsiBlockStatement)) return null; + parent = parent.getParent(); + } + if (parent instanceof PsiIfStatement) return getNextReturnStatement((PsiStatement)parent); + return null; + } + + /** + * @param statement statement to test + * @return true if statement is reachable or code is incomplete and reachability cannot be defined + */ + public static boolean isReachable(@NotNull PsiStatement statement) { + ControlFlow flow; + PsiCodeBlock block = PsiTreeUtil.getParentOfType(statement, PsiCodeBlock.class); + if (block == null) return true; + try { + flow = ControlFlowFactory.getInstance(statement.getProject()) + .getControlFlow(block, LocalsOrMyInstanceFieldsControlFlowPolicy.getInstance()); + } + catch (AnalysisCanceledException e) { + return true; + } + return ControlFlowUtil.isInstructionReachable(flow, flow.getStartOffset(statement), 0); + } + public enum InitializerUsageStatus { // Variable is declared just before the wanted place DECLARED_JUST_BEFORE, diff --git a/plugins/InspectionGadgets/test/com/siyeh/igfixes/controlflow/trivialIf/Nested.after.java b/plugins/InspectionGadgets/test/com/siyeh/igfixes/controlflow/trivialIf/Nested.after.java new file mode 100644 index 000000000000..7fa03aafbe42 --- /dev/null +++ b/plugins/InspectionGadgets/test/com/siyeh/igfixes/controlflow/trivialIf/Nested.after.java @@ -0,0 +1,9 @@ +class Nested { + public boolean test(String s) { + if(s != null) { + int i = Integer.parseInt(s); + return i > 0; + } + return false; + } +} \ No newline at end of file diff --git a/plugins/InspectionGadgets/test/com/siyeh/igfixes/controlflow/trivialIf/Nested.java b/plugins/InspectionGadgets/test/com/siyeh/igfixes/controlflow/trivialIf/Nested.java new file mode 100644 index 000000000000..1f3f3c4494eb --- /dev/null +++ b/plugins/InspectionGadgets/test/com/siyeh/igfixes/controlflow/trivialIf/Nested.java @@ -0,0 +1,11 @@ +class Nested { + public boolean test(String s) { + if(s != null) { + int i = Integer.parseInt(s); + if(i > 0) { + return true; + } + } + return false; + } +} \ No newline at end of file diff --git a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/controlflow/TrivialIfInspectionTest.java b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/controlflow/TrivialIfInspectionTest.java index e0ee6a8b12dd..49ebbd9521a8 100644 --- a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/controlflow/TrivialIfInspectionTest.java +++ b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/controlflow/TrivialIfInspectionTest.java @@ -33,6 +33,18 @@ public class TrivialIfInspectionTest extends LightInspectionTestCase { "}"); } + public void testParenthesesReturnNestedIf() { + doMemberTest("\n" + + " boolean b(int[] array) {\n" + + " if (array != null) {\n" + + " int len = array.length;\n" + + " /*'if' statement can be simplified*/if/**/(len == 10) return true;\n" + + " }\n" + + " return false;\n" + + " }\n" + + ""); + } + public void testParenthesesAssignment() { doMemberTest("void b(int[] array) {" + " boolean result;" + diff --git a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/controlflow/TrivialIfFixTest.java b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/controlflow/TrivialIfFixTest.java index 14fac6e77367..2d008112f409 100644 --- a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/controlflow/TrivialIfFixTest.java +++ b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/controlflow/TrivialIfFixTest.java @@ -35,4 +35,5 @@ public class TrivialIfFixTest extends IGQuickFixesTestCase { public void testAssert1() { doTest(); } public void testAssert2() { doTest(); } public void testParentheses() { doTest(); } + public void testNested() { doTest(); } } \ No newline at end of file