diff --git a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/commonUtils.kt b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/commonUtils.kt index 82d83ab1bdd..7d8d3db2b41 100644 --- a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/commonUtils.kt +++ b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/commonUtils.kt @@ -67,7 +67,7 @@ fun KtCallableDeclaration.hasUsages(inElements: Collection): Boolean // return ReferencesSearch.search(this, LocalSearchScope(inElements.toTypedArray())).any() } -fun KtProperty.hasWriteUsages(): Boolean { +fun KtVariableDeclaration.hasWriteUsages(): Boolean { assert(this.isPhysical) if (!isVar) return false return ReferencesSearch.search(this, useScope).any { @@ -75,23 +75,23 @@ fun KtProperty.hasWriteUsages(): Boolean { } } -fun KtProperty.countUsages(inElement: KtElement): Int { +fun KtCallableDeclaration.countUsages(inElement: KtElement): Int { assert(this.isPhysical) return ReferencesSearch.search(this, LocalSearchScope(inElement)).count() } -fun KtProperty.countUsages(inElements: Collection): Int { +fun KtCallableDeclaration.countUsages(inElements: Collection): Int { assert(this.isPhysical) // TODO: it's a temporary workaround about strange dead-lock when running inspections return inElements.sumBy { ReferencesSearch.search(this, LocalSearchScope(it)).count() } } -fun KtProperty.countUsages(): Int { +fun KtCallableDeclaration.countUsages(): Int { assert(this.isPhysical) return ReferencesSearch.search(this, useScope).count() } -fun KtProperty.countWriteUsages(inElement: KtElement): Int { +fun KtVariableDeclaration.countWriteUsages(inElement: KtElement): Int { assert(this.isPhysical) if (!isVar) return 0 return ReferencesSearch.search(this, LocalSearchScope(inElement)).count { @@ -99,6 +99,14 @@ fun KtProperty.countWriteUsages(inElement: KtElement): Int { } } +fun KtVariableDeclaration.hasWriteUsages(inElement: KtElement): Boolean { + assert(this.isPhysical) + if (!isVar) return false + return ReferencesSearch.search(this, LocalSearchScope(inElement)).any { + (it as? KtSimpleNameReference)?.element?.readWriteAccess(useResolveForReadWrite = true)?.isWrite == true + } +} + fun KtExpressionWithLabel.targetLoop(): KtLoopExpression? { val label = getTargetLabel() if (label == null) { @@ -148,4 +156,3 @@ fun PsiChildRange.withoutLastStatement(): PsiChildRange { val newLast = last!!.siblings(forward = false, withItself = false).first { it !is PsiWhiteSpace } return PsiChildRange(first, newLast) } - diff --git a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/result/AddToCollectionTransformation.kt b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/result/AddToCollectionTransformation.kt index 53f3c6d56c6..23bdb827302 100644 --- a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/result/AddToCollectionTransformation.kt +++ b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/result/AddToCollectionTransformation.kt @@ -93,13 +93,15 @@ class AddToCollectionTransformation( //TODO: it can be implicit 'this' too val qualifiedExpression = statement as? KtDotQualifiedExpression ?: return null val targetCollection = qualifiedExpression.receiverExpression - //TODO: check that receiver is stable! val callExpression = qualifiedExpression.selectorExpression as? KtCallExpression ?: return null if (callExpression.getCallNameExpression()?.getReferencedName() != "add") return null //TODO: check that it's MutableCollection's add val argument = callExpression.valueArguments.singleOrNull() ?: return null val argumentValue = argument.getArgumentExpression() ?: return null + //TODO: should work when ".asSequence()" used when there are other read-usages in the loop + if (!targetCollection.isStableInLoop(state.outerLoop, checkNoOtherUsagesInLoop = true)) return null + if (state.indexVariable == null) { matchWithCollectionInitializationReplacement(state, targetCollection, argumentValue) ?.let { return it } diff --git a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/utils.kt b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/utils.kt index 6e117054267..e896b3e31d5 100644 --- a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/utils.kt +++ b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/utils.kt @@ -19,10 +19,12 @@ package org.jetbrains.kotlin.idea.intentions.loopToCallChain import com.intellij.openapi.util.Key import com.intellij.openapi.util.UserDataHolderBase import org.jetbrains.kotlin.descriptors.ConstructorDescriptor +import org.jetbrains.kotlin.descriptors.VariableDescriptor import org.jetbrains.kotlin.diagnostics.Severity import org.jetbrains.kotlin.idea.analysis.analyzeInContext import org.jetbrains.kotlin.idea.caches.resolve.analyze import org.jetbrains.kotlin.idea.caches.resolve.getResolutionFacade +import org.jetbrains.kotlin.idea.caches.resolve.resolveToDescriptorIfAny import org.jetbrains.kotlin.idea.core.copied import org.jetbrains.kotlin.idea.core.replaced import org.jetbrains.kotlin.idea.imports.importableFqName @@ -257,3 +259,28 @@ fun canSwapExecutionOrder(expressionBefore: KtExpression, expressionAfter: KtExp //TODO: more cases return false } + +fun KtExpression.isStableInLoop(loop: KtLoopExpression, checkNoOtherUsagesInLoop: Boolean): Boolean { + when { + isConstant() -> return true + + this is KtSimpleNameExpression -> { + val declaration = mainReference.resolve() as? KtCallableDeclaration ?: return false + if (loop.isAncestor(declaration)) return false // should be declared outside the loop + val variable = declaration.resolveToDescriptorIfAny() as? VariableDescriptor ?: return false + + if (checkNoOtherUsagesInLoop && declaration.countUsages(loop) > 1) return false + + if (!variable.isVar) return true + if (declaration !is KtVariableDeclaration) return false + if (!KtPsiUtil.isLocal(declaration)) return false // it's difficult to analyze non-local declarations + //TODO: check that there are no local functions or lambdas that can modify it implicitly + return !declaration.hasWriteUsages(loop) + } + + //TODO: qualified expression? + //TODO: this + + else -> return false + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/addToCollection_badReceiver1.kt b/idea/testData/intentions/loopToCallChain/addToCollection_badReceiver1.kt new file mode 100644 index 00000000000..782a7f33822 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/addToCollection_badReceiver1.kt @@ -0,0 +1,7 @@ +// WITH_RUNTIME +// IS_APPLICABLE: false +fun foo(list: List>) { + for (collection in list) { + collection.add(collection.size) + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/addToCollection_badReceiver2.kt b/idea/testData/intentions/loopToCallChain/addToCollection_badReceiver2.kt new file mode 100644 index 00000000000..cda76bca6d2 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/addToCollection_badReceiver2.kt @@ -0,0 +1,11 @@ +// WITH_RUNTIME +// IS_APPLICABLE: false +import java.util.ArrayList + +var globalCollection = ArrayList() + +fun foo(list: List>) { + for (collection in list) { + globalCollection.add(collection.size) + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/addToCollection_goodReceiver.kt b/idea/testData/intentions/loopToCallChain/addToCollection_goodReceiver.kt new file mode 100644 index 00000000000..a247578c6dc --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/addToCollection_goodReceiver.kt @@ -0,0 +1,16 @@ +// WITH_RUNTIME +// INTENTION_TEXT: "Replace with 'mapTo(){}'" +import java.util.ArrayList + +fun foo(list: List>) { + var target: MutableCollection() + target = ArrayList() + + for (collection in list) { + target.add(collection.size) + } + + if (target.size > 100) { + target = ArrayList() + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/addToCollection_goodReceiver.kt.after b/idea/testData/intentions/loopToCallChain/addToCollection_goodReceiver.kt.after new file mode 100644 index 00000000000..098edcd0e77 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/addToCollection_goodReceiver.kt.after @@ -0,0 +1,12 @@ +// WITH_RUNTIME +// INTENTION_TEXT: "Replace with 'mapTo(){}'" +import java.util.ArrayList + +fun foo(list: List>) { + var target: MutableCollection() + target = list.mapTo(ArrayList()) { it.size } + + if (target.size > 100) { + target = ArrayList() + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/resultCollectionUsedInsideLoop.kt b/idea/testData/intentions/loopToCallChain/resultCollectionUsedInsideLoop.kt index 9d934a5b4ef..192ededcd81 100644 --- a/idea/testData/intentions/loopToCallChain/resultCollectionUsedInsideLoop.kt +++ b/idea/testData/intentions/loopToCallChain/resultCollectionUsedInsideLoop.kt @@ -1,5 +1,5 @@ // WITH_RUNTIME -// INTENTION_TEXT: "Replace with 'filter{}.mapTo(){}'" +// IS_APPLICABLE: false import java.util.ArrayList fun foo(list: List): List { diff --git a/idea/testData/intentions/loopToCallChain/resultCollectionUsedInsideLoop.kt.after b/idea/testData/intentions/loopToCallChain/resultCollectionUsedInsideLoop.kt.after deleted file mode 100644 index c82ea8451c3..00000000000 --- a/idea/testData/intentions/loopToCallChain/resultCollectionUsedInsideLoop.kt.after +++ /dev/null @@ -1,11 +0,0 @@ -// WITH_RUNTIME -// INTENTION_TEXT: "Replace with 'filter{}.mapTo(){}'" -import java.util.ArrayList - -fun foo(list: List): List { - val result = ArrayList() - list - .filter { it.length > result.size } - .mapTo(result) { it.hashCode() } - return result -} \ No newline at end of file