From a6132c7db9a97b3079ae080538c2fb335e871562 Mon Sep 17 00:00:00 2001 From: Valentin Kipyatkov Date: Fri, 22 Apr 2016 11:35:13 +0300 Subject: [PATCH] Prohibiting incorrect transformations of loops with expression-embedded break or continue + allowed "?: continue" pattern for mapNotNull --- .../intentions/loopToCallChain/interfaces.kt | 13 +++- .../loopToCallChain/matchAndConvert.kt | 14 ++++- .../sequence/MapTransformation.kt | 63 +++++++++++++++++-- .../idea/intentions/loopToCallChain/utils.kt | 23 +++++++ .../loopToCallChain/embeddedBreak1.kt | 8 +++ .../loopToCallChain/embeddedBreak2.kt | 8 +++ .../loopToCallChain/embeddedContinue.kt | 8 +++ .../mapIndexedNotNull_elvisContinue.kt | 8 +++ .../mapIndexedNotNull_elvisContinue.kt.after | 7 +++ .../mapNotNull_elvisContinue.kt | 8 +++ .../mapNotNull_elvisContinue.kt.after | 7 +++ .../mapNotNull_nestedLoopElvisContinue.kt | 12 ++++ ...apNotNull_nestedLoopElvisContinue.kt.after | 10 +++ .../mapNotNull_wrongElvisContinue.kt | 13 ++++ 14 files changed, 195 insertions(+), 7 deletions(-) create mode 100644 idea/testData/intentions/loopToCallChain/embeddedBreak1.kt create mode 100644 idea/testData/intentions/loopToCallChain/embeddedBreak2.kt create mode 100644 idea/testData/intentions/loopToCallChain/embeddedContinue.kt create mode 100644 idea/testData/intentions/loopToCallChain/mapIndexedNotNull_elvisContinue.kt create mode 100644 idea/testData/intentions/loopToCallChain/mapIndexedNotNull_elvisContinue.kt.after create mode 100644 idea/testData/intentions/loopToCallChain/mapNotNull_elvisContinue.kt create mode 100644 idea/testData/intentions/loopToCallChain/mapNotNull_elvisContinue.kt.after create mode 100644 idea/testData/intentions/loopToCallChain/mapNotNull_nestedLoopElvisContinue.kt create mode 100644 idea/testData/intentions/loopToCallChain/mapNotNull_nestedLoopElvisContinue.kt.after create mode 100644 idea/testData/intentions/loopToCallChain/mapNotNull_wrongElvisContinue.kt diff --git a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/interfaces.kt b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/interfaces.kt index 589f2b927ca..6809f069d0f 100644 --- a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/interfaces.kt +++ b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/interfaces.kt @@ -100,10 +100,19 @@ data class MatchingState( val initializationStatementsToDelete: Collection = emptyList() ) +interface BaseMatcher { + /** + * Implementors should return true if they match some constructs with expression-embedded break or continue. + * In this case they are obliged to deal with them and filter out invalid cases. + */ + val embeddedBreakOrContinuePossible: Boolean + get() = false +} + /** * A matcher that can recognize one or more [SequenceTransformation]'s */ -interface SequenceTransformationMatcher { +interface SequenceTransformationMatcher : BaseMatcher { fun match(state: MatchingState): SequenceTransformationMatch? } @@ -118,7 +127,7 @@ class SequenceTransformationMatch( * A matcher that can recognize a [ResultTransformation] (optionally prepended by some [SequenceTransformation]'s). * Should match the whole rest part of the loop. */ -interface ResultTransformationMatcher { +interface ResultTransformationMatcher : BaseMatcher { fun match(state: MatchingState): ResultTransformationMatch? val indexVariableUsePossible: Boolean diff --git a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/matchAndConvert.kt b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/matchAndConvert.kt index 901c9187468..53e6fafe072 100644 --- a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/matchAndConvert.kt +++ b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/matchAndConvert.kt @@ -66,6 +66,9 @@ data class MatchResult( fun match(loop: KtForExpression): MatchResult? { val (inputVariable, indexVariable, sequenceExpression) = extractLoopData(loop) ?: return null + // used just as optimization to avoid unnecessary checks + val loopContainsEmbeddedBreakOrContinue = loop.containsEmbeddedBreakOrContinue() + val sequenceTransformations = ArrayList() var state = MatchingState( outerLoop = loop, @@ -89,8 +92,11 @@ fun match(loop: KtForExpression): MatchResult? { state = state.copy(indexVariable = null) } + val restContainsEmbeddedBreakOrContinue = loopContainsEmbeddedBreakOrContinue && state.statements.any { it.containsEmbeddedBreakOrContinue() } + for (matcher in MatcherRegistrar.resultMatchers) { if (state.indexVariable != null && !matcher.indexVariableUsePossible) continue + if (restContainsEmbeddedBreakOrContinue && !matcher.embeddedBreakOrContinuePossible) continue val match = matcher.match(state) if (match != null) { @@ -115,8 +121,8 @@ fun match(loop: KtForExpression): MatchResult? { return null } - var newState = match.newState // check that old input variable is not needed anymore + var newState = match.newState if (state.inputVariable != newState.inputVariable && state.inputVariable.hasUsages(newState.statements)) return null if (state.indexVariable != null && match.transformations.any { it.affectsIndex }) { @@ -125,6 +131,12 @@ fun match(loop: KtForExpression): MatchResult? { newState = newState.copy(indexVariable = null) } + if (restContainsEmbeddedBreakOrContinue && !matcher.embeddedBreakOrContinuePossible) { + val countBefore = state.statements.sumBy { it.countEmbeddedBreaksAndContinues() } + val countAfter = newState.statements.sumBy { it.countEmbeddedBreaksAndContinues() } + if (countAfter != countBefore) continue // some embedded break or continue in the matched part + } + sequenceTransformations.addAll(match.transformations) state = newState continue@MatchLoop diff --git a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/sequence/MapTransformation.kt b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/sequence/MapTransformation.kt index be63b828d4c..25b1ef03804 100644 --- a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/sequence/MapTransformation.kt +++ b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/sequence/MapTransformation.kt @@ -17,10 +17,8 @@ package org.jetbrains.kotlin.idea.intentions.loopToCallChain.sequence import org.jetbrains.kotlin.idea.intentions.loopToCallChain.* -import org.jetbrains.kotlin.psi.KtCallableDeclaration -import org.jetbrains.kotlin.psi.KtExpression -import org.jetbrains.kotlin.psi.KtForExpression -import org.jetbrains.kotlin.psi.KtProperty +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.* class MapTransformation( override val loop: KtForExpression, @@ -47,12 +45,32 @@ class MapTransformation( * } */ object Matcher : SequenceTransformationMatcher { + override val embeddedBreakOrContinuePossible: Boolean + get() = true + override fun match(state: MatchingState): SequenceTransformationMatch? { val declaration = state.statements.firstOrNull() as? KtProperty ?: return null //TODO: support multi-variables val initializer = declaration.initializer ?: return null if (declaration.hasWriteUsages()) return null val restStatements = state.statements.drop(1) + if (initializer is KtBinaryExpression && initializer.operationToken == KtTokens.ELVIS) { + val continueExpression = initializer.right as? KtContinueExpression ?: return null + if (continueExpression.targetLoop() != state.innerLoop) return null + + val mapping = initializer.left ?: return null + if (mapping.containsEmbeddedBreakOrContinue()) return null + + val transformation = if (state.indexVariable != null && state.indexVariable.hasUsages(mapping)) + MapIndexedNotNullTransformation(state.outerLoop, state.inputVariable, state.indexVariable, mapping) + else + MapNotNullTransformation(state.outerLoop, state.inputVariable, mapping) + val newState = state.copy(statements = restStatements, inputVariable = declaration) + return SequenceTransformationMatch(transformation, newState) + } + + if (initializer.containsEmbeddedBreakOrContinue()) return null + val transformation = if (state.indexVariable != null && state.indexVariable.hasUsages(initializer)) MapIndexedTransformation(state.outerLoop, state.inputVariable, state.indexVariable, initializer) else @@ -81,3 +99,40 @@ class MapIndexedTransformation( return chainedCallGenerator.generate("mapIndexed $0:'{}'", lambda) } } + +class MapNotNullTransformation( + override val loop: KtForExpression, + val inputVariable: KtCallableDeclaration, + val mapping: KtExpression +) : SequenceTransformation { + + override val affectsIndex: Boolean + get() = true + + override val presentation: String + get() = "mapNotNull{}" + + override fun generateCode(chainedCallGenerator: ChainedCallGenerator): KtExpression { + val lambda = generateLambda(inputVariable, mapping) + return chainedCallGenerator.generate("mapNotNull$0:'{}'", lambda) + } +} + +class MapIndexedNotNullTransformation( + override val loop: KtForExpression, + val inputVariable: KtCallableDeclaration, + val indexVariable: KtCallableDeclaration, + val mapping: KtExpression +) : SequenceTransformation { + + override val affectsIndex: Boolean + get() = false + + override val presentation: String + get() = "mapIndexedNotNull{}" + + override fun generateCode(chainedCallGenerator: ChainedCallGenerator): KtExpression { + val lambda = generateLambda(mapping, indexVariable, inputVariable) + return chainedCallGenerator.generate("mapIndexedNotNull $0:'{}'", lambda) + } +} 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 e896b3e31d5..2f5f7daad06 100644 --- a/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/utils.kt +++ b/idea/src/org/jetbrains/kotlin/idea/intentions/loopToCallChain/utils.kt @@ -283,4 +283,27 @@ fun KtExpression.isStableInLoop(loop: KtLoopExpression, checkNoOtherUsagesInLoop else -> return false } +} + +fun KtExpression.containsEmbeddedBreakOrContinue(): Boolean { + return anyDescendantOfType(::isEmbeddedBreakOrContinue) +} + +fun KtExpression.countEmbeddedBreaksAndContinues(): Int { + return collectDescendantsOfType(::isEmbeddedBreakOrContinue).size +} + +private fun isEmbeddedBreakOrContinue(expression: KtExpressionWithLabel): Boolean { + if (expression !is KtBreakExpression && expression !is KtContinueExpression) return false + val parent = expression.parent + when (parent) { + is KtBlockExpression -> return false + + is KtContainerNode -> { + val containerExpression = parent.parent as KtExpression + return containerExpression.analyze(BodyResolveMode.PARTIAL)[BindingContext.USED_AS_EXPRESSION, containerExpression] == true + } + + else -> return true + } } \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/embeddedBreak1.kt b/idea/testData/intentions/loopToCallChain/embeddedBreak1.kt new file mode 100644 index 00000000000..28d09aae8cc --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/embeddedBreak1.kt @@ -0,0 +1,8 @@ +// WITH_RUNTIME +// IS_APPLICABLE: false +fun foo(list: List, target: MutableList) { + for (s in list) { + val length = s?.length ?: break + target.add(length.toString()) + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/embeddedBreak2.kt b/idea/testData/intentions/loopToCallChain/embeddedBreak2.kt new file mode 100644 index 00000000000..6d77729d536 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/embeddedBreak2.kt @@ -0,0 +1,8 @@ +// WITH_RUNTIME +// IS_APPLICABLE: false +fun foo(list: List, target: MutableList) { + for (s in list) { + val length = if (s.isNotEmpty()) s.length else break + target.add(length.toString()) + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/embeddedContinue.kt b/idea/testData/intentions/loopToCallChain/embeddedContinue.kt new file mode 100644 index 00000000000..aecfd7fb845 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/embeddedContinue.kt @@ -0,0 +1,8 @@ +// WITH_RUNTIME +// IS_APPLICABLE: false +fun foo(list: List, target: MutableList) { + for (s in list) { + val length = if (s.isNotEmpty()) s.length else continue + target.add(length.toString()) + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/mapIndexedNotNull_elvisContinue.kt b/idea/testData/intentions/loopToCallChain/mapIndexedNotNull_elvisContinue.kt new file mode 100644 index 00000000000..9fc0ec93437 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/mapIndexedNotNull_elvisContinue.kt @@ -0,0 +1,8 @@ +// WITH_RUNTIME +// INTENTION_TEXT: "Replace with 'mapIndexedNotNull{}.mapTo(){}'" +fun foo(list: List, target: MutableList) { + for ((index, s) in list.withIndex()) { + val length = s?.substring(index)?.length ?: continue + target.add(length.toString()) + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/mapIndexedNotNull_elvisContinue.kt.after b/idea/testData/intentions/loopToCallChain/mapIndexedNotNull_elvisContinue.kt.after new file mode 100644 index 00000000000..1f5245bb3eb --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/mapIndexedNotNull_elvisContinue.kt.after @@ -0,0 +1,7 @@ +// WITH_RUNTIME +// INTENTION_TEXT: "Replace with 'mapIndexedNotNull{}.mapTo(){}'" +fun foo(list: List, target: MutableList) { + list + .mapIndexedNotNull { index, s -> s?.substring(index)?.length } + .mapTo(target) { it.toString() } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/mapNotNull_elvisContinue.kt b/idea/testData/intentions/loopToCallChain/mapNotNull_elvisContinue.kt new file mode 100644 index 00000000000..f7dcf57747d --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/mapNotNull_elvisContinue.kt @@ -0,0 +1,8 @@ +// WITH_RUNTIME +// INTENTION_TEXT: "Replace with 'mapNotNull{}.mapTo(){}'" +fun foo(list: List, target: MutableList) { + for (s in list) { + val length = s?.length ?: continue + target.add(length.toString()) + } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/mapNotNull_elvisContinue.kt.after b/idea/testData/intentions/loopToCallChain/mapNotNull_elvisContinue.kt.after new file mode 100644 index 00000000000..b9a82c98c29 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/mapNotNull_elvisContinue.kt.after @@ -0,0 +1,7 @@ +// WITH_RUNTIME +// INTENTION_TEXT: "Replace with 'mapNotNull{}.mapTo(){}'" +fun foo(list: List, target: MutableList) { + list + .mapNotNull { it?.length } + .mapTo(target) { it.toString() } +} \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/mapNotNull_nestedLoopElvisContinue.kt b/idea/testData/intentions/loopToCallChain/mapNotNull_nestedLoopElvisContinue.kt new file mode 100644 index 00000000000..174b37f87c7 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/mapNotNull_nestedLoopElvisContinue.kt @@ -0,0 +1,12 @@ +// WITH_RUNTIME +// INTENTION_TEXT: "Replace with 'flatMap{}.mapNotNull{}.mapTo(){}'" +fun foo(list: List, target: MutableList) { + for (s in list) { + for (i in s.indices) { + val v = bar(i) ?: continue + target.add(v.substring(1)) + } + } +} + +fun bar(p: Int): String? = null \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/mapNotNull_nestedLoopElvisContinue.kt.after b/idea/testData/intentions/loopToCallChain/mapNotNull_nestedLoopElvisContinue.kt.after new file mode 100644 index 00000000000..bf6eaa76fc4 --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/mapNotNull_nestedLoopElvisContinue.kt.after @@ -0,0 +1,10 @@ +// WITH_RUNTIME +// INTENTION_TEXT: "Replace with 'flatMap{}.mapNotNull{}.mapTo(){}'" +fun foo(list: List, target: MutableList) { + list + .flatMap { it.indices } + .mapNotNull { bar(it) } + .mapTo(target) { it.substring(1) } +} + +fun bar(p: Int): String? = null \ No newline at end of file diff --git a/idea/testData/intentions/loopToCallChain/mapNotNull_wrongElvisContinue.kt b/idea/testData/intentions/loopToCallChain/mapNotNull_wrongElvisContinue.kt new file mode 100644 index 00000000000..3968ce9be2e --- /dev/null +++ b/idea/testData/intentions/loopToCallChain/mapNotNull_wrongElvisContinue.kt @@ -0,0 +1,13 @@ +// WITH_RUNTIME +// IS_APPLICABLE: false +fun foo(list: List, target: MutableList) { + Loop@ + for (s in list) { + for (i in s.indices) { + val v = bar(i) ?: continue@Loop + target.add(v.substring(1)) + } + } +} + +fun bar(p: Int): String? = null \ No newline at end of file