Destructure intention now handles the case with manual destructuring inside #KT-13943 Fixed

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

@@ -62,7 +62,7 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
         val factory = KtPsiFactory(element)
         val validator = NewDeclarationNameValidator(
                 element.parent, element, NewDeclarationNameValidator.Target.VARIABLES,
-                excludedDeclarations = usagesToRemove.map { it.variableToDrop.singletonOrEmptyList() }.flatten()
+                excludedDeclarations = usagesToRemove.map { it.declarationToDrop.singletonOrEmptyList() }.flatten()
         )
         val names = ArrayList<String>()
         usagesToRemove.forEach { (descriptor, usagesToReplace, variableToDrop, name) ->
@@ -154,7 +154,7 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
 
         // Note: list should contains properties in order to create destructuring declaration
         val usagesToRemove = mutableListOf<UsageData>()
-        var badUsageFound = false
+        var noBadUsages = true
         val removeSelectorInLoopRange: Boolean
         if (forLoop != null && DescriptorUtils.isSubclass(classDescriptor, mapEntryClassDescriptor)) {
             val loopRangeDescriptorName = forLoop.loopRange.getResolvedCall(context)?.resultingDescriptor?.name
@@ -167,8 +167,8 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
 
             ReferencesSearch.search(declaration).iterateOverMapEntryPropertiesUsages(
                     context,
-                    { index, usageData -> usagesToRemove[index].add(usageData) },
+                    { index, usageData -> noBadUsages = usagesToRemove[index].add(usageData, index) && noBadUsages },
-                    { badUsageFound = true }
+                    { noBadUsages = false }
             )
         }
         else if (classDescriptor.isData) {
@@ -199,29 +199,35 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
                     context,
                     nameToSearch,
                     descriptorToIndex,
-                    { index, usageData -> usagesToRemove[index].add(usageData) },
+                    { index, usageData -> noBadUsages = usagesToRemove[index].add(usageData, index) && noBadUsages },
-                    { badUsageFound = true }
+                    { noBadUsages = false }
             )
         }
         else {
             return null
         }
-        if (badUsageFound) return null
+        if (!noBadUsages) return null
 
-        val droppedLastUnused = usagesToRemove.dropLastWhile { it.usagesToReplace.isEmpty() && it.variableToDrop == null }
+        val droppedLastUnused = usagesToRemove.dropLastWhile { it.usagesToReplace.isEmpty() && it.declarationToDrop == null }
         return UsagesToRemove(droppedLastUnused, removeSelectorInLoopRange)
     }
 
     private fun Query<PsiReference>.iterateOverMapEntryPropertiesUsages(
             context: BindingContext,
-            process: (Int, UsageData) -> Unit,
+            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) {
-                when (applicableUsage.descriptor.name.asString()) {
+                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
@@ -242,7 +248,7 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
             context: BindingContext,
             parameterName: Name,
             descriptorToIndex: Map<CallableDescriptor, Int>,
-            process: (Int, UsageData) -> Unit,
+            process: (Int, SingleUsageData) -> Unit,
             cancel: () -> Unit
     ) {
         anyDescendantOfType<KtNameReferenceExpression> {
@@ -250,7 +256,14 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
             else {
                 val applicableUsage = getDataIfUsageIsApplicable(it, context)
                 if (applicableUsage != null) {
-                    val index = descriptorToIndex[applicableUsage.descriptor]
+                    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
@@ -266,7 +279,11 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
     private fun getDataIfUsageIsApplicable(dataClassUsage: PsiReference, context: BindingContext) =
             (dataClassUsage.element as? KtReferenceExpression)?.let { getDataIfUsageIsApplicable(it, context) }
 
-    private fun getDataIfUsageIsApplicable(dataClassUsage: KtReferenceExpression, context: BindingContext): UsageData? {
+    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) {
@@ -282,22 +299,37 @@ class DestructureIntention : SelfTargetingRangeIntention<KtDeclaration>(
         if (property != null && property.isVar) return null
 
         val descriptor = qualifiedExpression.getResolvedCall(context)?.resultingDescriptor ?: return null
-        return UsageData(
+        return SingleUsageData(descriptor = descriptor, usageToReplace = qualifiedExpression, declarationToDrop = property)
-                descriptor = descriptor,
-                usagesToReplace = mutableListOf(qualifiedExpression),
-                variableToDrop = property)
     }
 
+    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 variableToDrop: KtProperty? = null,
+            var declarationToDrop: KtDeclaration? = null,
-            var name: String? = variableToDrop?.name
+            var name: String? = declarationToDrop?.name
     ) {
-        fun add(newData: UsageData) {
+        // Returns true if data is successfully added, false otherwise
-            variableToDrop = variableToDrop ?: newData.variableToDrop
+        fun add(newData: SingleUsageData, componentIndex: Int): Boolean {
-            usagesToReplace.addAll(newData.usagesToReplace)
+            if (newData.declarationToDrop is KtDestructuringDeclaration) {
-            name = name ?: newData.name
+                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
         }
     }
 }

idea/testData/intentions/destructuringInLambda/noItWithDestructuring.kt

@@ -0,0 +1,8 @@
+data class XY(val x: String, val y: String)
+
+fun convert(xy: XY, foo: (XY) -> String) = foo(xy)
+
+fun foo(xy: XY) = convert(xy) <caret>{
+    val (x, y) = it
+    x + y
+}

idea/testData/intentions/destructuringInLambda/noItWithDestructuring.kt.after

@@ -0,0 +1,8 @@
+data class XY(val x: String, val y: String)
+
+fun convert(xy: XY, foo: (XY) -> String) = foo(xy)
+
+fun foo(xy: XY) = convert(xy) {
+    (x, y) ->
+    x + y
+}

idea/testData/intentions/iterationOverMap/DataClassWithDestructuring.kt

@@ -0,0 +1,9 @@
+// WITH_RUNTIME
+
+data class XY(val x: Int, val y: Int)
+
+fun foo(list: List<XY>) {
+    for (element<caret> in list) {
+        val (x, y) = element
+    }
+}

idea/testData/intentions/iterationOverMap/DataClassWithDestructuring.kt.after

@@ -0,0 +1,8 @@
+// WITH_RUNTIME
+
+data class XY(val x: Int, val y: Int)
+
+fun foo(list: List<XY>) {
+    for ((x, y) in list) {
+    }
+}

idea/testData/intentions/iterationOverMap/DataClassWithDestructuringConflict.kt

@@ -0,0 +1,11 @@
+// IS_APPLICABLE: false
+// WITH_RUNTIME
+
+data class XY(val x: Int, val y: Int)
+
+fun foo(list: List<XY>) {
+    for (element<caret> in list) {
+        val z = element.y
+        val (x, y) = element
+    }
+}

idea/testData/intentions/iterationOverMap/DataClassWithDestructuringFakeConflict.kt

@@ -0,0 +1,10 @@
+// WITH_RUNTIME
+
+data class XY(val x: Int, val y: Int)
+
+fun foo(list: List<XY>) {
+    for (element<caret> in list) {
+        val z = element.y
+        val (x) = element
+    }
+}

idea/testData/intentions/iterationOverMap/DataClassWithDestructuringFakeConflict.kt.after

@@ -0,0 +1,8 @@
+// WITH_RUNTIME
+
+data class XY(val x: Int, val y: Int)
+
+fun foo(list: List<XY>) {
+    for ((x, z) in list) {
+    }
+}

idea/testData/intentions/iterationOverMap/DataClassWithDestructuringPartial.kt

@@ -0,0 +1,13 @@
+// WITH_RUNTIME
+
+data class XY(val x: Int, val y: Int)
+
+fun foo(list: List<XY>): Int {
+    var result = 0
+    for (element<caret> in list) {
+        val (x) = element
+        result += x
+        result += element.y
+    }
+    return result
+}

idea/testData/intentions/iterationOverMap/DataClassWithDestructuringPartial.kt.after

@@ -0,0 +1,12 @@
+// WITH_RUNTIME
+
+data class XY(val x: Int, val y: Int)
+
+fun foo(list: List<XY>): Int {
+    var result = 0
+    for ((x, y) in list) {
+        result += x
+        result += y
+    }
+    return result
+}

idea/testData/intentions/iterationOverMap/KeyOnlyWithDestructuring.kt

@@ -0,0 +1,7 @@
+// WITH_RUNTIME
+
+fun foo(map: Map<Int, Int>) {
+    for (entry<caret> in map.entries) {
+        val (key) = entry
+    }
+}

idea/testData/intentions/iterationOverMap/KeyOnlyWithDestructuring.kt.after

@@ -0,0 +1,6 @@
+// WITH_RUNTIME
+
+fun foo(map: Map<Int, Int>) {
+    for ((key) in map) {
+    }
+}

idea/testData/intentions/iterationOverMap/KeyValueWithDestructuring.kt

@@ -0,0 +1,7 @@
+// WITH_RUNTIME
+
+fun foo(map: Map<Int, Int>) {
+    for (entry<caret> in map.entries) {
+        val (key, value) = entry
+    }
+}

idea/testData/intentions/iterationOverMap/KeyValueWithDestructuring.kt.after

@@ -0,0 +1,6 @@
+// WITH_RUNTIME
+
+fun foo(map: Map<Int, Int>) {
+    for ((key, value) in map) {
+    }
+}

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

@@ -191,4 +191,44 @@
         <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
         <description>Use destructuring declaration</description>
     </problem>
+    <problem>
+        <file>DataClassWithDestructuring.kt</file>
+        <line>6</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/DataClassWithDestructuring.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>DataClassWithDestructuringFakeConflict.kt</file>
+        <line>6</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/DataClassWithDestructuringFakeConflict.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>DataClassWithDestructuringPartial.kt</file>
+        <line>7</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/DataClassWithDestructuringPartial.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>KeyOnlyWithDestructuring.kt</file>
+        <line>4</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/KeyOnlyWithDestructuring.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
+    <problem>
+        <file>KeyValueWithDestructuring.kt</file>
+        <line>4</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/KeyValueWithDestructuring.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Use destructuring declaration</problem_class>
+        <description>Use destructuring declaration</description>
+    </problem>
 </problems>

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

@@ -6306,6 +6306,12 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
             doTest(fileName);
         }
 
+        @TestMetadata("noItWithDestructuring.kt")
+        public void testNoItWithDestructuring() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/destructuringInLambda/noItWithDestructuring.kt");
+            doTest(fileName);
+        }
+
         @TestMetadata("nullable.kt")
         public void testNullable() throws Exception {
             String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/destructuringInLambda/nullable.kt");
@@ -7644,6 +7650,30 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
             doTest(fileName);
         }
 
+        @TestMetadata("DataClassWithDestructuring.kt")
+        public void testDataClassWithDestructuring() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/DataClassWithDestructuring.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("DataClassWithDestructuringConflict.kt")
+        public void testDataClassWithDestructuringConflict() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/DataClassWithDestructuringConflict.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("DataClassWithDestructuringFakeConflict.kt")
+        public void testDataClassWithDestructuringFakeConflict() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/DataClassWithDestructuringFakeConflict.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("DataClassWithDestructuringPartial.kt")
+        public void testDataClassWithDestructuringPartial() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/DataClassWithDestructuringPartial.kt");
+            doTest(fileName);
+        }
+
         @TestMetadata("DataClassWithExternalUsage.kt")
         public void testDataClassWithExternalUsage() throws Exception {
             String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/DataClassWithExternalUsage.kt");
@@ -7680,6 +7710,18 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
             doTest(fileName);
         }
 
+        @TestMetadata("KeyOnlyWithDestructuring.kt")
+        public void testKeyOnlyWithDestructuring() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/KeyOnlyWithDestructuring.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("KeyValueWithDestructuring.kt")
+        public void testKeyValueWithDestructuring() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/KeyValueWithDestructuring.kt");
+            doTest(fileName);
+        }
+
         @TestMetadata("MapNoProperties.kt")
         public void testMapNoProperties() throws Exception {
             String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/MapNoProperties.kt");