From f859a4c439db756988b066e0616cdb33536da1ad Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 26 Sep 2018 11:58:20 +0700 Subject: [PATCH] Support BreakConverter in ConvertSwitchToIfIntention Fixes IDEA-141261 'Replace 'switch' with 'if'' intention produces incorrect code if some 'case' clause contains 'break' statement inside 'if' --- .../quickfix/ConvertSwitchToIfIntention.java | 51 ++++++------------- .../convertSwitchToIf/afterBreakInIf.java | 15 ++++++ .../convertSwitchToIf/afterBreakInIf2.java | 14 +++++ .../afterBreakWithLabel.java | 23 +++++++++ .../convertSwitchToIf/afterString.java | 8 ++- .../convertSwitchToIf/beforeBreakInIf.java | 14 +++++ .../convertSwitchToIf/beforeBreakInIf2.java | 16 ++++++ .../beforeBreakInIfCannotConvert.java | 15 ++++++ .../beforeBreakWithLabel.java | 22 ++++++++ .../fixes/DeleteUnnecessaryStatementFix.java | 7 +-- .../com/siyeh/ig/psiutils/BreakConverter.java | 28 +++++----- ...hStatementWithSingleDefaultInspection.java | 2 +- .../siyeh/ipp/switchtoif/SwitchPredicate.java | 4 ++ 13 files changed, 161 insertions(+), 58 deletions(-) create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakInIf.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakInIf2.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakWithLabel.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIf.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIf2.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIfCannotConvert.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakWithLabel.java diff --git a/java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/ConvertSwitchToIfIntention.java b/java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/ConvertSwitchToIfIntention.java index 2b139106082b..1583d937dc74 100644 --- a/java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/ConvertSwitchToIfIntention.java +++ b/java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/ConvertSwitchToIfIntention.java @@ -7,14 +7,16 @@ import com.intellij.openapi.editor.Editor; import com.intellij.openapi.project.Project; import com.intellij.psi.*; import com.intellij.psi.codeStyle.JavaCodeStyleManager; -import com.intellij.psi.controlFlow.*; import com.intellij.psi.search.LocalSearchScope; import com.intellij.psi.search.searches.ReferencesSearch; import com.intellij.psi.util.PsiUtil; import com.intellij.psi.util.TypeConversionUtil; import com.intellij.refactoring.util.RefactoringUtil; +import com.siyeh.ig.psiutils.BreakConverter; import com.siyeh.ig.psiutils.CommentTracker; +import com.siyeh.ig.psiutils.ControlFlowUtils; import com.siyeh.ig.psiutils.ParenthesesUtils; +import one.util.streamex.StreamEx; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; @@ -45,7 +47,7 @@ public class ConvertSwitchToIfIntention implements IntentionAction { @Override public boolean isAvailable(@NotNull Project project, Editor editor, PsiFile file) { final PsiCodeBlock body = mySwitchExpression.getBody(); - return body != null && !body.isEmpty(); + return body != null && !body.isEmpty() && BreakConverter.from(mySwitchExpression) != null; } @Override @@ -110,6 +112,14 @@ public class ConvertSwitchToIfIntention implements IntentionAction { if (body == null) { return; } + Set fallThroughTargets = + StreamEx.of(body.getStatements()) + .pairMap((s1, s2) -> s2 instanceof PsiSwitchLabelStatement && ControlFlowUtils.statementMayCompleteNormally(s1) + ? (PsiSwitchLabelStatement)s2 : null) + .nonNull().toSet(); + BreakConverter converter = BreakConverter.from(switchStatement); + if (converter == null) return; + converter.process(); final List openBranches = new ArrayList<>(); final Set declaredVariables = new HashSet<>(); final List allBranches = new ArrayList<>(); @@ -119,7 +129,7 @@ public class ConvertSwitchToIfIntention implements IntentionAction { final PsiElement statement = children[i]; if (statement instanceof PsiSwitchLabelStatement) { final PsiSwitchLabelStatement label = (PsiSwitchLabelStatement)statement; - if (currentBranch == null) { + if (currentBranch == null || !fallThroughTargets.contains(statement)) { openBranches.clear(); currentBranch = new SwitchStatementBranch(); currentBranch.addPendingVariableDeclarations(declaredVariables); @@ -153,18 +163,6 @@ public class ConvertSwitchToIfIntention implements IntentionAction { for (SwitchStatementBranch branch : openBranches) { branch.addStatement(statement); } - try { - ControlFlow controlFlow = - ControlFlowFactory.getInstance(project).getControlFlow(statement, LocalsOrMyInstanceFieldsControlFlowPolicy.getInstance()); - int startOffset = controlFlow.getStartOffset(statement); - int endOffset = controlFlow.getEndOffset(statement); - if (startOffset != -1 && endOffset != -1 && !ControlFlowUtil.canCompleteNormally(controlFlow, startOffset, endOffset)) { - currentBranch = null; - } - } - catch (AnalysisCanceledException e) { - currentBranch = null; - } } else { for (SwitchStatementBranch branch : openBranches) { @@ -280,35 +278,18 @@ public class ConvertSwitchToIfIntention implements IntentionAction { } } - boolean addLineBreak = true; for (PsiElement bodyStatement : bodyStatements) { if (bodyStatement instanceof PsiBlockStatement) { final PsiBlockStatement blockStatement = (PsiBlockStatement)bodyStatement; final PsiCodeBlock codeBlock = blockStatement.getCodeBlock(); for (PsiStatement statement : codeBlock.getStatements()) { - appendElement(statement, out, commentTracker); + out.append(commentTracker.text(statement)); } } else { - addLineBreak = appendElement(bodyStatement, out, commentTracker); + out.append(commentTracker.text(bodyStatement)); } } - if (addLineBreak) { - out.append("\n"); - } - out.append("}"); - } - - private static boolean appendElement(PsiElement element, @NonNls StringBuilder out, CommentTracker commentTracker) { - if (element instanceof PsiBreakStatement) { - final PsiBreakStatement breakStatement = (PsiBreakStatement)element; - final PsiIdentifier identifier = breakStatement.getLabelIdentifier(); - if (identifier == null) { - return false; - } - } - out.append(commentTracker.text(element)); - - return true; + out.append("\n").append("}"); } } diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakInIf.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakInIf.java new file mode 100644 index 000000000000..a41b9f2bd22c --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakInIf.java @@ -0,0 +1,15 @@ +// "Replace 'switch' with 'if'" "true" +class X { + void m(String s, boolean r) { + if ("a".equals(s)) { + System.out.println("a"); + if (r) { + return; + } + + System.out.println("d"); + } else { + System.out.println("d"); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakInIf2.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakInIf2.java new file mode 100644 index 000000000000..8c007a92ecae --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakInIf2.java @@ -0,0 +1,14 @@ +// "Replace 'switch' with 'if'" "true" +class X { + void m(String s, boolean r) { + if ("a".equals(s)) { + System.out.println("a"); + if (r) { + } else { + throw new RuntimeException(); + } + } else { + System.out.println("d"); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakWithLabel.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakWithLabel.java new file mode 100644 index 000000000000..8d22cf53ec9b --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterBreakWithLabel.java @@ -0,0 +1,23 @@ +// "Replace 'switch' with 'if'" "true" +class X { + int m(String s, int x) { + if (x > 0) { + SWITCH: + if ("a".equals(s)) { + System.out.println("a"); + for (int i = 0; i < 10; i++) { + System.out.println(i); + if (i == x) return 0; + if (i == x * 2) break; + } + + System.out.println("d"); + } else { + System.out.println("d"); + } + } else { + return 1; + } + return 0; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterString.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterString.java index 8beef0578e14..151cdbf4fe51 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterString.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/afterString.java @@ -2,15 +2,13 @@ class X { public void doSomething( String value) { //comment1 - //comment3 //comment4 - //comment5 //comment6 - //comment7 //comment8 if ("case1".equals(value)) {//comment2 - } else if ("case2".equals(value)) { - } else { + //comment3 + } else if ("case2".equals(value)) {//comment5 + } else {//comment7 } } } \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIf.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIf.java new file mode 100644 index 000000000000..d82b2338958c --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIf.java @@ -0,0 +1,14 @@ +// "Replace 'switch' with 'if'" "true" +class X { + void m(String s, boolean r) { + switch (s) { + case "a": + System.out.println("a"); + if (r) { + break; + } + default: + System.out.println("d"); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIf2.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIf2.java new file mode 100644 index 000000000000..6f2f1d228aa9 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIf2.java @@ -0,0 +1,16 @@ +// "Replace 'switch' with 'if'" "true" +class X { + void m(String s, boolean r) { + switch (s) { + case "a": + System.out.println("a"); + if (r) { + break; + } else { + throw new RuntimeException(); + } + default: + System.out.println("d"); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIfCannotConvert.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIfCannotConvert.java new file mode 100644 index 000000000000..f9892379960a --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakInIfCannotConvert.java @@ -0,0 +1,15 @@ +// "Replace 'switch' with 'if'" "false" +class X { + void m(String s, boolean r) { + switch (s) { + case "a": + System.out.println("a"); + if (r) { + break; + } + default: + System.out.println("d"); + } + System.out.println("oops"); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakWithLabel.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakWithLabel.java new file mode 100644 index 000000000000..37417154c195 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertSwitchToIf/beforeBreakWithLabel.java @@ -0,0 +1,22 @@ +// "Replace 'switch' with 'if'" "true" +class X { + int m(String s, int x) { + if (x > 0) { + SWITCH: + switch (s){ + case "a": + System.out.println("a"); + for(int i=0; i<10; i++) { + System.out.println(i); + if(i == x) break SWITCH; + if(i == x*2) break; + } + default: + System.out.println("d"); + } + } else { + return 1; + } + return 0; + } +} \ No newline at end of file diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/fixes/DeleteUnnecessaryStatementFix.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/fixes/DeleteUnnecessaryStatementFix.java index b2f937e80a65..4df5c1d8f252 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/fixes/DeleteUnnecessaryStatementFix.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/fixes/DeleteUnnecessaryStatementFix.java @@ -21,7 +21,7 @@ import com.intellij.psi.*; import com.intellij.psi.util.PsiTreeUtil; import com.siyeh.InspectionGadgetsBundle; import com.siyeh.ig.InspectionGadgetsFix; -import com.siyeh.ig.PsiReplacementUtil; +import com.siyeh.ig.psiutils.CommentTracker; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; @@ -57,16 +57,17 @@ public class DeleteUnnecessaryStatementFix extends InspectionGadgetsFix { } public static void deleteUnnecessaryStatement(PsiStatement statement) { + CommentTracker ct = new CommentTracker(); final PsiElement parent = statement.getParent(); if (parent instanceof PsiIfStatement || parent instanceof PsiWhileStatement || parent instanceof PsiDoWhileStatement || parent instanceof PsiForeachStatement || parent instanceof PsiForStatement) { - PsiReplacementUtil.replaceStatement(statement, "{}"); + ct.replaceAndRestoreComments(statement, "{}"); } else { - deleteElement(statement); + ct.deleteAndRestoreComments(statement); } } } \ No newline at end of file diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/BreakConverter.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/BreakConverter.java index f8ff1977499c..17060fbd5044 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/BreakConverter.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/BreakConverter.java @@ -23,13 +23,11 @@ public class BreakConverter { myReplacement = replacement; } - public void process(boolean removeRemovable) { + public void process() { List breaks = collectBreaks(); for (PsiBreakStatement breakStatement : breaks) { if (isRemovable(mySwitchStatement, breakStatement)) { - if (removeRemovable) { - DeleteUnnecessaryStatementFix.deleteUnnecessaryStatement(breakStatement); - } + DeleteUnnecessaryStatementFix.deleteUnnecessaryStatement(breakStatement); } else { assert myReplacement != null; new CommentTracker().replaceAndRestoreComments(breakStatement, myReplacement); @@ -100,17 +98,19 @@ public class BreakConverter { return isRemovable(switchStatement, (PsiStatement)parent); } PsiStatement nextStatement = PsiTreeUtil.getNextSiblingOfType(statement, PsiStatement.class); - if (nextStatement != null) { - return nextStatement instanceof PsiBreakStatement && - ((PsiBreakStatement)nextStatement).findExitedStatement() == switchStatement; - } - if (parent == null) return false; - if (parent instanceof PsiCodeBlock) { - PsiElement grandParent = parent.getParent(); - if (grandParent instanceof PsiBlockStatement) { - return isRemovable(switchStatement, (PsiStatement)grandParent); + if (nextStatement == null) { + if (parent instanceof PsiCodeBlock) { + PsiElement grandParent = parent.getParent(); + return grandParent == switchStatement || + grandParent instanceof PsiBlockStatement && isRemovable(switchStatement, (PsiStatement)grandParent); } - return grandParent == switchStatement; + } + if (nextStatement instanceof PsiSwitchLabelStatement) { + return (((PsiSwitchLabelStatement)nextStatement).getEnclosingSwitchStatement() == switchStatement && + !ControlFlowUtils.statementMayCompleteNormally(statement)); + } + if (nextStatement instanceof PsiBreakStatement) { + return ((PsiBreakStatement)nextStatement).findExitedStatement() == switchStatement; } return false; } diff --git a/plugins/InspectionGadgets/src/com/siyeh/ig/redundancy/SwitchStatementWithSingleDefaultInspection.java b/plugins/InspectionGadgets/src/com/siyeh/ig/redundancy/SwitchStatementWithSingleDefaultInspection.java index 4e70ee763f6d..3c6f0223866d 100644 --- a/plugins/InspectionGadgets/src/com/siyeh/ig/redundancy/SwitchStatementWithSingleDefaultInspection.java +++ b/plugins/InspectionGadgets/src/com/siyeh/ig/redundancy/SwitchStatementWithSingleDefaultInspection.java @@ -64,7 +64,7 @@ public class SwitchStatementWithSingleDefaultInspection extends AbstractBaseJava if (body == null) return; BreakConverter breakConverter = BreakConverter.from(statement); if (breakConverter == null) return; - breakConverter.process(true); + breakConverter.process(); PsiSwitchLabelStatement defaultCase = PsiTreeUtil.getChildOfType(body, PsiSwitchLabelStatement.class); if (defaultCase == null || !defaultCase.isDefaultCase()) return; defaultCase.delete(); diff --git a/plugins/IntentionPowerPak/src/com/siyeh/ipp/switchtoif/SwitchPredicate.java b/plugins/IntentionPowerPak/src/com/siyeh/ipp/switchtoif/SwitchPredicate.java index 736ca3fcbd0a..6128e0f4ead7 100644 --- a/plugins/IntentionPowerPak/src/com/siyeh/ipp/switchtoif/SwitchPredicate.java +++ b/plugins/IntentionPowerPak/src/com/siyeh/ipp/switchtoif/SwitchPredicate.java @@ -17,6 +17,7 @@ package com.siyeh.ipp.switchtoif; import com.intellij.psi.*; import com.intellij.psi.tree.IElementType; +import com.siyeh.ig.psiutils.BreakConverter; import com.siyeh.ipp.base.PsiElementPredicate; import com.siyeh.ipp.psiutils.ErrorUtil; import org.jetbrains.annotations.NotNull; @@ -52,6 +53,9 @@ class SwitchPredicate implements PsiElementPredicate { if (ErrorUtil.containsError(switchStatement)) { return false; } + if (BreakConverter.from(switchStatement) == null) { + return false; + } final PsiStatement[] statements = body.getStatements(); for (PsiStatement statement : statements) { if (statement instanceof PsiSwitchLabelStatement && !((PsiSwitchLabelStatement)statement).isDefaultCase()) {