diff --git a/java/java-impl/src/com/intellij/codeInsight/intention/impl/singlereturn/ExitContext.java b/java/java-impl/src/com/intellij/codeInsight/intention/impl/singlereturn/ExitContext.java index fd756b70e717..ed56a060df3d 100644 --- a/java/java-impl/src/com/intellij/codeInsight/intention/impl/singlereturn/ExitContext.java +++ b/java/java-impl/src/com/intellij/codeInsight/intention/impl/singlereturn/ExitContext.java @@ -27,7 +27,7 @@ class ExitContext { private final @NotNull String myReturnVariable; private final @NotNull PsiElementFactory myFactory; boolean myReturnVariableUsed = false; - PsiExpression myReturnVariableDefaultValue; + final PsiExpression myReturnVariableDefaultValue; ExitContext(@NotNull PsiCodeBlock block, @NotNull PsiType returnType, @NotNull FinishMarker marker) { myBlock = block; @@ -35,9 +35,10 @@ class ExitContext { myReturnType = returnType; myReturnVariable = new VariableNameGenerator(block, VariableKind.LOCAL_VARIABLE).byName("result", "res").byType(returnType).generate(true); - myReturnVariableDefaultValue = marker.myDefaultValue; - if (myReturnVariableDefaultValue != null && myReturnVariableDefaultValue.isPhysical()) { - myReturnVariableDefaultValue = (PsiExpression)myReturnVariableDefaultValue.copy(); + if (marker.myDefaultValue != null && marker.myDefaultValue.isPhysical()) { + myReturnVariableDefaultValue = (PsiExpression)marker.myDefaultValue.copy(); + } else { + myReturnVariableDefaultValue = marker.myDefaultValue; } myFinishMarkerType = marker.myType; } @@ -76,12 +77,7 @@ class ExitContext { void registerReturnValue(PsiExpression value, List replacements) { myReturnVariableUsed = true; - if (FinishMarker.canMoveToStart(value) && - (myReturnVariableDefaultValue == null || - EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(myReturnVariableDefaultValue, value))) { - myReturnVariableDefaultValue = (PsiExpression)value.copy(); - } - else { + if (!EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(myReturnVariableDefaultValue, value)) { replacements.add(0, myReturnVariable + "=" + value.getText() + ";"); } } @@ -115,11 +111,11 @@ class ExitContext { } if (myReturnVariableUsed) { PsiJavaToken start = requireNonNull(myBlock.getLBrace()); - if (myReturnVariableDefaultValue == null && myFinishedVariable != null) { - myReturnVariableDefaultValue = myFactory.createExpressionFromText(PsiTypesUtil.getDefaultValueOfType(myReturnType), null); + PsiExpression initializer = myReturnVariableDefaultValue; + if (initializer == null && myFinishedVariable != null) { + initializer = myFactory.createExpressionFromText(PsiTypesUtil.getDefaultValueOfType(myReturnType), null); } - PsiDeclarationStatement declaration = - myFactory.createVariableDeclarationStatement(myReturnVariable, myReturnType, myReturnVariableDefaultValue); + PsiDeclarationStatement declaration = myFactory.createVariableDeclarationStatement(myReturnVariable, myReturnType, initializer); PsiLocalVariable var = (PsiLocalVariable)((PsiDeclarationStatement)myBlock.addAfter(declaration, start)).getDeclaredElements()[0]; if (var.hasModifierProperty(PsiModifier.FINAL) && !RefactoringUtil.canBeDeclaredFinal(var)) { // Keep final when possible to respect code style setting "generate local variables as 'final'" diff --git a/java/java-impl/src/com/intellij/codeInsight/intention/impl/singlereturn/FinishMarker.java b/java/java-impl/src/com/intellij/codeInsight/intention/impl/singlereturn/FinishMarker.java index 7465520dc3c0..28118c6bb7ab 100644 --- a/java/java-impl/src/com/intellij/codeInsight/intention/impl/singlereturn/FinishMarker.java +++ b/java/java-impl/src/com/intellij/codeInsight/intention/impl/singlereturn/FinishMarker.java @@ -13,11 +13,12 @@ import com.intellij.util.ArrayUtil; import com.siyeh.ig.psiutils.ControlFlowUtils; import com.siyeh.ig.psiutils.ExpressionUtils; import one.util.streamex.StreamEx; +import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.List; -import java.util.Set; +import java.util.*; +import java.util.stream.Collectors; import static com.intellij.util.ObjectUtils.NULL; import static com.intellij.util.ObjectUtils.tryCast; @@ -146,7 +147,7 @@ class FinishMarker { if (nonTerminalReturnValues.size() == 1 && nonTerminalReturnValues.iterator().next() != NULL) { return new FinishMarker(FinishMarkerType.SEPARATE_VAR, nonTerminalReturns.iterator().next()); } - return new FinishMarker(FinishMarkerType.SEPARATE_VAR, null); + return new FinishMarker(FinishMarkerType.SEPARATE_VAR, findBestExpression(terminalReturn, nonTerminalReturns)); } if (PsiType.BOOLEAN.equals(returnType)) { if (nonTerminalReturnValues.size() == 1) { @@ -173,7 +174,25 @@ class FinishMarker { return new FinishMarker(FinishMarkerType.SEPARATE_VAR, value); } } - return new FinishMarker(FinishMarkerType.SEPARATE_VAR, null); + return new FinishMarker(FinishMarkerType.SEPARATE_VAR, findBestExpression(terminalReturn, nonTerminalReturns)); + } + + @Nullable + private static PsiExpression findBestExpression(PsiReturnStatement terminalReturn, List nonTerminalReturns) { + List bestGroup = StreamEx.of(nonTerminalReturns) + .filter(FinishMarker::canMoveToStart) + .groupingBy(PsiExpression::getText, LinkedHashMap::new, Collectors.toList()) + .values() + .stream() + .max(Comparator.comparingInt(List::size)) + .orElse(Collections.emptyList()); + if (bestGroup.size() >= 2) { + return bestGroup.get(0); + } + if (terminalReturn != null && canMoveToStart(terminalReturn.getReturnValue())) { + return terminalReturn.getReturnValue(); + } + return null; } @NotNull @@ -217,9 +236,10 @@ class FinishMarker { return new FinishMarker(FinishMarkerType.VALUE_NON_EQUAL, factory.createExpressionFromText(text, null)); } } - return new FinishMarker(FinishMarkerType.SEPARATE_VAR, null); + return new FinishMarker(FinishMarkerType.SEPARATE_VAR, findBestExpression(terminalReturn, nonTerminalReturns)); } + @Contract("null -> false") static boolean canMoveToStart(PsiExpression value) { if (!ExpressionUtils.isSafelyRecomputableExpression(value)) return false; PsiReferenceExpression ref = tryCast(PsiUtil.skipParenthesizedExprDown(value), PsiReferenceExpression.class); diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterChangedParameter.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterChangedParameter.java index fc236e8d7572..16249f260997 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterChangedParameter.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterChangedParameter.java @@ -1,12 +1,13 @@ // "Transform body to single exit-point form" "true" class Test { String test2(List list, String foo, String bar) { - String result = foo; + String result = null; boolean finished = false; for (String s : list) { for (int i = 0; i < 10; i++) { bar = s; if (s.length() == i) { + result = foo; finished = true; break; } diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterIntIfs.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterIntIfs.java index f457c1fd86ab..5b154366c86b 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterIntIfs.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterIntIfs.java @@ -1,9 +1,11 @@ // "Transform body to single exit-point form" "true" class Test { int test(String s) { - int result = 2; + int result = 1; if (s == null) { - if (!(Math.random() > 0.5)) { + if (Math.random() > 0.5) { + result = 2; + } else { result = 4; } } else { @@ -11,7 +13,6 @@ class Test { result = 3; } else { System.out.println(s); - result = 1; } } return result; diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterMostPopularReturn.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterMostPopularReturn.java new file mode 100644 index 000000000000..a09af984a3f0 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterMostPopularReturn.java @@ -0,0 +1,15 @@ +// "Transform body to single exit-point form" "true" +import java.util.Collections; + +class Test { + List test(int x) { + List result = Collections.emptyList(); + if (x != 0) { + int rem = x % 3; + if (rem != 1) { + result = Collections.singletonList("foo"); + } + } + return result; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterSwitch.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterSwitch.java index b047555c1a34..73a56c766e41 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterSwitch.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterSwitch.java @@ -1,9 +1,10 @@ // "Transform body to single exit-point form" "true" class Test { String test(int x) { - String result = "foo"; + String result; switch (x) { case 1: + result = "foo"; break; case 2: result = "bar"; diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterSynchronized.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterSynchronized.java index 061d8944ce8b..663f0f98f668 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterSynchronized.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterSynchronized.java @@ -1,9 +1,11 @@ // "Transform body to single exit-point form" "true" class Test { String test(int x) { - String result = "foo"; + String result; synchronized (this) { - if (x != 0) { + if (x == 0) { + result = "foo"; + } else { if (x == 1) { result = "bar"; } else { diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch.java index 8b838bcbeaf1..1e131d79e280 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch.java @@ -1,10 +1,11 @@ // "Transform body to single exit-point form" "true" class Test { int test(String s) { - int result = -1; + int result; try { result = Integer.parseInt(s); } catch (NumberFormatException ex) { + result = -1; } return result; } diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch2.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch2.java index a53a237788bd..ee690a5e0059 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch2.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch2.java @@ -1,7 +1,7 @@ // "Transform body to single exit-point form" "true" class Test { int test(String s) { - int res = -1; + int res = -2; boolean finished = false; try { res = Integer.parseInt(s); @@ -9,12 +9,12 @@ class Test { } catch (NumberFormatException ex) { boolean result = s.isEmpty(); if (result) { + res = -1; finished = true; } } if (!finished) { System.out.println("oops"); - res = -2; } return res; } diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch3.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch3.java new file mode 100644 index 000000000000..7c333a1be567 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/afterTryCatch3.java @@ -0,0 +1,18 @@ +// "Transform body to single exit-point form" "true" +class Test { + int test(String s) { + int res = -2; + try { + res = Math.abs(Integer.parseInt(s)); + } catch (NumberFormatException ex) { + boolean result = s.isEmpty(); + if (result) { + res = -1; + } + } + if (res == -2) { + System.out.println("oops"); + } + return res; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/beforeMostPopularReturn.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/beforeMostPopularReturn.java new file mode 100644 index 000000000000..35fc66a4d137 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/beforeMostPopularReturn.java @@ -0,0 +1,11 @@ +// "Transform body to single exit-point form" "true" +import java.util.Collections; + +class Test { + List test(int x) { + if (x == 0) return Collections.emptyList(); + int rem = x % 3; + if (rem == 1) return Collections.emptyList(); + return Collections.singletonList("foo"); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/beforeTryCatch3.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/beforeTryCatch3.java new file mode 100644 index 000000000000..ebb0c228fc83 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/convertToSingleReturn/beforeTryCatch3.java @@ -0,0 +1,14 @@ +// "Transform body to single exit-point form" "true" +class Test { + int test(String s) { + try { + return Math.abs(Integer.parseInt(s)); + } + catch(NumberFormatException ex) { + boolean result = s.isEmpty(); + if (result) return -1; + } + System.out.println("oops"); + return -2; + } +} \ No newline at end of file