diff --git a/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties b/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties index 64a76b8fc8e8..26516c619369 100644 --- a/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties +++ b/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties @@ -243,8 +243,10 @@ inspection.class.has.no.to.string.method.ignore.test.classes.option=Ignore test inspection.common.if.parts.disable.highlight.tail.call=Do not highlight common parts, if tail statement is call inspection.common.if.parts.family.else.if.description='else if' can be merged inspection.common.if.parts.family.else.if=Merge 'else if' statement +inspection.common.if.parts.family.else.if.invert=Merge 'else if' statement inverting the second condition inspection.common.if.parts.family=Extract common parts of 'if' statement inspection.common.if.parts.settings.highlight.when.tail.call=Highlight when the last common statement is a call +inspection.common.if.parts.settings.highlight.else.if=Highlight else-if chains that can be simplified inspection.compiler.javac.quirks.anno.array.comma.fix=Remove trailing comma inspection.compiler.javac.quirks.anno.array.comma.problem=Trailing comma in annotation array initializer may cause compilation error in some Javac versions (e.g. JDK 5 and JDK 6). inspection.compiler.javac.quirks.name=Javac quirks diff --git a/java/java-tests/testData/inspection/commonIfParts/afterNeedInvert.java b/java/java-tests/testData/inspection/commonIfParts/afterNeedInvert.java new file mode 100644 index 000000000000..2c5e7285bc3e --- /dev/null +++ b/java/java-tests/testData/inspection/commonIfParts/afterNeedInvert.java @@ -0,0 +1,11 @@ +// "Merge 'else if' statement inverting the second condition" "true" + +class Test { + String[] foo(Object[] value, String[] defaultValue) { + if (value == null || !(value instanceof String[])) { + return defaultValue; + } else { + return (String[]) value; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/commonIfParts/beforeNeedInvert.java b/java/java-tests/testData/inspection/commonIfParts/beforeNeedInvert.java new file mode 100644 index 000000000000..17752035d506 --- /dev/null +++ b/java/java-tests/testData/inspection/commonIfParts/beforeNeedInvert.java @@ -0,0 +1,13 @@ +// "Merge 'else if' statement inverting the second condition" "true" + +class Test { + String[] foo(Object[] value, String[] defaultValue) { + if (value == null) { + return defaultValue; + } else if (value instanceof String[]) { + return (String[]) value; + } else { + return defaultValue; + } + } +} \ No newline at end of file diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/IfStatementWithIdenticalBranchesInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/IfStatementWithIdenticalBranchesInspection.java index 6bf82ee8768e..d494d9ef3756 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/IfStatementWithIdenticalBranchesInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/controlflow/IfStatementWithIdenticalBranchesInspection.java @@ -45,6 +45,7 @@ import static com.intellij.util.ObjectUtils.tryCast; // Not really with identical branches, but also common parts public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJavaLocalInspectionTool { public boolean myHighlightWhenLastStatementIsCall = false; + public boolean myHighlightElseIf = false; private static final List ourInspectors = List.of( ImplicitElse::inspect, @@ -57,7 +58,10 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava public @NotNull OptPane getOptionsPane() { return pane( checkbox("myHighlightWhenLastStatementIsCall", - JavaAnalysisBundle.message("inspection.common.if.parts.settings.highlight.when.tail.call"))); + JavaAnalysisBundle.message("inspection.common.if.parts.settings.highlight.when.tail.call")), + checkbox("myHighlightElseIf", + JavaAnalysisBundle.message("inspection.common.if.parts.settings.highlight.else.if")) + ); } @NotNull @@ -69,7 +73,8 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava PsiStatement[] thenStatements = unwrap(ifStatement.getThenBranch()); PsiStatement[] elseStatements = unwrap(ifStatement.getElseBranch()); for (IfStatementInspector inspector : ourInspectors) { - IfInspectionResult result = inspector.inspect(ifStatement, thenStatements, elseStatements, isOnTheFly, myHighlightWhenLastStatementIsCall); + IfInspectionResult result = inspector.inspect(ifStatement, thenStatements, elseStatements, isOnTheFly, + IfStatementWithIdenticalBranchesInspection.this); if (result != null) { ProblemHighlightType highlightType; if (result.myIsWarning) { @@ -108,7 +113,7 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava PsiStatement @NotNull [] thenBranch, PsiStatement @NotNull [] elseBranch, boolean isOnTheFly, - boolean highlightWhenLastStatementIsCall); + IfStatementWithIdenticalBranchesInspection inspection); } private static class IfInspectionResult { @@ -129,11 +134,18 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava } private static class MergeElseIfsFix extends PsiUpdateModCommandQuickFix { + private final boolean myInvert; + + private MergeElseIfsFix(boolean invert) { + myInvert = invert; + } + @Nls @NotNull @Override public String getName() { - return JavaAnalysisBundle.message("inspection.common.if.parts.family.else.if"); + return myInvert ? JavaAnalysisBundle.message("inspection.common.if.parts.family.else.if.invert") + : JavaAnalysisBundle.message("inspection.common.if.parts.family.else.if"); } @Nls @@ -152,13 +164,14 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava PsiExpression condition = ifStatement.getCondition(); if (condition == null) return; CommentTracker ct = new CommentTracker(); - ct.markUnchanged(elseIf.myElseIfThen); + ct.markUnchanged(elseIf.myElseIfBranchToRemove); ct.markUnchanged(condition); - ct.markUnchanged(elseIf.myElseIfCondition); - ct.replace(elseIf.myElseBranch, elseIf.myElseIfElseStatement); + ct.replace(elseIf.myElseBranch, elseIf.myElseIfBranchToKeep); String firstCondition = ParenthesesUtils.getText(condition, ParenthesesUtils.OR_PRECEDENCE); - String secondCondition = ParenthesesUtils.getText(elseIf.myElseIfCondition, ParenthesesUtils.OR_PRECEDENCE); + String secondCondition = elseIf.myInvert ? + BoolUtils.getNegatedExpressionText(elseIf.myElseIfCondition, ParenthesesUtils.OR_PRECEDENCE, ct) : + ParenthesesUtils.getText(ct.markUnchanged(elseIf.myElseIfCondition), ParenthesesUtils.OR_PRECEDENCE); String newCondition = firstCondition + "||" + secondCondition; ct.replaceAndRestoreComments(condition, newCondition); @@ -746,7 +759,7 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava PsiStatement @NotNull [] thenBranch, PsiStatement @NotNull [] elseBranch, boolean isOnTheFly, - boolean highlightWhenLastStatementIsCall) { + IfStatementWithIdenticalBranchesInspection inspection) { ImplicitElse implicitElse = from(thenBranch, elseBranch, ifStatement); if (implicitElse == null) return null; CommonPartType type = implicitElse.getType(); @@ -952,14 +965,14 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava PsiStatement @NotNull [] thenBranch, PsiStatement @NotNull [] elseBranch, boolean isOnTheFly, - boolean highlightWhenLastStatementIsCall) { + IfStatementWithIdenticalBranchesInspection inspection) { ThenElse thenElse = from(ifStatement, thenBranch, elseBranch, isOnTheFly); if (thenElse == null) return null; boolean isNotInCodeBlock = !(ifStatement.getParent() instanceof PsiCodeBlock); boolean mayChangeSemantics = thenElse.myMayChangeSemantics; CommonPartType type = thenElse.myCommonPartType; ExtractCommonIfPartsFix fix = new ExtractCommonIfPartsFix(type, mayChangeSemantics, isOnTheFly); - boolean tailStatementIsSingleCall = !highlightWhenLastStatementIsCall + boolean tailStatementIsSingleCall = !inspection.myHighlightWhenLastStatementIsCall && isSingleCallTail(thenElse.myTailStatementsOfThen) && thenElse.myHeadUnitsOfThen.isEmpty(); boolean isInfoLevel = mayChangeSemantics @@ -1155,20 +1168,23 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava private static final class ElseIf { final @NotNull PsiStatement myElseBranch; - final @NotNull PsiStatement myElseIfElseStatement; - final @NotNull PsiElement myElseIfThen; + final @NotNull PsiStatement myElseIfBranchToKeep; + final @NotNull PsiElement myElseIfBranchToRemove; final @NotNull PsiExpression myElseIfCondition; final @NotNull Map mySubstitutionTable; + final boolean myInvert; private ElseIf(@NotNull PsiStatement elseBranch, - @NotNull PsiStatement elseIfElseStatement, - @NotNull PsiElement then, @NotNull PsiExpression elseIfCondition, - @NotNull Map table) { + @NotNull PsiStatement elseIfBranchToKeep, + @NotNull PsiStatement elseIfBranchToRemove, + @NotNull PsiExpression elseIfCondition, + @NotNull Map table, boolean needInvert) { myElseBranch = elseBranch; - myElseIfElseStatement = elseIfElseStatement; - myElseIfThen = then; + myElseIfBranchToKeep = elseIfBranchToKeep; + myElseIfBranchToRemove = elseIfBranchToRemove; myElseIfCondition = elseIfCondition; mySubstitutionTable = table; + myInvert = needInvert; } @Nullable @@ -1184,21 +1200,31 @@ public class IfStatementWithIdenticalBranchesInspection extends AbstractBaseJava PsiStatement[] elseIfThen = ControlFlowUtils.unwrapBlock(elseIfThenBranch); PsiStatement elseIfElseBranch = elseIf.getElseBranch(); if (elseIfElseBranch == null) return null; - if (elseIfThen.length != thenStatements.length) return null; - LocalEquivalenceChecker equivalence = getChecker(thenStatements, elseIfThen); - if (!branchesAreEquivalent(thenStatements, Arrays.asList(elseIfThen), equivalence)) return null; - return new ElseIf(elseBranch, elseIfElseBranch, elseIfThenBranch, elseIfCondition, equivalence.mySubstitutionTable); + if (elseIfThen.length == thenStatements.length) { + LocalEquivalenceChecker equivalence = getChecker(thenStatements, elseIfThen); + if (branchesAreEquivalent(thenStatements, Arrays.asList(elseIfThen), equivalence)) { + return new ElseIf(elseBranch, elseIfElseBranch, elseIfThenBranch, elseIfCondition, equivalence.mySubstitutionTable, false); + } + } + PsiStatement[] elseIfElse = ControlFlowUtils.unwrapBlock(elseIfElseBranch); + if (elseIfElse.length == thenStatements.length) { + LocalEquivalenceChecker equivalence = getChecker(thenStatements, elseIfThen); + if (branchesAreEquivalent(thenStatements, Arrays.asList(elseIfElse), equivalence)) { + return new ElseIf(elseBranch, elseIfThenBranch, elseIfElseBranch, elseIfCondition, equivalence.mySubstitutionTable, true); + } + } + return null; } @Nullable static IfInspectionResult inspect(@NotNull PsiIfStatement ifStatement, PsiStatement @NotNull [] thenBranch, PsiStatement @NotNull [] elseBranch, boolean isOnTheFly, - boolean highlightWhenLastStatementIsCall) { + IfStatementWithIdenticalBranchesInspection inspection) { ElseIf elseIf = from(ifStatement, thenBranch); if (elseIf == null) return null; String message = JavaAnalysisBundle.message("inspection.common.if.parts.family.else.if.description"); - return new IfInspectionResult(ifStatement.getFirstChild(), false, new MergeElseIfsFix(), message); + return new IfInspectionResult(ifStatement.getFirstChild(), inspection.myHighlightElseIf, new MergeElseIfsFix(elseIf.myInvert), message); } }