diff --git a/java/java-impl/src/com/intellij/codeInspection/intermediaryVariable/ReturnSeparatedFromComputationInspection.java b/java/java-impl/src/com/intellij/codeInspection/intermediaryVariable/ReturnSeparatedFromComputationInspection.java index 8882f2945a88..ce98af30385d 100644 --- a/java/java-impl/src/com/intellij/codeInspection/intermediaryVariable/ReturnSeparatedFromComputationInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/intermediaryVariable/ReturnSeparatedFromComputationInspection.java @@ -18,7 +18,9 @@ package com.intellij.codeInspection.intermediaryVariable; import com.intellij.codeInspection.*; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.project.Project; +import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; +import com.intellij.psi.codeStyle.CodeStyleManager; import com.intellij.psi.controlFlow.*; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.refactoring.util.RefactoringUtil; @@ -194,26 +196,26 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal } private static void applyChanges(@NotNull Mover mover, @NotNull ReturnContext context, boolean removeReturn) { - PsiReturnStatement returnStatement = (PsiReturnStatement)context.returnStatement.copy(); + mover.insertBefore.forEach(e -> e.getParent().addBefore(context.returnStatement, e)); + mover.replaceInline.forEach(e -> { + if (e instanceof PsiBreakStatement) { + replaceStatementKeepComments((PsiBreakStatement)e, context.returnStatement); + } + else if (e instanceof PsiAssignmentExpression) { + inlineAssignment((PsiAssignmentExpression)e, context.returnStatement); + } + }); + mover.removeCompletely.forEach(e -> removeElementKeepComment(e)); if (removeReturn) { removeReturn(context); } - else { - //inlineReturnedValue(mover, context); - } - mover.insertBefore.forEach(e -> e.getParent().addBefore(returnStatement, e)); - mover.replaceInline.forEach(e -> { - if (e instanceof PsiBreakStatement) e.replace(returnStatement); - if (e instanceof PsiAssignmentExpression) inlineAssignment((PsiAssignmentExpression)e, returnStatement); - }); - mover.removeCompletely.forEach(PsiElement::delete); } private static void removeReturn(@NotNull ReturnContext context) { Set skippedEmptyStatements = new THashSet<>(); getPrevNonEmptyStatement(context.returnStatement, skippedEmptyStatements); skippedEmptyStatements.forEach(PsiElement::delete); - context.returnStatement.delete(); + removeElementKeepComment(context.returnStatement); } private static void inlineAssignment(PsiAssignmentExpression assignmentExpression, PsiReturnStatement returnStatement) { @@ -222,9 +224,52 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal PsiReturnStatement returnStatementCopy = (PsiReturnStatement)returnStatement.copy(); PsiExpression rExpression = assignmentExpression.getRExpression(); PsiExpression returnValue = returnStatementCopy.getReturnValue(); - if (rExpression != null && returnValue!=null) { + if (rExpression != null && returnValue != null) { returnValue.replace(rExpression); - assignmentParent.replace(returnStatementCopy); + replaceStatementKeepComments((PsiExpressionStatement)assignmentParent, returnStatementCopy); + } + } + + private static void replaceStatementKeepComments(PsiStatement replacedStatement, PsiReturnStatement returnStatement) { + List keptComments = new ArrayList<>(); + for (PsiElement element = replacedStatement.getFirstChild(); element != null; element = element.getNextSibling()) { + if (element instanceof PsiComment) { + keptComments.add((PsiComment)element); + } + } + if (!keptComments.isEmpty()) { + returnStatement = (PsiReturnStatement)returnStatement.copy(); + PsiElement lastReturnChild = returnStatement.getLastChild(); + Project project = returnStatement.getProject(); + PsiParserFacade parserFacade = PsiParserFacade.SERVICE.getInstance(project); + PsiElementFactory elementFactory = JavaPsiFacade.getInstance(project).getElementFactory(); + if (lastReturnChild instanceof PsiComment && ((PsiComment)lastReturnChild).getTokenType() == JavaTokenType.END_OF_LINE_COMMENT) { + String commentText = StringUtil.trimStart(lastReturnChild.getText(), "//"); + PsiComment inlineComment = elementFactory.createCommentFromText("/* " + commentText + " */", returnStatement); + lastReturnChild = lastReturnChild.replace(inlineComment); + } + for (PsiComment comment : keptComments) { + lastReturnChild = returnStatement.addAfter(parserFacade.createWhiteSpaceFromText(" "), lastReturnChild); + lastReturnChild = returnStatement.addAfter(comment, lastReturnChild); + } + CodeStyleManager.getInstance(project).reformat(returnStatement, true); + } + replacedStatement.replace(returnStatement); + } + + private static void removeElementKeepComment(PsiElement element) { + PsiComment comment = null; + for (PsiElement child = element.getLastChild(); child != null; child = child.getPrevSibling()) { + if (child instanceof PsiComment) { + comment = (PsiComment)child; + break; + } + } + if (comment != null) { + element.replace(comment); + } + else { + element.delete(); } } @@ -279,6 +324,9 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal if (targetStatement instanceof PsiForeachStatement) { return moveToForeach((PsiForeachStatement)targetStatement); } + if (targetStatement instanceof PsiSwitchStatement) { + return moveToSwitch((PsiSwitchStatement)targetStatement, returnAtTheEnd); + } if (targetStatement instanceof PsiTryStatement) { return moveToTry((PsiTryStatement)targetStatement, returnAtTheEnd); } @@ -348,6 +396,12 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal return false; } + private boolean moveToSwitch(PsiSwitchStatement targetStatement, boolean returnAtTheEnd) { + moveToBreaks(targetStatement, false); + PsiCodeBlock body = targetStatement.getBody(); + return body != null && moveToBlockBody(body, returnAtTheEnd); + } + private boolean moveToTry(@NotNull PsiTryStatement targetStatement, boolean returnAtTheEnd) { PsiCodeBlock tryBlock = targetStatement.getTryBlock(); if (tryBlock == null) { diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterForBreakComment.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterForBreakComment.java new file mode 100644 index 000000000000..5c9602050626 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterForBreakComment.java @@ -0,0 +1,17 @@ +// "Move 'return' to computation of the value of 'n'" "true" +class T { + int f(int a[]) { + int n = 0; + for (int i=0; i 100) { + return n; // at the end 1 + } + if (n < 0) { + return 0; // at the end 2 + /* inline */ + } + } + return n; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterIfComment.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterIfComment.java new file mode 100644 index 000000000000..ce1aa7216759 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterIfComment.java @@ -0,0 +1,16 @@ +// "Move 'return' to computation of the value of 's'" "true" +class T { + int f(String a) { + String s = a; + if (s == null) { + return ""; /* return comment */ // end of line + } + else if (s.startsWith("@")) { + return s.substring(1); // return comment + } + else if (s.startsWith("#")) { + return "#"; /* return comment */ /* inline */ + } + return s; // return comment + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeForBreakComment.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeForBreakComment.java new file mode 100644 index 000000000000..8ee9c838a6c7 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeForBreakComment.java @@ -0,0 +1,17 @@ +// "Move 'return' to computation of the value of 'n'" "true" +class T { + int f(int a[]) { + int n = 0; + for (int i=0; i 100) { + break; // at the end 1 + } + if (n < 0) { + n = 0; // at the end 2 + break; /* inline */ + } + } + return n; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeIfComment.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeIfComment.java new file mode 100644 index 000000000000..ac6cb9259988 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeIfComment.java @@ -0,0 +1,16 @@ +// "Move 'return' to computation of the value of 's'" "true" +class T { + int f(String a) { + String s = a; + if (s == null) { + s = ""; // end of line + } + else if (s.startsWith("@")) { + s = s.substring(1); + } + else if (s.startsWith("#")) { + s = "#"; /* inline */ + } + return s; // return comment + } +} \ No newline at end of file diff --git a/resources-en/src/inspectionDescriptions/ReturnSeparatedFromComputation.html b/resources-en/src/inspectionDescriptions/ReturnSeparatedFromComputation.html index 83e1a8c0a149..6c3f71f1f4d0 100644 --- a/resources-en/src/inspectionDescriptions/ReturnSeparatedFromComputation.html +++ b/resources-en/src/inspectionDescriptions/ReturnSeparatedFromComputation.html @@ -3,12 +3,24 @@ This inspection detects return statements which return a local variable, where the value of the variable is computed somewhere else within the same code block with the return statement.

The quick fix inlines the returned variable by moving the return statement to the location where the value of the variable is computed. - For example, the code below could be simplified: + When the returned value can't be inlined into return statement, the quick fix attempts to move the return statement as close to the + computation of the returned value as possible. +

For example, the code below could be simplified:

int n = -1;
-if (condition) n = compute();
+for(int i = 0; i < a.length; i++) {
+    if (a[i] == b) {
+        n = i;
+        break;
+    }
+}
+return n;
+After the quick fix it becomes the following: +
int n = -1;
+for(int i = 0; i < a.length; i++) {
+    if (a[i] == b) {
+        return i;
+    }
+}
 return n;
- After the quick fix it becomes the following: -
if (condition) return compute();
-return -1;
\ No newline at end of file