From 1010ecca8c9fb7bc723a6e8dcaddfa1ea6af9e14 Mon Sep 17 00:00:00 2001 From: Mikhail Glukhikh Date: Wed, 5 Jul 2017 14:56:24 +0300 Subject: [PATCH] Lift return out: do not report for zero returns Also, do not highlight (but suggest fix) for exactly one return Related to KT-14900 --- .../LiftReturnOrAssignmentInspection.kt | 9 +++-- .../BranchedFoldingUtils.kt | 39 +++++++++++++------ .../liftOut/whenToReturn/insideLoop.kt | 2 + .../liftOut/whenToReturn/insideLoop.kt.after | 2 + .../liftOut/whenToReturn/whenOneReturn.kt | 8 ++++ .../whenToReturn/whenOneReturn.kt.after | 8 ++++ .../liftOut/whenToReturn/whenThrowOnly.kt | 7 ++++ .../LocalInspectionTestGenerated.java | 12 ++++++ 8 files changed, 72 insertions(+), 15 deletions(-) create mode 100644 idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt create mode 100644 idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt.after create mode 100644 idea/testData/inspectionsLocal/liftOut/whenToReturn/whenThrowOnly.kt diff --git a/idea/src/org/jetbrains/kotlin/idea/inspections/LiftReturnOrAssignmentInspection.kt b/idea/src/org/jetbrains/kotlin/idea/inspections/LiftReturnOrAssignmentInspection.kt index 5bc50ffa98d..b26fde1ce91 100644 --- a/idea/src/org/jetbrains/kotlin/idea/inspections/LiftReturnOrAssignmentInspection.kt +++ b/idea/src/org/jetbrains/kotlin/idea/inspections/LiftReturnOrAssignmentInspection.kt @@ -31,15 +31,18 @@ class LiftReturnOrAssignmentInspection : AbstractKotlinInspection() { override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean, session: LocalInspectionToolSession) = object : KtVisitorVoid() { private fun visitIfOrWhen(expression: KtExpression, keyword: PsiElement) { - if (BranchedFoldingUtils.canFoldToReturn(expression)) { + val returnNumber = BranchedFoldingUtils.getFoldableReturnNumber(expression) + if (returnNumber > 0) { holder.registerProblem( keyword, "Return can be lifted out of '${keyword.text}'", - ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + if (returnNumber > 1) ProblemHighlightType.GENERIC_ERROR_OR_WARNING + else ProblemHighlightType.INFORMATION, LiftReturnOutFix(keyword.text) ) + return } - else if (BranchedFoldingUtils.canFoldToAssignment(expression)) { + if (BranchedFoldingUtils.canFoldToAssignment(expression)) { holder.registerProblem( keyword, "Assignment can be lifted out of '${keyword.text}'", diff --git a/idea/src/org/jetbrains/kotlin/idea/intentions/branchedTransformations/BranchedFoldingUtils.kt b/idea/src/org/jetbrains/kotlin/idea/intentions/branchedTransformations/BranchedFoldingUtils.kt index a5e11541cef..d832822b088 100644 --- a/idea/src/org/jetbrains/kotlin/idea/intentions/branchedTransformations/BranchedFoldingUtils.kt +++ b/idea/src/org/jetbrains/kotlin/idea/intentions/branchedTransformations/BranchedFoldingUtils.kt @@ -82,30 +82,45 @@ object BranchedFoldingUtils { return assignments.all { BranchedFoldingUtils.checkAssignmentsMatch(it, firstAssignment) } } - fun canFoldToReturn(expression: KtExpression?): Boolean = when (expression) { + private fun getFoldableReturnNumber(branches: List) = + branches.fold(0) { prevNumber, branch -> + when { + prevNumber == -1 -> -1 + getFoldableBranchedReturn(branch) != null -> prevNumber + 1 + else -> { + val currNumber = getFoldableReturnNumber(branch?.lastBlockStatementOrThis()) + if (currNumber == -1) -1 else prevNumber + currNumber + } + } + } + + internal fun getFoldableReturnNumber(expression: KtExpression?): Int = when (expression) { is KtWhenExpression -> { val entries = expression.entries - KtPsiUtil.checkWhenExpressionHasSingleElse(expression) && - entries.isNotEmpty() && - entries.all { entry -> - getFoldableBranchedReturn(entry.expression) != null || canFoldToReturn(entry.expression?.lastBlockStatementOrThis()) + when { + !KtPsiUtil.checkWhenExpressionHasSingleElse(expression) -> -1 + entries.isEmpty() -> -1 + else -> getFoldableReturnNumber(entries.map { it.expression }) } } is KtIfExpression -> { val branches = expression.branches - branches.isNotEmpty() && - (branches.lastOrNull()?.getStrictParentOfType()?.`else` != null) && - branches.all { branch -> - getFoldableBranchedReturn(branch) != null || canFoldToReturn(branch?.lastBlockStatementOrThis()) + when { + branches.isEmpty() -> -1 + branches.lastOrNull()?.getStrictParentOfType()?.`else` == null -> -1 + else -> getFoldableReturnNumber(branches) } } is KtCallExpression -> { - expression.analyze().getType(expression)?.isNothing() ?: false + if (expression.analyze().getType(expression)?.isNothing() == true) 0 else -1 } - is KtBreakExpression, is KtContinueExpression, is KtThrowExpression -> true - else -> false + is KtBreakExpression, is KtContinueExpression, is KtThrowExpression -> 0 + else -> -1 } + fun canFoldToReturn(expression: KtExpression?): Boolean = + getFoldableReturnNumber(expression) > 0 + fun foldToAssignment(expression: KtExpression) { var lhs: KtExpression? = null var op: String? = null diff --git a/idea/testData/inspectionsLocal/liftOut/whenToReturn/insideLoop.kt b/idea/testData/inspectionsLocal/liftOut/whenToReturn/insideLoop.kt index 687852d811f..061bc3628c4 100644 --- a/idea/testData/inspectionsLocal/liftOut/whenToReturn/insideLoop.kt +++ b/idea/testData/inspectionsLocal/liftOut/whenToReturn/insideLoop.kt @@ -1,3 +1,5 @@ +// HIGHLIGHT: GENERIC_ERROR_OR_WARNING + fun foo(): Int { loop@ while (true) { when (1) { diff --git a/idea/testData/inspectionsLocal/liftOut/whenToReturn/insideLoop.kt.after b/idea/testData/inspectionsLocal/liftOut/whenToReturn/insideLoop.kt.after index 8f5252f48db..3f28eaf94e1 100644 --- a/idea/testData/inspectionsLocal/liftOut/whenToReturn/insideLoop.kt.after +++ b/idea/testData/inspectionsLocal/liftOut/whenToReturn/insideLoop.kt.after @@ -1,3 +1,5 @@ +// HIGHLIGHT: GENERIC_ERROR_OR_WARNING + fun foo(): Int { loop@ while (true) { return when (1) { diff --git a/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt b/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt new file mode 100644 index 00000000000..d427aaea5de --- /dev/null +++ b/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt @@ -0,0 +1,8 @@ +// HIGHLIGHT: INFORMATION + +fun foo(arg: Int): Int { + when (arg) { + 0 -> return 0 + else -> throw Exception() + } +} \ No newline at end of file diff --git a/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt.after b/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt.after new file mode 100644 index 00000000000..96a015ad423 --- /dev/null +++ b/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt.after @@ -0,0 +1,8 @@ +// HIGHLIGHT: INFORMATION + +fun foo(arg: Int): Int { + return when (arg) { + 0 -> 0 + else -> throw Exception() + } +} \ No newline at end of file diff --git a/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenThrowOnly.kt b/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenThrowOnly.kt new file mode 100644 index 00000000000..56615b29f4d --- /dev/null +++ b/idea/testData/inspectionsLocal/liftOut/whenToReturn/whenThrowOnly.kt @@ -0,0 +1,7 @@ +// PROBLEM: none + +fun foo(): Int { + when { + else -> throw Exception() + } +} \ No newline at end of file diff --git a/idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java b/idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java index e9aa900abbe..872d6634047 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java +++ b/idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java @@ -636,6 +636,18 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest { String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/whenToReturn/simpleWhenWithBlocks.kt"); doTest(fileName); } + + @TestMetadata("whenOneReturn.kt") + public void testWhenOneReturn() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/whenToReturn/whenOneReturn.kt"); + doTest(fileName); + } + + @TestMetadata("whenThrowOnly.kt") + public void testWhenThrowOnly() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/whenToReturn/whenThrowOnly.kt"); + doTest(fileName); + } } }