From 2aea2bfe50630ea686266ba70bd47438ad5e5986 Mon Sep 17 00:00:00 2001 From: Marie Katrine Ekeberg Date: Fri, 22 Apr 2022 18:16:44 +0200 Subject: [PATCH 1/5] Tests for verifying current implemention of abstract method quickfix --- .../kotlin/org/javacs/kt/CodeActionTest.kt | 75 +++++++++++++++++++ .../test/resources/codeactions/samefile.kt | 15 ++++ 2 files changed, 90 insertions(+) create mode 100644 server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt create mode 100644 server/src/test/resources/codeactions/samefile.kt diff --git a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt new file mode 100644 index 000000000..363c935b9 --- /dev/null +++ b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt @@ -0,0 +1,75 @@ +package org.javacs.kt + +import org.eclipse.lsp4j.* +import org.javacs.kt.SingleFileTestFixture +import org.junit.Test +import org.junit.Assert.assertThat +import org.junit.Assert.fail +import org.hamcrest.Matchers.* + +// TODO: naming +class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("codeactions", "samefile.kt") { + + @Test + fun `should find no code actions`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 3, 1, 3, 22, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(0)) + } + + @Test + fun `should find one abstract method to implement`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 7, 1, 7, 14, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(7, 30, 7, 30))) + assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun test(input: String, otherInput: Int) { }")) + } + + @Test + fun `should find several abstract methods to implement`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 15, 1, 15, 21, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(2)) + + val firstFunctionToImplementEdit = textEdit[key]?.get(0) + assertThat(firstFunctionToImplementEdit?.range, equalTo(range(15, 49, 15, 49))) + assertThat(firstFunctionToImplementEdit?.newText, equalTo("\n\n override fun print() { }")) + + val secondFunctionToImplementEdit = textEdit[key]?.get(1) + assertThat(secondFunctionToImplementEdit?.range, equalTo(range(15, 49, 15, 49))) + assertThat(secondFunctionToImplementEdit?.newText, equalTo("\n\n override fun test(input: String, otherInput: Int) { }")) + } +} + + + diff --git a/server/src/test/resources/codeactions/samefile.kt b/server/src/test/resources/codeactions/samefile.kt new file mode 100644 index 000000000..946706364 --- /dev/null +++ b/server/src/test/resources/codeactions/samefile.kt @@ -0,0 +1,15 @@ +package test.kotlin.lsp + +interface MyInterface { + fun test(input: String, otherInput: Int) +} + +class MyClass : MyInterface { +} + + +abstract class CanPrint { + abstract fun print() +} + +class PrintableClass : CanPrint(), MyInterface {} From b89ec14e3b55475e2337101f74a2936dd67c5887 Mon Sep 17 00:00:00 2001 From: Marie Katrine Ekeberg Date: Sun, 24 Apr 2022 15:10:45 +0200 Subject: [PATCH 2/5] WIP. Import abstract method quick fix, external methods as well. External methods include Java standard library, Kotlin standard library and more. Some additional cleanup is needed. --- .../ImplementAbstractFunctionsQuickFix.kt | 84 ++++++++++++++++++- .../kotlin/org/javacs/kt/CodeActionTest.kt | 49 +++++++++++ .../implementabstract_standardlib.kt | 7 ++ 3 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 server/src/test/resources/codeactions/implementabstract_standardlib.kt diff --git a/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt b/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt index cb063951f..651036c1e 100644 --- a/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt +++ b/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt @@ -7,16 +7,24 @@ import org.javacs.kt.index.SymbolIndex import org.javacs.kt.position.offset import org.javacs.kt.position.position import org.javacs.kt.util.toPath +import org.jetbrains.kotlin.descriptors.ClassDescriptor +import org.jetbrains.kotlin.descriptors.FunctionDescriptor +import org.jetbrains.kotlin.descriptors.isInterface +import org.jetbrains.kotlin.descriptors.Modality import org.jetbrains.kotlin.js.resolve.diagnostics.findPsi import org.jetbrains.kotlin.lexer.KtTokens import org.jetbrains.kotlin.psi.KtClass import org.jetbrains.kotlin.psi.KtDeclaration import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtSuperTypeListEntry +import org.jetbrains.kotlin.psi.KtTypeArgumentList import org.jetbrains.kotlin.psi.psiUtil.containingClass import org.jetbrains.kotlin.psi.psiUtil.endOffset import org.jetbrains.kotlin.psi.psiUtil.isAbstract import org.jetbrains.kotlin.psi.psiUtil.startOffset import org.jetbrains.kotlin.resolve.diagnostics.Diagnostics +import org.jetbrains.kotlin.types.TypeProjection +import org.jetbrains.kotlin.types.typeUtil.asTypeProjection private const val DEFAULT_TAB_SIZE = 4 @@ -70,8 +78,13 @@ private fun getAbstractFunctionStubs(file: CompiledFile, kotlinClass: KtClass) = // For each of the super types used by this class kotlinClass.superTypeListEntries.mapNotNull { // Find the definition of this super type - val descriptor = file.referenceAtPoint(it.startOffset)?.second + val referenceAtPoint = file.referenceExpressionAtPoint(it.startOffset) + val descriptor = referenceAtPoint?.second val superClass = descriptor?.findPsi() + + val classDescriptor = descriptor as? ClassDescriptor + val superClassTypeArguments = getSuperClassTypeArguments(file, it) + // If the super class is abstract or an interface if (superClass is KtClass && (superClass.isAbstract() || superClass.isInterface())) { // Get the abstract functions of this super type that are currently not implemented by this class @@ -80,11 +93,27 @@ private fun getAbstractFunctionStubs(file: CompiledFile, kotlinClass: KtClass) = } // Get stubs for each function abstractFunctions.map { function -> getFunctionStub(function as KtNamedFunction) } + } else if(null != classDescriptor && (classDescriptor.kind.isInterface || classDescriptor.modality == Modality.ABSTRACT)) { + // TODO: refactor and prettify + classDescriptor.getMemberScope(superClassTypeArguments).getContributedDescriptors().filter { classMember -> + classMember is FunctionDescriptor && classMember.modality == Modality.ABSTRACT && !overridesDeclaration(kotlinClass, classMember) + }.map { function -> + createFunctionStub(function as FunctionDescriptor) + } } else { null } }.flatten() +// TODO: better name? +private fun getSuperClassTypeArguments(file: CompiledFile, superType: KtSuperTypeListEntry): List = superType.typeReference?.typeElement?.children?.filter { + it is KtTypeArgumentList +}?.flatMap { + (it as KtTypeArgumentList).arguments +}?.mapNotNull { + (file.referenceExpressionAtPoint(it?.startOffset ?: 0)?.second as? ClassDescriptor)?.defaultType?.asTypeProjection() +} ?: emptyList() + private fun isAbstractFunction(declaration: KtDeclaration): Boolean = declaration is KtNamedFunction && !declaration.hasBody() && (declaration.containingClass()?.isInterface() ?: false || declaration.hasModifier(KtTokens.ABSTRACT_KEYWORD)) @@ -103,6 +132,19 @@ private fun overridesDeclaration(kotlinClass: KtClass, declaration: KtDeclaratio } } +private fun overridesDeclaration(kotlinClass: KtClass, descriptor: FunctionDescriptor): Boolean = + kotlinClass.declarations.any { + if (it.name == descriptor.name.asString() && it.hasModifier(KtTokens.OVERRIDE_KEYWORD)) { + if (it is KtNamedFunction) { + parametersMatch(it, descriptor) + } else { + true + } + } else { + false + } + } + // Checks if two functions have matching parameters private fun parametersMatch(function: KtNamedFunction, functionDeclaration: KtNamedFunction): Boolean { if (function.valueParameters.size == functionDeclaration.valueParameters.size) { @@ -128,9 +170,49 @@ private fun parametersMatch(function: KtNamedFunction, functionDeclaration: KtNa return false } +private fun parametersMatch(function: KtNamedFunction, functionDescriptor: FunctionDescriptor): Boolean { + if (function.valueParameters.size == functionDescriptor.valueParameters.size) { + for (index in 0 until function.valueParameters.size) { + if (function.valueParameters[index].name != functionDescriptor.valueParameters[index].name.asString()) { + return false + } else if (function.valueParameters[index].typeReference?.name != functionDescriptor.valueParameters[index].type.toString()) { + // TODO: here we have exactly the old issue of type as above + return false + } + } + + if (function.typeParameters.size == functionDescriptor.typeParameters.size) { + for (index in 0 until function.typeParameters.size) { + if (function.typeParameters[index].variance != functionDescriptor.typeParameters[index].variance) { + return false + } + } + } + + return true + } + + return false +} + private fun getFunctionStub(function: KtNamedFunction): String = "override fun" + function.text.substringAfter("fun") + " { }" +private fun createFunctionStub(function: FunctionDescriptor): String { + // TODO: clean + val name = function.name + val arguments = function.valueParameters.map { argument -> + val argumentName = argument.name + // TODO: how to check if we actually should unwrap and make non-nullable? + val argumentType = argument.type.unwrap().makeNullableAsSpecified(false) + + "$argumentName: $argumentType" + }.joinToString(", ") + val returnType = function.returnType?.toString() + + return "override fun $name($arguments)${returnType?.takeIf { "Unit" != it }?.let { ": $it" } ?: ""} { }" +} + private fun getDeclarationPadding(file: CompiledFile, kotlinClass: KtClass): String { // If the class is not empty, the amount of padding is the same as the one in the last declaration of the class val paddingSize = if (kotlinClass.declarations.isNotEmpty()) { diff --git a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt index 363c935b9..67d2e1902 100644 --- a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt +++ b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt @@ -72,4 +72,53 @@ class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("code } +// TODO: naming +class ImplementAbstractMembersQuickFixExternalLibraryTest : SingleFileTestFixture("codeactions", "implementabstract_standardlib.kt") { + @Test + fun `should find one abstract method from Runnable to implement`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 5, 1, 5, 15, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(5, 28, 5, 28))) + assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun run() { }")) + } + + @Test + fun `should find one abstract method from Comparable to implement`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 7, 1, 7, 19, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(7, 42, 7, 42))) + assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun compare(p0: String, p1: String): Int { }")) + } +} +// TODO: should we have tests for one method already being in place? and then trying to implement the rest? diff --git a/server/src/test/resources/codeactions/implementabstract_standardlib.kt b/server/src/test/resources/codeactions/implementabstract_standardlib.kt new file mode 100644 index 000000000..665eab2d2 --- /dev/null +++ b/server/src/test/resources/codeactions/implementabstract_standardlib.kt @@ -0,0 +1,7 @@ +package test.kotlin.lsp + +import java.util.Comparator + +class MyThread : Runnable {} + +class MyComperable : Comparator {} From f94a586c928a2d71e28654a2eea7aaa66d4ebbca Mon Sep 17 00:00:00 2001 From: Marie Katrine Ekeberg Date: Sun, 24 Apr 2022 17:14:00 +0200 Subject: [PATCH 3/5] Unified the old implementation and the new. Better nullability handling --- .../ImplementAbstractFunctionsQuickFix.kt | 96 +++++++------------ .../kotlin/org/javacs/kt/CodeActionTest.kt | 53 ++++++++-- ...efile.kt => implementabstract_samefile.kt} | 10 ++ 3 files changed, 91 insertions(+), 68 deletions(-) rename server/src/test/resources/codeactions/{samefile.kt => implementabstract_samefile.kt} (50%) diff --git a/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt b/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt index 651036c1e..40b452b72 100644 --- a/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt +++ b/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt @@ -6,8 +6,11 @@ import org.javacs.kt.CompiledFile import org.javacs.kt.index.SymbolIndex import org.javacs.kt.position.offset import org.javacs.kt.position.position +import org.javacs.kt.LOG import org.javacs.kt.util.toPath import org.jetbrains.kotlin.descriptors.ClassDescriptor +import org.jetbrains.kotlin.descriptors.ClassConstructorDescriptor +import org.jetbrains.kotlin.descriptors.DeclarationDescriptor import org.jetbrains.kotlin.descriptors.FunctionDescriptor import org.jetbrains.kotlin.descriptors.isInterface import org.jetbrains.kotlin.descriptors.Modality @@ -16,8 +19,10 @@ import org.jetbrains.kotlin.lexer.KtTokens import org.jetbrains.kotlin.psi.KtClass import org.jetbrains.kotlin.psi.KtDeclaration import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtSimpleNameExpression import org.jetbrains.kotlin.psi.KtSuperTypeListEntry import org.jetbrains.kotlin.psi.KtTypeArgumentList +import org.jetbrains.kotlin.psi.KtTypeReference import org.jetbrains.kotlin.psi.psiUtil.containingClass import org.jetbrains.kotlin.psi.psiUtil.endOffset import org.jetbrains.kotlin.psi.psiUtil.isAbstract @@ -36,6 +41,8 @@ class ImplementAbstractFunctionsQuickFix : QuickFix { val endCursor = offset(file.content, range.end) val kotlinDiagnostics = file.compile.diagnostics + //LOG.info("start: {}, end: {}, range {}, diagnostics {}", startCursor, endCursor, range, diagnostics) + // If the client side and the server side diagnostics contain a valid diagnostic for this range. if (diagnostic != null && anyDiagnosticMatch(kotlinDiagnostics, startCursor, endCursor)) { // Get the class with the missing functions @@ -80,24 +87,15 @@ private fun getAbstractFunctionStubs(file: CompiledFile, kotlinClass: KtClass) = // Find the definition of this super type val referenceAtPoint = file.referenceExpressionAtPoint(it.startOffset) val descriptor = referenceAtPoint?.second - val superClass = descriptor?.findPsi() - val classDescriptor = descriptor as? ClassDescriptor - val superClassTypeArguments = getSuperClassTypeArguments(file, it) + val classDescriptor = getClassDescriptor(descriptor) + val superClassTypeArguments = getSuperClassTypeArguments(file, it) // If the super class is abstract or an interface - if (superClass is KtClass && (superClass.isAbstract() || superClass.isInterface())) { - // Get the abstract functions of this super type that are currently not implemented by this class - val abstractFunctions = superClass.declarations.filter { - declaration -> isAbstractFunction(declaration) && !overridesDeclaration(kotlinClass, declaration) - } - // Get stubs for each function - abstractFunctions.map { function -> getFunctionStub(function as KtNamedFunction) } - } else if(null != classDescriptor && (classDescriptor.kind.isInterface || classDescriptor.modality == Modality.ABSTRACT)) { - // TODO: refactor and prettify + if(null != classDescriptor && (classDescriptor.kind.isInterface || classDescriptor.modality == Modality.ABSTRACT)) { classDescriptor.getMemberScope(superClassTypeArguments).getContributedDescriptors().filter { classMember -> - classMember is FunctionDescriptor && classMember.modality == Modality.ABSTRACT && !overridesDeclaration(kotlinClass, classMember) - }.map { function -> + classMember is FunctionDescriptor && classMember.modality == Modality.ABSTRACT && !overridesDeclaration(kotlinClass, classMember) + }.map { function -> createFunctionStub(function as FunctionDescriptor) } } else { @@ -105,6 +103,16 @@ private fun getAbstractFunctionStubs(file: CompiledFile, kotlinClass: KtClass) = } }.flatten() +// TODO: better name? +// interfaces are ClassDescriptors by default. When calling AbstractClass super methods, we get a ClassConstructorDescriptor +private fun getClassDescriptor(descriptor: DeclarationDescriptor?): ClassDescriptor? = if(descriptor is ClassDescriptor) { + descriptor +} else if(descriptor is ClassConstructorDescriptor) { + descriptor.containingDeclaration +} else { + null +} + // TODO: better name? private fun getSuperClassTypeArguments(file: CompiledFile, superType: KtSuperTypeListEntry): List = superType.typeReference?.typeElement?.children?.filter { it is KtTypeArgumentList @@ -113,25 +121,8 @@ private fun getSuperClassTypeArguments(file: CompiledFile, superType: KtSuperTyp }?.mapNotNull { (file.referenceExpressionAtPoint(it?.startOffset ?: 0)?.second as? ClassDescriptor)?.defaultType?.asTypeProjection() } ?: emptyList() - -private fun isAbstractFunction(declaration: KtDeclaration): Boolean = - declaration is KtNamedFunction && !declaration.hasBody() - && (declaration.containingClass()?.isInterface() ?: false || declaration.hasModifier(KtTokens.ABSTRACT_KEYWORD)) // Checks if the class overrides the given declaration -private fun overridesDeclaration(kotlinClass: KtClass, declaration: KtDeclaration): Boolean = - kotlinClass.declarations.any { - if (it.name == declaration.name && it.hasModifier(KtTokens.OVERRIDE_KEYWORD)) { - if (it is KtNamedFunction && declaration is KtNamedFunction) { - parametersMatch(it, declaration) - } else { - true - } - } else { - false - } - } - private fun overridesDeclaration(kotlinClass: KtClass, descriptor: FunctionDescriptor): Boolean = kotlinClass.declarations.any { if (it.name == descriptor.name.asString() && it.hasModifier(KtTokens.OVERRIDE_KEYWORD)) { @@ -146,37 +137,13 @@ private fun overridesDeclaration(kotlinClass: KtClass, descriptor: FunctionDescr } // Checks if two functions have matching parameters -private fun parametersMatch(function: KtNamedFunction, functionDeclaration: KtNamedFunction): Boolean { - if (function.valueParameters.size == functionDeclaration.valueParameters.size) { - for (index in 0 until function.valueParameters.size) { - if (function.valueParameters[index].name != functionDeclaration.valueParameters[index].name) { - return false - } else if (function.valueParameters[index].typeReference?.name != functionDeclaration.valueParameters[index].typeReference?.name) { - return false - } - } - - if (function.typeParameters.size == functionDeclaration.typeParameters.size) { - for (index in 0 until function.typeParameters.size) { - if (function.typeParameters[index].variance != functionDeclaration.typeParameters[index].variance) { - return false - } - } - } - - return true - } - - return false -} - private fun parametersMatch(function: KtNamedFunction, functionDescriptor: FunctionDescriptor): Boolean { if (function.valueParameters.size == functionDescriptor.valueParameters.size) { for (index in 0 until function.valueParameters.size) { if (function.valueParameters[index].name != functionDescriptor.valueParameters[index].name.asString()) { return false - } else if (function.valueParameters[index].typeReference?.name != functionDescriptor.valueParameters[index].type.toString()) { - // TODO: here we have exactly the old issue of type as above + } else if (getTypeNameFromTypeReference(function.valueParameters[index].typeReference) != functionDescriptor.valueParameters[index].type.toString()) { + // TODO: here we have exactly the old issue of type as above. Maybe a common method that can be called to get the correct one? or do we maybe have to check both null and not null types here? return false } } @@ -195,20 +162,25 @@ private fun parametersMatch(function: KtNamedFunction, functionDescriptor: Funct return false } -private fun getFunctionStub(function: KtNamedFunction): String = - "override fun" + function.text.substringAfter("fun") + " { }" +// typeReference.name is often null. This fetches the name directly from the KtSimpleNameExpression instead +private fun getTypeNameFromTypeReference(typeReference: KtTypeReference?): String? = typeReference?.typeElement?.children?.filter { + it is KtSimpleNameExpression +}?.map { + (it as KtSimpleNameExpression).getReferencedName() +}?.get(0) private fun createFunctionStub(function: FunctionDescriptor): String { // TODO: clean val name = function.name val arguments = function.valueParameters.map { argument -> val argumentName = argument.name - // TODO: how to check if we actually should unwrap and make non-nullable? - val argumentType = argument.type.unwrap().makeNullableAsSpecified(false) + // about argument.type: regular Kotlin types are marked T or T?, but types from Java are (T..T?) because nullability cannot be decided. + // Therefore we have to unpack in case we have the Java type. Fortunately, the Java types are not marked nullable, so we default to non nullable types. Let the user decide if they want nullable types instead. With this implementation Kotlin types also keeps their nullability + val argumentType = argument.type.unwrap().makeNullableAsSpecified(argument.type.isMarkedNullable) "$argumentName: $argumentType" }.joinToString(", ") - val returnType = function.returnType?.toString() + val returnType = function.returnType?.unwrap()?.makeNullableAsSpecified(function.returnType?.isMarkedNullable ?: false)?.toString() return "override fun $name($arguments)${returnType?.takeIf { "Unit" != it }?.let { ": $it" } ?: ""} { }" } diff --git a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt index 67d2e1902..d591ac46a 100644 --- a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt +++ b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt @@ -7,8 +7,7 @@ import org.junit.Assert.assertThat import org.junit.Assert.fail import org.hamcrest.Matchers.* -// TODO: naming -class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("codeactions", "samefile.kt") { +class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("codeactions", "implementabstract_samefile.kt") { @Test fun `should find no code actions`() { @@ -69,10 +68,54 @@ class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("code assertThat(secondFunctionToImplementEdit?.range, equalTo(range(15, 49, 15, 49))) assertThat(secondFunctionToImplementEdit?.newText, equalTo("\n\n override fun test(input: String, otherInput: Int) { }")) } -} + @Test + fun `should find only one abstract method when the other one is already implemented`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 17, 1, 17, 26, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(18, 57, 18, 57))) + assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun print() { }")) + } + + @Test + fun `should respect nullability of parameter and return value in abstract method`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 25, 1, 25, 16, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(25, 48, 25, 48))) + assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun myMethod(myStr: String?): String? { }")) + } +} -// TODO: naming class ImplementAbstractMembersQuickFixExternalLibraryTest : SingleFileTestFixture("codeactions", "implementabstract_standardlib.kt") { @Test fun `should find one abstract method from Runnable to implement`() { @@ -120,5 +163,3 @@ class ImplementAbstractMembersQuickFixExternalLibraryTest : SingleFileTestFixtur assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun compare(p0: String, p1: String): Int { }")) } } - -// TODO: should we have tests for one method already being in place? and then trying to implement the rest? diff --git a/server/src/test/resources/codeactions/samefile.kt b/server/src/test/resources/codeactions/implementabstract_samefile.kt similarity index 50% rename from server/src/test/resources/codeactions/samefile.kt rename to server/src/test/resources/codeactions/implementabstract_samefile.kt index 946706364..b4d2eb3d5 100644 --- a/server/src/test/resources/codeactions/samefile.kt +++ b/server/src/test/resources/codeactions/implementabstract_samefile.kt @@ -13,3 +13,13 @@ abstract class CanPrint { } class PrintableClass : CanPrint(), MyInterface {} + +class OtherPrintableClass : CanPrint(), MyInterface { + override fun test(input: String, otherInput: Int) {} +} + +interface NullMethodAndReturn { + fun myMethod(myStr: T?): T? +} + +class NullClass : NullMethodAndReturn {} From 0e5ab967ca8681ab972653377da5db477c8b1abd Mon Sep 17 00:00:00 2001 From: Marie Katrine Ekeberg Date: Sun, 24 Apr 2022 20:45:57 +0200 Subject: [PATCH 4/5] Refactoring and minor cleanup. --- .../ImplementAbstractFunctionsQuickFix.kt | 41 +++++++++---------- .../kotlin/org/javacs/kt/CodeActionTest.kt | 9 +--- .../codeactions/implementabstract_samefile.kt | 2 +- 3 files changed, 22 insertions(+), 30 deletions(-) diff --git a/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt b/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt index 40b452b72..12910c87d 100644 --- a/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt +++ b/server/src/main/kotlin/org/javacs/kt/codeaction/quickfix/ImplementAbstractFunctionsQuickFix.kt @@ -6,7 +6,6 @@ import org.javacs.kt.CompiledFile import org.javacs.kt.index.SymbolIndex import org.javacs.kt.position.offset import org.javacs.kt.position.position -import org.javacs.kt.LOG import org.javacs.kt.util.toPath import org.jetbrains.kotlin.descriptors.ClassDescriptor import org.jetbrains.kotlin.descriptors.ClassConstructorDescriptor @@ -28,6 +27,7 @@ import org.jetbrains.kotlin.psi.psiUtil.endOffset import org.jetbrains.kotlin.psi.psiUtil.isAbstract import org.jetbrains.kotlin.psi.psiUtil.startOffset import org.jetbrains.kotlin.resolve.diagnostics.Diagnostics +import org.jetbrains.kotlin.types.KotlinType import org.jetbrains.kotlin.types.TypeProjection import org.jetbrains.kotlin.types.typeUtil.asTypeProjection @@ -40,8 +40,6 @@ class ImplementAbstractFunctionsQuickFix : QuickFix { val startCursor = offset(file.content, range.start) val endCursor = offset(file.content, range.end) val kotlinDiagnostics = file.compile.diagnostics - - //LOG.info("start: {}, end: {}, range {}, diagnostics {}", startCursor, endCursor, range, diagnostics) // If the client side and the server side diagnostics contain a valid diagnostic for this range. if (diagnostic != null && anyDiagnosticMatch(kotlinDiagnostics, startCursor, endCursor)) { @@ -89,10 +87,10 @@ private fun getAbstractFunctionStubs(file: CompiledFile, kotlinClass: KtClass) = val descriptor = referenceAtPoint?.second val classDescriptor = getClassDescriptor(descriptor) - val superClassTypeArguments = getSuperClassTypeArguments(file, it) // If the super class is abstract or an interface - if(null != classDescriptor && (classDescriptor.kind.isInterface || classDescriptor.modality == Modality.ABSTRACT)) { + if (null != classDescriptor && (classDescriptor.kind.isInterface || classDescriptor.modality == Modality.ABSTRACT)) { + val superClassTypeArguments = getSuperClassTypeProjections(file, it) classDescriptor.getMemberScope(superClassTypeArguments).getContributedDescriptors().filter { classMember -> classMember is FunctionDescriptor && classMember.modality == Modality.ABSTRACT && !overridesDeclaration(kotlinClass, classMember) }.map { function -> @@ -103,18 +101,16 @@ private fun getAbstractFunctionStubs(file: CompiledFile, kotlinClass: KtClass) = } }.flatten() -// TODO: better name? // interfaces are ClassDescriptors by default. When calling AbstractClass super methods, we get a ClassConstructorDescriptor -private fun getClassDescriptor(descriptor: DeclarationDescriptor?): ClassDescriptor? = if(descriptor is ClassDescriptor) { +private fun getClassDescriptor(descriptor: DeclarationDescriptor?): ClassDescriptor? = if (descriptor is ClassDescriptor) { descriptor -} else if(descriptor is ClassConstructorDescriptor) { +} else if (descriptor is ClassConstructorDescriptor) { descriptor.containingDeclaration } else { null } - -// TODO: better name? -private fun getSuperClassTypeArguments(file: CompiledFile, superType: KtSuperTypeListEntry): List = superType.typeReference?.typeElement?.children?.filter { + +private fun getSuperClassTypeProjections(file: CompiledFile, superType: KtSuperTypeListEntry): List = superType.typeReference?.typeElement?.children?.filter { it is KtTypeArgumentList }?.flatMap { (it as KtTypeArgumentList).arguments @@ -142,8 +138,9 @@ private fun parametersMatch(function: KtNamedFunction, functionDescriptor: Funct for (index in 0 until function.valueParameters.size) { if (function.valueParameters[index].name != functionDescriptor.valueParameters[index].name.asString()) { return false - } else if (getTypeNameFromTypeReference(function.valueParameters[index].typeReference) != functionDescriptor.valueParameters[index].type.toString()) { - // TODO: here we have exactly the old issue of type as above. Maybe a common method that can be called to get the correct one? or do we maybe have to check both null and not null types here? + } else if (function.valueParameters[index].typeReference?.typeName() != functionDescriptor.valueParameters[index].type.unwrappedType().toString()) { + // Note: Since we treat Java overrides as non nullable by default, the above test will fail when the user has made the type nullable. + // TODO: look into this return false } } @@ -162,29 +159,29 @@ private fun parametersMatch(function: KtNamedFunction, functionDescriptor: Funct return false } -// typeReference.name is often null. This fetches the name directly from the KtSimpleNameExpression instead -private fun getTypeNameFromTypeReference(typeReference: KtTypeReference?): String? = typeReference?.typeElement?.children?.filter { +private fun KtTypeReference.typeName(): String? = this.name ?: this.typeElement?.children?.filter { it is KtSimpleNameExpression }?.map { (it as KtSimpleNameExpression).getReferencedName() -}?.get(0) +}?.firstOrNull() private fun createFunctionStub(function: FunctionDescriptor): String { - // TODO: clean val name = function.name val arguments = function.valueParameters.map { argument -> val argumentName = argument.name - // about argument.type: regular Kotlin types are marked T or T?, but types from Java are (T..T?) because nullability cannot be decided. - // Therefore we have to unpack in case we have the Java type. Fortunately, the Java types are not marked nullable, so we default to non nullable types. Let the user decide if they want nullable types instead. With this implementation Kotlin types also keeps their nullability - val argumentType = argument.type.unwrap().makeNullableAsSpecified(argument.type.isMarkedNullable) + val argumentType = argument.type.unwrappedType() "$argumentName: $argumentType" }.joinToString(", ") - val returnType = function.returnType?.unwrap()?.makeNullableAsSpecified(function.returnType?.isMarkedNullable ?: false)?.toString() + val returnType = function.returnType?.unwrappedType()?.toString()?.takeIf { "Unit" != it } - return "override fun $name($arguments)${returnType?.takeIf { "Unit" != it }?.let { ": $it" } ?: ""} { }" + return "override fun $name($arguments)${returnType?.let { ": $it" } ?: ""} { }" } +// about types: regular Kotlin types are marked T or T?, but types from Java are (T..T?) because nullability cannot be decided. +// Therefore we have to unpack in case we have the Java type. Fortunately, the Java types are not marked nullable, so we default to non nullable types. Let the user decide if they want nullable types instead. With this implementation Kotlin types also keeps their nullability +private fun KotlinType.unwrappedType(): KotlinType = this.unwrap().makeNullableAsSpecified(this.isMarkedNullable) + private fun getDeclarationPadding(file: CompiledFile, kotlinClass: KtClass): String { // If the class is not empty, the amount of padding is the same as the one in the last declaration of the class val paddingSize = if (kotlinClass.declarations.isNotEmpty()) { diff --git a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt index d591ac46a..49c92b1a5 100644 --- a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt +++ b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt @@ -4,8 +4,8 @@ import org.eclipse.lsp4j.* import org.javacs.kt.SingleFileTestFixture import org.junit.Test import org.junit.Assert.assertThat -import org.junit.Assert.fail -import org.hamcrest.Matchers.* +import org.hamcrest.Matchers.equalTo +import org.hamcrest.Matchers.hasSize class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("codeactions", "implementabstract_samefile.kt") { @@ -53,7 +53,6 @@ class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("code val codeAction = codeActionResult[0].right assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) assertThat(codeAction.title, equalTo("Implement abstract functions")) - //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) val textEdit = codeAction.edit.changes val key = workspaceRoot.resolve(file).toUri().toString() @@ -80,7 +79,6 @@ class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("code val codeAction = codeActionResult[0].right assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) assertThat(codeAction.title, equalTo("Implement abstract functions")) - //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) val textEdit = codeAction.edit.changes val key = workspaceRoot.resolve(file).toUri().toString() @@ -103,7 +101,6 @@ class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("code val codeAction = codeActionResult[0].right assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) assertThat(codeAction.title, equalTo("Implement abstract functions")) - //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) val textEdit = codeAction.edit.changes val key = workspaceRoot.resolve(file).toUri().toString() @@ -128,7 +125,6 @@ class ImplementAbstractMembersQuickFixExternalLibraryTest : SingleFileTestFixtur val codeAction = codeActionResult[0].right assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) assertThat(codeAction.title, equalTo("Implement abstract functions")) - //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) val textEdit = codeAction.edit.changes val key = workspaceRoot.resolve(file).toUri().toString() @@ -151,7 +147,6 @@ class ImplementAbstractMembersQuickFixExternalLibraryTest : SingleFileTestFixtur val codeAction = codeActionResult[0].right assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) assertThat(codeAction.title, equalTo("Implement abstract functions")) - //assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) val textEdit = codeAction.edit.changes val key = workspaceRoot.resolve(file).toUri().toString() diff --git a/server/src/test/resources/codeactions/implementabstract_samefile.kt b/server/src/test/resources/codeactions/implementabstract_samefile.kt index b4d2eb3d5..6e322092d 100644 --- a/server/src/test/resources/codeactions/implementabstract_samefile.kt +++ b/server/src/test/resources/codeactions/implementabstract_samefile.kt @@ -19,7 +19,7 @@ class OtherPrintableClass : CanPrint(), MyInterface { } interface NullMethodAndReturn { - fun myMethod(myStr: T?): T? + fun myMethod(myStr: T?): T? } class NullClass : NullMethodAndReturn {} From ce9311f84873b35b043e1925c79f236fd10f6d75 Mon Sep 17 00:00:00 2001 From: Marie Katrine Ekeberg Date: Mon, 25 Apr 2022 20:48:47 +0200 Subject: [PATCH 5/5] Unified test files of quickfix tests --- .../kotlin/org/javacs/kt/CodeActionTest.kt | 160 ------------- .../kotlin/org/javacs/kt/QuickFixesTest.kt | 224 +++++++++++++++--- .../samefile.kt} | 0 .../standardlib.kt} | 0 4 files changed, 188 insertions(+), 196 deletions(-) delete mode 100644 server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt rename server/src/test/resources/{codeactions/implementabstract_samefile.kt => quickfixes/samefile.kt} (100%) rename server/src/test/resources/{codeactions/implementabstract_standardlib.kt => quickfixes/standardlib.kt} (100%) diff --git a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt b/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt deleted file mode 100644 index 49c92b1a5..000000000 --- a/server/src/test/kotlin/org/javacs/kt/CodeActionTest.kt +++ /dev/null @@ -1,160 +0,0 @@ -package org.javacs.kt - -import org.eclipse.lsp4j.* -import org.javacs.kt.SingleFileTestFixture -import org.junit.Test -import org.junit.Assert.assertThat -import org.hamcrest.Matchers.equalTo -import org.hamcrest.Matchers.hasSize - -class ImplementAbstractMembersQuickFixSameFileTest : SingleFileTestFixture("codeactions", "implementabstract_samefile.kt") { - - @Test - fun `should find no code actions`() { - val only = listOf(CodeActionKind.QuickFix) - val codeActionParams = codeActionParams(file, 3, 1, 3, 22, diagnostics, only) - - val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() - - assertThat(codeActionResult, hasSize(0)) - } - - @Test - fun `should find one abstract method to implement`() { - val only = listOf(CodeActionKind.QuickFix) - val codeActionParams = codeActionParams(file, 7, 1, 7, 14, diagnostics, only) - - val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() - - assertThat(codeActionResult, hasSize(1)) - val codeAction = codeActionResult[0].right - assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) - assertThat(codeAction.title, equalTo("Implement abstract functions")) - assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) - - val textEdit = codeAction.edit.changes - val key = workspaceRoot.resolve(file).toUri().toString() - assertThat(textEdit.containsKey(key), equalTo(true)) - assertThat(textEdit[key], hasSize(1)) - - val functionToImplementEdit = textEdit[key]?.get(0) - assertThat(functionToImplementEdit?.range, equalTo(range(7, 30, 7, 30))) - assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun test(input: String, otherInput: Int) { }")) - } - - @Test - fun `should find several abstract methods to implement`() { - val only = listOf(CodeActionKind.QuickFix) - val codeActionParams = codeActionParams(file, 15, 1, 15, 21, diagnostics, only) - - val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() - - assertThat(codeActionResult, hasSize(1)) - val codeAction = codeActionResult[0].right - assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) - assertThat(codeAction.title, equalTo("Implement abstract functions")) - - val textEdit = codeAction.edit.changes - val key = workspaceRoot.resolve(file).toUri().toString() - assertThat(textEdit.containsKey(key), equalTo(true)) - assertThat(textEdit[key], hasSize(2)) - - val firstFunctionToImplementEdit = textEdit[key]?.get(0) - assertThat(firstFunctionToImplementEdit?.range, equalTo(range(15, 49, 15, 49))) - assertThat(firstFunctionToImplementEdit?.newText, equalTo("\n\n override fun print() { }")) - - val secondFunctionToImplementEdit = textEdit[key]?.get(1) - assertThat(secondFunctionToImplementEdit?.range, equalTo(range(15, 49, 15, 49))) - assertThat(secondFunctionToImplementEdit?.newText, equalTo("\n\n override fun test(input: String, otherInput: Int) { }")) - } - - @Test - fun `should find only one abstract method when the other one is already implemented`() { - val only = listOf(CodeActionKind.QuickFix) - val codeActionParams = codeActionParams(file, 17, 1, 17, 26, diagnostics, only) - - val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() - - assertThat(codeActionResult, hasSize(1)) - val codeAction = codeActionResult[0].right - assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) - assertThat(codeAction.title, equalTo("Implement abstract functions")) - - val textEdit = codeAction.edit.changes - val key = workspaceRoot.resolve(file).toUri().toString() - assertThat(textEdit.containsKey(key), equalTo(true)) - assertThat(textEdit[key], hasSize(1)) - - val functionToImplementEdit = textEdit[key]?.get(0) - assertThat(functionToImplementEdit?.range, equalTo(range(18, 57, 18, 57))) - assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun print() { }")) - } - - @Test - fun `should respect nullability of parameter and return value in abstract method`() { - val only = listOf(CodeActionKind.QuickFix) - val codeActionParams = codeActionParams(file, 25, 1, 25, 16, diagnostics, only) - - val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() - - assertThat(codeActionResult, hasSize(1)) - val codeAction = codeActionResult[0].right - assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) - assertThat(codeAction.title, equalTo("Implement abstract functions")) - - val textEdit = codeAction.edit.changes - val key = workspaceRoot.resolve(file).toUri().toString() - assertThat(textEdit.containsKey(key), equalTo(true)) - assertThat(textEdit[key], hasSize(1)) - - val functionToImplementEdit = textEdit[key]?.get(0) - assertThat(functionToImplementEdit?.range, equalTo(range(25, 48, 25, 48))) - assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun myMethod(myStr: String?): String? { }")) - } -} - -class ImplementAbstractMembersQuickFixExternalLibraryTest : SingleFileTestFixture("codeactions", "implementabstract_standardlib.kt") { - @Test - fun `should find one abstract method from Runnable to implement`() { - val only = listOf(CodeActionKind.QuickFix) - val codeActionParams = codeActionParams(file, 5, 1, 5, 15, diagnostics, only) - - val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() - - assertThat(codeActionResult, hasSize(1)) - val codeAction = codeActionResult[0].right - assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) - assertThat(codeAction.title, equalTo("Implement abstract functions")) - - val textEdit = codeAction.edit.changes - val key = workspaceRoot.resolve(file).toUri().toString() - assertThat(textEdit.containsKey(key), equalTo(true)) - assertThat(textEdit[key], hasSize(1)) - - val functionToImplementEdit = textEdit[key]?.get(0) - assertThat(functionToImplementEdit?.range, equalTo(range(5, 28, 5, 28))) - assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun run() { }")) - } - - @Test - fun `should find one abstract method from Comparable to implement`() { - val only = listOf(CodeActionKind.QuickFix) - val codeActionParams = codeActionParams(file, 7, 1, 7, 19, diagnostics, only) - - val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() - - assertThat(codeActionResult, hasSize(1)) - val codeAction = codeActionResult[0].right - assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) - assertThat(codeAction.title, equalTo("Implement abstract functions")) - - val textEdit = codeAction.edit.changes - val key = workspaceRoot.resolve(file).toUri().toString() - assertThat(textEdit.containsKey(key), equalTo(true)) - assertThat(textEdit[key], hasSize(1)) - - val functionToImplementEdit = textEdit[key]?.get(0) - assertThat(functionToImplementEdit?.range, equalTo(range(7, 42, 7, 42))) - assertThat(functionToImplementEdit?.newText, equalTo("\n\n override fun compare(p0: String, p1: String): Int { }")) - } -} diff --git a/server/src/test/kotlin/org/javacs/kt/QuickFixesTest.kt b/server/src/test/kotlin/org/javacs/kt/QuickFixesTest.kt index 229a01859..4a514b89d 100644 --- a/server/src/test/kotlin/org/javacs/kt/QuickFixesTest.kt +++ b/server/src/test/kotlin/org/javacs/kt/QuickFixesTest.kt @@ -3,8 +3,9 @@ package org.javacs.kt import org.eclipse.lsp4j.CodeActionKind import org.eclipse.lsp4j.Diagnostic import org.eclipse.lsp4j.jsonrpc.messages.Either -import org.hamcrest.Matchers -import org.junit.Assert +import org.hamcrest.Matchers.equalTo +import org.hamcrest.Matchers.hasSize +import org.junit.Assert.assertThat import org.junit.Test class ImplementAbstractFunctionsQuickFixTest : SingleFileTestFixture("quickfixes", "SomeSubclass.kt") { @@ -16,27 +17,27 @@ class ImplementAbstractFunctionsQuickFixTest : SingleFileTestFixture("quickfixes val codeActions = languageServer.textDocumentService.codeAction(codeActionParams).get() - Assert.assertThat(codeActions.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.kind, Matchers.equalTo(CodeActionKind.QuickFix)) - Assert.assertThat(codeActions[0].right.diagnostics.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.diagnostics[0], Matchers.equalTo(diagnostic)) - Assert.assertThat(codeActions[0].right.edit.changes.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.size, Matchers.equalTo(2)) - Assert.assertThat( + assertThat(codeActions.size, equalTo(1)) + assertThat(codeActions[0].right.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeActions[0].right.diagnostics.size, equalTo(1)) + assertThat(codeActions[0].right.diagnostics[0], equalTo(diagnostic)) + assertThat(codeActions[0].right.edit.changes.size, equalTo(1)) + assertThat(codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.size, equalTo(2)) + assertThat( codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.get(0)?.range, - Matchers.equalTo(range(3, 55, 3, 55)) + equalTo(range(3, 55, 3, 55)) ) - Assert.assertThat( + assertThat( codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.get(0)?.newText, - Matchers.equalTo(System.lineSeparator() + System.lineSeparator() + " override fun someSuperMethod(someParameter: String): Int { }") + equalTo(System.lineSeparator() + System.lineSeparator() + " override fun someSuperMethod(someParameter: String): Int { }") ) - Assert.assertThat( + assertThat( codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.get(1)?.range, - Matchers.equalTo(range(3, 55, 3, 55)) + equalTo(range(3, 55, 3, 55)) ) - Assert.assertThat( + assertThat( codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.get(1)?.newText, - Matchers.equalTo(System.lineSeparator() + System.lineSeparator() + " override fun someInterfaceMethod() { }") + equalTo(System.lineSeparator() + System.lineSeparator() + " override fun someInterfaceMethod() { }") ) } @@ -48,19 +49,19 @@ class ImplementAbstractFunctionsQuickFixTest : SingleFileTestFixture("quickfixes val codeActions = languageServer.textDocumentService.codeAction(codeActionParams).get() - Assert.assertThat(codeActions.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.kind, Matchers.equalTo(CodeActionKind.QuickFix)) - Assert.assertThat(codeActions[0].right.diagnostics.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.diagnostics[0], Matchers.equalTo(diagnostic)) - Assert.assertThat(codeActions[0].right.edit.changes.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.size, Matchers.equalTo(1)) - Assert.assertThat( + assertThat(codeActions.size, equalTo(1)) + assertThat(codeActions[0].right.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeActions[0].right.diagnostics.size, equalTo(1)) + assertThat(codeActions[0].right.diagnostics[0], equalTo(diagnostic)) + assertThat(codeActions[0].right.edit.changes.size, equalTo(1)) + assertThat(codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.size, equalTo(1)) + assertThat( codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.get(0)?.range, - Matchers.equalTo(range(7, 74, 7, 74)) + equalTo(range(7, 74, 7, 74)) ) - Assert.assertThat( + assertThat( codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.get(0)?.newText, - Matchers.equalTo(System.lineSeparator() + System.lineSeparator() + " override fun someInterfaceMethod() { }") + equalTo(System.lineSeparator() + System.lineSeparator() + " override fun someInterfaceMethod() { }") ) } @@ -72,19 +73,170 @@ class ImplementAbstractFunctionsQuickFixTest : SingleFileTestFixture("quickfixes val codeActions = languageServer.textDocumentService.codeAction(codeActionParams).get() - Assert.assertThat(codeActions.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.kind, Matchers.equalTo(CodeActionKind.QuickFix)) - Assert.assertThat(codeActions[0].right.diagnostics.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.diagnostics[0], Matchers.equalTo(diagnostic)) - Assert.assertThat(codeActions[0].right.edit.changes.size, Matchers.equalTo(1)) - Assert.assertThat(codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.size, Matchers.equalTo(1)) - Assert.assertThat( + assertThat(codeActions.size, equalTo(1)) + assertThat(codeActions[0].right.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeActions[0].right.diagnostics.size, equalTo(1)) + assertThat(codeActions[0].right.diagnostics[0], equalTo(diagnostic)) + assertThat(codeActions[0].right.edit.changes.size, equalTo(1)) + assertThat(codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.size, equalTo(1)) + assertThat( codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.get(0)?.range, - Matchers.equalTo(range(11, 43, 11, 43)) + equalTo(range(11, 43, 11, 43)) ) - Assert.assertThat( + assertThat( codeActions[0].right.edit.changes[codeActionParams.textDocument.uri]?.get(0)?.newText, - Matchers.equalTo(System.lineSeparator() + System.lineSeparator() + " override fun someSuperMethod(someParameter: String): Int { }") + equalTo(System.lineSeparator() + System.lineSeparator() + " override fun someSuperMethod(someParameter: String): Int { }") ) } } + +class ImplementAbstractFunctionsQuickFixSameFileTest : SingleFileTestFixture("quickfixes", "samefile.kt") { + @Test + fun `should find no code actions`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 3, 1, 3, 22, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(0)) + } + + @Test + fun `should find one abstract method to implement`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 7, 1, 7, 14, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + assertThat(codeAction.diagnostics, equalTo(listOf(diagnostics[0]))) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(7, 30, 7, 30))) + assertThat(functionToImplementEdit?.newText, equalTo(System.lineSeparator() + System.lineSeparator() + " override fun test(input: String, otherInput: Int) { }")) + } + + @Test + fun `should find several abstract methods to implement`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 15, 1, 15, 21, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(2)) + + val firstFunctionToImplementEdit = textEdit[key]?.get(0) + assertThat(firstFunctionToImplementEdit?.range, equalTo(range(15, 49, 15, 49))) + assertThat(firstFunctionToImplementEdit?.newText, equalTo(System.lineSeparator() + System.lineSeparator() + " override fun print() { }")) + + val secondFunctionToImplementEdit = textEdit[key]?.get(1) + assertThat(secondFunctionToImplementEdit?.range, equalTo(range(15, 49, 15, 49))) + assertThat(secondFunctionToImplementEdit?.newText, equalTo(System.lineSeparator() + System.lineSeparator() + " override fun test(input: String, otherInput: Int) { }")) + } + + @Test + fun `should find only one abstract method when the other one is already implemented`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 17, 1, 17, 26, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(18, 57, 18, 57))) + assertThat(functionToImplementEdit?.newText, equalTo(System.lineSeparator() + System.lineSeparator() + " override fun print() { }")) + } + + @Test + fun `should respect nullability of parameter and return value in abstract method`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 25, 1, 25, 16, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(25, 48, 25, 48))) + assertThat(functionToImplementEdit?.newText, equalTo(System.lineSeparator() + System.lineSeparator() + " override fun myMethod(myStr: String?): String? { }")) + } +} + +class ImplementAbstractFunctionsQuickFixExternalLibraryTest : SingleFileTestFixture("quickfixes", "standardlib.kt") { + @Test + fun `should find one abstract method from Runnable to implement`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 5, 1, 5, 15, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(5, 28, 5, 28))) + assertThat(functionToImplementEdit?.newText, equalTo(System.lineSeparator() + System.lineSeparator() + " override fun run() { }")) + } + + @Test + fun `should find one abstract method from Comparable to implement`() { + val only = listOf(CodeActionKind.QuickFix) + val codeActionParams = codeActionParams(file, 7, 1, 7, 19, diagnostics, only) + + val codeActionResult = languageServer.textDocumentService.codeAction(codeActionParams).get() + + assertThat(codeActionResult, hasSize(1)) + val codeAction = codeActionResult[0].right + assertThat(codeAction.kind, equalTo(CodeActionKind.QuickFix)) + assertThat(codeAction.title, equalTo("Implement abstract functions")) + + val textEdit = codeAction.edit.changes + val key = workspaceRoot.resolve(file).toUri().toString() + assertThat(textEdit.containsKey(key), equalTo(true)) + assertThat(textEdit[key], hasSize(1)) + + val functionToImplementEdit = textEdit[key]?.get(0) + assertThat(functionToImplementEdit?.range, equalTo(range(7, 42, 7, 42))) + assertThat(functionToImplementEdit?.newText, equalTo(System.lineSeparator() + System.lineSeparator() + " override fun compare(p0: String, p1: String): Int { }")) + } +} diff --git a/server/src/test/resources/codeactions/implementabstract_samefile.kt b/server/src/test/resources/quickfixes/samefile.kt similarity index 100% rename from server/src/test/resources/codeactions/implementabstract_samefile.kt rename to server/src/test/resources/quickfixes/samefile.kt diff --git a/server/src/test/resources/codeactions/implementabstract_standardlib.kt b/server/src/test/resources/quickfixes/standardlib.kt similarity index 100% rename from server/src/test/resources/codeactions/implementabstract_standardlib.kt rename to server/src/test/resources/quickfixes/standardlib.kt