From e2dc7ba37edb639924c3ef14f0453855d48927c6 Mon Sep 17 00:00:00 2001 From: Alexey Andreev Date: Wed, 16 Nov 2016 12:13:30 +0300 Subject: [PATCH] JS: coroutines: fixes after code review --- .../coroutines/controlFlow/breakFinally.kt | 3 +- .../box/coroutines/controlFlow/forContinue.kt | 28 +++++++++ .../coroutines/controlFlow/switchLikeWhen.kt | 34 +++++++++++ .../ir/IrBlackBoxCodegenTestGenerated.java | 6 ++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 ++ .../js/coroutine/CoroutineBodyTransformer.kt | 58 ++++++++++--------- .../kotlin/js/coroutine/CoroutinePasses.kt | 53 ++++++----------- .../kotlin/js/inline/util/collectUtils.kt | 32 +++++----- .../semantics/JsCodegenBoxTestGenerated.java | 12 ++++ .../js/translate/context/UsageTracker.kt | 9 +-- 10 files changed, 156 insertions(+), 85 deletions(-) create mode 100644 compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt create mode 100644 compiler/testData/codegen/box/coroutines/controlFlow/switchLikeWhen.kt diff --git a/compiler/testData/codegen/box/coroutines/controlFlow/breakFinally.kt b/compiler/testData/codegen/box/coroutines/controlFlow/breakFinally.kt index 32996e58a76..64817dab553 100644 --- a/compiler/testData/codegen/box/coroutines/controlFlow/breakFinally.kt +++ b/compiler/testData/codegen/box/coroutines/controlFlow/breakFinally.kt @@ -38,13 +38,14 @@ fun box(): String { } result += "ignore" } + result += "*" } finally { result += "finally" } result += "." } - if (value != "AC!ED!@finally.") return "fail: $value" + if (value != "AC!ED!@*finally.") return "fail: $value" return "OK" } \ No newline at end of file diff --git a/compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt b/compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt new file mode 100644 index 00000000000..fba1e746198 --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt @@ -0,0 +1,28 @@ +// WITH_RUNTIME + +class Controller { + var result = "" + + suspend fun suspendWithResult(value: T, c: Continuation) { + c.resume(value) + } +} + +fun builder(coroutine c: Controller.() -> Continuation): String { + val controller = Controller() + c(controller).resume(Unit) + return controller.result +} + +fun box(): String { + val value = builder { + for (x in listOf("O", "$", "K")) { + if (x == "$") continue + result += suspendWithResult(x) + } + result += "." + } + if (value != "OK.") return "fail: suspend in for body: $value" + + return "OK" +} \ No newline at end of file diff --git a/compiler/testData/codegen/box/coroutines/controlFlow/switchLikeWhen.kt b/compiler/testData/codegen/box/coroutines/controlFlow/switchLikeWhen.kt new file mode 100644 index 00000000000..a757817e081 --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/controlFlow/switchLikeWhen.kt @@ -0,0 +1,34 @@ +// WITH_RUNTIME + +// TODO: fix bug in JVM backend and remove this directive +// TARGET_BACKEND: JS + +class Controller { + var result = "" + + suspend fun suspendWithResult(value: T, c: Continuation) { + result += "[" + c.resume(value) + } +} + +fun builder(coroutine c: Controller.() -> Continuation): String { + val controller = Controller() + c(controller).resume(Unit) + return controller.result +} + +fun box(): String { + var value = builder { + for (v in listOf("A", "B", "C")) { + when (v) { + "A" -> result += "A;" + "B" -> result += suspendWithResult(v) + "]" + else -> result += suspendWithResult(v) + "]!" + } + } + } + if (value != "A;[B][C]!") return "fail: suspend as if condition: $value" + + return "OK" +} \ No newline at end of file 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 e581c7c4f74..d44091a5cab 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 @@ -4744,6 +4744,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes doTest(fileName); } + @TestMetadata("forContinue.kt") + public void testForContinue() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt"); + doTest(fileName); + } + @TestMetadata("forStatement.kt") public void testForStatement() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forStatement.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index ffc713e9199..14614dcbda1 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -4744,6 +4744,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { doTest(fileName); } + @TestMetadata("forContinue.kt") + public void testForContinue() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt"); + doTest(fileName); + } + @TestMetadata("forStatement.kt") public void testForStatement() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forStatement.kt"); diff --git a/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineBodyTransformer.kt b/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineBodyTransformer.kt index 01cb0a6ce94..13d65f08786 100644 --- a/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineBodyTransformer.kt +++ b/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineBodyTransformer.kt @@ -112,29 +112,28 @@ class CoroutineBodyTransformer( } override fun visitWhile(x: JsWhile) = splitIfNecessary(x) { - val predecessor = currentBlock val successor = CoroutineBlock() - val bodyEntryBlock = CoroutineBlock() + currentStatements += stateAndJump(bodyEntryBlock) + currentBlock = bodyEntryBlock + if (x.condition != JsLiteral.TRUE) { + currentStatements += JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source } + } + withBreakAndContinue(x, successor, bodyEntryBlock) { x.body.accept(this) } - if (x.condition != JsLiteral.TRUE) { - val jsIf = JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source } - bodyEntryBlock.statements.add(0, jsIf) - } - currentBlock.statements += stateAndJump(bodyEntryBlock) - predecessor.statements += stateAndJump(bodyEntryBlock) + currentStatements += stateAndJump(bodyEntryBlock) currentBlock = successor } override fun visitDoWhile(x: JsDoWhile) = splitIfNecessary(x) { - val predecessor = currentBlock val successor = CoroutineBlock() - val bodyEntryBlock = CoroutineBlock() + currentStatements += stateAndJump(bodyEntryBlock) + currentBlock = bodyEntryBlock withBreakAndContinue(x, successor, bodyEntryBlock) { x.body.accept(this) @@ -142,10 +141,10 @@ class CoroutineBodyTransformer( if (x.condition != JsLiteral.TRUE) { val jsIf = JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source } - currentBlock.statements.add(jsIf) + currentStatements.add(jsIf) } currentBlock.statements += stateAndJump(bodyEntryBlock) - predecessor.statements += stateAndJump(bodyEntryBlock) + currentBlock = successor } @@ -159,29 +158,26 @@ class CoroutineBodyTransformer( } } - val predecessor = currentBlock val increment = CoroutineBlock() val successor = CoroutineBlock() - val bodyEntryBlock = CoroutineBlock() + currentStatements += stateAndJump(bodyEntryBlock) + currentBlock = bodyEntryBlock - withBreakAndContinue(x, successor, predecessor) { + if (x.condition != null && x.condition != JsLiteral.TRUE) { + currentStatements += JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source } + } + + withBreakAndContinue(x, successor, increment) { x.body.accept(this) } - val bodyExitBlock = currentBlock - if (x.condition != null && x.condition != JsLiteral.TRUE) { - val jsIf = JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source } - bodyEntryBlock.statements.add(0, jsIf) - } - - bodyExitBlock.statements += stateAndJump(increment) + currentStatements += stateAndJump(increment) currentBlock = increment x.incrementExpression?.let { JsExpressionStatement(it).accept(this) } currentStatements += stateAndJump(bodyEntryBlock) - predecessor.statements += stateAndJump(bodyEntryBlock) currentBlock = successor } @@ -201,6 +197,10 @@ class CoroutineBodyTransformer( currentStatements += jump() } + /** + * When we perform break, continue or return, we can leave try blocks, so we should update $exceptionHandler correspondingly. + * Also, these try blocks can contain finally clauses, therefore we need to update $finallyPath as well. + */ private fun jumpWithFinally(targetTryDepth: Int, successor: CoroutineBlock) { if (targetTryDepth < tryStack.size) { val tryBlock = tryStack[targetTryDepth] @@ -223,15 +223,17 @@ class CoroutineBodyTransformer( val catchBlock = CoroutineBlock() val finallyBlock = CoroutineBlock() - val tryBlock = TryBlock(catchBlock, if (finallyNode != null) finallyBlock else null) - tryStack += tryBlock + tryStack += TryBlock(catchBlock, if (finallyNode != null) finallyBlock else null) val oldCatchBlock = currentCatchBlock currentCatchBlock = catchBlock currentStatements += exceptionState(catchBlock) x.tryBlock.statements.forEach { it.accept(this) } + currentStatements += exceptionState(oldCatchBlock) + currentCatchBlock = oldCatchBlock + if (finallyNode != null) { currentStatements += updateFinallyPath(listOf(successor)) currentStatements += stateAndJump(finallyBlock) @@ -240,8 +242,6 @@ class CoroutineBodyTransformer( currentStatements += stateAndJump(successor) } - currentCatchBlock = oldCatchBlock - // Handle catch node currentBlock = catchBlock @@ -279,6 +279,9 @@ class CoroutineBodyTransformer( currentBlock = successor } + // There's no implementation for JsSwitch, since we don't generate it. However, when we implement optimization + // for simple `when` statement, we will need to support JsSwitch here + private fun generateFinallyExit() { val finallyPathRef = JsNameRef(context.finallyPathFieldName, JsLiteral.THIS) val stateRef = JsNameRef(context.stateFieldName, JsLiteral.THIS) @@ -320,6 +323,7 @@ class CoroutineBodyTransformer( override fun visitThrow(x: JsThrow) { if (throwFunctionName != null) { + // TODO: what if we catch exception in coroutine? val methodRef = JsNameRef(throwFunctionName, JsNameRef(context.controllerFieldName, JsLiteral.THIS)) val invocation = JsInvocation(methodRef, x.expression).apply { source = x.source diff --git a/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutinePasses.kt b/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutinePasses.kt index c9865c04a1e..60905514f49 100644 --- a/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutinePasses.kt +++ b/js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutinePasses.kt @@ -46,6 +46,10 @@ fun JsNode.collectNodesToSplit(breakContinueTargets: Map) { - val declaredNames = x.vars.map { it.name } - val totalCount = declaredNames.size - val localVarCount = declaredNames.count() - - when { - totalCount == localVarCount -> { - val assignments = x.vars.mapNotNull { - val fieldName = scope.getFieldName(it.name) - val initExpression = it.initExpression - if (initExpression != null) { - JsAstUtils.assignment(JsNameRef(fieldName, JsLiteral.THIS), it.initExpression) - } - else { - null - } - } - if (assignments.isNotEmpty()) { - ctx.replaceMe(JsExpressionStatement(JsAstUtils.newSequence(assignments))) - } - else { - ctx.removeMe() - } + val assignments = x.vars.mapNotNull { + val fieldName = scope.getFieldName(it.name) + val initExpression = it.initExpression + if (initExpression != null) { + JsAstUtils.assignment(JsNameRef(fieldName, JsLiteral.THIS), it.initExpression) } - localVarCount > 0 -> { - for (declaration in x.vars) { - if (declaration.name in localVariables) { - val fieldName = scope.getFieldName(declaration.name) - val assignment = JsAstUtils.assignment(JsNameRef(fieldName, JsLiteral.THIS), declaration.initExpression) - ctx.addPrevious(assignment.makeStmt()) - } - else { - ctx.addPrevious(JsVars(declaration)) - } - } - ctx.removeMe() + else { + null } } + + if (assignments.isNotEmpty()) { + ctx.replaceMe(JsExpressionStatement(JsAstUtils.newSequence(assignments))) + } + else { + ctx.removeMe() + } + super.endVisit(x, ctx) } } diff --git a/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt b/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt index 15dd91150e9..daa8998c364 100644 --- a/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt +++ b/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt @@ -191,9 +191,9 @@ fun JsNode.collectBreakContinueTargets(): Map { accept(object : RecursiveJsVisitor() { var defaultBreakTarget: JsStatement? = null - var breakTargets = mutableMapOf() + var breakTargets = mutableMapOf() var defaultContinueTarget: JsStatement? = null - var continueTargets = mutableMapOf() + var continueTargets = mutableMapOf() override fun visitLabel(x: JsLabel) { val inner = x.statement @@ -204,6 +204,8 @@ fun JsNode.collectBreakContinueTargets(): Map { is JsFor -> handleLoop(inner, inner.body, x.name) + is JsSwitch -> handleSwitch(inner, x.name) + else -> { withBreakAndContinue(x.name, x.statement, null) { accept(inner) @@ -218,6 +220,14 @@ fun JsNode.collectBreakContinueTargets(): Map { override fun visitFor(x: JsFor) = handleLoop(x, x.body, null) + override fun visit(x: JsSwitch) = handleSwitch(x, null) + + private fun handleSwitch(statement: JsSwitch, label: JsName?) { + withBreakAndContinue(label, statement) { + statement.cases.forEach { accept(it) } + } + } + private fun handleLoop(loop: JsStatement, body: JsStatement, label: JsName?) { withBreakAndContinue(label, loop, loop) { body.accept(this) @@ -262,9 +272,7 @@ fun JsNode.collectBreakContinueTargets(): Map { defaultBreakTarget = breakTargetStatement if (label != null) { breakTargets[label] = breakTargetStatement - if (continueTargetStatement != null) { - continueTargets[label] = continueTargetStatement - } + continueTargets[label] = continueTargetStatement } if (continueTargetStatement != null) { defaultContinueTarget = continueTargetStatement @@ -275,18 +283,8 @@ fun JsNode.collectBreakContinueTargets(): Map { defaultBreakTarget = oldDefaultBreakTarget defaultContinueTarget = oldDefaultContinueTarget if (label != null) { - if (oldBreakTarget == null) { - breakTargets.keys -= label - } - else { - breakTargets[label] = oldBreakTarget - } - if (oldContinueTarget == null) { - continueTargets.keys -= label - } - else { - continueTargets[label] = oldContinueTarget - } + breakTargets[label] = oldBreakTarget + continueTargets[label] = oldContinueTarget } } }) 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 caeda1737b2..c54745131b2 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 @@ -5920,6 +5920,12 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest { doTest(fileName); } + @TestMetadata("forContinue.kt") + public void testForContinue() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt"); + doTest(fileName); + } + @TestMetadata("forStatement.kt") public void testForStatement() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forStatement.kt"); @@ -5938,6 +5944,12 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest { doTest(fileName); } + @TestMetadata("switchLikeWhen.kt") + public void testSwitchLikeWhen() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/switchLikeWhen.kt"); + doTest(fileName); + } + @TestMetadata("throwFromCatch.kt") public void testThrowFromCatch() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/throwFromCatch.kt"); diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/UsageTracker.kt b/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/UsageTracker.kt index de0c6fd6942..2e1ca7cf769 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/UsageTracker.kt +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/UsageTracker.kt @@ -49,13 +49,10 @@ class UsageTracker( // local named function if (descriptor is FunctionDescriptor && descriptor.visibility == Visibilities.LOCAL) { - if (descriptor.isCoroutineLambda) { - captureIfNeed(descriptor) - } - else { - assert(!descriptor.getName().isSpecial) { "Function with special name can not be captured, descriptor: $descriptor" } - captureIfNeed(descriptor) + assert(descriptor.isCoroutineLambda || !descriptor.getName().isSpecial) { + "Function with special name can not be captured, descriptor: $descriptor" } + captureIfNeed(descriptor) } // local variable else if (descriptor is VariableDescriptor && descriptor !is PropertyDescriptor) {