Simplify for intention: applicable of any numbers of properties used #KT-10779 Fixed

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

@@ -26,12 +26,15 @@ import org.jetbrains.kotlin.descriptors.CallableDescriptor
 import org.jetbrains.kotlin.descriptors.ClassDescriptor
 import org.jetbrains.kotlin.idea.caches.resolve.analyzeFullyAndGetResult
 import org.jetbrains.kotlin.idea.inspections.IntentionBasedInspection
+import org.jetbrains.kotlin.incremental.components.NoLookupLocation
 import org.jetbrains.kotlin.lexer.KtTokens
+import org.jetbrains.kotlin.name.Name
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.resolve.BindingContext
 import org.jetbrains.kotlin.resolve.DescriptorUtils
 import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
 import org.jetbrains.kotlin.resolve.descriptorUtil.builtIns
+import org.jetbrains.kotlin.resolve.scopes.DescriptorKindFilter
 import kotlin.collections.forEach
 
 class SimplifyForInspection : IntentionBasedInspection<KtForExpression>(SimplifyForIntention())
@@ -83,42 +86,46 @@ class SimplifyForIntention : SelfTargetingRangeIntention<KtForExpression>(
         val classDescriptor = loopParameterType.constructor.declarationDescriptor as? ClassDescriptor ?: return null
 
         var otherUsages = false
-        val usagesToRemove : Array<UsageData?>
+        val usagesToRemove : Array<UsageData>
 
-        if (DescriptorUtils.isSubclass(classDescriptor, classDescriptor.builtIns.mapEntry)) {
+        val mapEntry = classDescriptor.builtIns.mapEntry
+        val removeSelectorInLoopRange: Boolean
+        if (DescriptorUtils.isSubclass(classDescriptor, mapEntry)) {
             val loopRangeDescriptorName = element.loopRange.getResolvedCall(context)?.resultingDescriptor?.name
-            val removeSelectorInLoopRange = loopRangeDescriptorName?.asString().let { it == "entries" || it == "entrySet" }
+            removeSelectorInLoopRange = loopRangeDescriptorName?.asString().let { it == "entries" || it == "entrySet" }
 
-            usagesToRemove = arrayOfNulls(2)
+            usagesToRemove = Array(2, {
+                UsageData(mapEntry.unsubstitutedMemberScope.getContributedVariables(Name.identifier(if (it == 0) "key" else "value"),
+                                                                                    NoLookupLocation.FROM_BUILTINS).first())
+            })
 
             ReferencesSearch.search(loopParameter).iterateOverMapEntryPropertiesUsages(
                     context,
-                    { index, usageData -> usagesToRemove[index] += usageData.named(if (index == 0) "key" else "value") },
+                    { index, usageData -> usagesToRemove[index] += usageData },
                     { otherUsages = true }
             )
-
-            if (!otherUsages && usagesToRemove.all { it != null && it.name != null && it.properties.size <= 1 }) {
-                return usagesToRemove.mapNotNull { it } to removeSelectorInLoopRange
-            }
         }
         else if (classDescriptor.isData) {
+            removeSelectorInLoopRange = false
+
             val valueParameters = classDescriptor.unsubstitutedPrimaryConstructor?.valueParameters ?: return null
-            usagesToRemove = arrayOfNulls(valueParameters.size)
+            usagesToRemove = Array(valueParameters.size, { UsageData(valueParameters[it] )})
 
             ReferencesSearch.search(loopParameter).iterateOverDataClassPropertiesUsagesWithIndex(
                     context,
                     classDescriptor,
-                    { index, usageData -> usagesToRemove[index] += usageData.named(valueParameters[index].name.asString()) },
+                    { index, usageData -> usagesToRemove[index] += usageData },
                     { otherUsages = true }
             )
+        }
+        else {
+            return null
+        }
+        if (otherUsages) return null
 
-            if (otherUsages) return null
+        val droppedLastUnused = usagesToRemove.dropLastWhile { it.usersToReplace.isEmpty() && it.properties.isEmpty() }
-
+        if (droppedLastUnused.all { it.properties.size <= 1 } ) {
-            val notNullUsages = usagesToRemove.filterNotNull().filter { it.name != null }
+            return droppedLastUnused to removeSelectorInLoopRange
-            val droppedLastUnused = usagesToRemove.dropLastWhile { it == null }
-            if (droppedLastUnused.size == notNullUsages.size && droppedLastUnused.all { it != null && it.properties.size <= 1 } ) {
-                return notNullUsages to false
-            }
         }
 
         return null
@@ -197,17 +204,18 @@ class SimplifyForIntention : SelfTargetingRangeIntention<KtForExpression>(
                                  val descriptor: CallableDescriptor,
                                  val name: String? = properties.firstOrNull()?.name
     ) {
+        constructor(descriptor: CallableDescriptor):
+                this(emptyList(), emptyList(), descriptor, descriptor.name.asString())
+
         constructor(property: KtProperty?, user: KtExpression, descriptor: CallableDescriptor):
                 this(listOfNotNull(property), if (property != null) emptyList() else listOf(user), descriptor)
-
-        fun named(suggested: String) = if (name != null) this else copy(name = suggested)
     }
 
     private operator fun UsageData?.plus(newData: UsageData): UsageData {
         if (this == null) return newData
         val allUsersToReplace = usersToReplace + newData.usersToReplace
         val allProperties = properties + newData.properties
-        if (properties.isNotEmpty()) return copy(properties = allProperties, usersToReplace = allUsersToReplace)
+        val name = if (properties.isNotEmpty()) name else newData.name ?: name
-        else return newData.copy(properties = allProperties, usersToReplace = allUsersToReplace)
+        return UsageData(allProperties, allUsersToReplace, descriptor, name)
     }
 }

idea/testData/intentions/iterationOverMap/DataClassLast.kt

@@ -0,0 +1,9 @@
+// WITH_RUNTIME
+
+data class My(val first: String, val second: Int)
+
+fun foo(list: List<My>) {
+    for (<caret>my in list) {
+        println(my.second)
+    }
+}

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

@@ -0,0 +1,9 @@
+// WITH_RUNTIME
+
+data class My(val first: String, val second: Int)
+
+fun foo(list: List<My>) {
+    for ((first, second) in list) {
+        println(second)
+    }
+}

idea/testData/intentions/iterationOverMap/DataClassNotAllPropertiesUsed.kt

@@ -1,4 +1,3 @@
-// IS_APPLICABLE: false
 // WITH_RUNTIME
 
 fun main(args: Array<String>) {
@@ -6,6 +5,7 @@ fun main(args: Array<String>) {
     for (<caret>klass in list) {
         val a = klass.a
         val c = klass.c
+        println("$a$c")
     }
 }
 

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

@@ -0,0 +1,10 @@
+// WITH_RUNTIME
+
+fun main(args: Array<String>) {
+    val list = listOf(MyClass(1, 2, 3, 4))
+    for ((a, b, c) in list) {
+        println("$a$c")
+    }
+}
+
+data class MyClass(val a: Int, val b: Int, val c: Int, val d: Int)

idea/testData/intentions/iterationOverMap/KeyOnly.kt

@@ -0,0 +1,8 @@
+// WITH_RUNTIME
+
+fun main(args: Array<String>) {
+    val map = hashMapOf(1 to 1)
+    for (<caret>entry in map) {
+        println(entry.key)
+    }
+}

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

@@ -0,0 +1,8 @@
+// WITH_RUNTIME
+
+fun main(args: Array<String>) {
+    val map = hashMapOf(1 to 1)
+    for ((key) in map) {
+        println(key)
+    }
+}

idea/testData/intentions/iterationOverMap/ValueOnly.kt

@@ -0,0 +1,8 @@
+// WITH_RUNTIME
+
+fun main(args: Array<String>) {
+    val map = hashMapOf(1 to 1)
+    for (<caret>entry in map) {
+        println(entry.value)
+    }
+}

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

@@ -0,0 +1,8 @@
+// WITH_RUNTIME
+
+fun main(args: Array<String>) {
+    val map = hashMapOf(1 to 1)
+    for ((key, value) in map) {
+        println(value)
+    }
+}

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

@@ -135,4 +135,36 @@
         <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">'for' that can be simplified using destructing declaration</problem_class>
         <description>Simplify 'for' using destructing declaration</description>
     </problem>
+    <problem>
+        <file>ValueOnly.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/ValueOnly.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">'for' that can be simplified using destructing declaration</problem_class>
+        <description>Simplify 'for' using destructing declaration</description>
+    </problem>
+    <problem>
+        <file>KeyOnly.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/KeyOnly.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">'for' that can be simplified using destructing declaration</problem_class>
+        <description>Simplify 'for' using destructing declaration</description>
+    </problem>
+    <problem>
+        <file>DataClassNotAllPropertiesUsed.kt</file>
+        <line>5</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/DataClassNotAllPropertiesUsed.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">'for' that can be simplified using destructing declaration</problem_class>
+        <description>Simplify 'for' using destructing declaration</description>
+    </problem>
+    <problem>
+        <file>DataClassLast.kt</file>
+        <line>6</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="temp:///src/DataClassLast.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">'for' that can be simplified using destructing declaration</problem_class>
+        <description>Simplify 'for' using destructing declaration</description>
+    </problem>
 </problems>

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

@@ -6549,6 +6549,12 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
             doTest(fileName);
         }
 
+        @TestMetadata("DataClassLast.kt")
+        public void testDataClassLast() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/DataClassLast.kt");
+            doTest(fileName);
+        }
+
         @TestMetadata("DataClassNoVariablesInside.kt")
         public void testDataClassNoVariablesInside() throws Exception {
             String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/DataClassNoVariablesInside.kt");
@@ -6639,6 +6645,12 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
             doTest(fileName);
         }
 
+        @TestMetadata("KeyOnly.kt")
+        public void testKeyOnly() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/KeyOnly.kt");
+            doTest(fileName);
+        }
+
         @TestMetadata("MapNoProperties.kt")
         public void testMapNoProperties() throws Exception {
             String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/MapNoProperties.kt");
@@ -6681,6 +6693,12 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
             doTest(fileName);
         }
 
+        @TestMetadata("ValueOnly.kt")
+        public void testValueOnly() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/intentions/iterationOverMap/ValueOnly.kt");
+            doTest(fileName);
+        }
+
     }
 
     @TestMetadata("idea/testData/intentions/moveAssignmentToInitializer")