Introduce "Lift return out of try" intention #KT-18830 Fixed

2017-07-08 18:24:31 +03:00
parent 8f9b680fc6
commit 8cc9330e63
38 changed files with 601 additions and 7 deletions
@@ -1,6 +1,6 @@
 <html>
 <body>
-This inspection reports if and when statements that can be converted to expressions
+This inspection reports if, when and try statements that can be converted to expressions
 by lifting return or assignment out. Typical example:
 <code><pre>
 <b>fun</b> foo(arg: Boolean): String {
@@ -30,7 +30,7 @@ class LiftReturnOrAssignmentInspection : AbstractKotlinInspection() {

    override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean, session: LocalInspectionToolSession) =
            object : KtVisitorVoid() {
-                private fun visitIfOrWhen(expression: KtExpression, keyword: PsiElement) {
+                private fun visitIfOrWhenOrTry(expression: KtExpression, keyword: PsiElement) {
                    if (expression.lineCount() > LINES_LIMIT) return
                    if (expression.isElseIf()) return

@@ -60,12 +60,19 @@ class LiftReturnOrAssignmentInspection : AbstractKotlinInspection() {

                override fun visitIfExpression(expression: KtIfExpression) {
                    super.visitIfExpression(expression)
-                    visitIfOrWhen(expression, expression.ifKeyword)
+                    visitIfOrWhenOrTry(expression, expression.ifKeyword)
                }

                override fun visitWhenExpression(expression: KtWhenExpression) {
                    super.visitWhenExpression(expression)
-                    visitIfOrWhen(expression, expression.whenKeyword)
+                    visitIfOrWhenOrTry(expression, expression.whenKeyword)
+                }
+
+                override fun visitTryExpression(expression: KtTryExpression) {
+                    super.visitTryExpression(expression)
+                    expression.tryKeyword?.let {
+                        visitIfOrWhenOrTry(expression, it)
+                    }
                }
            }

@@ -21,6 +21,7 @@ import org.jetbrains.kotlin.idea.intentions.branches
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.anyDescendantOfType
+import org.jetbrains.kotlin.psi.psiUtil.getNonStrictParentOfType
 import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType
 import org.jetbrains.kotlin.psi.psiUtil.lastBlockStatementOrThis
 import org.jetbrains.kotlin.types.typeUtil.isNothing
@@ -75,6 +76,12 @@ object BranchedFoldingUtils {
                    assignment != null || collectAssignmentsAndCheck(branch?.lastBlockStatementOrThis())
                }
            }
+            is KtTryExpression -> {
+                e.tryBlockAndCatchBodies().all {
+                    val assignment = getFoldableBranchedAssignment(it)?.run { assignments.add(this) }
+                    assignment != null || collectAssignmentsAndCheck(it?.lastBlockStatementOrThis())
+                }
+            }
            is KtCallExpression -> {
                e.analyze().getType(e)?.isNothing() ?: false
            }
@@ -83,13 +90,19 @@ object BranchedFoldingUtils {
            else -> false
        }
        if (!collectAssignmentsAndCheck(expression)) return -1
-        val firstAssignment = assignments.firstOrNull()
-        if (firstAssignment != null && assignments.any { !BranchedFoldingUtils.checkAssignmentsMatch(it, firstAssignment) }) {
+        val firstAssignment = assignments.firstOrNull() ?: return 0
+        if (assignments.any { !BranchedFoldingUtils.checkAssignmentsMatch(it, firstAssignment) }) {
            return -1
        }
        if (expression.anyDescendantOfType<KtBinaryExpression>(
                predicate = {
-                    it.operationToken in KtTokens.ALL_ASSIGNMENTS && it !in assignments
+                    if (it.operationToken in KtTokens.ALL_ASSIGNMENTS)
+                        if (it.getNonStrictParentOfType<KtFinallySection>() != null)
+                            BranchedFoldingUtils.checkAssignmentsMatch(it, firstAssignment)
+                        else
+                            it !in assignments
+                    else
+                        false
                }
        )) {
            return -1
@@ -128,6 +141,12 @@ object BranchedFoldingUtils {
                else -> getFoldableReturns(branches)
            }
        }
+        is KtTryExpression -> {
+            if (expression.finallyBlock?.finalExpression?.let { getFoldableReturns(listOf(it)) }?.isNotEmpty() == true)
+                null
+            else
+                getFoldableReturns(expression.tryBlockAndCatchBodies())
+        }
        is KtCallExpression -> {
            if (expression.analyze().getType(expression)?.isNothing() == true) emptyList() else null
        }
@@ -157,6 +176,9 @@ object BranchedFoldingUtils {
                is KtIfExpression -> e.branches.forEach { branch ->
                    getFoldableBranchedAssignment(branch)?.replaceWithRHS() ?: lift(branch?.lastBlockStatementOrThis())
                }
+                is KtTryExpression -> e.tryBlockAndCatchBodies().forEach {
+                    getFoldableBranchedAssignment(it)?.replaceWithRHS() ?: lift(it?.lastBlockStatementOrThis())
+                }
            }
        }
        lift(expression)
@@ -177,9 +199,16 @@ object BranchedFoldingUtils {
                    getFoldableBranchedReturn(branch)?.replaceWithReturned() ?:
                    lift(branch?.lastBlockStatementOrThis())
                }
+                is KtTryExpression -> e.tryBlockAndCatchBodies().forEach {
+                    getFoldableBranchedReturn(it)?.replaceWithReturned() ?:
+                    lift(it?.lastBlockStatementOrThis())
+                }
            }
        }
        lift(expression)
        expression.replace(KtPsiFactory(expression).createExpressionByPattern("return $0", expression))
    }
+
+    private fun KtTryExpression.tryBlockAndCatchBodies(): List<KtExpression?> = listOf(tryBlock) + catchClauses.map { it.catchBody }
+
 }
@@ -0,0 +1 @@
+org.jetbrains.kotlin.idea.inspections.LiftReturnOrAssignmentInspection
@@ -0,0 +1,11 @@
+// HIGHLIGHT: INFORMATION
+
+fun test() {
+    var res: String? = null
+
+    <caret>try {
+        res = "success"
+    } catch (e: Exception) {
+        throw e
+    }
+}
@@ -0,0 +1,11 @@
+// HIGHLIGHT: INFORMATION
+
+fun test() {
+    var res: String? = null
+
+    <caret>res = try {
+        "success"
+    } catch (e: Exception) {
+        throw e
+    }
+}
@@ -0,0 +1,15 @@
+// HIGHLIGHT: GENERIC_ERROR_OR_WARNING
+
+fun doSomething() {}
+
+fun test() {
+    var res: String? = null
+
+    <caret>try {
+        doSomething()
+        res = "success"
+    } catch (e: Exception) {
+        doSomething()
+        res = "failure"
+    }
+}
@@ -0,0 +1,15 @@
+// HIGHLIGHT: GENERIC_ERROR_OR_WARNING
+
+fun doSomething() {}
+
+fun test() {
+    var res: String? = null
+
+    <caret>res = try {
+        doSomething()
+        "success"
+    } catch (e: Exception) {
+        doSomething()
+        "failure"
+    }
+}
@@ -0,0 +1,18 @@
+// WITH_RUNTIME
+fun test() {
+    var res: String? = null
+
+    <caret>try {
+        try {
+            res = "success"
+        } catch (e: Exception) {
+            TODO()
+        }
+    } catch (e: Exception) {
+        try {
+            TODO()
+        } catch (e: Exception) {
+            res = "failure"
+        }
+    }
+}
@@ -0,0 +1,18 @@
+// WITH_RUNTIME
+fun test() {
+    var res: String? = null
+
+    <caret>res = try {
+        try {
+            "success"
+        } catch (e: Exception) {
+            TODO()
+        }
+    } catch (e: Exception) {
+        try {
+            TODO()
+        } catch (e: Exception) {
+            "failure"
+        }
+    }
+}
@@ -0,0 +1,13 @@
+fun doSomething() {}
+
+fun test() {
+    var res: String? = null
+
+    <caret>try {
+        res = "success"
+    } catch (e: Exception) {
+        res = "failure"
+    } finally {
+        doSomething()
+    }
+}
@@ -0,0 +1,13 @@
+fun doSomething() {}
+
+fun test() {
+    var res: String? = null
+
+    <caret>res = try {
+        "success"
+    } catch (e: Exception) {
+        "failure"
+    } finally {
+        doSomething()
+    }
+}
@@ -0,0 +1,12 @@
+// PROBLEM: none
+fun test() {
+    var res: String? = null
+
+    <caret>try {
+        res = "success"
+    } catch (e: Exception) {
+        res = "failure"
+    } finally {
+        res = "finally"
+    }
+}
@@ -0,0 +1,17 @@
+// PROBLEM: none
+fun doSomething() {}
+
+fun test(n: Int) {
+    var res: String? = null
+
+    <caret>try {
+        res = "success"
+    } catch (e: Exception) {
+        throw e
+    } finally {
+        if (n == 1)
+            doSomething()
+        else
+            res = "finally"
+    }
+}
@@ -0,0 +1,17 @@
+fun doSomething() {}
+
+fun test(n: Int) {
+    var res: String? = null
+    var foo: String? = null
+
+    <caret>try {
+        res = "success"
+    } catch (e: Exception) {
+        throw e
+    } finally {
+        if (n == 1)
+            doSomething()
+        else
+            foo = "finally"
+    }
+}
@@ -0,0 +1,17 @@
+fun doSomething() {}
+
+fun test(n: Int) {
+    var res: String? = null
+    var foo: String? = null
+
+    <caret>res = try {
+        "success"
+    } catch (e: Exception) {
+        throw e
+    } finally {
+        if (n == 1)
+            doSomething()
+        else
+            foo = "finally"
+    }
+}
@@ -0,0 +1,12 @@
+fun test() {
+    var res: String? = null
+    var foo: String? = null
+
+    <caret>try {
+        res = "success"
+    } catch (e: Exception) {
+        res = "failure"
+    } finally {
+        foo = "finally"
+    }
+}
@@ -0,0 +1,12 @@
+fun test() {
+    var res: String? = null
+    var foo: String? = null
+
+    <caret>res = try {
+        "success"
+    } catch (e: Exception) {
+        "failure"
+    } finally {
+        foo = "finally"
+    }
+}
@@ -0,0 +1,14 @@
+fun test(n: Int) {
+    var res: String? = null
+
+    if (n == 1) {
+        <caret>try {
+            res = "success"
+        } catch (e: Exception) {
+            throw e
+        }
+    }
+    else {
+        res = "else"
+    }
+}
@@ -0,0 +1,14 @@
+fun test(n: Int) {
+    var res: String? = null
+
+    if (n == 1) {
+        <caret>res = try {
+            "success"
+        } catch (e: Exception) {
+            throw e
+        }
+    }
+    else {
+        res = "else"
+    }
+}
@@ -0,0 +1,11 @@
+// PROBLEM: none
+fun test() {
+    var res: String? = null
+    var foo: String? = null
+
+    <caret>try {
+        res = "success"
+    } catch (e: Exception) {
+        foo = "exception"
+    }
+}
@@ -0,0 +1,13 @@
+// PROBLEM: none
+fun doSomething() {}
+
+fun test() {
+    var res: String? = null
+
+    <caret>try {
+        res = "success"
+    } catch (e: Exception) {
+        res = "failure"
+        doSomething()
+    }
+}
@@ -0,0 +1 @@
+org.jetbrains.kotlin.idea.inspections.LiftReturnOrAssignmentInspection
@@ -0,0 +1,9 @@
+// HIGHLIGHT: INFORMATION
+
+fun test(): String {
+    <caret>try {
+        return "success"
+    } catch (e: Exception) {
+        throw e
+    }
+}
@@ -0,0 +1,9 @@
+// HIGHLIGHT: INFORMATION
+
+fun test(): String {
+    <caret>return try {
+        "success"
+    } catch (e: Exception) {
+        throw e
+    }
+}
@@ -0,0 +1,13 @@
+// HIGHLIGHT: GENERIC_ERROR_OR_WARNING
+
+fun doSomething() {}
+
+fun test(): String {
+    <caret>try {
+        doSomething()
+        return "success"
+    } catch (e: Exception) {
+        doSomething()
+        return "failure"
+    }
+}
@@ -0,0 +1,13 @@
+// HIGHLIGHT: GENERIC_ERROR_OR_WARNING
+
+fun doSomething() {}
+
+fun test(): String {
+    <caret>return try {
+        doSomething()
+        "success"
+    } catch (e: Exception) {
+        doSomething()
+        "failure"
+    }
+}
@@ -0,0 +1,16 @@
+// WITH_RUNTIME
+fun test(): String {
+    <caret>try {
+        try {
+            return "success"
+        } catch (e: Exception) {
+            TODO()
+        }
+    } catch (e: Exception) {
+        try {
+            TODO()
+        } catch (e: Exception) {
+            return "failure"
+        }
+    }
+}
@@ -0,0 +1,16 @@
+// WITH_RUNTIME
+fun test(): String {
+    <caret>return try {
+        try {
+            "success"
+        } catch (e: Exception) {
+            TODO()
+        }
+    } catch (e: Exception) {
+        try {
+            TODO()
+        } catch (e: Exception) {
+            "failure"
+        }
+    }
+}
@@ -0,0 +1,11 @@
+fun doSomething() {}
+
+fun test(): String {
+    <caret>try {
+        return "success"
+    } catch (e: Exception) {
+        return "failure"
+    } finally {
+        doSomething()
+    }
+}
@@ -0,0 +1,11 @@
+fun doSomething() {}
+
+fun test(): String {
+    <caret>return try {
+        "success"
+    } catch (e: Exception) {
+        "failure"
+    } finally {
+        doSomething()
+    }
+}
@@ -0,0 +1,14 @@
+fun doSomething() {}
+
+fun test(n: Int): String {
+    <caret>try {
+        return "success"
+    } catch (e: Exception) {
+        throw e
+    } finally {
+        if (n == 1)
+            doSomething()
+        else
+            return "finally"
+    }
+}
@@ -0,0 +1,14 @@
+fun doSomething() {}
+
+fun test(n: Int): String {
+    return try {
+        "success"
+    } catch (e: Exception) {
+        throw e
+    } finally {
+        if (n == 1)
+            doSomething()
+        else
+            return "finally"
+    }
+}
@@ -0,0 +1,10 @@
+// PROBLEM: none
+fun test(): String {
+    <caret>try {
+        return "success"
+    } catch (e: Exception) {
+        return "failure"
+    } finally {
+        return "finally"
+    }
+}
@@ -0,0 +1,12 @@
+fun test(n: Int): String {
+    if (n == 1) {
+        <caret>try {
+            return "success"
+        } catch (e: Exception) {
+            throw e
+        }
+    }
+    else {
+        return "else"
+    }
+}
@@ -0,0 +1,12 @@
+fun test(n: Int): String {
+    if (n == 1) {
+        <caret>return try {
+            "success"
+        } catch (e: Exception) {
+            throw e
+        }
+    }
+    else {
+        return "else"
+    }
+}
@@ -0,0 +1,11 @@
+// ERROR: A 'return' expression required in a function with a block body ('{...}')
+// PROBLEM: none
+fun doSomething() {}
+
+fun test(): String {
+    <caret>try {
+        return "success"
+    } catch (e: Exception) {
+        doSomething()
+    }
+}
@@ -629,6 +629,138 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
            }
        }

+        @TestMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment")
+        @TestDataPath("$PROJECT_ROOT")
+        @RunWith(JUnit3RunnerWithInners.class)
+        public static class TryToAssignment extends AbstractLocalInspectionTest {
+            public void testAllFilesPresentInTryToAssignment() throws Exception {
+                KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/inspectionsLocal/liftOut/tryToAssignment"), Pattern.compile("^([\\w\\-_]+)\\.kt$"), TargetBackend.ANY, true);
+            }
+
+            @TestMetadata("basic.kt")
+            public void testBasic() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/basic.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("block.kt")
+            public void testBlock() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/block.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("cascade.kt")
+            public void testCascade() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/cascade.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("finally.kt")
+            public void testFinally() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/finally.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("finallyWithAssignment.kt")
+            public void testFinallyWithAssignment() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/finallyWithAssignment.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("finallyWithCascadeAssignment.kt")
+            public void testFinallyWithCascadeAssignment() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/finallyWithCascadeAssignment.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("finallyWithCascadeUnmatchedAssignment.kt")
+            public void testFinallyWithCascadeUnmatchedAssignment() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/finallyWithCascadeUnmatchedAssignment.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("finallyWithUnmatchedAssignments.kt")
+            public void testFinallyWithUnmatchedAssignments() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/finallyWithUnmatchedAssignments.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("inner.kt")
+            public void testInner() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/inner.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("withUnmatchedAssignments.kt")
+            public void testWithUnmatchedAssignments() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/withUnmatchedAssignments.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("withoutTerminatingAssignment.kt")
+            public void testWithoutTerminatingAssignment() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToAssignment/withoutTerminatingAssignment.kt");
+                doTest(fileName);
+            }
+        }
+
+        @TestMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn")
+        @TestDataPath("$PROJECT_ROOT")
+        @RunWith(JUnit3RunnerWithInners.class)
+        public static class TryToReturn extends AbstractLocalInspectionTest {
+            public void testAllFilesPresentInTryToReturn() throws Exception {
+                KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/inspectionsLocal/liftOut/tryToReturn"), Pattern.compile("^([\\w\\-_]+)\\.kt$"), TargetBackend.ANY, true);
+            }
+
+            @TestMetadata("basic.kt")
+            public void testBasic() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn/basic.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("block.kt")
+            public void testBlock() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn/block.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("cascade.kt")
+            public void testCascade() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn/cascade.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("finally.kt")
+            public void testFinally() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn/finally.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("finallyWithCascadeReturn.kt")
+            public void testFinallyWithCascadeReturn() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn/finallyWithCascadeReturn.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("finallyWithReturn.kt")
+            public void testFinallyWithReturn() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn/finallyWithReturn.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("inner.kt")
+            public void testInner() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn/inner.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("withoutReturn.kt")
+            public void testWithoutReturn() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspectionsLocal/liftOut/tryToReturn/withoutReturn.kt");
+                doTest(fileName);
+            }
+        }
+
        @TestMetadata("idea/testData/inspectionsLocal/liftOut/whenToAssignment")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)
				`@@ -0,0 +1 @@`
				`org.jetbrains.kotlin.idea.inspections.LiftReturnOrAssignmentInspection`