From 081caadec13be120a11f09e86ed33bbf17ff640e Mon Sep 17 00:00:00 2001 From: Mikhail Glukhikh Date: Wed, 22 Mar 2017 22:24:38 +0300 Subject: [PATCH] Fix code inlining for expression body with multiple occurrences So #KT-17022 Fixed --- .../kotlin/idea/codeInliner/CodeInliner.kt | 9 +--- .../codeInliner/UsageReplacementStrategy.kt | 42 ++++++++++++++++--- .../kotlin/idea/codeInliner/introduceValue.kt | 12 ++++-- .../expressionBody/MultipleInComposition.kt | 4 ++ .../MultipleInComposition.kt.after | 4 ++ .../MultipleInExpression.kt.after | 6 +-- .../inline/InlineTestGenerated.java | 6 +++ 7 files changed, 64 insertions(+), 19 deletions(-) create mode 100644 idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt create mode 100644 idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt.after diff --git a/idea/src/org/jetbrains/kotlin/idea/codeInliner/CodeInliner.kt b/idea/src/org/jetbrains/kotlin/idea/codeInliner/CodeInliner.kt index f5c919b6737..0311d076c0b 100644 --- a/idea/src/org/jetbrains/kotlin/idea/codeInliner/CodeInliner.kt +++ b/idea/src/org/jetbrains/kotlin/idea/codeInliner/CodeInliner.kt @@ -29,7 +29,6 @@ import org.jetbrains.kotlin.idea.core.* import org.jetbrains.kotlin.idea.intentions.RemoveExplicitTypeArgumentsIntention import org.jetbrains.kotlin.idea.util.* import org.jetbrains.kotlin.lexer.KtTokens -import org.jetbrains.kotlin.name.Name import org.jetbrains.kotlin.psi.* import org.jetbrains.kotlin.psi.psiUtil.* import org.jetbrains.kotlin.resolve.BindingContext @@ -141,14 +140,10 @@ class CodeInliner( private fun renameDuplicates(declarations: List, lexicalScope: LexicalScope) { - fun String.hasConflicts() = lexicalScope.getAllAccessibleVariables(Name.identifier(this)).any { - !it.isExtension && it.isVisible(lexicalScope.ownerDescriptor) - } - - val validator = CollectingNameValidator { !it.hasConflicts() } + val validator = CollectingNameValidator { !it.nameHasConflictsInScope(lexicalScope) } for (declaration in declarations) { val oldName = declaration.name - if (oldName != null && oldName.hasConflicts()) { + if (oldName != null && oldName.nameHasConflictsInScope(lexicalScope)) { val newName = KotlinNameSuggester.suggestNameByName(oldName, validator) val renameProcessor = RenameProcessor(project, declaration, newName, false, false) renameProcessor.run() diff --git a/idea/src/org/jetbrains/kotlin/idea/codeInliner/UsageReplacementStrategy.kt b/idea/src/org/jetbrains/kotlin/idea/codeInliner/UsageReplacementStrategy.kt index 68384332a02..f2b4278a2a4 100644 --- a/idea/src/org/jetbrains/kotlin/idea/codeInliner/UsageReplacementStrategy.kt +++ b/idea/src/org/jetbrains/kotlin/idea/codeInliner/UsageReplacementStrategy.kt @@ -26,16 +26,21 @@ import com.intellij.psi.PsiElement import com.intellij.psi.search.GlobalSearchScope import com.intellij.psi.search.searches.ReferencesSearch import com.intellij.ui.GuiUtils -import com.intellij.util.ui.UIUtil +import org.jetbrains.kotlin.idea.caches.resolve.analyze import org.jetbrains.kotlin.idea.core.targetDescriptors import org.jetbrains.kotlin.idea.references.KtSimpleNameReference +import org.jetbrains.kotlin.idea.search.usagesSearch.descriptor import org.jetbrains.kotlin.idea.stubindex.KotlinSourceFilterScope import org.jetbrains.kotlin.idea.util.application.executeWriteCommand import org.jetbrains.kotlin.idea.util.application.runReadAction import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.psi.KtImportDirective +import org.jetbrains.kotlin.psi.KtNamedDeclaration import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.jetbrains.kotlin.psi.psiUtil.forEachDescendantOfType import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType +import org.jetbrains.kotlin.resolve.BindingContext +import org.jetbrains.kotlin.resolve.lazy.BodyResolveMode interface UsageReplacementStrategy { fun createReplacer(usage: KtSimpleNameExpression): (() -> KtElement?)? @@ -59,13 +64,14 @@ fun UsageReplacementStrategy.replaceUsagesInWholeProject( .filterIsInstance() .map { ref -> ref.expression } } - this@replaceUsagesInWholeProject.replaceUsages(usages, project, commandName, postAction) + this@replaceUsagesInWholeProject.replaceUsages(usages, targetPsiElement, project, commandName, postAction) } }) } private fun UsageReplacementStrategy.replaceUsages( usages: Collection, + targetPsiElement: PsiElement, project: Project, commandName: String, postAction: () -> Unit @@ -74,10 +80,16 @@ private fun UsageReplacementStrategy.replaceUsages( project.executeWriteCommand(commandName) { // we should delete imports later to not affect other usages val importsToDelete = arrayListOf() + val replacements = mutableListOf() - for (usage in usages) { + var invalidUsagesFound = false + // NB: reversed order is better in case of composition like sqr(sqr(x)) + for (usage in usages.reversed()) { try { - if (!usage.isValid) continue // TODO: nested calls + if (!usage.isValid) { + invalidUsagesFound = true + continue + } //TODO: keep the import if we don't know how to replace some of the usages val importDirective = usage.getStrictParentOfType() @@ -88,13 +100,33 @@ private fun UsageReplacementStrategy.replaceUsages( continue } - createReplacer(usage)?.invoke() + val replacement = createReplacer(usage)?.invoke() + if (replacement != null) { + replacements += replacement + } } catch (e: Throwable) { LOG.error(e) } } + if (invalidUsagesFound && targetPsiElement is KtNamedDeclaration) { + val name = targetPsiElement.name + val targetDescriptor = targetPsiElement.descriptor + if (name != null && targetDescriptor != null) { + for (replacement in replacements) { + replacement.forEachDescendantOfType { usage -> + if (usage.isValid && usage.getReferencedName() == name) { + val context = usage.analyze(BodyResolveMode.PARTIAL) + if (targetDescriptor == context[BindingContext.REFERENCE_TARGET, usage]) { + createReplacer(usage)?.invoke() + } + } + } + } + } + } + importsToDelete.forEach { it.delete() } postAction() diff --git a/idea/src/org/jetbrains/kotlin/idea/codeInliner/introduceValue.kt b/idea/src/org/jetbrains/kotlin/idea/codeInliner/introduceValue.kt index 37f3e4ae854..553f865f5c1 100644 --- a/idea/src/org/jetbrains/kotlin/idea/codeInliner/introduceValue.kt +++ b/idea/src/org/jetbrains/kotlin/idea/codeInliner/introduceValue.kt @@ -20,7 +20,9 @@ import org.jetbrains.kotlin.idea.analysis.computeTypeInContext import org.jetbrains.kotlin.idea.caches.resolve.analyze import org.jetbrains.kotlin.idea.caches.resolve.getResolutionFacade import org.jetbrains.kotlin.idea.core.KotlinNameSuggester +import org.jetbrains.kotlin.idea.core.isVisible import org.jetbrains.kotlin.idea.core.setType +import org.jetbrains.kotlin.idea.util.getAllAccessibleVariables import org.jetbrains.kotlin.idea.util.getResolutionScope import org.jetbrains.kotlin.name.Name import org.jetbrains.kotlin.psi.* @@ -30,9 +32,9 @@ import org.jetbrains.kotlin.psi.psiUtil.isAncestor import org.jetbrains.kotlin.renderer.render import org.jetbrains.kotlin.resolve.BindingContext import org.jetbrains.kotlin.resolve.bindingContextUtil.getDataFlowInfoBefore +import org.jetbrains.kotlin.resolve.descriptorUtil.isExtension import org.jetbrains.kotlin.resolve.lazy.BodyResolveMode import org.jetbrains.kotlin.resolve.scopes.LexicalScope -import org.jetbrains.kotlin.resolve.scopes.utils.findLocalVariable import org.jetbrains.kotlin.types.ErrorUtils import org.jetbrains.kotlin.types.KotlinType @@ -85,9 +87,7 @@ internal fun MutableCodeToInline.introduceValue( if (usages.isNotEmpty()) { val resolutionScope = expressionToBeReplaced.getResolutionScope(bindingContext, expressionToBeReplaced.getResolutionFacade()) - val name = suggestName { name -> - resolutionScope.findLocalVariable(Name.identifier(name)) == null && !isNameUsed(name) - } + val name = suggestName { name -> !name.nameHasConflictsInScope(resolutionScope) && !isNameUsed(name) } val declaration = psiFactory.createDeclarationByPattern("val $0 = $1", name, value) statementsBefore.add(0, declaration) @@ -134,6 +134,10 @@ internal fun MutableCodeToInline.introduceValue( } } +fun String.nameHasConflictsInScope(lexicalScope: LexicalScope) = lexicalScope.getAllAccessibleVariables(Name.identifier(this)).any { + !it.isExtension && it.isVisible(lexicalScope.ownerDescriptor) +} + private fun variableNeedsExplicitType( initializer: KtExpression, initializerType: KotlinType, diff --git a/idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt b/idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt new file mode 100644 index 00000000000..c14a9ebf984 --- /dev/null +++ b/idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt @@ -0,0 +1,4 @@ + +fun sqr(x: Double) = x * x + +fun bisqr(x: Double) = sqr(sqr(x)) diff --git a/idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt.after b/idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt.after new file mode 100644 index 00000000000..c147f98ec3f --- /dev/null +++ b/idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt.after @@ -0,0 +1,4 @@ +fun bisqr(x: Double): Double { + val x1 = x * x + return x1 * x1 +} diff --git a/idea/testData/refactoring/inline/function/expressionBody/MultipleInExpression.kt.after b/idea/testData/refactoring/inline/function/expressionBody/MultipleInExpression.kt.after index 39fb8652d28..304de55b65a 100644 --- a/idea/testData/refactoring/inline/function/expressionBody/MultipleInExpression.kt.after +++ b/idea/testData/refactoring/inline/function/expressionBody/MultipleInExpression.kt.after @@ -1,8 +1,8 @@ class Point(val x: Double, val y: Double) { // After sqr inlining, only first usage of sqr is replaced fun distance(other: Point): Double { - val x1 = x - other.x - // See KT-17022 - return Math.sqrt(x1 * x1 + sqr(y - other.y)) + val x1 = y - other.y + val x2 = x - other.x + return Math.sqrt(x2 * x2 + x1 * x1) } } \ No newline at end of file diff --git a/idea/tests/org/jetbrains/kotlin/idea/refactoring/inline/InlineTestGenerated.java b/idea/tests/org/jetbrains/kotlin/idea/refactoring/inline/InlineTestGenerated.java index 14d90928ba6..17e4cc8236d 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/refactoring/inline/InlineTestGenerated.java +++ b/idea/tests/org/jetbrains/kotlin/idea/refactoring/inline/InlineTestGenerated.java @@ -130,6 +130,12 @@ public class InlineTestGenerated extends AbstractInlineTest { doTest(fileName); } + @TestMetadata("MultipleInComposition.kt") + public void testMultipleInComposition() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("idea/testData/refactoring/inline/function/expressionBody/MultipleInComposition.kt"); + doTest(fileName); + } + @TestMetadata("MultipleInExpression.kt") public void testMultipleInExpression() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("idea/testData/refactoring/inline/function/expressionBody/MultipleInExpression.kt");