Skip to content

Commit e2dc7ba

Browse files
Alexey AndreevAlexey Andreev
Alexey Andreev
authored and
Alexey Andreev
committed
JS: coroutines: fixes after code review
1 parent 43525ab commit e2dc7ba

File tree

10 files changed

+155
-84
lines changed

10 files changed

+155
-84
lines changed

‎compiler/testData/codegen/box/coroutines/controlFlow/breakFinally.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,14 @@ fun box(): String {
3838
}
3939
result += "ignore"
4040
}
41+
result += "*"
4142
}
4243
finally {
4344
result += "finally"
4445
}
4546
result += "."
4647
}
47-
if (value != "AC!ED!@finally.") return "fail: $value"
48+
if (value != "AC!ED!@*finally.") return "fail: $value"
4849

4950
return "OK"
5051
}

‎compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// WITH_RUNTIME
2+
3+
class Controller {
4+
var result = ""
5+
6+
suspend fun <T> suspendWithResult(value: T, c: Continuation<T>) {
7+
c.resume(value)
8+
}
9+
}
10+
11+
fun builder(coroutine c: Controller.() -> Continuation<Unit>): String {
12+
val controller = Controller()
13+
c(controller).resume(Unit)
14+
return controller.result
15+
}
16+
17+
fun box(): String {
18+
val value = builder {
19+
for (x in listOf("O", "$", "K")) {
20+
if (x == "$") continue
21+
result += suspendWithResult(x)
22+
}
23+
result += "."
24+
}
25+
if (value != "OK.") return "fail: suspend in for body: $value"
26+
27+
return "OK"
28+
}

‎compiler/testData/codegen/box/coroutines/controlFlow/switchLikeWhen.kt

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// WITH_RUNTIME
2+
3+
// TODO: fix bug in JVM backend and remove this directive
4+
// TARGET_BACKEND: JS
5+
6+
class Controller {
7+
var result = ""
8+
9+
suspend fun <T> suspendWithResult(value: T, c: Continuation<T>) {
10+
result += "["
11+
c.resume(value)
12+
}
13+
}
14+
15+
fun builder(coroutine c: Controller.() -> Continuation<Unit>): String {
16+
val controller = Controller()
17+
c(controller).resume(Unit)
18+
return controller.result
19+
}
20+
21+
fun box(): String {
22+
var value = builder {
23+
for (v in listOf("A", "B", "C")) {
24+
when (v) {
25+
"A" -> result += "A;"
26+
"B" -> result += suspendWithResult(v) + "]"
27+
else -> result += suspendWithResult(v) + "]!"
28+
}
29+
}
30+
}
31+
if (value != "A;[B][C]!") return "fail: suspend as if condition: $value"
32+
33+
return "OK"
34+
}

‎compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4744,6 +4744,12 @@ public void testDoWhileStatement() throws Exception {
47444744
doTest(fileName);
47454745
}
47464746

4747+
@TestMetadata("forContinue.kt")
4748+
public void testForContinue() throws Exception {
4749+
String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt");
4750+
doTest(fileName);
4751+
}
4752+
47474753
@TestMetadata("forStatement.kt")
47484754
public void testForStatement() throws Exception {
47494755
String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forStatement.kt");

‎compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4744,6 +4744,12 @@ public void testDoWhileStatement() throws Exception {
47444744
doTest(fileName);
47454745
}
47464746

4747+
@TestMetadata("forContinue.kt")
4748+
public void testForContinue() throws Exception {
4749+
String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forContinue.kt");
4750+
doTest(fileName);
4751+
}
4752+
47474753
@TestMetadata("forStatement.kt")
47484754
public void testForStatement() throws Exception {
47494755
String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/controlFlow/forStatement.kt");

‎js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineBodyTransformer.kt

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -112,40 +112,39 @@ class CoroutineBodyTransformer(
112112
}
113113

114114
override fun visitWhile(x: JsWhile) = splitIfNecessary(x) {
115-
val predecessor = currentBlock
116115
val successor = CoroutineBlock()
117-
118116
val bodyEntryBlock = CoroutineBlock()
117+
currentStatements += stateAndJump(bodyEntryBlock)
118+
119119
currentBlock = bodyEntryBlock
120+
if (x.condition != JsLiteral.TRUE) {
121+
currentStatements += JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source }
122+
}
123+
120124
withBreakAndContinue(x, successor, bodyEntryBlock) {
121125
x.body.accept(this)
122126
}
123127

124-
if (x.condition != JsLiteral.TRUE) {
125-
val jsIf = JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source }
126-
bodyEntryBlock.statements.add(0, jsIf)
127-
}
128-
currentBlock.statements += stateAndJump(bodyEntryBlock)
129-
predecessor.statements += stateAndJump(bodyEntryBlock)
128+
currentStatements += stateAndJump(bodyEntryBlock)
130129
currentBlock = successor
131130
}
132131

133132
override fun visitDoWhile(x: JsDoWhile) = splitIfNecessary(x) {
134-
val predecessor = currentBlock
135133
val successor = CoroutineBlock()
136-
137134
val bodyEntryBlock = CoroutineBlock()
135+
currentStatements += stateAndJump(bodyEntryBlock)
136+
138137
currentBlock = bodyEntryBlock
139138
withBreakAndContinue(x, successor, bodyEntryBlock) {
140139
x.body.accept(this)
141140
}
142141

143142
if (x.condition != JsLiteral.TRUE) {
144143
val jsIf = JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source }
145-
currentBlock.statements.add(jsIf)
144+
currentStatements.add(jsIf)
146145
}
147146
currentBlock.statements += stateAndJump(bodyEntryBlock)
148-
predecessor.statements += stateAndJump(bodyEntryBlock)
147+
149148
currentBlock = successor
150149
}
151150

@@ -159,29 +158,26 @@ class CoroutineBodyTransformer(
159158
}
160159
}
161160

162-
val predecessor = currentBlock
163161
val increment = CoroutineBlock()
164162
val successor = CoroutineBlock()
165-
166163
val bodyEntryBlock = CoroutineBlock()
164+
currentStatements += stateAndJump(bodyEntryBlock)
165+
167166
currentBlock = bodyEntryBlock
168-
withBreakAndContinue(x, successor, predecessor) {
169-
x.body.accept(this)
167+
if (x.condition != null && x.condition != JsLiteral.TRUE) {
168+
currentStatements += JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source }
170169
}
171-
val bodyExitBlock = currentBlock
172170

173-
if (x.condition != null && x.condition != JsLiteral.TRUE) {
174-
val jsIf = JsIf(JsAstUtils.notOptimized(x.condition), JsBlock(stateAndJump(successor))).apply { source = x.source }
175-
bodyEntryBlock.statements.add(0, jsIf)
171+
withBreakAndContinue(x, successor, increment) {
172+
x.body.accept(this)
176173
}
177174

178-
bodyExitBlock.statements += stateAndJump(increment)
175+
currentStatements += stateAndJump(increment)
179176
currentBlock = increment
180177

181178
x.incrementExpression?.let { JsExpressionStatement(it).accept(this) }
182179
currentStatements += stateAndJump(bodyEntryBlock)
183180

184-
predecessor.statements += stateAndJump(bodyEntryBlock)
185181
currentBlock = successor
186182
}
187183

@@ -201,6 +197,10 @@ class CoroutineBodyTransformer(
201197
currentStatements += jump()
202198
}
203199

200+
/**
201+
* When we perform break, continue or return, we can leave try blocks, so we should update $exceptionHandler correspondingly.
202+
* Also, these try blocks can contain finally clauses, therefore we need to update $finallyPath as well.
203+
*/
204204
private fun jumpWithFinally(targetTryDepth: Int, successor: CoroutineBlock) {
205205
if (targetTryDepth < tryStack.size) {
206206
val tryBlock = tryStack[targetTryDepth]
@@ -223,15 +223,17 @@ class CoroutineBodyTransformer(
223223
val catchBlock = CoroutineBlock()
224224
val finallyBlock = CoroutineBlock()
225225

226-
val tryBlock = TryBlock(catchBlock, if (finallyNode != null) finallyBlock else null)
227-
tryStack += tryBlock
226+
tryStack += TryBlock(catchBlock, if (finallyNode != null) finallyBlock else null)
228227

229228
val oldCatchBlock = currentCatchBlock
230229
currentCatchBlock = catchBlock
231230
currentStatements += exceptionState(catchBlock)
232231

233232
x.tryBlock.statements.forEach { it.accept(this) }
233+
234234
currentStatements += exceptionState(oldCatchBlock)
235+
currentCatchBlock = oldCatchBlock
236+
235237
if (finallyNode != null) {
236238
currentStatements += updateFinallyPath(listOf(successor))
237239
currentStatements += stateAndJump(finallyBlock)
@@ -240,8 +242,6 @@ class CoroutineBodyTransformer(
240242
currentStatements += stateAndJump(successor)
241243
}
242244

243-
currentCatchBlock = oldCatchBlock
244-
245245
// Handle catch node
246246
currentBlock = catchBlock
247247

@@ -279,6 +279,9 @@ class CoroutineBodyTransformer(
279279
currentBlock = successor
280280
}
281281

282+
// There's no implementation for JsSwitch, since we don't generate it. However, when we implement optimization
283+
// for simple `when` statement, we will need to support JsSwitch here
284+
282285
private fun generateFinallyExit() {
283286
val finallyPathRef = JsNameRef(context.finallyPathFieldName, JsLiteral.THIS)
284287
val stateRef = JsNameRef(context.stateFieldName, JsLiteral.THIS)
@@ -320,6 +323,7 @@ class CoroutineBodyTransformer(
320323

321324
override fun visitThrow(x: JsThrow) {
322325
if (throwFunctionName != null) {
326+
// TODO: what if we catch exception in coroutine?
323327
val methodRef = JsNameRef(throwFunctionName, JsNameRef(context.controllerFieldName, JsLiteral.THIS))
324328
val invocation = JsInvocation(methodRef, x.expression).apply {
325329
source = x.source

‎js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutinePasses.kt

Lines changed: 19 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@ fun JsNode.collectNodesToSplit(breakContinueTargets: Map<JsContinue, JsStatement
4646
}
4747
}
4848

49+
// We don't handle JsThrow case here the same way as we do for JsReturn.
50+
// Exception will be caught by the surrounding catch and then dispatched to a corresponding $exceptionState.
51+
// Even if there's no `catch` clause, we generate a fake one that dispatches to a finally block.
52+
4953
override fun visitBreak(x: JsBreak) {
5054
super.visitBreak(x)
5155

@@ -218,43 +222,24 @@ fun JsBlock.replaceLocalVariables(scope: JsScope, context: CoroutineTransformati
218222
}
219223

220224
override fun endVisit(x: JsVars, ctx: JsContext<in JsStatement>) {
221-
val declaredNames = x.vars.map { it.name }
222-
val totalCount = declaredNames.size
223-
val localVarCount = declaredNames.count()
224-
225-
when {
226-
totalCount == localVarCount -> {
227-
val assignments = x.vars.mapNotNull {
228-
val fieldName = scope.getFieldName(it.name)
229-
val initExpression = it.initExpression
230-
if (initExpression != null) {
231-
JsAstUtils.assignment(JsNameRef(fieldName, JsLiteral.THIS), it.initExpression)
232-
}
233-
else {
234-
null
235-
}
236-
}
237-
if (assignments.isNotEmpty()) {
238-
ctx.replaceMe(JsExpressionStatement(JsAstUtils.newSequence(assignments)))
239-
}
240-
else {
241-
ctx.removeMe()
242-
}
225+
val assignments = x.vars.mapNotNull {
226+
val fieldName = scope.getFieldName(it.name)
227+
val initExpression = it.initExpression
228+
if (initExpression != null) {
229+
JsAstUtils.assignment(JsNameRef(fieldName, JsLiteral.THIS), it.initExpression)
243230
}
244-
localVarCount > 0 -> {
245-
for (declaration in x.vars) {
246-
if (declaration.name in localVariables) {
247-
val fieldName = scope.getFieldName(declaration.name)
248-
val assignment = JsAstUtils.assignment(JsNameRef(fieldName, JsLiteral.THIS), declaration.initExpression)
249-
ctx.addPrevious(assignment.makeStmt())
250-
}
251-
else {
252-
ctx.addPrevious(JsVars(declaration))
253-
}
254-
}
255-
ctx.removeMe()
231+
else {
232+
null
256233
}
257234
}
235+
236+
if (assignments.isNotEmpty()) {
237+
ctx.replaceMe(JsExpressionStatement(JsAstUtils.newSequence(assignments)))
238+
}
239+
else {
240+
ctx.removeMe()
241+
}
242+
258243
super.endVisit(x, ctx)
259244
}
260245
}

‎js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -191,9 +191,9 @@ fun JsNode.collectBreakContinueTargets(): Map<JsContinue, JsStatement> {
191191

192192
accept(object : RecursiveJsVisitor() {
193193
var defaultBreakTarget: JsStatement? = null
194-
var breakTargets = mutableMapOf<JsName, JsStatement>()
194+
var breakTargets = mutableMapOf<JsName, JsStatement?>()
195195
var defaultContinueTarget: JsStatement? = null
196-
var continueTargets = mutableMapOf<JsName, JsStatement>()
196+
var continueTargets = mutableMapOf<JsName, JsStatement?>()
197197

198198
override fun visitLabel(x: JsLabel) {
199199
val inner = x.statement
@@ -204,6 +204,8 @@ fun JsNode.collectBreakContinueTargets(): Map<JsContinue, JsStatement> {
204204

205205
is JsFor -> handleLoop(inner, inner.body, x.name)
206206

207+
is JsSwitch -> handleSwitch(inner, x.name)
208+
207209
else -> {
208210
withBreakAndContinue(x.name, x.statement, null) {
209211
accept(inner)
@@ -218,6 +220,14 @@ fun JsNode.collectBreakContinueTargets(): Map<JsContinue, JsStatement> {
218220

219221
override fun visitFor(x: JsFor) = handleLoop(x, x.body, null)
220222

223+
override fun visit(x: JsSwitch) = handleSwitch(x, null)
224+
225+
private fun handleSwitch(statement: JsSwitch, label: JsName?) {
226+
withBreakAndContinue(label, statement) {
227+
statement.cases.forEach { accept(it) }
228+
}
229+
}
230+
221231
private fun handleLoop(loop: JsStatement, body: JsStatement, label: JsName?) {
222232
withBreakAndContinue(label, loop, loop) {
223233
body.accept(this)
@@ -262,9 +272,7 @@ fun JsNode.collectBreakContinueTargets(): Map<JsContinue, JsStatement> {
262272
defaultBreakTarget = breakTargetStatement
263273
if (label != null) {
264274
breakTargets[label] = breakTargetStatement
265-
if (continueTargetStatement != null) {
266-
continueTargets[label] = continueTargetStatement
267-
}
275+
continueTargets[label] = continueTargetStatement
268276
}
269277
if (continueTargetStatement != null) {
270278
defaultContinueTarget = continueTargetStatement
@@ -275,18 +283,8 @@ fun JsNode.collectBreakContinueTargets(): Map<JsContinue, JsStatement> {
275283
defaultBreakTarget = oldDefaultBreakTarget
276284
defaultContinueTarget = oldDefaultContinueTarget
277285
if (label != null) {
278-
if (oldBreakTarget == null) {
279-
breakTargets.keys -= label
280-
}
281-
else {
282-
breakTargets[label] = oldBreakTarget
283-
}
284-
if (oldContinueTarget == null) {
285-
continueTargets.keys -= label
286-
}
287-
else {
288-
continueTargets[label] = oldContinueTarget
289-
}
286+
breakTargets[label] = oldBreakTarget
287+
continueTargets[label] = oldContinueTarget
290288
}
291289
}
292290
})

0 commit comments

Comments
 (0)