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.
2018-08-24 13:17:40 +03:00
parent 7a2d0a3bb2
commit c12f29ae30
16 changed files with 185 additions and 22 deletions
@@ -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<ValueParameterDescriptor>,
+        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<KtFunction>() ?: 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")
    }
 }
@@ -89,4 +89,22 @@
    <problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Function returning SuccessOrFailure</problem_class>
    <description>Function returning SuccessOrFailure with a name that does not end with Catching</description>
  </problem>
+  <problem>
+    <file>test.kt</file>
+    <line>60</line>
+    <module>light_idea_test_case</module>
+    <package>&lt;default&gt;</package>
+    <entry_point TYPE="file" FQNAME="temp:///src/test.kt" />
+    <problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Function returning SuccessOrFailure</problem_class>
+    <description>Function 'calcComplexCatching' returning 'SuccessOrFailure&lt;Double&gt;' without the corresponding function 'calcComplex' returning 'Double'</description>
+  </problem>
+  <problem>
+    <file>test.kt</file>
+    <line>67</line>
+    <module>light_idea_test_case</module>
+    <package>&lt;default&gt;</package>
+    <entry_point TYPE="file" FQNAME="temp:///src/test.kt" />
+    <problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Function returning SuccessOrFailure</problem_class>
+    <description>Function 'extensionActCatching' returning 'SuccessOrFailure&lt;Int&gt;' without the corresponding function 'extensionAct' returning 'Int'</description>
+  </problem>
 </problems>
@@ -4,18 +4,18 @@ class SuccessOrFailure<T>(val value: T?) {
    fun getOrThrow(): T = value ?: throw AssertionError("")
 }
 // YES
-fun getSuccess() = SuccessOrFailure("123")
+fun getSuccess() = success()
 // YES
 fun getSuccessExplicit(): SuccessOrFailure<Int> = 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<Int> = 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 (stdlib)
+fun success() = SuccessOrFailure(true)
+// NO (stdlib)
+fun failure() = SuccessOrFailure(false)
+// NO (stdlib)
+fun <T> runCatching(block: () -> T) = SuccessOrFailure(block())
+// NO (stdlib)
+fun <T> SuccessOrFailure<T>.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)
@@ -0,0 +1,10 @@
+// FIX: Unwrap 'SuccessOrFailure' return type (breaks use-sites!)
+package kotlin
+
+class SuccessOrFailure<T>(val value: T?) {
+    fun getOrThrow(): T = value ?: throw AssertionError("")
+}
+
+abstract class Abstract {
+    abstract fun <caret>foo(): SuccessOrFailure<Int>
+}
@@ -0,0 +1,10 @@
+// FIX: Unwrap 'SuccessOrFailure' return type (breaks use-sites!)
+package kotlin
+
+class SuccessOrFailure<T>(val value: T?) {
+    fun getOrThrow(): T = value ?: throw AssertionError("")
+}
+
+abstract class Abstract {
+    abstract fun foo(): Int
+}
@@ -1,3 +1,4 @@
+// FIX: Add '.getOrThrow()' to function result (breaks use-sites!)
 // WITH_RUNTIME

 package kotlin
@@ -1,3 +1,4 @@
+// FIX: Add '.getOrThrow()' to function result (breaks use-sites!)
 // WITH_RUNTIME

 package kotlin
@@ -1,3 +1,4 @@
+// FIX: Add '.getOrThrow()' to function result (breaks use-sites!)
 package kotlin

 class SuccessOrFailure<T>(val value: T?) {
@@ -1,3 +1,4 @@
+// FIX: Add '.getOrThrow()' to function result (breaks use-sites!)
 package kotlin

 class SuccessOrFailure<T>(val value: T?) {
@@ -0,0 +1,8 @@
+// FIX: Rename to 'incorrectCatching'
+package kotlin
+
+class SuccessOrFailure<T>(val value: T?) {
+    fun getOrThrow(): T = value ?: throw AssertionError("")
+}
+
+fun <caret>incorrect() = SuccessOrFailure("123")
@@ -0,0 +1,8 @@
+// FIX: Rename to 'incorrectCatching'
+package kotlin
+
+class SuccessOrFailure<T>(val value: T?) {
+    fun getOrThrow(): T = value ?: throw AssertionError("")
+}
+
+fun <caret>incorrectCatching() = SuccessOrFailure("123")
@@ -1,3 +1,4 @@
+// FIX: Add '.getOrThrow()' to function result (breaks use-sites!)
 package kotlin

 class SuccessOrFailure<T>(val value: T?) {
@@ -1,3 +1,4 @@
+// FIX: Add '.getOrThrow()' to function result (breaks use-sites!)
 package kotlin

 class SuccessOrFailure<T>(val value: T?) {
@@ -1,3 +1,4 @@
+// FIX: Add '.getOrThrow()' to function result (breaks use-sites!)
 package kotlin

 class SuccessOrFailure<T>(val value: T?) {
@@ -1,3 +1,4 @@
+// FIX: Add '.getOrThrow()' to function result (breaks use-sites!)
 package kotlin

 class SuccessOrFailure<T>(val value: T?) {
@@ -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");