From 7e6df034212735b38821c1952cc2c34a7d805209 Mon Sep 17 00:00:00 2001 From: Alexey Andreev Date: Thu, 13 Apr 2017 12:40:09 +0300 Subject: [PATCH] Fix improper handling of coroutine interceptors Don't use coroutine facade in following cases: * When calling coroutine functions in non-suspend mode (i.e. suspend flag is either false or skipped). * When passing continuation from suspend function to suspend function. Use facade only for corresponding intrinsics. Reason: interceptor should not be called on each suspend function invocation, it should be called after resume from innermost suspend function. Fix KT-17067. The example provided with the issue is very similar to dispatchResume.kt which passes after these changes. Add test to prove that KT-17446 is no more reproducible. --- ...outineUninterceptedOrReturnInterception.kt | 2 +- .../suspendCoroutineFromStateMachine.kt | 48 +++++++++++++++++++ .../dispatchResume.kt | 2 +- .../ir/IrBlackBoxCodegenTestGenerated.java | 6 +++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 +++ .../LightAnalysisModeTestGenerated.java | 6 +++ .../ast/metadata/metadataProperties.kt | 2 - .../coroutine/CoroutineFunctionTransformer.kt | 2 +- .../jetbrains/kotlin/js/inline/JsInliner.java | 10 +--- .../src/core/coroutinesIntrinsics.kt | 4 +- .../semantics/JsCodegenBoxTestGenerated.java | 22 ++++----- .../declaration/AbstractDeclarationVisitor.kt | 2 +- .../expression/LiteralFunctionTranslator.kt | 2 +- .../reference/CallArgumentTranslator.kt | 9 +++- .../kotlin/js/translate/utils/utils.kt | 4 +- 15 files changed, 91 insertions(+), 36 deletions(-) create mode 100644 compiler/testData/codegen/box/coroutines/suspendCoroutineFromStateMachine.kt diff --git a/compiler/testData/codegen/box/coroutines/intrinsicSemantics/startCoroutineUninterceptedOrReturnInterception.kt b/compiler/testData/codegen/box/coroutines/intrinsicSemantics/startCoroutineUninterceptedOrReturnInterception.kt index ac80967bc34..acf58ce553e 100644 --- a/compiler/testData/codegen/box/coroutines/intrinsicSemantics/startCoroutineUninterceptedOrReturnInterception.kt +++ b/compiler/testData/codegen/box/coroutines/intrinsicSemantics/startCoroutineUninterceptedOrReturnInterception.kt @@ -1,6 +1,6 @@ // WITH_RUNTIME // WITH_COROUTINES -// IGNORE_BACKEND: JS, NATIVE +// IGNORE_BACKEND: NATIVE import kotlin.coroutines.experimental.* import kotlin.coroutines.experimental.intrinsics.* import kotlin.test.assertEquals diff --git a/compiler/testData/codegen/box/coroutines/suspendCoroutineFromStateMachine.kt b/compiler/testData/codegen/box/coroutines/suspendCoroutineFromStateMachine.kt new file mode 100644 index 00000000000..650eb832356 --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/suspendCoroutineFromStateMachine.kt @@ -0,0 +1,48 @@ +// IGNORE_BACKEND: NATIVE +// WITH_RUNTIME +// WITH_COROUTINES +import kotlin.coroutines.experimental.* +import kotlin.coroutines.experimental.intrinsics.suspendCoroutineOrReturn +import kotlin.coroutines.experimental.intrinsics.COROUTINE_SUSPENDED + +fun box(): String { + async { + log += "1;" + wait() + log += "2;" + } + + while (postponed != null) { + log += "suspended;" + postponed!!() + } + + if (log != "1;wait2;suspended;wait;suspended;2;") return "fail: $log" + + return "OK" +} + +fun async(f: suspend () -> Unit) { + f.startCoroutine(handleResultContinuation { + postponed = null + }) +} + +suspend fun wait(): Unit { + wait2() + log += "wait;" + return suspendCoroutineOrReturn { c -> + postponed = { c.resume(Unit) } + COROUTINE_SUSPENDED + } +} + +suspend fun wait2(): Unit = suspendCoroutineOrReturn { c -> + log += "wait2;" + postponed = { c.resume(Unit) } + COROUTINE_SUSPENDED +} + +var postponed: (() -> Unit)? = { } + +var log = "" \ No newline at end of file diff --git a/compiler/testData/codegen/box/coroutines/suspendFunctionAsCoroutine/dispatchResume.kt b/compiler/testData/codegen/box/coroutines/suspendFunctionAsCoroutine/dispatchResume.kt index 46a70de7856..0037fcda1dc 100644 --- a/compiler/testData/codegen/box/coroutines/suspendFunctionAsCoroutine/dispatchResume.kt +++ b/compiler/testData/codegen/box/coroutines/suspendFunctionAsCoroutine/dispatchResume.kt @@ -1,4 +1,4 @@ -// IGNORE_BACKEND: JS, NATIVE +// IGNORE_BACKEND: NATIVE // WITH_RUNTIME // WITH_COROUTINES import kotlin.coroutines.experimental.* diff --git a/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java b/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java index 10bcf64b91c..80b1bde6a90 100644 --- a/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java @@ -5150,6 +5150,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes doTest(fileName); } + @TestMetadata("suspendCoroutineFromStateMachine.kt") + public void testSuspendCoroutineFromStateMachine() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendCoroutineFromStateMachine.kt"); + doTest(fileName); + } + @TestMetadata("suspendDefaultImpl.kt") public void testSuspendDefaultImpl() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendDefaultImpl.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index 95652efcd78..428dbccd3ea 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -5150,6 +5150,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { doTest(fileName); } + @TestMetadata("suspendCoroutineFromStateMachine.kt") + public void testSuspendCoroutineFromStateMachine() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendCoroutineFromStateMachine.kt"); + doTest(fileName); + } + @TestMetadata("suspendDefaultImpl.kt") public void testSuspendDefaultImpl() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendDefaultImpl.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 52b0163942f..4f2ab3eed93 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -5150,6 +5150,12 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes doTest(fileName); } + @TestMetadata("suspendCoroutineFromStateMachine.kt") + public void testSuspendCoroutineFromStateMachine() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendCoroutineFromStateMachine.kt"); + doTest(fileName); + } + @TestMetadata("suspendDefaultImpl.kt") public void testSuspendDefaultImpl() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendDefaultImpl.kt"); diff --git a/js/js.ast/src/org/jetbrains/kotlin/js/backend/ast/metadata/metadataProperties.kt b/js/js.ast/src/org/jetbrains/kotlin/js/backend/ast/metadata/metadataProperties.kt index d37179da43e..8a964e6e5bc 100644 --- a/js/js.ast/src/org/jetbrains/kotlin/js/backend/ast/metadata/metadataProperties.kt +++ b/js/js.ast/src/org/jetbrains/kotlin/js/backend/ast/metadata/metadataProperties.kt @@ -107,10 +107,8 @@ class CoroutineMetadata( val finallyPathName: JsName, val resultName: JsName, val exceptionName: JsName, - val facadeName: JsName, val baseClassRef: JsExpression, val suspendObjectRef: JsExpression, - val isLambda: Boolean, val hasController: Boolean, val hasReceiver: Boolean ) diff --git a/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineFunctionTransformer.kt b/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineFunctionTransformer.kt index 788d5c9eaaf..d5005f119c9 100644 --- a/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineFunctionTransformer.kt +++ b/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineFunctionTransformer.kt @@ -161,7 +161,7 @@ class CoroutineFunctionTransformer(private val program: JsProgram, private val f functionWithBody.parameters += JsParameter(suspendedName) val instanceName = functionWithBody.scope.declareFreshName("instance") - functionWithBody.body.statements += JsAstUtils.newVar(instanceName, JsNameRef(context.metadata.facadeName, instantiation)) + functionWithBody.body.statements += JsAstUtils.newVar(instanceName, instantiation) val invokeResume = JsReturn(JsInvocation(JsNameRef(context.metadata.doResumeName, instanceName.makeRef()), JsLiteral.NULL)) diff --git a/js/js.inliner/src/org/jetbrains/kotlin/js/inline/JsInliner.java b/js/js.inliner/src/org/jetbrains/kotlin/js/inline/JsInliner.java index 35c7eecefcd..3040d2b5ff7 100644 --- a/js/js.inliner/src/org/jetbrains/kotlin/js/inline/JsInliner.java +++ b/js/js.inliner/src/org/jetbrains/kotlin/js/inline/JsInliner.java @@ -213,12 +213,10 @@ public class JsInliner extends JsVisitorWithContextImpl { } private void inlineSuspendWithCurrentContinuation(@NotNull JsInvocation call, @NotNull JsContext context) { - JsInliningContext inliningContext = getInliningContext(); - JsFunction containingFunction = inliningContext.function; JsExpression lambda = call.getArguments().get(0); - JsParameter continuationParam = containingFunction.getParameters().get(containingFunction.getParameters().size() - 1); + JsExpression continuationArg = call.getArguments().get(call.getArguments().size() - 1); - JsInvocation invocation = new JsInvocation(lambda, continuationParam.getName().makeRef()); + JsInvocation invocation = new JsInvocation(lambda, continuationArg); MetadataProperties.setSuspend(invocation, true); context.replaceMe(accept(invocation)); } @@ -268,11 +266,7 @@ public class JsInliner extends JsVisitorWithContextImpl { private class JsInliningContext implements InliningContext { private final FunctionContext functionContext; - @NotNull - public final JsFunction function; - JsInliningContext(@NotNull JsFunction function) { - this.function = function; functionContext = new FunctionContext(function, functionReader) { @Nullable @Override diff --git a/js/js.libraries/src/core/coroutinesIntrinsics.kt b/js/js.libraries/src/core/coroutinesIntrinsics.kt index 8fa75b06965..0a129c2350e 100644 --- a/js/js.libraries/src/core/coroutinesIntrinsics.kt +++ b/js/js.libraries/src/core/coroutinesIntrinsics.kt @@ -37,9 +37,9 @@ public inline fun (suspend R.() -> T).startCoroutineUninterceptedOrReturn public fun (suspend R.() -> T).createCoroutineUnchecked( receiver: R, completion: Continuation -): Continuation = this.asDynamic()(receiver, completion, true) +): Continuation = this.asDynamic()(receiver, completion, true).facade @SinceKotlin("1.1") public fun (suspend () -> T).createCoroutineUnchecked( completion: Continuation -): Continuation = this.asDynamic()(completion, true) +): Continuation = this.asDynamic()(completion, true).facade diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java index fded6e22a09..3e945a243e8 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java @@ -5865,6 +5865,12 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest { doTest(fileName); } + @TestMetadata("suspendCoroutineFromStateMachine.kt") + public void testSuspendCoroutineFromStateMachine() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendCoroutineFromStateMachine.kt"); + doTest(fileName); + } + @TestMetadata("suspendDefaultImpl.kt") public void testSuspendDefaultImpl() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendDefaultImpl.kt"); @@ -6143,13 +6149,7 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest { @TestMetadata("startCoroutineUninterceptedOrReturnInterception.kt") public void testStartCoroutineUninterceptedOrReturnInterception() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/intrinsicSemantics/startCoroutineUninterceptedOrReturnInterception.kt"); - try { - doTest(fileName); - } - catch (Throwable ignore) { - return; - } - throw new AssertionError("Looks like this test can be unmuted. Remove IGNORE_BACKEND directive for that."); + doTest(fileName); } } @@ -6224,13 +6224,7 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest { @TestMetadata("dispatchResume.kt") public void testDispatchResume() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendFunctionAsCoroutine/dispatchResume.kt"); - try { - doTest(fileName); - } - catch (Throwable ignore) { - return; - } - throw new AssertionError("Looks like this test can be unmuted. Remove IGNORE_BACKEND directive for that."); + doTest(fileName); } @TestMetadata("handleException.kt") diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/declaration/AbstractDeclarationVisitor.kt b/js/js.translator/src/org/jetbrains/kotlin/js/translate/declaration/AbstractDeclarationVisitor.kt index 6dc77a8e09f..fb3a691cfd1 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/declaration/AbstractDeclarationVisitor.kt +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/declaration/AbstractDeclarationVisitor.kt @@ -102,7 +102,7 @@ abstract class AbstractDeclarationVisitor : TranslatorVisitor() { if (descriptor.isSuspend) { if (descriptor.requiresStateMachineTransformation(context)) { - function.fillCoroutineMetadata(context, descriptor, hasController = false, isLambda = false) + function.fillCoroutineMetadata(context, descriptor, hasController = false) } } diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/expression/LiteralFunctionTranslator.kt b/js/js.translator/src/org/jetbrains/kotlin/js/translate/expression/LiteralFunctionTranslator.kt index 57f783d6b19..875d041c203 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/expression/LiteralFunctionTranslator.kt +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/expression/LiteralFunctionTranslator.kt @@ -92,7 +92,7 @@ class LiteralFunctionTranslator(context: TranslationContext) : AbstractTranslato fun JsFunction.fillCoroutineMetadata(context: TranslationContext, descriptor: FunctionDescriptor) { if (!descriptor.isSuspend) return - fillCoroutineMetadata(context, descriptor, hasController = descriptor.extensionReceiverParameter != null, isLambda = true) + fillCoroutineMetadata(context, descriptor, hasController = descriptor.extensionReceiverParameter != null) } fun ValueParameterDescriptorImpl.WithDestructuringDeclaration.translate(context: TranslationContext): JsVars { diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/reference/CallArgumentTranslator.kt b/js/js.translator/src/org/jetbrains/kotlin/js/translate/reference/CallArgumentTranslator.kt index 24c9a99b728..8f2ebfb3988 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/reference/CallArgumentTranslator.kt +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/reference/CallArgumentTranslator.kt @@ -16,6 +16,7 @@ package org.jetbrains.kotlin.js.translate.reference +import org.jetbrains.kotlin.backend.common.isBuiltInSuspendCoroutineOrReturn import org.jetbrains.kotlin.builtins.KotlinBuiltIns import org.jetbrains.kotlin.builtins.PrimitiveType import org.jetbrains.kotlin.descriptors.FunctionDescriptor @@ -154,8 +155,12 @@ class CallArgumentTranslator private constructor( val callableDescriptor = resolvedCall.resultingDescriptor if (callableDescriptor is FunctionDescriptor && callableDescriptor.isSuspend) { - val facadeName = context().getNameForDescriptor(TranslationUtils.getCoroutineProperty(context(), "facade")) - result.add(JsAstUtils.pureFqn(facadeName, TranslationUtils.translateContinuationArgument(context()))) + var continuationArg: JsExpression = TranslationUtils.translateContinuationArgument(context()) + if (callableDescriptor.original.isBuiltInSuspendCoroutineOrReturn()) { + val facadeName = context().getNameForDescriptor(TranslationUtils.getCoroutineProperty(context(), "facade")) + continuationArg = JsAstUtils.pureFqn(facadeName, continuationArg) + } + result.add(continuationArg) } removeLastUndefinedArguments(result) diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/utils.kt b/js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/utils.kt index b6f35e5cd5f..b1e1de7ca30 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/utils.kt +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/utils.kt @@ -144,7 +144,7 @@ fun FunctionDescriptor.requiresStateMachineTransformation(context: TranslationCo fun JsFunction.fillCoroutineMetadata( context: TranslationContext, descriptor: FunctionDescriptor, - hasController: Boolean, isLambda: Boolean + hasController: Boolean ) { if (!descriptor.requiresStateMachineTransformation(context)) return @@ -166,9 +166,7 @@ fun JsFunction.fillCoroutineMetadata( finallyPathName = getCoroutinePropertyName("finallyPath"), resultName = getCoroutinePropertyName("result"), exceptionName = getCoroutinePropertyName("exception"), - facadeName = getCoroutinePropertyName("facade"), hasController = hasController, - isLambda = isLambda, hasReceiver = descriptor.dispatchReceiverParameter != null ) }