Support BreakConverter in ConvertSwitchToIfIntention

Fixes IDEA-141261 'Replace 'switch' with 'if'' intention produces incorrect code if some 'case' clause contains 'break' statement inside 'if'

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<PsiSwitchLabelStatement> 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<SwitchStatementBranch> openBranches = new ArrayList<>();
     final Set<PsiLocalVariable> declaredVariables = new HashSet<>();
     final List<SwitchStatementBranch> 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("}");
   }
 }

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");
+      }
+  }
+}

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");
+      }
+  }
+}

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;
+  }
+}

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
       }
   }
 }

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) {
+    swi<caret>tch (s) {
+      case "a":
+        System.out.println("a");
+        if (r) {
+          break;
+        }
+      default:
+        System.out.println("d");
+    }
+  }
+}

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) {
+    swi<caret>tch (s) {
+      case "a":
+        System.out.println("a");
+        if (r) {
+          break;
+        } else {
+          throw new RuntimeException();
+        }
+      default:
+        System.out.println("d");
+    }
+  }
+}

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) {
+    swi<caret>tch (s) {
+      case "a":
+        System.out.println("a");
+        if (r) {
+          break;
+        }
+      default:
+        System.out.println("d");
+    }
+    System.out.println("oops");
+  }
+}

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:
+      swi<caret>tch (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;
+  }
+}

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);
     }
   }
 }

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<PsiBreakStatement> 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;
   }

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();

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()) {