From c12f29ae307dbfa28372252fec8d9e5f237f102a Mon Sep 17 00:00:00 2001 From: Mikhail Glukhikh Date: Fri, 24 Aug 2018 13:17:40 +0300 Subject: [PATCH] Enhance inspection about SuccessOrFailure (related to KT-25621) Increase performance by searching first SuccessOrFailure/runCatching/etc in text of functions without return type. Remove stdlib false positives, like success() & failure(). For catching extension, check also non-catching members. Add "rename to *Catching" fix. --- .../ResultIsSuccessOrFailureInspection.kt | 97 ++++++++++++++++--- .../inspectionData/expected.xml | 18 ++++ .../resultIsSuccessOrFailure/test.kt | 38 ++++++-- .../resultIsSuccessOrFailure/abstract.kt | 10 ++ .../abstract.kt.after | 10 ++ .../resultIsSuccessOrFailure/complexBlock.kt | 1 + .../complexBlock.kt.after | 1 + .../needParentheses.kt | 1 + .../needParentheses.kt.after | 1 + .../resultIsSuccessOrFailure/rename.kt | 8 ++ .../resultIsSuccessOrFailure/rename.kt.after | 8 ++ .../resultIsSuccessOrFailure/simple.kt | 1 + .../resultIsSuccessOrFailure/simple.kt.after | 1 + .../resultIsSuccessOrFailure/simpleBlock.kt | 1 + .../simpleBlock.kt.after | 1 + .../LocalInspectionTestGenerated.java | 10 ++ 16 files changed, 185 insertions(+), 22 deletions(-) create mode 100644 idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt create mode 100644 idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt.after create mode 100644 idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt create mode 100644 idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt.after diff --git a/idea/src/org/jetbrains/kotlin/idea/inspections/coroutines/ResultIsSuccessOrFailureInspection.kt b/idea/src/org/jetbrains/kotlin/idea/inspections/coroutines/ResultIsSuccessOrFailureInspection.kt index e6cc4f8923b..bfbfb14c631 100644 --- a/idea/src/org/jetbrains/kotlin/idea/inspections/coroutines/ResultIsSuccessOrFailureInspection.kt +++ b/idea/src/org/jetbrains/kotlin/idea/inspections/coroutines/ResultIsSuccessOrFailureInspection.kt @@ -12,14 +12,15 @@ import com.intellij.codeInspection.ProblemsHolder import com.intellij.openapi.project.Project import com.intellij.psi.PsiElement import com.intellij.psi.PsiElementVisitor +import com.intellij.refactoring.rename.RenameProcessor import org.jetbrains.kotlin.descriptors.ClassDescriptor import org.jetbrains.kotlin.descriptors.FunctionDescriptor import org.jetbrains.kotlin.descriptors.PackageFragmentDescriptor +import org.jetbrains.kotlin.descriptors.ValueParameterDescriptor import org.jetbrains.kotlin.idea.caches.resolve.analyze import org.jetbrains.kotlin.idea.caches.resolve.resolveToDescriptorIfAny import org.jetbrains.kotlin.idea.core.setType import org.jetbrains.kotlin.idea.inspections.AbstractKotlinInspection -import org.jetbrains.kotlin.idea.intentions.branchedTransformations.BranchedFoldingUtils import org.jetbrains.kotlin.idea.quickfix.createFromUsage.callableBuilder.getReturnTypeReference import org.jetbrains.kotlin.incremental.components.NoLookupLocation import org.jetbrains.kotlin.name.Name @@ -29,32 +30,80 @@ import org.jetbrains.kotlin.psi.psiUtil.getNonStrictParentOfType import org.jetbrains.kotlin.resolve.BindingContext import org.jetbrains.kotlin.resolve.bindingContextUtil.getTargetFunctionDescriptor import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe +import org.jetbrains.kotlin.resolve.scopes.MemberScope +import org.jetbrains.kotlin.types.KotlinType class ResultIsSuccessOrFailureInspection : AbstractKotlinInspection() { + + private fun MemberScope.hasCorrespondingNonCatchingFunction( + nameWithoutCatching: String, + valueParameters: List, + returnType: KotlinType?, + extensionReceiverType: KotlinType? + ): Boolean { + val nonCatchingFunctions = getContributedFunctions(Name.identifier(nameWithoutCatching), NoLookupLocation.FROM_IDE) + return nonCatchingFunctions.any { nonCatchingFun -> + nonCatchingFun.valueParameters.size == valueParameters.size + && nonCatchingFun.returnType == returnType + && nonCatchingFun.extensionReceiverParameter?.type == extensionReceiverType + } + } + + private fun FunctionDescriptor.hasCorrespondingNonCatchingFunction(returnType: KotlinType, nameWithoutCatching: String): Boolean? { + val containingDescriptor = containingDeclaration + val scope = when (containingDescriptor) { + is ClassDescriptor -> containingDescriptor.unsubstitutedMemberScope + is PackageFragmentDescriptor -> containingDescriptor.getMemberScope() + else -> return null + } + val returnTypeArgument = returnType.arguments.firstOrNull()?.type + val extensionReceiverType = extensionReceiverParameter?.type + if (scope.hasCorrespondingNonCatchingFunction(nameWithoutCatching, valueParameters, returnTypeArgument, extensionReceiverType)) { + return true + } + if (extensionReceiverType != null) { + val extensionClassDescriptor = extensionReceiverType.constructor.declarationDescriptor as? ClassDescriptor + if (extensionClassDescriptor != null) { + val extensionClassScope = extensionClassDescriptor.unsubstitutedMemberScope + if (extensionClassScope.hasCorrespondingNonCatchingFunction( + nameWithoutCatching, valueParameters, returnTypeArgument, extensionReceiverType = null + ) + ) { + return true + } + } + } + return false + } + private fun analyzeFunction(function: KtFunction, toReport: PsiElement, holder: ProblemsHolder) { if (function is KtConstructor<*>) return val returnTypeText = function.getReturnTypeReference()?.text if (returnTypeText != null && SHORT_NAME !in returnTypeText) return + val name = (function as? KtNamedFunction)?.nameAsName?.asString() + // Filter names from stdlib + if (name in ALLOWED_NAMES) return + if (function is KtNamedFunction) { + val receiverTypeReference = function.receiverTypeReference + // Filter SuccessOrFailure extensions + if (receiverTypeReference != null && SHORT_NAME in receiverTypeReference.text) return + } + if (function is KtFunctionLiteral || returnTypeText == null) { + // Heuristics to save performance + val text = function.bodyExpression?.text + // Check there is something creating SuccessOrFailure in function text + if (text != null && ALLOWED_NAMES.none { it in text } && SHORT_NAME !in text && CATCHING !in text) return + } val descriptor = function.resolveToDescriptorIfAny() as? FunctionDescriptor ?: return val returnType = descriptor.returnType ?: return val returnTypeClass = returnType.constructor.declarationDescriptor as? ClassDescriptor ?: return if (returnTypeClass.fqNameSafe.asString() != FULL_NAME) return - val name = (function as? KtNamedFunction)?.nameAsName?.asString() if (name != null && name.endsWith(CATCHING)) { val nameWithoutCatching = name.substringBeforeLast(CATCHING) - val containingDescriptor = descriptor.containingDeclaration - val scope = when (containingDescriptor) { - is ClassDescriptor -> containingDescriptor.unsubstitutedMemberScope - is PackageFragmentDescriptor -> containingDescriptor.getMemberScope() - else -> return - } - val returnTypeArgument = returnType.arguments.firstOrNull()?.type - val nonCatchingFunctions = scope.getContributedFunctions(Name.identifier(nameWithoutCatching), NoLookupLocation.FROM_IDE) - if (nonCatchingFunctions.none { nonCatchingFun -> - nonCatchingFun.returnType == returnTypeArgument - }) { + if (descriptor.hasCorrespondingNonCatchingFunction(returnType, nameWithoutCatching) == false) { + val returnTypeArgument = returnType.arguments.firstOrNull()?.type val typeName = returnTypeArgument?.constructor?.declarationDescriptor?.name?.asString() ?: "T" holder.registerProblem( toReport, @@ -68,7 +117,10 @@ class ResultIsSuccessOrFailureInspection : AbstractKotlinInspection() { toReport, "Function returning $SHORT_NAME with a name that does not end with $CATCHING", ProblemHighlightType.GENERIC_ERROR_OR_WARNING, - AddGetOrThrowFix(withBody = function.hasBody()) + *listOfNotNull( + AddGetOrThrowFix(withBody = function.hasBody()), + name?.let { RenameToCatchingFix("$it$CATCHING") } + ).toTypedArray() ) } } @@ -88,7 +140,7 @@ class ResultIsSuccessOrFailureInspection : AbstractKotlinInspection() { private class AddGetOrThrowFix(val withBody: Boolean) : LocalQuickFix { override fun getName(): String = if (withBody) "Add '.$GET_OR_THROW()' to function result (breaks use-sites!)" - else "Unfold '$SHORT_NAME' return type (breaks use-sites!)" + else "Unwrap '$SHORT_NAME' return type (breaks use-sites!)" override fun getFamilyName(): String = name @@ -126,6 +178,19 @@ class ResultIsSuccessOrFailureInspection : AbstractKotlinInspection() { } } + private class RenameToCatchingFix(val newName: String) : LocalQuickFix { + override fun getName(): String = "Rename to '$newName'" + + override fun getFamilyName(): String = name + + override fun startInWriteAction(): Boolean = false + + override fun applyFix(project: Project, descriptor: ProblemDescriptor) { + val function = descriptor.psiElement.getNonStrictParentOfType() ?: return + RenameProcessor(project, function, newName, false, false).run() + } + } + companion object { private const val SHORT_NAME = "SuccessOrFailure" @@ -134,5 +199,7 @@ class ResultIsSuccessOrFailureInspection : AbstractKotlinInspection() { private const val CATCHING = "Catching" private const val GET_OR_THROW = "getOrThrow" + + private val ALLOWED_NAMES = setOf("success", "failure", "runCatching") } } \ No newline at end of file diff --git a/idea/testData/inspections/coroutines/resultIsSuccessOrFailure/inspectionData/expected.xml b/idea/testData/inspections/coroutines/resultIsSuccessOrFailure/inspectionData/expected.xml index a2823c81296..b1b4be9224b 100644 --- a/idea/testData/inspections/coroutines/resultIsSuccessOrFailure/inspectionData/expected.xml +++ b/idea/testData/inspections/coroutines/resultIsSuccessOrFailure/inspectionData/expected.xml @@ -89,4 +89,22 @@ Function returning SuccessOrFailure Function returning SuccessOrFailure with a name that does not end with Catching + + test.kt + 60 + light_idea_test_case + <default> + + Function returning SuccessOrFailure + Function 'calcComplexCatching' returning 'SuccessOrFailure<Double>' without the corresponding function 'calcComplex' returning 'Double' + + + test.kt + 67 + light_idea_test_case + <default> + + Function returning SuccessOrFailure + Function 'extensionActCatching' returning 'SuccessOrFailure<Int>' without the corresponding function 'extensionAct' returning 'Int' + \ No newline at end of file diff --git a/idea/testData/inspections/coroutines/resultIsSuccessOrFailure/test.kt b/idea/testData/inspections/coroutines/resultIsSuccessOrFailure/test.kt index d74649a3fde..0d127ea1e7c 100644 --- a/idea/testData/inspections/coroutines/resultIsSuccessOrFailure/test.kt +++ b/idea/testData/inspections/coroutines/resultIsSuccessOrFailure/test.kt @@ -4,18 +4,18 @@ class SuccessOrFailure(val value: T?) { fun getOrThrow(): T = value ?: throw AssertionError("") } // YES -fun getSuccess() = SuccessOrFailure("123") +fun getSuccess() = success() // YES fun getSuccessExplicit(): SuccessOrFailure = SuccessOrFailure(456) -// NO (noCatching available) +// NO (not catching 'correct' available) fun correctCatching() = SuccessOrFailure(true) // NO (not SuccessOrFailure) fun correct() = true // YES fun incorrectCatching() = SuccessOrFailure(3.14) // YES -fun strangeCatching() = SuccessOrFailure(false) -// YES +fun strangeCatching() = runCatching { false } +// NO (not SuccessOrFailure) fun strange() = 1 class Container { @@ -23,7 +23,7 @@ class Container { fun classGetSuccess() = SuccessOrFailure("123") // YES fun classGetSuccessExplicit(): SuccessOrFailure = SuccessOrFailure(456) - // NO (noCatching available) + // NO (not catching 'classCorrect' available) fun classCorrectCatching() = SuccessOrFailure(true) // NO (not SuccessOrFailure) fun classCorrect() = true @@ -38,6 +38,30 @@ fun test() { val anonymous = fun() = SuccessOrFailure(45) // YES val lambda = { SuccessOrFailure(true) } - // NO yet + // NO yet (we do not report local *catching functions) fun localCatching() = SuccessOrFailure(2.72) -} \ No newline at end of file +} + +// NO (stdlib) +fun success() = SuccessOrFailure(true) +// NO (stdlib) +fun failure() = SuccessOrFailure(false) +// NO (stdlib) +fun runCatching(block: () -> T) = SuccessOrFailure(block()) +// NO (stdlib) +fun SuccessOrFailure.id() = this + +class ClassWithExtension() { + // NO (not SuccessOrFailure) + fun calc() = 12345 + // NO (not SuccessOrFailure) + fun calcComplex(arg1: Int, arg2: Double): Double = arg1 + arg2 + // YES (different parameters) + fun calcComplexCatching() = SuccessOrFailure(0.0) +} +// NO (extension to calc) +fun ClassWithExtension.calcCatching() = SuccessOrFailure(calc()) +// NO (not SuccessOrFailure) +fun Container.extensionAct() = 42 +// YES (different extension receiver) +fun ClassWithExtension.extensionActCatching() = SuccessOrFailure(42) \ No newline at end of file diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt new file mode 100644 index 00000000000..3c3db3c7f7b --- /dev/null +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt @@ -0,0 +1,10 @@ +// FIX: Unwrap 'SuccessOrFailure' return type (breaks use-sites!) +package kotlin + +class SuccessOrFailure(val value: T?) { + fun getOrThrow(): T = value ?: throw AssertionError("") +} + +abstract class Abstract { + abstract fun foo(): SuccessOrFailure +} diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt.after b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt.after new file mode 100644 index 00000000000..6e5fc1805e0 --- /dev/null +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt.after @@ -0,0 +1,10 @@ +// FIX: Unwrap 'SuccessOrFailure' return type (breaks use-sites!) +package kotlin + +class SuccessOrFailure(val value: T?) { + fun getOrThrow(): T = value ?: throw AssertionError("") +} + +abstract class Abstract { + abstract fun foo(): Int +} diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/complexBlock.kt b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/complexBlock.kt index 1261d0f05d9..0f4d7455fc1 100644 --- a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/complexBlock.kt +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/complexBlock.kt @@ -1,3 +1,4 @@ +// FIX: Add '.getOrThrow()' to function result (breaks use-sites!) // WITH_RUNTIME package kotlin diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/complexBlock.kt.after b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/complexBlock.kt.after index b3702bad2f7..a52b32cf2f5 100644 --- a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/complexBlock.kt.after +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/complexBlock.kt.after @@ -1,3 +1,4 @@ +// FIX: Add '.getOrThrow()' to function result (breaks use-sites!) // WITH_RUNTIME package kotlin diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt index 56d81fbae7e..2f0c2434e1a 100644 --- a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt @@ -1,3 +1,4 @@ +// FIX: Add '.getOrThrow()' to function result (breaks use-sites!) package kotlin class SuccessOrFailure(val value: T?) { diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt.after b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt.after index 7b609262134..465d66ebf0a 100644 --- a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt.after +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt.after @@ -1,3 +1,4 @@ +// FIX: Add '.getOrThrow()' to function result (breaks use-sites!) package kotlin class SuccessOrFailure(val value: T?) { diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt new file mode 100644 index 00000000000..1128e002515 --- /dev/null +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt @@ -0,0 +1,8 @@ +// FIX: Rename to 'incorrectCatching' +package kotlin + +class SuccessOrFailure(val value: T?) { + fun getOrThrow(): T = value ?: throw AssertionError("") +} + +fun incorrect() = SuccessOrFailure("123") diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt.after b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt.after new file mode 100644 index 00000000000..e8d10496ddb --- /dev/null +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt.after @@ -0,0 +1,8 @@ +// FIX: Rename to 'incorrectCatching' +package kotlin + +class SuccessOrFailure(val value: T?) { + fun getOrThrow(): T = value ?: throw AssertionError("") +} + +fun incorrectCatching() = SuccessOrFailure("123") diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt index efbb712c002..728fef4971c 100644 --- a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt @@ -1,3 +1,4 @@ +// FIX: Add '.getOrThrow()' to function result (breaks use-sites!) package kotlin class SuccessOrFailure(val value: T?) { diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt.after b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt.after index d73256d6e9c..7a8e2795d11 100644 --- a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt.after +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt.after @@ -1,3 +1,4 @@ +// FIX: Add '.getOrThrow()' to function result (breaks use-sites!) package kotlin class SuccessOrFailure(val value: T?) { diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simpleBlock.kt b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simpleBlock.kt index d84e1b7fffa..b1b09e1904c 100644 --- a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simpleBlock.kt +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simpleBlock.kt @@ -1,3 +1,4 @@ +// FIX: Add '.getOrThrow()' to function result (breaks use-sites!) package kotlin class SuccessOrFailure(val value: T?) { diff --git a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simpleBlock.kt.after b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simpleBlock.kt.after index 09d2f05843a..ed2f4fb131c 100644 --- a/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simpleBlock.kt.after +++ b/idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simpleBlock.kt.after @@ -1,3 +1,4 @@ +// FIX: Add '.getOrThrow()' to function result (breaks use-sites!) package kotlin class SuccessOrFailure(val value: T?) { diff --git a/idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java b/idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java index 6309607f25b..f3e61fc2f61 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java +++ b/idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java @@ -1891,6 +1891,11 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest { KotlinTestUtils.runTest(this::doTest, TargetBackend.ANY, testDataFilePath); } + @TestMetadata("abstract.kt") + public void testAbstract() throws Exception { + runTest("idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/abstract.kt"); + } + public void testAllFilesPresentInResultIsSuccessOrFailure() throws Exception { KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure"), Pattern.compile("^([\\w\\-_]+)\\.(kt|kts)$"), TargetBackend.ANY, true); } @@ -1925,6 +1930,11 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest { runTest("idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/needParentheses.kt"); } + @TestMetadata("rename.kt") + public void testRename() throws Exception { + runTest("idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/rename.kt"); + } + @TestMetadata("simple.kt") public void testSimple() throws Exception { runTest("idea/testData/inspectionsLocal/coroutines/resultIsSuccessOrFailure/simple.kt");