KT-13943 related: inspection is now always reported if manual destructuring is available

idea/src/org/jetbrains/kotlin/idea/intentions/DestructureIntention.kt

@@ -45,8 +45,12 @@ import java.util.*
 
 class DestructureInspection : IntentionBasedInspection<KtDeclaration>(
         DestructureIntention::class,
-        additionalChecker = { declaration ->
+        { element, inspection ->
-            declaration !is KtVariableDeclaration && declaration !is KtFunctionLiteral
+            if (element is KtParameter) true
+            else with(inspection as DestructureInspection) {
+                val usagesToRemove = DestructureIntention.collectUsagesToRemove(element, element.parent as? KtForExpression)?.data
+                usagesToRemove != null && usagesToRemove.any { it.declarationToDrop is KtDestructuringDeclaration }
+            }
         }
 )
 
@@ -134,202 +138,205 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
         else -> false
     }
 
-    private data class UsagesToRemove(val data: List<UsageData>, val removeSelectorInLoopRange: Boolean)
+    companion object {
 
-    private fun collectUsagesToRemove(declaration: KtDeclaration, forLoop: KtForExpression?): UsagesToRemove? {
+        internal data class UsagesToRemove(val data: List<UsageData>, val removeSelectorInLoopRange: Boolean)
-        val context = declaration.analyze()
 
-        val variableDescriptor = when (declaration) {
+        internal fun collectUsagesToRemove(declaration: KtDeclaration, forLoop: KtForExpression?): UsagesToRemove? {
-            is KtParameter -> context.get(BindingContext.VALUE_PARAMETER, declaration)
+            val context = declaration.analyze()
-            is KtFunctionLiteral -> context.get(BindingContext.FUNCTION, declaration)?.valueParameters?.singleOrNull()
-            is KtVariableDeclaration -> context.get(BindingContext.VARIABLE, declaration)
-            else -> null
-        } ?: return null
 
-        val variableType = variableDescriptor.type
+            val variableDescriptor = when (declaration) {
-        if (variableType.isMarkedNullable) return null
+                                         is KtParameter -> context.get(BindingContext.VALUE_PARAMETER, declaration)
-        val classDescriptor = variableType.constructor.declarationDescriptor as? ClassDescriptor ?: return null
+                                         is KtFunctionLiteral -> context.get(BindingContext.FUNCTION, declaration)?.valueParameters?.singleOrNull()
+                                         is KtVariableDeclaration -> context.get(BindingContext.VARIABLE, declaration)
+                                         else -> null
+                                     } ?: return null
 
-        val mapEntryClassDescriptor = classDescriptor.builtIns.mapEntry
+            val variableType = variableDescriptor.type
+            if (variableType.isMarkedNullable) return null
+            val classDescriptor = variableType.constructor.declarationDescriptor as? ClassDescriptor ?: return null
 
-        // Note: list should contains properties in order to create destructuring declaration
+            val mapEntryClassDescriptor = classDescriptor.builtIns.mapEntry
-        val usagesToRemove = mutableListOf<UsageData>()
-        var noBadUsages = true
-        val removeSelectorInLoopRange: Boolean
-        if (forLoop != null && DescriptorUtils.isSubclass(classDescriptor, mapEntryClassDescriptor)) {
-            val loopRangeDescriptorName = forLoop.loopRange.getResolvedCall(context)?.resultingDescriptor?.name
-            removeSelectorInLoopRange = loopRangeDescriptorName?.asString().let { it == "entries" || it == "entrySet" }
 
-            listOf("key", "value").mapTo(usagesToRemove) {
+            // Note: list should contains properties in order to create destructuring declaration
-                UsageData(mapEntryClassDescriptor.unsubstitutedMemberScope.getContributedVariables(
+            val usagesToRemove = mutableListOf<UsageData>()
-                        Name.identifier(it), NoLookupLocation.FROM_BUILTINS).single())
+            var noBadUsages = true
-            }
+            val removeSelectorInLoopRange: Boolean
+            if (forLoop != null && DescriptorUtils.isSubclass(classDescriptor, mapEntryClassDescriptor)) {
+                val loopRangeDescriptorName = forLoop.loopRange.getResolvedCall(context)?.resultingDescriptor?.name
+                removeSelectorInLoopRange = loopRangeDescriptorName?.asString().let { it == "entries" || it == "entrySet" }
 
-            ReferencesSearch.search(declaration).iterateOverMapEntryPropertiesUsages(
+                listOf("key", "value").mapTo(usagesToRemove) {
-                    context,
+                    UsageData(mapEntryClassDescriptor.unsubstitutedMemberScope.getContributedVariables(
-                    { index, usageData -> noBadUsages = usagesToRemove[index].add(usageData, index) && noBadUsages },
+                            Name.identifier(it), NoLookupLocation.FROM_BUILTINS).single())
-                    { noBadUsages = false }
-            )
-        }
-        else if (classDescriptor.isData) {
-            removeSelectorInLoopRange = false
-
-            val valueParameters = classDescriptor.unsubstitutedPrimaryConstructor?.valueParameters ?: return null
-            valueParameters.mapTo(usagesToRemove) { UsageData(it) }
-
-            // inference bug: remove as? PsiElement when fixed
-            val usageScopeElement: PsiElement = forLoop as? PsiElement
-                                                ?: (declaration as? KtFunctionLiteral)
-                                                ?: (declaration.parent?.parent as? KtFunctionLiteral)
-                                                ?: (declaration as? KtVariableDeclaration)?.parent
-                                                ?: return null
-
-            val nameToSearch = when (declaration) {
-                is KtParameter -> declaration.nameAsName
-                is KtVariableDeclaration -> declaration.nameAsName
-                else -> Name.identifier("it")
-            } ?: return null
-
-            val descriptorToIndex = mutableMapOf<CallableDescriptor, Int>()
-            for (valueParameter in valueParameters) {
-                val propertyDescriptor = context.get(BindingContext.VALUE_PARAMETER_AS_PROPERTY, valueParameter) ?: continue
-                descriptorToIndex[propertyDescriptor] = valueParameter.index
-            }
-            usageScopeElement.iterateOverDataClassPropertiesUsagesWithIndex(
-                    context,
-                    nameToSearch,
-                    descriptorToIndex,
-                    { index, usageData -> noBadUsages = usagesToRemove[index].add(usageData, index) && noBadUsages },
-                    { noBadUsages = false }
-            )
-        }
-        else {
-            return null
-        }
-        if (!noBadUsages) return null
-
-        val droppedLastUnused = usagesToRemove.dropLastWhile { it.usagesToReplace.isEmpty() && it.declarationToDrop == null }
-        return UsagesToRemove(droppedLastUnused, removeSelectorInLoopRange)
-    }
-
-    private fun Query<PsiReference>.iterateOverMapEntryPropertiesUsages(
-            context: BindingContext,
-            process: (Int, SingleUsageData) -> Unit,
-            cancel: () -> Unit
-    ) {
-        // TODO: Remove SAM-constructor when KT-11265 will be fixed
-        forEach(Processor forEach@{
-            val applicableUsage = getDataIfUsageIsApplicable(it, context)
-            if (applicableUsage != null) {
-                val usageDescriptor = applicableUsage.descriptor
-                if (usageDescriptor == null) {
-                    process(0, applicableUsage)
-                    process(1, applicableUsage)
-                    return@forEach true
-                }
-                when (usageDescriptor.name.asString()) {
-                    "key", "getKey" -> {
-                        process(0, applicableUsage)
-                        return@forEach true
-                    }
-                    "value", "getValue" -> {
-                        process(1, applicableUsage)
-                        return@forEach true
-                    }
                 }
+
+                ReferencesSearch.search(declaration).iterateOverMapEntryPropertiesUsages(
+                        context,
+                        { index, usageData -> noBadUsages = usagesToRemove[index].add(usageData, index) && noBadUsages },
+                        { noBadUsages = false }
+                )
             }
+            else if (classDescriptor.isData) {
+                removeSelectorInLoopRange = false
 
-            cancel()
+                val valueParameters = classDescriptor.unsubstitutedPrimaryConstructor?.valueParameters ?: return null
-            return@forEach false
+                valueParameters.mapTo(usagesToRemove) { UsageData(it) }
-        })
-    }
 
-    private fun PsiElement.iterateOverDataClassPropertiesUsagesWithIndex(
+                // inference bug: remove as? PsiElement when fixed
-            context: BindingContext,
+                val usageScopeElement: PsiElement = forLoop as? PsiElement
-            parameterName: Name,
+                                                    ?: (declaration as? KtFunctionLiteral)
-            descriptorToIndex: Map<CallableDescriptor, Int>,
+                                                    ?: (declaration.parent?.parent as? KtFunctionLiteral)
-            process: (Int, SingleUsageData) -> Unit,
+                                                    ?: (declaration as? KtVariableDeclaration)?.parent
-            cancel: () -> Unit
+                                                    ?: return null
-    ) {
+
-        anyDescendantOfType<KtNameReferenceExpression> {
+                val nameToSearch = when (declaration) {
-            if (it.getReferencedNameAsName() != parameterName) false
+                                       is KtParameter -> declaration.nameAsName
+                                       is KtVariableDeclaration -> declaration.nameAsName
+                                       else -> Name.identifier("it")
+                                   } ?: return null
+
+                val descriptorToIndex = mutableMapOf<CallableDescriptor, Int>()
+                for (valueParameter in valueParameters) {
+                    val propertyDescriptor = context.get(BindingContext.VALUE_PARAMETER_AS_PROPERTY, valueParameter) ?: continue
+                    descriptorToIndex[propertyDescriptor] = valueParameter.index
+                }
+                usageScopeElement.iterateOverDataClassPropertiesUsagesWithIndex(
+                        context,
+                        nameToSearch,
+                        descriptorToIndex,
+                        { index, usageData -> noBadUsages = usagesToRemove[index].add(usageData, index) && noBadUsages },
+                        { noBadUsages = false }
+                )
+            }
             else {
+                return null
+            }
+            if (!noBadUsages) return null
+
+            val droppedLastUnused = usagesToRemove.dropLastWhile { it.usagesToReplace.isEmpty() && it.declarationToDrop == null }
+            return UsagesToRemove(droppedLastUnused, removeSelectorInLoopRange)
+        }
+
+        private fun Query<PsiReference>.iterateOverMapEntryPropertiesUsages(
+                context: BindingContext,
+                process: (Int, SingleUsageData) -> Unit,
+                cancel: () -> Unit
+        ) {
+            // TODO: Remove SAM-constructor when KT-11265 will be fixed
+            forEach(Processor forEach@{
                 val applicableUsage = getDataIfUsageIsApplicable(it, context)
                 if (applicableUsage != null) {
                     val usageDescriptor = applicableUsage.descriptor
                     if (usageDescriptor == null) {
-                        for (index in descriptorToIndex.values) {
+                        process(0, applicableUsage)
-                            process(index, applicableUsage)
+                        process(1, applicableUsage)
-                        }
+                        return@forEach true
-                        return@anyDescendantOfType false
                     }
-                    val index = descriptorToIndex[usageDescriptor]
+                    when (usageDescriptor.name.asString()) {
-                    if (index != null) {
+                        "key", "getKey" -> {
-                        process(index, applicableUsage)
+                            process(0, applicableUsage)
-                        return@anyDescendantOfType false
+                            return@forEach true
+                        }
+                        "value", "getValue" -> {
+                            process(1, applicableUsage)
+                            return@forEach true
+                        }
                     }
                 }
 
                 cancel()
-                true
+                return@forEach false
-            }
+            })
-        }
-    }
-
-    private fun getDataIfUsageIsApplicable(dataClassUsage: PsiReference, context: BindingContext) =
-            (dataClassUsage.element as? KtReferenceExpression)?.let { getDataIfUsageIsApplicable(it, context) }
-
-    private fun getDataIfUsageIsApplicable(dataClassUsage: KtReferenceExpression, context: BindingContext): SingleUsageData? {
-        val destructuringDecl = dataClassUsage.parent as? KtDestructuringDeclaration
-        if (destructuringDecl != null && destructuringDecl.initializer == dataClassUsage) {
-            return SingleUsageData(descriptor = null, usageToReplace = null, declarationToDrop = destructuringDecl)
-        }
-        val qualifiedExpression = dataClassUsage.getQualifiedExpressionForReceiver() ?: return null
-        val parent = qualifiedExpression.parent
-        when (parent) {
-            is KtBinaryExpression -> {
-                if (parent.operationToken in KtTokens.ALL_ASSIGNMENTS && parent.left == qualifiedExpression) return null
-            }
-            is KtUnaryExpression -> {
-                if (parent.operationToken == KtTokens.PLUSPLUS || parent.operationToken == KtTokens.MINUSMINUS) return null
-            }
         }
 
-        val property = parent as? KtProperty // val x = d.y
+        private fun PsiElement.iterateOverDataClassPropertiesUsagesWithIndex(
-        if (property != null && property.isVar) return null
+                context: BindingContext,
+                parameterName: Name,
+                descriptorToIndex: Map<CallableDescriptor, Int>,
+                process: (Int, SingleUsageData) -> Unit,
+                cancel: () -> Unit
+        ) {
+            anyDescendantOfType<KtNameReferenceExpression> {
+                if (it.getReferencedNameAsName() != parameterName) false
+                else {
+                    val applicableUsage = getDataIfUsageIsApplicable(it, context)
+                    if (applicableUsage != null) {
+                        val usageDescriptor = applicableUsage.descriptor
+                        if (usageDescriptor == null) {
+                            for (index in descriptorToIndex.values) {
+                                process(index, applicableUsage)
+                            }
+                            return@anyDescendantOfType false
+                        }
+                        val index = descriptorToIndex[usageDescriptor]
+                        if (index != null) {
+                            process(index, applicableUsage)
+                            return@anyDescendantOfType false
+                        }
+                    }
 
-        val descriptor = qualifiedExpression.getResolvedCall(context)?.resultingDescriptor ?: return null
+                    cancel()
-        return SingleUsageData(descriptor = descriptor, usageToReplace = qualifiedExpression, declarationToDrop = property)
+                    true
-    }
-
-    private data class SingleUsageData(
-            val descriptor: CallableDescriptor?,
-            val usageToReplace: KtExpression?,
-            val declarationToDrop: KtDeclaration?
-    )
-
-    private data class UsageData(
-            val descriptor: CallableDescriptor,
-            val usagesToReplace: MutableList<KtExpression> = mutableListOf(),
-            var declarationToDrop: KtDeclaration? = null,
-            var name: String? = declarationToDrop?.name
-    ) {
-        // Returns true if data is successfully added, false otherwise
-        fun add(newData: SingleUsageData, componentIndex: Int): Boolean {
-            if (newData.declarationToDrop is KtDestructuringDeclaration) {
-                val destructuringEntries = newData.declarationToDrop.entries
-                if (componentIndex < destructuringEntries.size) {
-                    if (declarationToDrop != null) return false
-                    name = destructuringEntries[componentIndex].name ?: return false
-                    declarationToDrop = newData.declarationToDrop
                 }
             }
-            else {
+        }
-                name = name ?: newData.declarationToDrop?.name
+
-                declarationToDrop = declarationToDrop ?: newData.declarationToDrop
+        private fun getDataIfUsageIsApplicable(dataClassUsage: PsiReference, context: BindingContext) =
+                (dataClassUsage.element as? KtReferenceExpression)?.let { getDataIfUsageIsApplicable(it, context) }
+
+        private fun getDataIfUsageIsApplicable(dataClassUsage: KtReferenceExpression, context: BindingContext): SingleUsageData? {
+            val destructuringDecl = dataClassUsage.parent as? KtDestructuringDeclaration
+            if (destructuringDecl != null && destructuringDecl.initializer == dataClassUsage) {
+                return SingleUsageData(descriptor = null, usageToReplace = null, declarationToDrop = destructuringDecl)
+            }
+            val qualifiedExpression = dataClassUsage.getQualifiedExpressionForReceiver() ?: return null
+            val parent = qualifiedExpression.parent
+            when (parent) {
+                is KtBinaryExpression -> {
+                    if (parent.operationToken in KtTokens.ALL_ASSIGNMENTS && parent.left == qualifiedExpression) return null
+                }
+                is KtUnaryExpression -> {
+                    if (parent.operationToken == KtTokens.PLUSPLUS || parent.operationToken == KtTokens.MINUSMINUS) return null
+                }
+            }
+
+            val property = parent as? KtProperty // val x = d.y
+            if (property != null && property.isVar) return null
+
+            val descriptor = qualifiedExpression.getResolvedCall(context)?.resultingDescriptor ?: return null
+            return SingleUsageData(descriptor = descriptor, usageToReplace = qualifiedExpression, declarationToDrop = property)
+        }
+
+        internal data class SingleUsageData(
+                val descriptor: CallableDescriptor?,
+                val usageToReplace: KtExpression?,
+                val declarationToDrop: KtDeclaration?
+        )
+
+        internal data class UsageData(
+                val descriptor: CallableDescriptor,
+                val usagesToReplace: MutableList<KtExpression> = mutableListOf(),
+                var declarationToDrop: KtDeclaration? = null,
+                var name: String? = declarationToDrop?.name
+        ) {
+            // Returns true if data is successfully added, false otherwise
+            fun add(newData: SingleUsageData, componentIndex: Int): Boolean {
+                if (newData.declarationToDrop is KtDestructuringDeclaration) {
+                    val destructuringEntries = newData.declarationToDrop.entries
+                    if (componentIndex < destructuringEntries.size) {
+                        if (declarationToDrop != null) return false
+                        name = destructuringEntries[componentIndex].name ?: return false
+                        declarationToDrop = newData.declarationToDrop
+                    }
+                }
+                else {
+                    name = name ?: newData.declarationToDrop?.name
+                    declarationToDrop = declarationToDrop ?: newData.declarationToDrop
+                }
+                newData.usageToReplace?.let { usagesToReplace.add(it) }
+                return true
             }
-            newData.usageToReplace?.let { usagesToReplace.add(it) }
-            return true
         }
     }
 }

idea/testData/intentions/destructuringInLambda/inspectionData/expected.xml

@@ -0,0 +1,66 @@
+<problems>
+    <problem>
+        <file>dependentLocal.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/dependentLocal.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>firstProperties.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/firstProperties.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>fold.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/fold.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>last.kt</file>
+        <line>6</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/last.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>list.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/list.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>noItWithDestructuring.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/noItWithDestructuring.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>simple.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/simple.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>variables.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/variables.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+</problems>

idea/testData/intentions/destructuringInLambda/inspectionData/inspections.test

@@ -0,0 +1 @@
+// INSPECTION_CLASS: org.jetbrains.kotlin.idea.intentions.DestructureInspection

idea/tests/org/jetbrains/kotlin/idea/codeInsight/InspectionTestGenerated.java

@@ -67,6 +67,12 @@ public class InspectionTestGenerated extends AbstractInspectionTest {
             doTest(fileName);
         }
 
+        @TestMetadata("destructuringInLambda/inspectionData/inspections.test")
+        public void testDestructuringInLambda_inspectionData_Inspections_test() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/destructuringInLambda/inspectionData/inspections.test");
+            doTest(fileName);
+        }
+
         @TestMetadata("iterationOverMap/inspectionData/inspections.test")
         public void testIterationOverMap_inspectionData_Inspections_test() throws Exception {
             String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/inspectionData/inspections.test");

idea/tests/org/jetbrains/kotlin/idea/intentions/IntentionTestGenerated.java

@@ -6329,6 +6329,7 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
             String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/destructuringInLambda/variables.kt");
             doTest(fileName);
         }
+
     }
 
     @TestMetadata("idea/testData/intentions/destructuringVariables")