Extract Function: Implement validation

2014-04-21 18:23:28 +04:00
parent e052a64767
commit 0e9676173b
17 changed files with 230 additions and 32 deletions
@@ -36,4 +36,5 @@ refactoring.move.non.kotlin.file=Target must be a Kotlin file

 package.private.0.will.no.longer.be.accessible.from.1=Package-private {0} will no longer be accessible from {1}
 0.uses.package.private.1={0} uses package-private {1}
-0.will.no.longer.be.accessible.after.extraction={0} will no longer be accessible after extraction
+0.will.no.longer.be.accessible.after.extraction={0} will no longer be accessible after extraction
+0.will.become.invisible.after.extraction={0} will become invisible after extraction
@@ -106,10 +106,6 @@ class ExtractionData(
    }

    fun getBrokenReferencesInfo(body: JetBlockExpression): List<ResolvedReferenceInfo> {
-        fun compareDescriptors(d1: DeclarationDescriptor?, d2: DeclarationDescriptor?): Boolean {
-            return d1 == d2 || (d1 != null && d2 != null && DescriptorRenderer.FQNAMES_IN_TYPES.render(d1) == DescriptorRenderer.FQNAMES_IN_TYPES.render(d2))
-        }
-
        val startOffset = body.getStatements().first!!.getTextRange()!!.getStartOffset()

        val referencesInfo = ArrayList<ResolvedReferenceInfo>()
@@ -137,3 +133,9 @@ class ExtractionData(
    fun getInferredResultType(): JetType? =
            getExpressions().last?.let { AnalyzerFacadeWithCache.getContextForElement(it)[BindingContext.EXPRESSION_TYPE, it] }
 }
+
+private fun compareDescriptors(d1: DeclarationDescriptor?, d2: DeclarationDescriptor?): Boolean {
+    return d1 == d2 ||
+            (d1 != null && d2 != null &&
+                    DescriptorRenderer.FQNAMES_IN_TYPES.render(d1) == DescriptorRenderer.FQNAMES_IN_TYPES.render(d2))
+}
@@ -27,6 +27,8 @@ import org.jetbrains.jet.lang.psi.JetThisExpression
 import org.jetbrains.jet.plugin.references.JetSimpleNameReference
 import org.jetbrains.jet.lang.resolve.name.FqName
 import org.jetbrains.jet.plugin.references.JetSimpleNameReference.ShorteningMode
+import org.jetbrains.jet.lang.psi.psiUtil.replaced
+import org.jetbrains.jet.lang.psi.JetQualifiedExpression

 data class Parameter(
        val argumentText: String,
@@ -38,7 +40,7 @@ data class Parameter(
    val nameForRef: String get() = mirrorVarName ?: name
 }

-trait Replacement: Function1<JetElement, Unit>
+trait Replacement: Function1<JetElement, JetElement>

 trait ParameterReplacement : Replacement {
    val parameter: Parameter
@@ -49,9 +51,9 @@ class RenameReplacement(override val parameter: Parameter): ParameterReplacement
    override fun copy(parameter: Parameter) = RenameReplacement(parameter)

    [suppress("PARAMETER_NAME_CHANGED_ON_OVERRIDE")]
-    override fun invoke(e: JetElement) {
+    override fun invoke(e: JetElement): JetElement {
        val thisExpr = e.getParent() as? JetThisExpression
-        (thisExpr ?: e).replace(JetPsiFactory.createSimpleName(e.getProject(), parameter.nameForRef))
+        return (thisExpr ?: e).replaced(JetPsiFactory.createSimpleName(e.getProject(), parameter.nameForRef))
    }
 }

@@ -59,16 +61,21 @@ class AddPrefixReplacement(override val parameter: Parameter): ParameterReplacem
    override fun copy(parameter: Parameter) = AddPrefixReplacement(parameter)

    [suppress("PARAMETER_NAME_CHANGED_ON_OVERRIDE")]
-    override fun invoke(e: JetElement) {
+    override fun invoke(e: JetElement): JetElement {
        val selector = (e.getParent() as? JetCallExpression) ?: e
-        selector.replace(JetPsiFactory.createExpression(e.getProject(), "${parameter.nameForRef}.${selector.getText()}"))
+        val newExpr = selector.replace(
+                JetPsiFactory.createExpression(e.getProject(), "${parameter.nameForRef}.${selector.getText()}")
+        ) as JetQualifiedExpression
+
+        return with(newExpr.getSelectorExpression()!!) { if (this is JetCallExpression) getCalleeExpression()!! else this }
    }
 }

 class FqNameReplacement(val fqName: FqName): Replacement {
    [suppress("PARAMETER_NAME_CHANGED_ON_OVERRIDE")]
-    override fun invoke(e: JetElement) {
-        (e.getReference() as? JetSimpleNameReference)?.bindToFqName(fqName, ShorteningMode.NO_SHORTENING)
+    override fun invoke(e: JetElement): JetElement {
+        val newExpr = (e.getReference() as? JetSimpleNameReference)?.bindToFqName(fqName, ShorteningMode.NO_SHORTENING) as JetElement
+        return if (newExpr is JetQualifiedExpression) newExpr.getSelectorExpression()!! else newExpr
    }
 }

@@ -82,6 +82,16 @@ import org.jetbrains.jet.lang.psi.JetTreeVisitorVoid
 import org.jetbrains.jet.lang.types.checker.JetTypeChecker
 import org.jetbrains.jet.lang.resolve.name.FqName
 import org.jetbrains.jet.lang.resolve.DescriptorUtils
+import com.intellij.refactoring.util.RefactoringUIUtil
+import org.jetbrains.jet.plugin.references.JetReference
+import org.jetbrains.jet.lang.psi.psiUtil.replaced
+import org.jetbrains.jet.lang.psi.psiUtil.isAncestor
+import java.util.Collections
+import com.intellij.psi.PsiNamedElement
+import org.jetbrains.jet.lang.diagnostics.Severity
+import org.jetbrains.jet.lang.diagnostics.Errors
+import org.jetbrains.jet.lang.descriptors.impl.LocalVariableDescriptor
+import org.jetbrains.jet.lang.types.ErrorUtils

 private val DEFAULT_FUNCTION_NAME = "myFun"
 private val DEFAULT_RETURN_TYPE = KotlinBuiltIns.getInstance().getUnitType()
@@ -337,9 +347,6 @@ private fun ExtractionData.inferParametersInfo(
                replacementMap[refInfo.offsetInBody] =
                if (hasThisReceiver && extractThis) AddPrefixReplacement(parameter) else RenameReplacement(parameter)
            }
-            else if (originalDeclaration is JetProperty) {
-                return MaybeError(JetRefactoringBundle.message("cannot.extract.non.local.declaration.ref")!!)
-            }
        }
    }

@@ -457,8 +464,57 @@ fun ExtractionData.performAnalysis(): Maybe<ExtractionDescriptor, String> {
 }

 fun ExtractionDescriptor.validate(): ExtractionDescriptorWithConflicts {
-    // todo: add name conflict checks (parameters, function, etc.)
-    return ExtractionDescriptorWithConflicts(this, MultiMap<PsiElement, String>())
+    val conflicts = MultiMap<PsiElement, String>()
+
+    val nameByOffset = HashMap<Int, JetElement>()
+    val function = generateFunction(true, nameByOffset)
+
+    val bindingContext = AnalyzerFacadeWithCache.getContextForElement(function.getBodyExpression()!!)
+
+    for ((originalOffset, resolveResult) in extractionData.refOffsetToDeclaration) {
+        if (resolveResult.declaration in extractionData.originalElements) continue
+
+        val currentRefExpr = nameByOffset[originalOffset] as JetSimpleNameExpression?
+        if (currentRefExpr == null) continue
+
+        if (currentRefExpr.getParent() is JetThisExpression) continue
+
+        val diagnostics = bindingContext.getDiagnostics().forElement(currentRefExpr)
+
+        val currentDescriptor = bindingContext[BindingContext.REFERENCE_TARGET, currentRefExpr]
+        val currentTarget = currentDescriptor?.let {
+            DescriptorToDeclarationUtil.getDeclaration(extractionData.project, it, bindingContext)
+        } as? PsiNamedElement
+        if (currentTarget is JetParameter && currentTarget.getParent() == function.getValueParameterList()) continue
+        if (currentDescriptor is LocalVariableDescriptor
+                && parameters.any { it.mirrorVarName == currentDescriptor.getName().asString() }) continue
+
+        if (diagnostics.any { it.getFactory() == Errors.UNRESOLVED_REFERENCE }
+                || (currentDescriptor != null
+                        && !ErrorUtils.isError(currentDescriptor)
+                        && !compareDescriptors(currentDescriptor, resolveResult.descriptor))) {
+            conflicts.putValue(
+                    currentRefExpr,
+                    JetRefactoringBundle.message(
+                            "0.will.no.longer.be.accessible.after.extraction",
+                            RefactoringUIUtil.getDescription(resolveResult.declaration, true)
+                    )
+            )
+            continue
+        }
+
+        diagnostics.firstOrNull { it.getFactory() == Errors.INVISIBLE_MEMBER }?.let {
+            conflicts.putValue(
+                    currentRefExpr,
+                    JetRefactoringBundle.message(
+                            "0.will.become.invisible.after.extraction",
+                            RefactoringUIUtil.getDescription(resolveResult.declaration, true)
+                    )
+            )
+        }
+    }
+
+    return ExtractionDescriptorWithConflicts(this, conflicts)
 }

 fun ExtractionDescriptor.getFunctionText(
@@ -488,7 +544,29 @@ fun ExtractionDescriptor.getFunctionText(
    }
 }

-fun ExtractionDescriptor.generateFunction(inTempFile: Boolean = false): JetNamedFunction {
+fun createNameCounterpartMap(from: JetElement, to: JetElement): Map<JetSimpleNameExpression, JetSimpleNameExpression> {
+    val map = HashMap<JetSimpleNameExpression, JetSimpleNameExpression>()
+
+    val fromOffset = from.getTextRange()!!.getStartOffset()
+    from.accept(
+            object: JetTreeVisitorVoid() {
+                override fun visitSimpleNameExpression(expression: JetSimpleNameExpression) {
+                    val offset = expression.getTextRange()!!.getStartOffset() - fromOffset
+                    val newExpression = to.findElementAt(offset)?.getParentByType(javaClass<JetSimpleNameExpression>())
+                    assert(newExpression!= null, "Couldn't find expression at $offset in '${to.getText()}'")
+
+                    map[expression] = newExpression!!
+                }
+            }
+    )
+
+    return map
+}
+
+fun ExtractionDescriptor.generateFunction(
+        inTempFile: Boolean = false,
+        nameByOffset: MutableMap<Int, JetElement> = HashMap()
+): JetNamedFunction {
    val project = extractionData.project

    fun createFunction(): JetNamedFunction {
@@ -509,7 +587,8 @@ fun ExtractionDescriptor.generateFunction(inTempFile: Boolean = false): JetNamed
    fun adjustFunctionBody(function: JetNamedFunction) {
        val body = function.getBodyExpression() as JetBlockExpression

-        val exprReplacementMap = HashMap<JetElement, (JetElement) -> Unit>()
+        val exprReplacementMap = HashMap<JetElement, (JetElement) -> JetElement>()
+        val originalOffsetByExpr = HashMap<JetElement, Int>()

        val range = body.getStatements().first?.getTextRange()
        if (range == null) return
@@ -517,31 +596,46 @@ fun ExtractionDescriptor.generateFunction(inTempFile: Boolean = false): JetNamed
        val bodyOffset = range.getStartOffset()
        val file = body.getContainingFile()!!

-        for ((offsetInBody, replacement) in replacementMap) {
-            if (replacement is ParameterReplacement && replacement.parameter == receiverParameter) continue
-
+        for ((offsetInBody, resolveResult) in extractionData.refOffsetToDeclaration) {
            val expr = file.findElementAt(bodyOffset + offsetInBody)?.getParentByType(javaClass<JetSimpleNameExpression>())
            assert(expr != null, "Couldn't find expression at $offsetInBody in '${body.getText()}'")

-            exprReplacementMap[expr!!] = replacement
+            originalOffsetByExpr[expr!!] = offsetInBody
+
+            replacementMap[offsetInBody]?.let { replacement ->
+                if (replacement !is ParameterReplacement || replacement.parameter != receiverParameter) {
+                    exprReplacementMap[expr] = replacement
+                }
+            }
        }

+        val replacingReturn: JetExpression?
+        val expressionsToReplaceWithReturn: List<JetElement>
        if (controlFlow is JumpBasedControlFlow) {
-            val replacementExpr = JetPsiFactory.createExpression(
-                    project,
-                    if (controlFlow is ConditionalJump) "return true" else "return"
-            )
-            for (jumpElement in controlFlow.elementsToReplace) {
+            replacingReturn = JetPsiFactory.createExpression(project, if (controlFlow is ConditionalJump) "return true" else "return")
+            expressionsToReplaceWithReturn = controlFlow.elementsToReplace.map { jumpElement ->
                val offsetInBody = jumpElement.getTextRange()!!.getStartOffset() - extractionData.originalStartOffset!!
                val expr = file.findElementAt(bodyOffset + offsetInBody)?.getParentByType(jumpElement.javaClass)
                assert(expr != null, "Couldn't find expression at $offsetInBody in '${body.getText()}'")

-                exprReplacementMap[expr!!] = { it.replace(replacementExpr) }
+                expr!!
+            }
+        }
+        else {
+            replacingReturn = null
+            expressionsToReplaceWithReturn = Collections.emptyList()
+        }
+
+        if (replacingReturn != null) {
+            for (expr in expressionsToReplaceWithReturn) {
+                expr.replace(replacingReturn)
            }
        }

-        for ((expr, replacement) in exprReplacementMap) {
-            replacement(expr)
+        for ((expr, originalOffset) in originalOffsetByExpr) {
+            if (expr.isValid()) {
+                nameByOffset.put(originalOffset, exprReplacementMap[expr]?.invoke(expr) ?: expr)
+            }
        }

        for (param in parameters) {
@@ -558,7 +652,11 @@ fun ExtractionDescriptor.generateFunction(inTempFile: Boolean = false): JetNamed
                body.appendElement(JetPsiFactory.createReturn(project, "false"))

            is ExpressionEvaluation ->
-                body.getStatements().last?.let { it.replace(JetPsiFactory.createReturn(project, it.getText()!!)) }
+                body.getStatements().last?.let {
+                    val newExpr = it.replaced(JetPsiFactory.createReturn(project, it.getText()!!)).getReturnedExpression()!!
+                    val counterpartMap = createNameCounterpartMap(it, newExpr)
+                    nameByOffset.entrySet().forEach { it.setValue(counterpartMap[it.getValue()]!!) }
+                }
        }
    }

@@ -0,0 +1,8 @@
+// NEXT_SIBLING:
+fun foo(a: Int): Int {
+    class A {
+        fun bar(): Int = a + 10
+    }
+
+    return <selection>A().bar()</selection>
+}
@@ -0,0 +1 @@
+class foo.A will no longer be accessible after extraction
@@ -0,0 +1,8 @@
+// NEXT_SIBLING:
+fun foo(a: Int): Int {
+    class A {
+        val bar: Int = a + 10
+    }
+
+    return <selection>A().bar</selection>
+}
@@ -0,0 +1 @@
+class foo.A will no longer be accessible after extraction
@@ -0,0 +1,6 @@
+// NEXT_SIBLING:
+fun foo(a: Int): Int {
+    fun bar(): Int = a + 10
+
+    return <selection>bar()</selection>
+}
@@ -0,0 +1 @@
+function foo.bar will no longer be accessible after extraction
@@ -0,0 +1,8 @@
+// NEXT_SIBLING:
+fun bar(): Int = 100
+
+fun foo(a: Int): Int {
+    fun bar(): Int = a + 10
+
+    return <selection>bar()</selection>
+}
@@ -0,0 +1 @@
+function foo.bar will no longer be accessible after extraction
@@ -0,0 +1,8 @@
+// NEXT_SIBLING:
+class A {
+    private val t: Int = 1
+
+    fun foo(): Int {
+        return <selection>t</selection>
+    }
+}
@@ -0,0 +1 @@
+property A.t will become invisible after extraction
@@ -0,0 +1,6 @@
+// NEXT_SIBLING:
+fun foo(a: Int): Int {
+    <selection>println(a)
+    if (a > 0) return a</selection>
+    return -a
+}
@@ -0,0 +1,11 @@
+// NEXT_SIBLING:
+fun b(a: Int): Boolean {
+    println(a)
+    if (a > 0) return true
+    return false
+}
+
+fun foo(a: Int): Int {
+    if (b(a)) return a
+    return -a
+}
@@ -198,6 +198,21 @@ public class JetExtractionTestGenerated extends AbstractJetExtractionTest {
                JetTestUtils.assertAllTestsPresentByMetadata(this.getClass(), "org.jetbrains.jet.generators.tests.TestsPackage", new File("idea/testData/refactoring/extractFunction/basic"), Pattern.compile("^(.+)\\.kt$"), true);
            }
            
+            @TestMetadata("localClassFunctionRef.kt")
+            public void testLocalClassFunctionRef() throws Exception {
+                doExtractFunctionTest("idea/testData/refactoring/extractFunction/basic/localClassFunctionRef.kt");
+            }
+            
+            @TestMetadata("localClassPropertyRef.kt")
+            public void testLocalClassPropertyRef() throws Exception {
+                doExtractFunctionTest("idea/testData/refactoring/extractFunction/basic/localClassPropertyRef.kt");
+            }
+            
+            @TestMetadata("localFunctionRef.kt")
+            public void testLocalFunctionRef() throws Exception {
+                doExtractFunctionTest("idea/testData/refactoring/extractFunction/basic/localFunctionRef.kt");
+            }
+            
            @TestMetadata("malformedExpression.kt")
            public void testMalformedExpression() throws Exception {
                doExtractFunctionTest("idea/testData/refactoring/extractFunction/basic/malformedExpression.kt");
@@ -208,6 +223,21 @@ public class JetExtractionTestGenerated extends AbstractJetExtractionTest {
                doExtractFunctionTest("idea/testData/refactoring/extractFunction/basic/malformedStatements.kt");
            }
            
+            @TestMetadata("misdirectedRef.kt")
+            public void testMisdirectedRef() throws Exception {
+                doExtractFunctionTest("idea/testData/refactoring/extractFunction/basic/misdirectedRef.kt");
+            }
+            
+            @TestMetadata("privateMemberRef.kt")
+            public void testPrivateMemberRef() throws Exception {
+                doExtractFunctionTest("idea/testData/refactoring/extractFunction/basic/privateMemberRef.kt");
+            }
+            
+            @TestMetadata("refInReturn.kt")
+            public void testRefInReturn() throws Exception {
+                doExtractFunctionTest("idea/testData/refactoring/extractFunction/basic/refInReturn.kt");
+            }
+            
        }
        
        @TestMetadata("idea/testData/refactoring/extractFunction/controlFlow")
				`@@ -0,0 +1 @@`
				`class foo.A will no longer be accessible after extraction`
				`@@ -0,0 +1 @@`
				`function foo.bar will no longer be accessible after extraction`
				`@@ -0,0 +1 @@`
				`property A.t will become invisible after extraction`