From b8e1ce7a054c155c387a0d81be4444ec9dc75ad1 Mon Sep 17 00:00:00 2001 From: Denis Zharkov Date: Wed, 30 Nov 2016 17:36:18 +0300 Subject: [PATCH] Report error on non-tail suspend-calls It only concerns calls inside another suspend function #KT-14924 In Progress --- .../cfg/ControlFlowInformationProvider.kt | 78 ++++++++++++++----- ...ionDetector.java => TailCallDetector.java} | 4 +- .../jetbrains/kotlin/diagnostics/Errors.java | 1 + .../rendering/DefaultErrorMessages.java | 1 + .../coroutines/controlFlow/throwFromCatch.kt | 3 +- .../tests/coroutines/tailCalls/forbidden.kt | 17 ++++ .../tests/coroutines/tailCalls/forbidden.txt | 6 ++ .../coroutines/tailCalls/localFunctions.kt | 17 ++++ .../coroutines/tailCalls/localFunctions.txt | 5 ++ .../tailCalls/nothingTypedSuspendFunction.kt | 5 ++ .../tailCalls/nothingTypedSuspendFunction.txt | 3 + .../tests/coroutines/tailCalls/recursive.kt | 18 +++++ .../tests/coroutines/tailCalls/recursive.txt | 7 ++ .../tests/coroutines/tailCalls/tryCatch.kt | 39 ++++++++++ .../tests/coroutines/tailCalls/tryCatch.txt | 8 ++ .../tests/coroutines/tailCalls/valid.kt | 29 +++++++ .../tests/coroutines/tailCalls/valid.txt | 9 +++ .../checkers/DiagnosticsTestGenerated.java | 45 +++++++++++ 18 files changed, 272 insertions(+), 23 deletions(-) rename compiler/frontend/src/org/jetbrains/kotlin/cfg/{TailRecursionDetector.java => TailCallDetector.java} (92%) create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.kt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.txt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.kt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.txt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.kt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.txt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.kt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.txt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.kt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.txt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.kt create mode 100644 compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.txt diff --git a/compiler/frontend/src/org/jetbrains/kotlin/cfg/ControlFlowInformationProvider.kt b/compiler/frontend/src/org/jetbrains/kotlin/cfg/ControlFlowInformationProvider.kt index 58cc897b080..e16d9980672 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/cfg/ControlFlowInformationProvider.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/cfg/ControlFlowInformationProvider.kt @@ -113,7 +113,7 @@ class ControlFlowInformationProvider private constructor( checkDefiniteReturn(expectedReturnType ?: NO_EXPECTED_TYPE, unreachableCode) - markTailCalls() + markAndCheckTailCalls() } private fun collectReturnExpressions(returnedExpressions: MutableCollection) { @@ -798,19 +798,45 @@ class ControlFlowInformationProvider private constructor( //////////////////////////////////////////////////////////////////////////////// // Tail calls - private fun markTailCalls() { + private fun markAndCheckTailCalls() { val subroutineDescriptor = trace.get(BindingContext.DECLARATION_TO_DESCRIPTOR, subroutine) as? FunctionDescriptor ?: return + + markAndCheckRecursiveTailCalls(subroutineDescriptor) + checkSuspendCalls(subroutineDescriptor) + } + + private fun checkSuspendCalls(currentFunction: FunctionDescriptor) { + + if (!currentFunction.isSuspend) return + + traverseCalls { instruction, resolvedCall -> + val calleeDescriptor = resolvedCall.resultingDescriptor as? FunctionDescriptor ?: return@traverseCalls + if (!calleeDescriptor.isSuspend) return@traverseCalls + + // Suspend functions are allowed to be called only within coroutines (may be non-tail calls of course) + // or another suspend function (here they must be called only in tail position) + val enclosingSuspendFunction = + trace.get(BindingContext.ENCLOSING_SUSPEND_FUNCTION_FOR_SUSPEND_FUNCTION_CALL, resolvedCall.call) + ?.let(DescriptorToSourceUtils::descriptorToDeclaration) as? KtElement + ?: return@traverseCalls + + val element = instruction.element + val isUsedAsExpression = instruction.owner.getUsages(instruction.outputValue).isNotEmpty() + + if (!isUsedAsExpression || !instruction.isTailCall(enclosingSuspendFunction) || isInsideTry(element)) { + trace.report(Errors.SUSPENSION_CALL_MUST_BE_USED_AS_RETURN_VALUE.on(element)) + } + } + } + + private fun markAndCheckRecursiveTailCalls(subroutineDescriptor: FunctionDescriptor) { if (!subroutineDescriptor.isTailrec) return // finally blocks are copied which leads to multiple diagnostics reported on one instruction class KindAndCall(var kind: TailRecursionKind, internal val call: ResolvedCall<*>) val calls = HashMap() - pseudocode.traverse(TraversalOrder.FORWARD) { instruction -> - - if (instruction !is CallInstruction) return@traverse - val resolvedCall = instruction.element.getResolvedCall(trace.bindingContext) ?: return@traverse - + traverseCalls traverse@{ instruction, resolvedCall -> // is this a recursive call? val functionDescriptor = resolvedCall.resultingDescriptor if (functionDescriptor.original != subroutineDescriptor) return@traverse @@ -821,25 +847,14 @@ class ControlFlowInformationProvider private constructor( val element = instruction.element //noinspection unchecked - val parent = PsiTreeUtil.getParentOfType( - element, - KtTryExpression::class.java, KtFunction::class.java, KtAnonymousInitializer::class.java - ) - if (parent is KtTryExpression) { + if (isInsideTry(element)) { // We do not support tail calls Collections.singletonMap() try-catch-finally, for simplicity of the mental model // very few cases there would be real tail-calls, and it's often not so easy for the user to see why calls.put(element, KindAndCall(IN_TRY, resolvedCall)) return@traverse } - val isTail = traverseFollowingInstructions( - instruction, - HashSet(), - TraversalOrder.FORWARD, - TailRecursionDetector(subroutine, instruction) - ) - // A tail call is not allowed to change dispatch receiver // class C { // fun foo(other: C) { @@ -848,7 +863,7 @@ class ControlFlowInformationProvider private constructor( // } val sameDispatchReceiver = resolvedCall.hasThisOrNoDispatchReceiver(trace.bindingContext) - val kind = if (isTail && sameDispatchReceiver) TAIL_CALL else NON_TAIL + val kind = if (sameDispatchReceiver && instruction.isTailCall()) TAIL_CALL else NON_TAIL val kindAndCall = calls[element] calls.put(element, KindAndCall(combineKinds(kind, kindAndCall?.kind), resolvedCall)) @@ -871,6 +886,29 @@ class ControlFlowInformationProvider private constructor( } } + private fun isInsideTry(element: KtElement) = + PsiTreeUtil.getParentOfType( + element, + KtTryExpression::class.java, KtFunction::class.java, KtAnonymousInitializer::class.java + ) is KtTryExpression + + private fun CallInstruction.isTailCall(subroutine: KtElement = this@ControlFlowInformationProvider.subroutine) = + traverseFollowingInstructions( + this, + HashSet(), + TraversalOrder.FORWARD, + TailCallDetector(subroutine, this) + ) + + private inline fun traverseCalls(crossinline onCall: (instruction: CallInstruction, resolvedCall: ResolvedCall<*>) -> Unit) { + pseudocode.traverse(TraversalOrder.FORWARD) { instruction -> + if (instruction !is CallInstruction) return@traverse + val resolvedCall = instruction.element.getResolvedCall(trace.bindingContext) ?: return@traverse + + onCall(instruction, resolvedCall) + } + } + //////////////////////////////////////////////////////////////////////////////// // Utility classes and methods diff --git a/compiler/frontend/src/org/jetbrains/kotlin/cfg/TailRecursionDetector.java b/compiler/frontend/src/org/jetbrains/kotlin/cfg/TailCallDetector.java similarity index 92% rename from compiler/frontend/src/org/jetbrains/kotlin/cfg/TailRecursionDetector.java rename to compiler/frontend/src/org/jetbrains/kotlin/cfg/TailCallDetector.java index e7fd4fd0cae..40d9d4649c9 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/cfg/TailRecursionDetector.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/cfg/TailCallDetector.java @@ -30,11 +30,11 @@ import org.jetbrains.kotlin.cfg.pseudocode.instructions.special.SubroutineSinkIn import org.jetbrains.kotlin.cfg.pseudocodeTraverser.TraverseInstructionResult; import org.jetbrains.kotlin.psi.KtElement; -public class TailRecursionDetector extends InstructionVisitorWithResult implements Function1 { +public class TailCallDetector extends InstructionVisitorWithResult implements Function1 { private final KtElement subroutine; private final Instruction start; - public TailRecursionDetector(@NotNull KtElement subroutine, @NotNull Instruction start) { + public TailCallDetector(@NotNull KtElement subroutine, @NotNull Instruction start) { this.subroutine = subroutine; this.start = start; } diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java index a9eb7d888ae..60f6555dd05 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java @@ -863,6 +863,7 @@ public interface Errors { DiagnosticFactory0 NON_LOCAL_SUSPENSION_POINT = DiagnosticFactory0.create(ERROR); DiagnosticFactory0 ILLEGAL_SUSPEND_FUNCTION_CALL = DiagnosticFactory0.create(ERROR); + DiagnosticFactory0 SUSPENSION_CALL_MUST_BE_USED_AS_RETURN_VALUE = DiagnosticFactory0.create(ERROR); // Error sets diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java index a537e69ce50..aa44ace030f 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java @@ -821,6 +821,7 @@ public class DefaultErrorMessages { MAP.put(NON_LOCAL_RETURN_IN_DISABLED_INLINE, "Non-local returns are not allowed with inlining disabled"); MAP.put(NON_LOCAL_SUSPENSION_POINT, "Suspension functions can be called only within coroutine body"); MAP.put(ILLEGAL_SUSPEND_FUNCTION_CALL, "Suspend functions are only allowed to be called from a coroutine or another suspend function"); + MAP.put(SUSPENSION_CALL_MUST_BE_USED_AS_RETURN_VALUE, "Only tail calls are allowed to another suspension function, and these calls must be used as return values"); MAP.setImmutable(); diff --git a/compiler/testData/codegen/box/coroutines/controlFlow/throwFromCatch.kt b/compiler/testData/codegen/box/coroutines/controlFlow/throwFromCatch.kt index e7e5f821519..8f317fccb51 100644 --- a/compiler/testData/codegen/box/coroutines/controlFlow/throwFromCatch.kt +++ b/compiler/testData/codegen/box/coroutines/controlFlow/throwFromCatch.kt @@ -11,7 +11,8 @@ class Controller { Suspend } - suspend fun suspendLogAndThrow(exception: Throwable): Nothing = suspendWithCurrentContinuation { c -> + // Tail calls are not allowed to be Nothing typed. See KT-15051 + suspend fun suspendLogAndThrow(exception: Throwable): Any? = suspendWithCurrentContinuation { c -> result += "throw(${exception.message});" c.resumeWithException(exception) Suspend diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.kt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.kt new file mode 100644 index 00000000000..be0228355d7 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.kt @@ -0,0 +1,17 @@ +// !DIAGNOSTICS: -UNUSED_PARAMETER + +fun nonSuspend() {} + +suspend fun foo() { + suspendWithCurrentContinuation { x: Continuation -> } + + nonSuspend() +} + +suspend fun unitSuspend() { + suspendWithCurrentContinuation { x: Continuation -> } +} + +suspend fun baz(): Int = run { + suspendWithCurrentContinuation { x: Continuation -> } +} diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.txt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.txt new file mode 100644 index 00000000000..e9bde98f2e9 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.txt @@ -0,0 +1,6 @@ +package + +public suspend fun baz(): kotlin.Int +public suspend fun foo(): kotlin.Unit +public fun nonSuspend(): kotlin.Unit +public suspend fun unitSuspend(): kotlin.Unit diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.kt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.kt new file mode 100644 index 00000000000..1f3218ce667 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.kt @@ -0,0 +1,17 @@ +suspend fun baz() = 1 +suspend fun unit() {} + +suspend fun foo() { + suspend fun bar() { + baz() + return unit() + } + + suspend fun foobar1(): Int { + return baz() + } + + suspend fun foobar2() { + return unit() + } +} diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.txt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.txt new file mode 100644 index 00000000000..7b4b856c745 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.txt @@ -0,0 +1,5 @@ +package + +public suspend fun baz(): kotlin.Int +public suspend fun foo(): kotlin.Unit +public suspend fun unit(): kotlin.Unit diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.kt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.kt new file mode 100644 index 00000000000..75526fb1205 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.kt @@ -0,0 +1,5 @@ +// Tail calls are not allowed to be Nothing typed. See KT-15051 +suspend fun suspendLogAndThrow(exception: Throwable): Nothing = suspendWithCurrentContinuation { c -> + c.resumeWithException(exception) + Suspend +} diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.txt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.txt new file mode 100644 index 00000000000..662ae2a6501 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.txt @@ -0,0 +1,3 @@ +package + +public suspend fun suspendLogAndThrow(/*0*/ exception: kotlin.Throwable): kotlin.Nothing diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.kt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.kt new file mode 100644 index 00000000000..3684c3c556f --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.kt @@ -0,0 +1,18 @@ +suspend fun unit1() { + unit1() +} + +suspend fun unit2() { + return unit2() +} + +suspend fun int1(): Int { + return int1() +} + +suspend fun int2(): Int = int2() + +suspend fun int3(): Int { + int3() + return int3() +} diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.txt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.txt new file mode 100644 index 00000000000..3810807a13b --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.txt @@ -0,0 +1,7 @@ +package + +public suspend fun int1(): kotlin.Int +public suspend fun int2(): kotlin.Int +public suspend fun int3(): kotlin.Int +public suspend fun unit1(): kotlin.Unit +public suspend fun unit2(): kotlin.Unit diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.kt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.kt new file mode 100644 index 00000000000..0d79dfa4401 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.kt @@ -0,0 +1,39 @@ +// !DIAGNOSTICS: -UNUSED_PARAMETER + +fun nonSuspend() {} + +suspend fun baz(): Int = 1 + +suspend fun tryCatch(): Int { + return try { + suspendWithCurrentContinuation { x: Continuation -> } + } catch (e: Exception) { + baz() // another suspend function + } +} + +suspend fun tryFinally(): Int { + return try { + suspendWithCurrentContinuation { x: Continuation -> } + } finally { + nonSuspend() + } +} + +suspend fun returnInFinally(): Int { + try { + } finally { + // Probably this is too restrictive, but it does not matter much + return baz() + } +} + +suspend fun tryCatchFinally(): Int { + return try { + suspendWithCurrentContinuation { x: Continuation -> } + } catch (e: Exception) { + baz() // another suspend function + } finally { + baz() + } +} diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.txt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.txt new file mode 100644 index 00000000000..8ecd251d2e1 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.txt @@ -0,0 +1,8 @@ +package + +public suspend fun baz(): kotlin.Int +public fun nonSuspend(): kotlin.Unit +public suspend fun returnInFinally(): kotlin.Int +public suspend fun tryCatch(): kotlin.Int +public suspend fun tryCatchFinally(): kotlin.Int +public suspend fun tryFinally(): kotlin.Int diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.kt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.kt new file mode 100644 index 00000000000..d7fcab941b4 --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.kt @@ -0,0 +1,29 @@ +// !DIAGNOSTICS: -UNUSED_PARAMETER + +suspend fun baz() = 1 + +suspend fun foo() {} + +suspend fun bar0() = baz() +suspend fun bar01(): Int { + return baz() +} + +suspend fun bar1() { + return if (1.hashCode() > 0) { + foo() + } + else suspendWithCurrentContinuation { x: Continuation -> } +} + +suspend fun bar2() = + if (1.hashCode() > 0) { + foo() + } + else suspendWithCurrentContinuation { x: Continuation -> } + +suspend fun bar3() = + when { + true -> { foo() } + else -> suspendWithCurrentContinuation { x: Continuation -> } + } diff --git a/compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.txt b/compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.txt new file mode 100644 index 00000000000..44b32385fab --- /dev/null +++ b/compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.txt @@ -0,0 +1,9 @@ +package + +public suspend fun bar0(): kotlin.Int +public suspend fun bar01(): kotlin.Int +public suspend fun bar1(): kotlin.Unit +public suspend fun bar2(): kotlin.Unit +public suspend fun bar3(): kotlin.Unit +public suspend fun baz(): kotlin.Int +public suspend fun foo(): kotlin.Unit diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java index 6c8ffe6a6f4..e9833b71c64 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java @@ -4335,6 +4335,51 @@ public class DiagnosticsTestGenerated extends AbstractDiagnosticsTest { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/coroutines/wrongHandleResult.kt"); doTest(fileName); } + + @TestMetadata("compiler/testData/diagnostics/tests/coroutines/tailCalls") + @TestDataPath("$PROJECT_ROOT") + @RunWith(JUnit3RunnerWithInners.class) + public static class TailCalls extends AbstractDiagnosticsTest { + public void testAllFilesPresentInTailCalls() throws Exception { + KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/diagnostics/tests/coroutines/tailCalls"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.ANY, true); + } + + @TestMetadata("forbidden.kt") + public void testForbidden() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/coroutines/tailCalls/forbidden.kt"); + doTest(fileName); + } + + @TestMetadata("localFunctions.kt") + public void testLocalFunctions() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/coroutines/tailCalls/localFunctions.kt"); + doTest(fileName); + } + + @TestMetadata("nothingTypedSuspendFunction.kt") + public void testNothingTypedSuspendFunction() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/coroutines/tailCalls/nothingTypedSuspendFunction.kt"); + doTest(fileName); + } + + @TestMetadata("recursive.kt") + public void testRecursive() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/coroutines/tailCalls/recursive.kt"); + doTest(fileName); + } + + @TestMetadata("tryCatch.kt") + public void testTryCatch() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/coroutines/tailCalls/tryCatch.kt"); + doTest(fileName); + } + + @TestMetadata("valid.kt") + public void testValid() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/coroutines/tailCalls/valid.kt"); + doTest(fileName); + } + } } @TestMetadata("compiler/testData/diagnostics/tests/cyclicHierarchy")