diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/newImpl/ExtractMethodAnalyzer.kt b/java/java-impl/src/com/intellij/refactoring/extractMethod/newImpl/ExtractMethodAnalyzer.kt index 66fef0ea7aae..fac18dc3bd54 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/newImpl/ExtractMethodAnalyzer.kt +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/newImpl/ExtractMethodAnalyzer.kt @@ -15,6 +15,7 @@ import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodHelper.findUs import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodHelper.getExpressionType import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodHelper.guessName import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodHelper.hasExplicitModifier +import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodHelper.haveReferenceToScope import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodHelper.inputParameterOf import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodHelper.normalizedAnchor import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodHelper.uniqueNameOf @@ -39,11 +40,17 @@ fun findExtractOptions(elements: List): ExtractOptions { val expression = elements.singleOrNull() as? PsiExpression + + fun canExtractStatementsFromScope(statements: List, scope: List): Boolean { + return ExtractMethodHelper.areSemanticallySame(statements) && !haveReferenceToScope(statements, scope) + } + //TODO use correct error messages val dataOutput = when { expression != null -> ExpressionOutput(getExpressionType(expression), null, listOf(expression), CodeFragmentAnalyzer.inferNullability(listOf(expression))) variableData is VariableOutput -> when { - ! ExtractMethodHelper.areSemanticallySame(flowOutput.statements) && flowOutput is ConditionalFlow -> throw PrepareFailedException("Out var and different flow statements", flowOutput.statements.first()) + flowOutput is ConditionalFlow && ! canExtractStatementsFromScope(flowOutput.statements, elements) + -> throw PrepareFailedException("Out var and different flow statements", flowOutput.statements.first()) variableData.nullability != Nullability.NOT_NULL && flowOutput is ConditionalFlow -> throw PrepareFailedException("Nullable out var and branching", variableData.variable) flowOutput is ConditionalFlow -> variableData.copy(nullability = Nullability.NULLABLE) else -> variableData diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/newImpl/ExtractMethodHelper.kt b/java/java-impl/src/com/intellij/refactoring/extractMethod/newImpl/ExtractMethodHelper.kt index a162ed9c0a33..7ed6e87e2eb2 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/newImpl/ExtractMethodHelper.kt +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/newImpl/ExtractMethodHelper.kt @@ -180,12 +180,14 @@ object ExtractMethodHelper { if (statements.isEmpty()) return true if (! areSame(statements)) return false val returnExpressions = statements.mapNotNull { statement -> (statement as? PsiReturnStatement)?.returnValue } - return returnExpressions.none { expression -> hasReference(expression) } - /* TODO it's also possible to extract single return expression with external references */ + return returnExpressions.all { expression -> PsiUtil.isConstantExpression(expression) || expression.type == PsiType.NULL } } - private fun hasReference(element: PsiElement): Boolean { - return PsiTreeUtil.findChildOfType(element, PsiJavaCodeReferenceElement::class.java, false) != null + fun haveReferenceToScope(elements: List, scope: List): Boolean { + val scopeRange = TextRange(scope.first().textRange.startOffset, scope.last().textRange.endOffset) + return elements.asSequence() + .flatMap { PsiTreeUtil.findChildrenOfAnyType(it, false, PsiJavaCodeReferenceElement::class.java).asSequence() } + .mapNotNull { reference -> reference.resolve() } + .any{ referencedElement -> referencedElement.textRange in scopeRange } } - } diff --git a/java/java-tests/testData/refactoring/extractMethodNew/DontExtractCustomFinalObjects.java b/java/java-tests/testData/refactoring/extractMethodNew/DontExtractCustomFinalObjects.java new file mode 100644 index 000000000000..65b6f27f8f54 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethodNew/DontExtractCustomFinalObjects.java @@ -0,0 +1,12 @@ +import java.io.InputStream; + +class Test { + InputStream test(boolean condition){ + final InputStream stream = System.in; + int x = 42; + if (condition) return stream; + if (!condition) return stream; + System.out.println(x); + return null; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodNew/DontExtractLocalConstant.java b/java/java-tests/testData/refactoring/extractMethodNew/DontExtractLocalConstant.java new file mode 100644 index 000000000000..b3bd3078e35f --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethodNew/DontExtractLocalConstant.java @@ -0,0 +1,11 @@ +class Test { + final String f1 = "field"; + String test(boolean condition){ + int x = 42; + final String f2 = "variable"; + if (condition) return f1 + f2 + "literal"; + if (!condition) return f1+f2+"literal"; + System.out.println(x); + return "default"; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodNew/ExtractConstantExpressions.java b/java/java-tests/testData/refactoring/extractMethodNew/ExtractConstantExpressions.java new file mode 100644 index 000000000000..7887478881f5 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethodNew/ExtractConstantExpressions.java @@ -0,0 +1,11 @@ +class Test { + final String f1 = "field"; + String test(boolean condition){ + final String f2 = "variable"; + int x = 42; + if (condition) return f1 + f2 + "literal"; + if (!condition) return f1+f2+"literal"; + System.out.println(x); + return "default"; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodNew/ExtractConstantExpressions_after.java b/java/java-tests/testData/refactoring/extractMethodNew/ExtractConstantExpressions_after.java new file mode 100644 index 000000000000..fc60f776e3e3 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethodNew/ExtractConstantExpressions_after.java @@ -0,0 +1,20 @@ +import org.jetbrains.annotations.Nullable; + +class Test { + final String f1 = "field"; + String test(boolean condition){ + final String f2 = "variable"; + Integer x = newMethod(condition, f2); + if (x == null) return f1 + f2 + "literal"; + System.out.println(x); + return "default"; + } + + @Nullable + private Integer newMethod(boolean condition, String f2) { + int x = 42; + if (condition) return null; + if (!condition) return null; + return x; + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodNewTest.java b/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodNewTest.java index 0149bc19b363..42de3bb98fb8 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodNewTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodNewTest.java @@ -735,6 +735,26 @@ public class ExtractMethodNewTest extends LightJavaCodeInsightTestCase { } } + public void testExtractConstantExpressions() throws Exception { + doTest(); + } + + public void testDontExtractLocalConstant() throws Exception { + try { + doTest(); + fail("Should fail if expression is linked to the scope"); + } catch (PrepareFailedException e){ + } + } + + public void testDontExtractCustomFinalObjects() throws Exception { + try { + doTest(); + fail("Should fail if expression contains mutable object"); + } catch (PrepareFailedException e){ + } + } + public void testReturnStatementFolding() throws Exception { doTest(); }