Rewrite RedundantSamConstructorInspection to support more cases

- Now inspection does not rely on synthetic descriptors at all, instead it uses `SamConversionOracle` and `SamConversionResolver` to detect if the argument type support SAM conversion - This transparently considers all language features like `SAM conversions for kotlin functions`, `Functional interfaces` etc. - In case of multiple SAM arguments, part of them can be converted only when `SAM conversion per argument` is enabled - Fix inspection and nj2k tests that were failing because of better working inspection - Rewrite automatically fixes multiple bugs that were present - ^KT-36367 ^KT-36368 ^KT-36296 ^KT-36395 Fixed
2020-02-10 13:12:21 +03:00
parent 9b5110b9f3
commit 46ae6136cb
23 changed files with 248 additions and 65 deletions
@@ -11,18 +11,12 @@ import com.intellij.openapi.project.Project
 import com.intellij.psi.PsiElementVisitor
 import org.jetbrains.kotlin.codegen.SamCodegenUtil
 import org.jetbrains.kotlin.config.LanguageFeature
-import org.jetbrains.kotlin.descriptors.ClassDescriptor
-import org.jetbrains.kotlin.descriptors.FunctionDescriptor
 import org.jetbrains.kotlin.idea.caches.resolve.analyze
 import org.jetbrains.kotlin.idea.caches.resolve.getResolutionFacade
-import org.jetbrains.kotlin.idea.codeInsight.forceEnableSamAdapters
 import org.jetbrains.kotlin.idea.project.languageVersionSettings
 import org.jetbrains.kotlin.idea.resolve.frontendService
 import org.jetbrains.kotlin.idea.util.getResolutionScope
-import org.jetbrains.kotlin.incremental.components.NoLookupLocation
-import org.jetbrains.kotlin.load.java.sam.SamAdapterDescriptor
 import org.jetbrains.kotlin.load.java.sam.SamConstructorDescriptor
-import org.jetbrains.kotlin.load.java.sam.JavaSingleAbstractMethodUtils
 import org.jetbrains.kotlin.name.Name
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.anyDescendantOfType
@@ -33,16 +27,17 @@ import org.jetbrains.kotlin.resolve.BindingContext
 import org.jetbrains.kotlin.resolve.BindingTraceContext
 import org.jetbrains.kotlin.resolve.bindingContextUtil.getDataFlowInfoBefore
 import org.jetbrains.kotlin.resolve.calls.CallResolver
+import org.jetbrains.kotlin.resolve.calls.callUtil.getParameterForArgument
 import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
 import org.jetbrains.kotlin.resolve.calls.model.isReallySuccess
 import org.jetbrains.kotlin.resolve.calls.tower.NewResolvedCallImpl
 import org.jetbrains.kotlin.resolve.calls.util.DelegatingCall
 import org.jetbrains.kotlin.resolve.lazy.BodyResolveMode
-import org.jetbrains.kotlin.resolve.scopes.SyntheticScopes
-import org.jetbrains.kotlin.resolve.scopes.collectSyntheticMemberFunctions
-import org.jetbrains.kotlin.resolve.scopes.collectSyntheticStaticFunctions
-import org.jetbrains.kotlin.synthetic.SamAdapterExtensionFunctionDescriptor
-import org.jetbrains.kotlin.types.TypeUtils
+import org.jetbrains.kotlin.resolve.sam.SamConversionOracle
+import org.jetbrains.kotlin.resolve.sam.SamConversionResolver
+import org.jetbrains.kotlin.resolve.sam.getFunctionTypeForPossibleSamType
+import org.jetbrains.kotlin.types.*
+import org.jetbrains.kotlin.types.typeUtil.makeNotNullable
 import org.jetbrains.kotlin.utils.keysToMapExceptNulls

 class RedundantSamConstructorInspection : AbstractKotlinInspection() {
@@ -168,53 +163,60 @@ class RedundantSamConstructorInspection : AbstractKotlinInspection() {
            val valueArguments = functionCall.valueArguments
            if (valueArguments.none { canBeSamConstructorCall(it) }) return emptyList()

-            val bindingContext = functionCall.analyze(BodyResolveMode.PARTIAL)
+            val resolutionFacade = functionCall.getResolutionFacade()
+            val oracle = resolutionFacade.frontendService<SamConversionOracle>()
+            val resolver = resolutionFacade.frontendService<SamConversionResolver>()
+
+            val bindingContext = functionCall.analyze(resolutionFacade, BodyResolveMode.PARTIAL)
            val functionResolvedCall = functionCall.getResolvedCall(bindingContext) ?: return emptyList()
            if (!functionResolvedCall.isReallySuccess()) return emptyList()

-            val samConstructorCallArgumentMap = valueArguments.keysToMapExceptNulls { it.toCallExpression() }.filterValues { call ->
-                call.getResolvedCall(bindingContext)?.resultingDescriptor?.original is SamConstructorDescriptor
+            /**
+             * Checks that SAM conversion for [arg] and [call] in the argument position is possible
+             * and does not loose any information.
+             *
+             * We want to do as many cheap checks as possible before actually trying to resolve substituted call in [canBeReplaced].
+             *
+             * Several cases where we do not want the conversion:
+             *
+             * - Expected argument type is inferred from the argument; for example when the expected type is `T`, and SAM constructor
+             * helps to deduce it.
+             * - Expected argument type is a base type for the actual argument type; for example when expected type is `Any`, and removing
+             * SAM constructor will lead to passing object of different type.
+             */
+            fun samConversionIsPossible(arg: KtValueArgument, call: KtCallExpression): Boolean {
+                val samConstructor =
+                    call.getResolvedCall(bindingContext)?.resultingDescriptor?.original as? SamConstructorDescriptor ?: return false
+
+                // we suppose that SAM constructors return type is always not nullable
+                val samConstructorReturnType = samConstructor.returnType?.unwrap()?.takeUnless { it.isNullable() } ?: return false
+
+                // we take original parameter descriptor to get type parameter instead of inferred type (e.g. `T` instead of `Runnable`)
+                val originalParameterDescriptor = functionResolvedCall.getParameterForArgument(arg)?.original ?: return false
+                val expectedNotNullableType = originalParameterDescriptor.type.makeNotNullable().unwrap()
+
+                if (resolver.getFunctionTypeForPossibleSamType(expectedNotNullableType, oracle) == null) return false
+
+                return samConstructorReturnType.constructor == expectedNotNullableType.constructor
            }

-            if (samConstructorCallArgumentMap.isEmpty()) return emptyList()
-
-            if (samConstructorCallArgumentMap.values.any(::containsLabeledReturnPreventingConversion)) return emptyList()
-
-            val originalFunctionDescriptor = functionResolvedCall.resultingDescriptor.original as? FunctionDescriptor ?: return emptyList()
-            val containingClass = originalFunctionDescriptor.containingDeclaration as? ClassDescriptor ?: return emptyList()
-
-            val syntheticScopes =
-                functionCall.getResolutionFacade().getFrontendService(SyntheticScopes::class.java).forceEnableSamAdapters()
-
-            // SAM adapters for static functions
-            val contributedFunctions = syntheticScopes.collectSyntheticStaticFunctions(containingClass.staticScope)
-            for (staticFunWithSameName in contributedFunctions) {
-                if (staticFunWithSameName is SamAdapterDescriptor<*>) {
-                    if (isSamAdapterSuitableForCall(
-                            staticFunWithSameName,
-                            originalFunctionDescriptor,
-                            samConstructorCallArgumentMap.size
-                        )
-                    ) {
-                        return samConstructorCallArgumentMap.takeIf { canBeReplaced(functionCall, it) }?.values ?: emptyList()
-                    }
-                }
+            val argumentsWithSamConstructors = valueArguments.keysToMapExceptNulls { arg ->
+                arg.toCallExpression()?.takeIf { call -> samConversionIsPossible(arg, call) }
            }

-            // SAM adapters for member functions
-            val syntheticExtensions = syntheticScopes.collectSyntheticMemberFunctions(
-                listOf(containingClass.defaultType),
-                functionResolvedCall.resultingDescriptor.name,
-                NoLookupLocation.FROM_IDE
-            )
-            for (syntheticExtension in syntheticExtensions) {
-                val samAdapter = syntheticExtension as? SamAdapterExtensionFunctionDescriptor ?: continue
-                if (isSamAdapterSuitableForCall(samAdapter, originalFunctionDescriptor, samConstructorCallArgumentMap.size)) {
-                    return samConstructorCallArgumentMap.takeIf { canBeReplaced(functionCall, it) }?.values ?: emptyList()
-                }
+            val haveToConvertAllArguments = !functionCall.languageVersionSettings.supportsFeature(LanguageFeature.SamConversionPerArgument)
+
+            val argumentsThatCanBeConverted = if (haveToConvertAllArguments) {
+                argumentsWithSamConstructors.takeIf { it.values.none(::containsLabeledReturnPreventingConversion) }.orEmpty()
+            } else {
+                argumentsWithSamConstructors.filterValues { !containsLabeledReturnPreventingConversion(it) }
            }

-            return emptyList()
+            return when {
+                argumentsThatCanBeConverted.isEmpty() -> emptyList()
+                !canBeReplaced(functionCall, argumentsThatCanBeConverted) -> emptyList()
+                else -> argumentsThatCanBeConverted.values
+            }
        }

        private fun canBeSamConstructorCall(argument: KtValueArgument) = argument.toCallExpression()?.samConstructorValueArgument() != null
@@ -231,18 +233,6 @@ class RedundantSamConstructorInspection : AbstractKotlinInspection() {
                argumentExpression) as? KtCallExpression
        }

-        private fun isSamAdapterSuitableForCall(
-            samAdapter: FunctionDescriptor,
-            originalFunction: FunctionDescriptor,
-            samConstructorsCount: Int
-        ): Boolean {
-            val samAdapterOriginalFunction = SamCodegenUtil.getOriginalIfSamAdapter(samAdapter)?.original
-            if (samAdapterOriginalFunction != originalFunction) return false
-
-            val parametersWithSamTypeCount = originalFunction.valueParameters.count { JavaSingleAbstractMethodUtils.isSamType(it.type) }
-            return parametersWithSamTypeCount == samConstructorsCount
-        }
-
        private fun containsLabeledReturnPreventingConversion(it: KtCallExpression): Boolean {
            val samValueArgument = it.samConstructorValueArgument()
            val samConstructorName = (it.calleeExpression as? KtSimpleNameExpression)?.getReferencedNameAsName()
@@ -103,6 +103,17 @@
        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Redundant SAM-constructor</problem_class>
        <description>Redundant SAM-constructor</description>
    </problem>
+    <problem>
+        <file>otherGenericsInParams.kt</file>
+        <line>7</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="method" FQNAME="redundantSamConstructor.OtherGenericsInParamsKt void test()" />
+        <problem_class severity="WARNING" attribute_key="NOT_USED_ELEMENT_ATTRIBUTES">Redundant SAM constructor</problem_class>
+        <description>Redundant SAM-constructor</description>
+        <highlighted_element>Runnable</highlighted_element>
+        <offset>21</offset>
+        <length>8</length>
+    </problem>
    <problem>
        <file>otherGenericsInParams.kt</file>
        <line>8</line>
@@ -111,6 +122,17 @@
        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Redundant SAM-constructor</problem_class>
        <description>Redundant SAM-constructor</description>
    </problem>
+    <problem>
+        <file>otherGenericsInParams.kt</file>
+        <line>9</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="method" FQNAME="redundantSamConstructor.OtherGenericsInParamsKt void test()" />
+        <problem_class severity="WARNING" attribute_key="NOT_USED_ELEMENT_ATTRIBUTES">Redundant SAM constructor</problem_class>
+        <description>Redundant SAM-constructor</description>
+        <highlighted_element>Runnable</highlighted_element>
+        <offset>35</offset>
+        <length>8</length>
+    </problem>
    <problem>
        <file>otherGenericsInParams.kt</file>
        <line>11</line>
@@ -4,9 +4,9 @@ import a.*

 fun test() {
    MyJavaClass.foo1({ }, 1)
-    MyJavaClass.foo1(Runnable { }, Runnable { })
+    MyJavaClass.foo1({ }, Runnable { })
    MyJavaClass.foo2(1, { })
-    MyJavaClass.foo2(Runnable { }, Runnable { })
+    MyJavaClass.foo2(Runnable { }, { })

    A<String>().foo({})
 }
@@ -0,0 +1,3 @@
+public class Test {
+    static public void test(Runnable r1, Runnable r2) {}
+}
@@ -0,0 +1,5 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion -XXLanguage:+SamConversionPerArgument
+
+fun usage(r1: Runnable) {
+    Test.test(r1, Runnable<caret> {})
+}
@@ -0,0 +1,5 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion -XXLanguage:+SamConversionPerArgument
+
+fun usage(r1: Runnable) {
+    Test.test(r1, {})
+}
@@ -0,0 +1,7 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion -XXLanguage:+SamConversionPerArgument
+
+fun test(r1: Runnable, r2: Runnable) {}
+
+fun usage(r1: Runnable) {
+    test(r1, Runnable<caret> {})
+}
@@ -0,0 +1,7 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion -XXLanguage:+SamConversionPerArgument
+
+fun test(r1: Runnable, r2: Runnable) {}
+
+fun usage(r1: Runnable) {
+    test(r1, {})
+}
@@ -0,0 +1,11 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion -XXLanguage:+SamConversionPerArgument
+
+fun interface KtRunnable {
+    fun run()
+}
+
+fun test(r1: KtRunnable, r2: KtRunnable) {}
+
+fun usage(r1: KtRunnable) {
+    test(r1, KtRunnable<caret> {})
+}
@@ -0,0 +1,11 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion -XXLanguage:+SamConversionPerArgument
+
+fun interface KtRunnable {
+    fun run()
+}
+
+fun test(r1: KtRunnable, r2: KtRunnable) {}
+
+fun usage(r1: KtRunnable) {
+    test(r1, {})
+}
@@ -0,0 +1,7 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion -XXLanguage:+SamConversionPerArgument
+
+fun test(r1: Runnable, r2: Runnable) {}
+
+fun usage() {
+    test(Runnable { return@Runnable }, Runnable<caret> {})
+}
@@ -0,0 +1,7 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion -XXLanguage:+SamConversionPerArgument
+
+fun test(r1: Runnable, r2: Runnable) {}
+
+fun usage() {
+    test(Runnable { return@Runnable }, {})
+}
@@ -0,0 +1,8 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions
+// PROBLEM: none
+
+fun <T> test(t: T): T = t
+
+fun usage() {
+    test(Runnable<caret> {})
+}
@@ -0,0 +1,14 @@
+interface Base {
+    void test1();
+}
+
+interface Extender extends Base {
+    @java.lang.Override
+    default void test1() { test2(); }
+
+    void test2();
+}
+
+class Taker {
+    static void take(Base b) {}
+}
@@ -0,0 +1,5 @@
+// PROBLEM: none
+
+fun usage() {
+    Taker.take(Extender<caret> {})
+}
@@ -0,0 +1,13 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion
+
+fun interface KtRunnable {
+    fun run()
+}
+
+class Test {
+    fun usage(r: KtRunnable) {}
+
+    fun test() {
+        usage(KtRunnable<caret> { })
+    }
+}
@@ -0,0 +1,13 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions -XXLanguage:+FunctionalInterfaceConversion
+
+fun interface KtRunnable {
+    fun run()
+}
+
+class Test {
+    fun usage(r: KtRunnable) {}
+
+    fun test() {
+        usage({ })
+    }
+}
@@ -0,0 +1,7 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions
+
+fun usage(r: Runnable) {}
+
+fun test() {
+    usage(Runnable<caret> { })
+}
@@ -0,0 +1,7 @@
+// COMPILER_ARGUMENTS: -XXLanguage:+NewInference -XXLanguage:+SamConversionForKotlinFunctions
+
+fun usage(r: Runnable) {}
+
+fun test() {
+    usage({ })
+}
@@ -7881,6 +7881,41 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/conversionPerArgumentDisabled3.kt");
        }

+        @TestMetadata("conversionPerArgumentEnabled1.kt")
+        public void testConversionPerArgumentEnabled1() throws Exception {
+            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/conversionPerArgumentEnabled1.kt");
+        }
+
+        @TestMetadata("conversionPerArgumentEnabled2.kt")
+        public void testConversionPerArgumentEnabled2() throws Exception {
+            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/conversionPerArgumentEnabled2.kt");
+        }
+
+        @TestMetadata("conversionPerArgumentEnabled3.kt")
+        public void testConversionPerArgumentEnabled3() throws Exception {
+            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/conversionPerArgumentEnabled3.kt");
+        }
+
+        @TestMetadata("conversionPerArgumentEnabled4.kt")
+        public void testConversionPerArgumentEnabled4() throws Exception {
+            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/conversionPerArgumentEnabled4.kt");
+        }
+
+        @TestMetadata("genericParameter.kt")
+        public void testGenericParameter() throws Exception {
+            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/genericParameter.kt");
+        }
+
+        @TestMetadata("javaInterfacesHierarchy.kt")
+        public void testJavaInterfacesHierarchy() throws Exception {
+            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/javaInterfacesHierarchy.kt");
+        }
+
+        @TestMetadata("kotlinFunInterface.kt")
+        public void testKotlinFunInterface() throws Exception {
+            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/kotlinFunInterface.kt");
+        }
+
        @TestMetadata("labeledReturn1.kt")
        public void testLabeledReturn1() throws Exception {
            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/labeledReturn1.kt");
@@ -7920,6 +7955,11 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
        public void testSimple2() throws Exception {
            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/simple2.kt");
        }
+
+        @TestMetadata("simple3.kt")
+        public void testSimple3() throws Exception {
+            runTest("idea/testData/inspectionsLocal/redundantSamConstructor/simple3.kt");
+        }
    }

    @TestMetadata("idea/testData/inspectionsLocal/redundantSemicolon")
@@ -41,6 +41,7 @@ dependencies {
    testRuntime(project(":sam-with-receiver-ide-plugin"))
    testRuntime(project(":allopen-ide-plugin"))
    testRuntime(project(":noarg-ide-plugin"))
+    testRuntime(project(":kotlin-reflect"))
    testRuntime(project(":kotlin-scripting-idea"))
    testRuntime(project(":kotlinx-serialization-ide-plugin"))
    testRuntime(intellijPluginDep("properties"))
@@ -118,7 +118,7 @@ internal class Java8Class {
        val memberFunFromClass = JFunction2<String, Int> { it.length }
        memberFunFromClass.foo("str")

-        Thread(Runnable { println() }).start()
+        Thread { println() }.start()
        Runnable { println() }.run()
    }

@@ -106,7 +106,7 @@ internal class Java8Class {
    fun testLibraryFunctions() {
        val memberFunFromClass = JFunction2 { obj: String -> obj.length }
        memberFunFromClass.foo("str")
-        Thread(Runnable { println() }).start()
+        Thread { println() }.start()
        Runnable { println() }.run()
    }