From a5dc2b5b2abc7429ad13e4793f29425efc111ad9 Mon Sep 17 00:00:00 2001 From: Valentin Kipyatkov Date: Thu, 26 Mar 2020 19:37:06 +0200 Subject: [PATCH] Suggested change signature: do not ask to enter values for optional parameters #KT-37797 Fixed --- ...linSuggestedRefactoringAvailability.kt.201 | 55 ++++++++++- ...KotlinSuggestedRefactoringExecution.kt.201 | 6 +- .../KotlinSuggestedRefactoringUI.kt.201 | 17 ++-- ...uggestedRefactoringAvailabilityTest.kt.201 | 91 +++++++++++++++++++ .../KotlinSuggestedRefactoringTest.kt.201 | 6 +- 5 files changed, 162 insertions(+), 13 deletions(-) diff --git a/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringAvailability.kt.201 b/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringAvailability.kt.201 index 7dd7495aea0..bd9bd7fa77e 100644 --- a/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringAvailability.kt.201 +++ b/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringAvailability.kt.201 @@ -14,6 +14,7 @@ import org.jetbrains.kotlin.idea.caches.resolve.resolveToDescriptorIfAny import org.jetbrains.kotlin.idea.refactoring.isInterfaceClass import org.jetbrains.kotlin.lexer.KtTokens import org.jetbrains.kotlin.psi.KtCallableDeclaration +import org.jetbrains.kotlin.psi.KtParameter import org.jetbrains.kotlin.psi.psiUtil.containingClassOrObject import org.jetbrains.kotlin.psi.psiUtil.hasBody import org.jetbrains.kotlin.renderer.DescriptorRenderer @@ -135,19 +136,67 @@ class KotlinSuggestedRefactoringAvailability(refactoringSupport: SuggestedRefact val oldSignature = state.oldSignature val newSignature = state.newSignature val updateUsagesData = SuggestedChangeSignatureData.create(state, USAGES) + val updateOverridesData = overridesName?.let { updateUsagesData.copy(nameOfStuffToUpdate = it) } + if (newSignature.parameters.size > oldSignature.parameters.size) { + val newParametersAtEndWithDefaults = newSignature.parameters.drop(oldSignature.parameters.size) + .all { oldSignature.parameterById(it.id) == null && it.defaultValue != null } + // special case if added new parameters with default values to the end + // we don't need to update usages if it's the only change + if (newParametersAtEndWithDefaults) { + val truncatedNewSignature = Signature.create( + newSignature.name, + newSignature.type, + newSignature.parameters.take(oldSignature.parameters.size), + newSignature.additionalData + )!! + val refactoringData = detectAvailableRefactoring( + oldSignature, + truncatedNewSignature, + updateUsagesData, + updateOverridesData, + declaration, + declaration.valueParameters.take(oldSignature.parameters.size) + ) + + return when (refactoringData) { + is SuggestedChangeSignatureData -> refactoringData + is SuggestedRenameData -> updateUsagesData + null -> updateOverridesData + } + } + } + + return detectAvailableRefactoring( + oldSignature, + newSignature, + updateUsagesData, + updateOverridesData, + declaration, + declaration.valueParameters + ) + } + + private fun detectAvailableRefactoring( + oldSignature: Signature, + newSignature: Signature, + updateUsagesData: SuggestedChangeSignatureData, + updateOverridesData: SuggestedChangeSignatureData?, + declaration: PsiNamedElement, + parameters: List + ): SuggestedRefactoringData? { if (hasParameterAddedRemovedOrReordered(oldSignature, newSignature)) return updateUsagesData // for non-virtual function we can add or remove receiver for usages but not change its type if ((oldSignature.receiverType == null) != (newSignature.receiverType == null)) return updateUsagesData - val (nameChanges, renameData) = nameChanges(oldSignature, newSignature, declaration, declaration.valueParameters) + val (nameChanges, renameData) = nameChanges(oldSignature, newSignature, declaration, parameters) if (hasTypeChanges(oldSignature, newSignature)) { return if (nameChanges > 0) updateUsagesData else - overridesName?.let { updateUsagesData.copy(nameOfStuffToUpdate = it) } + updateOverridesData } return when { @@ -156,7 +205,7 @@ class KotlinSuggestedRefactoringAvailability(refactoringSupport: SuggestedRefact else -> null } } - + private fun KtCallableDeclaration.overridesName(): String? { return when { hasModifier(KtTokens.ABSTRACT_KEYWORD) -> IMPLEMENTATIONS diff --git a/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringExecution.kt.201 b/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringExecution.kt.201 index 5f6367b51c9..bdc1072996a 100644 --- a/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringExecution.kt.201 +++ b/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringExecution.kt.201 @@ -105,7 +105,11 @@ class KotlinSuggestedRefactoringExecution( defaultValueForParameter = defaultValue, modifierList = modifierList ).apply { - setNewParameterValue(newParameterValues[newParameterValueIndex++]) + // a few last added parameters may be missing in newParameterValues + // because they have default values and we did not offer to enter value for them + if (newParameterValueIndex < newParameterValues.size) { + setNewParameterValue(newParameterValues[newParameterValueIndex++]) + } } } else { KotlinParameterInfo( diff --git a/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringUI.kt.201 b/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringUI.kt.201 index 5c736f3efca..ab7141d025a 100644 --- a/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringUI.kt.201 +++ b/idea/src/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringUI.kt.201 @@ -6,6 +6,7 @@ import com.intellij.refactoring.suggested.SuggestedRefactoringExecution.NewParam import com.intellij.refactoring.suggested.SuggestedRefactoringSupport.Signature import com.intellij.refactoring.suggested.SuggestedRefactoringUI import com.intellij.refactoring.suggested.SignaturePresentationBuilder +import com.intellij.refactoring.suggested.SuggestedRefactoringSupport.Parameter import org.jetbrains.kotlin.psi.KtExpressionCodeFragment import org.jetbrains.kotlin.psi.KtPsiFactory @@ -19,7 +20,7 @@ object KotlinSuggestedRefactoringUI : SuggestedRefactoringUI() { } override fun extractNewParameterData(data: SuggestedChangeSignatureData): List { - val newParameters = mutableListOf() + val result = mutableListOf() val declaration = data.declaration val factory = KtPsiFactory(declaration.project) @@ -27,16 +28,20 @@ object KotlinSuggestedRefactoringUI : SuggestedRefactoringUI() { fun createCodeFragment() = factory.createExpressionCodeFragment("", declaration) if (data.newSignature.receiverType != null && data.oldSignature.receiverType == null) { - newParameters.add(NewParameterData("", createCodeFragment(), false/*TODO*/)) + result.add(NewParameterData("", createCodeFragment(), false/*TODO*/)) } - for (parameter in data.newSignature.parameters) { - if (data.oldSignature.parameterById(parameter.id) == null) { - newParameters.add(NewParameterData(parameter.name, createCodeFragment(), false/*TODO*/)) + fun isNewParameter(parameter: Parameter) = data.oldSignature.parameterById(parameter.id) == null + + val newParameters = data.newSignature.parameters + val dropLastN = newParameters.reversed().count { isNewParameter(it) && it.defaultValue != null } + for (parameter in newParameters.dropLast(dropLastN)) { + if (isNewParameter(parameter)) { + result.add(NewParameterData(parameter.name, createCodeFragment(), false/*TODO*/)) } } - return newParameters + return result } override fun extractValue(fragment: PsiCodeFragment): NewParameterValue.Expression? { diff --git a/idea/tests/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringAvailabilityTest.kt.201 b/idea/tests/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringAvailabilityTest.kt.201 index b360a7330ea..7473e95b834 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringAvailabilityTest.kt.201 +++ b/idea/tests/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringAvailabilityTest.kt.201 @@ -439,6 +439,97 @@ class KotlinSuggestedRefactoringAvailabilityTest : BaseSuggestedRefactoringAvail ) } + fun testAddOptionalParameter() { + doTest( + """ + fun foo(p1: Int) { + } + + fun bar() { + foo(1) + } + """.trimIndent(), + { + myFixture.type(", p2: Int = 2") + }, + expectedAvailability = Availability.Disabled, + ) + } + + fun testAddOptionalParameterWithOverrides() { + doTest( + """ + interface I { + fun foo(p1: Int) + } + + class C : I { + override fun foo(p1: Int) { + } + } + """.trimIndent(), + { + myFixture.type(", p2: Int = 2") + }, + expectedAvailability = Availability.Available(changeSignatureAvailableTooltip("foo", "implementations")), + ) + } + + fun testAddOptionalParameterNotLast() { + doTest( + """ + fun foo(p1: Int) { + } + + fun bar() { + foo(1) + } + """.trimIndent(), + { + myFixture.type("p0: Int = 0, ") + }, + expectedAvailability = Availability.Available(changeSignatureAvailableTooltip("foo", "usages")), + ) + } + + fun testAddOptionalParameterAndRenameParameter() { + doTest( + """ + fun foo(p1: Int) { + } + + fun bar() { + foo(1) + } + """.trimIndent(), + { + replaceTextAtCaret("p1", "p1New") + }, + { + editor.caretModel.moveToOffset(editor.caretModel.offset + "p1New: Int".length) + myFixture.type(", p2: Int = 2") + }, + expectedAvailability = Availability.Available(changeSignatureAvailableTooltip("foo", "usages")), + ) + } + + fun testAddTwoParameters() { + doTest( + """ + fun foo(p1: Int) { + } + + fun bar() { + foo(1) + } + """.trimIndent(), + { + myFixture.type(", p2: Int, p3: Int = 3") + }, + expectedAvailability = Availability.Available(changeSignatureAvailableTooltip("foo", "usages")), + ) + } + private fun addImport(fqName: String) { (file as KtFile).importList!!.add(KtPsiFactory(project).createImportDirective(ImportPath.fromString(fqName))) } diff --git a/idea/tests/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringTest.kt.201 b/idea/tests/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringTest.kt.201 index a0693109115..8aaa1da954b 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringTest.kt.201 +++ b/idea/tests/org/jetbrains/kotlin/idea/refactoring/suggested/KotlinSuggestedRefactoringTest.kt.201 @@ -1345,7 +1345,7 @@ class KotlinSuggestedRefactoringTest : BaseSuggestedRefactoringTest() { ) } - fun testAddParameterWithDefaultValue() { + fun testAddOptionalParameter() { doTestChangeSignature( """ interface I { @@ -1375,7 +1375,7 @@ class KotlinSuggestedRefactoringTest : BaseSuggestedRefactoringTest() { i.foo(1) } """.trimIndent(), - "usages", + "implementations", { myFixture.type(", p2: Int = 10") }, @@ -1414,7 +1414,7 @@ class KotlinSuggestedRefactoringTest : BaseSuggestedRefactoringTest() { ) } - fun testReorderParameterWithDefaultValue() { + fun testReorderOptionalParameter() { doTestChangeSignature( """ interface I {