Java inspection: Fixed handling of break and continue in "Move return to computation" (IDEA-121153)

java/java-impl/src/com/intellij/codeInspection/intermediaryVariable/ReturnSeparatedFromComputationInspection.java

@@ -132,7 +132,7 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
       return false;
     }
 
-    Mover mover = new Mover(flow, context.refactoredStatement, context.returnedVariable);
+    Mover mover = new Mover(flow, context.refactoredStatement, context.returnedVariable, true);
     mover.moveTo(context.refactoredStatement, true);
     return !mover.isEmpty();
   }
@@ -142,7 +142,7 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
     if (context != null) {
       ControlFlow flow = createControlFlow(context);
       if (flow != null) {
-        Mover mover = new Mover(flow, context.refactoredStatement, context.returnedVariable);
+        Mover mover = new Mover(flow, context.refactoredStatement, context.returnedVariable, false);
         boolean removeReturn = mover.moveTo(context.refactoredStatement, true);
         if (!mover.isEmpty()) {
           applyChanges(mover, context, removeReturn);
@@ -159,7 +159,6 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
     else {
       //inlineReturnedValue(mover, context);
     }
-    mover.insertAfter.forEach(e -> e.getParent().addAfter(returnStatement, e));
     mover.insertBefore.forEach(e -> e.getParent().addBefore(returnStatement, e));
     mover.replaceInline.forEach(e -> {
       if (e instanceof PsiBreakStatement) e.replace(returnStatement);
@@ -253,21 +252,25 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
     final ControlFlow flow;
     final PsiStatement enclosingStatement;
     final PsiVariable resultVariable;
+    final boolean checkingApplicability;
     final Set<PsiElement> insertBefore = new THashSet<>();
-    final Set<PsiElement> insertAfter = new THashSet<>();
     final Set<PsiElement> replaceInline = new THashSet<>();
-    final Set<PsiElement> removeCompletely = new THashSet<>();
 
+    final Set<PsiElement> removeCompletely = new THashSet<>();
     private Map<PsiStatement, Set<PsiBreakStatement>> breakStatements;
 
-    private Mover(@NotNull ControlFlow flow, @NotNull PsiStatement enclosingStatement, @NotNull PsiVariable resultVariable) {
+    private Mover(@NotNull ControlFlow flow,
+                  @NotNull PsiStatement enclosingStatement,
+                  @NotNull PsiVariable resultVariable,
+                  boolean checkingApplicability) {
       this.flow = flow;
       this.enclosingStatement = enclosingStatement;
       this.resultVariable = resultVariable;
+      this.checkingApplicability = checkingApplicability;
     }
 
     boolean isEmpty() {
-      return insertBefore.isEmpty() && insertAfter.isEmpty() && replaceInline.isEmpty();
+      return insertBefore.isEmpty() && replaceInline.isEmpty();
     }
 
     /**
@@ -275,6 +278,9 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
      * so if the next statement is a return or a break it can be removed safely.
      */
     boolean moveTo(PsiStatement targetStatement, boolean returnAtTheEnd) {
+      if (checkingApplicability && !isEmpty()) {
+        return false; // optimization
+      }
       if (targetStatement instanceof PsiBlockStatement) {
         return moveToBlock((PsiBlockStatement)targetStatement, returnAtTheEnd);
       }
@@ -302,7 +308,10 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
       if (targetStatement instanceof PsiExpressionStatement) {
         return inlineExpression((PsiExpressionStatement)targetStatement);
       }
-      if (targetStatement instanceof PsiThrowStatement || targetStatement instanceof PsiReturnStatement) {
+      if (targetStatement instanceof PsiThrowStatement ||
+          targetStatement instanceof PsiReturnStatement ||
+          targetStatement instanceof PsiBreakStatement ||
+          targetStatement instanceof PsiContinueStatement) {
         return true;
       }
       return false;
@@ -459,6 +468,9 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
   }
 
   private static PsiStatement getPrevNonEmptyStatement(@NotNull PsiElement psiElement, @NotNull Set<PsiElement> skippedEmptyStatements) {
+    if (!(psiElement.getParent() instanceof PsiCodeBlock)) {
+      return null;
+    }
     PsiStatement prevStatement = PsiTreeUtil.getPrevSiblingOfType(psiElement, PsiStatement.class);
     List<PsiStatement> skipped = new ArrayList<>();
     while (prevStatement instanceof PsiEmptyStatement) {

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterWhileBreak1.java

@@ -0,0 +1,14 @@
+// "Move 'return' to computation of the value of 'n'" "true"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (true) {
+            n = g();
+            if(n != 0) return n;
+        }
+    }
+
+    int g() {
+        return 1;
+    }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterWhileBreak2.java

@@ -0,0 +1,15 @@
+// "Move 'return' to computation of the value of 'n'" "true"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (n <= 0) {
+            n = g();
+            if (n != 0) return n;
+        }
+        return n;
+    }
+
+    int g() {
+        return 1;
+    }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterWhileContinue.java

@@ -5,16 +5,15 @@ class T {
         int t = a;
         while (t != null) {
             if (t == 1) {
-                n = 10;
+                return 10;
             }
             else if (t == 2) {
-                n = 20;
+                return 20;
             }
             else {
                 t = t + 1;
                 continue;
             }
-            break;
         }
         return n;
     }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterWhileContinue1.java

@@ -0,0 +1,15 @@
+// "Move 'return' to computation of the value of 'n'" "true"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (true) {
+            n = g();
+            if(n == 0) continue;
+            else return n;
+        }
+    }
+
+    int g() {
+        return 1;
+    }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterWhileContinue2.java

@@ -0,0 +1,15 @@
+// "Move 'return' to computation of the value of 'n'" "true"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (true) {
+            n = g();
+            if(n == 0) continue;
+            return n;
+        }
+    }
+
+    int g() {
+        return 1;
+    }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeWhileBreak1.java

@@ -0,0 +1,15 @@
+// "Move 'return' to computation of the value of 'n'" "true"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (true) {
+            n = g();
+            if(n != 0) break;
+        }
+        ret<caret>urn n;
+    }
+
+    int g() {
+        return 1;
+    }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeWhileBreak2.java

@@ -0,0 +1,15 @@
+// "Move 'return' to computation of the value of 'n'" "true"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (n <= 0) {
+            n = g();
+            if (n != 0) break;
+        }
+        ret<caret>urn n;
+    }
+
+    int g() {
+        return 1;
+    }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeWhileContinue1.java

@@ -0,0 +1,16 @@
+// "Move 'return' to computation of the value of 'n'" "true"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (true) {
+            n = g();
+            if(n == 0) continue;
+            else break;
+        }
+        ret<caret>urn n;
+    }
+
+    int g() {
+        return 1;
+    }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeWhileContinue2.java

@@ -0,0 +1,16 @@
+// "Move 'return' to computation of the value of 'n'" "true"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (true) {
+            n = g();
+            if(n == 0) continue;
+            break;
+        }
+        ret<caret>urn n;
+    }
+
+    int g() {
+        return 1;
+    }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeWhileContinue3.java

@@ -0,0 +1,16 @@
+// "Move 'return' to computation of the value of 'n'" "false"
+class T {
+    int f(int a) {
+        int n = -1;
+        while (n <= 0) {
+            n = g();
+            if (n == 0) continue;
+            n++;
+        }
+        ret<caret>urn n;
+    }
+
+    int g() {
+        return 1;
+    }
+}

platform/platform-resources-en/src/messages/InspectionsBundle.properties

@@ -139,7 +139,7 @@ inspection.can.be.local.variable.problem.descriptor=Variable <code>#ref</code> c
 inspection.return.separated.from.computation.name=Return separated from computation of result
 inspection.return.separated.from.computation.descriptor=Return separated from computation of value of ''{0}''
 inspection.return.separated.from.computation.quickfix=Move ''return'' to computation of the value of ''{0}''
-inspection.return.separated.from.computation.family.quickfix=Move ''return'' to computation of the result
+inspection.return.separated.from.computation.family.quickfix=Move 'return' to computation of the result
 
 inspection.nullable.problems.display.name=@NotNull/@Nullable problems
 #check box options