8000 Fix #5165: Use defaultable types for the locals of `TryFinally`s. · sjrd/scala-js@15b437e · GitHub
[go: up one dir, main page]

Skip to content

Commit 15b437e

Browse files
committed
Fix scala-js#5165: Use defaultable types for the locals of TryFinallys.
As well as for the locals of `Labeled` blocks whose `Return`s cross a `try..finally` boundary. If their original type was not defaultable, we cast away nullability when reading them back.
1 parent f84fa2e commit 15b437e

File tree

3 files changed

+80
-5
lines changed

3 files changed

+80
-5
lines changed

linker/shared/src/main/scala/org/scalajs/linker/backend/wasmemitter/FunctionEmitter.scala

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,6 +1040,13 @@ private class FunctionEmitter private (
10401040
* not need to store the receiver in a local at all.
10411041
* For the case with the args, it does not hurt either way. We could
10421042
* move it out, but that would make for a less consistent codegen.
1043+
*
1044+
* Loading the arguments and storing them in locas inside the block
1045+
* only works if their type is defaultable. Currently, for instance
1046+
* methods, parameter types are always defaultable, so this is fine.
1047+
* We may need to revisit this strategy if that invariant changes.
1048+
* If we do, it may be better to use different code paths for the
1049+
* no-args case and the with-args case. See #5165 for more context.
10431050
*/
10441051
val argsLocals = fb.block(watpe.RefType.any) { labelNotOurObject =>
10451052
// Load receiver and arguments and store them in temporary variables
@@ -3623,6 +3630,11 @@ private class FunctionEmitter private (
36233630
* we cannot use the stack for the `try_table` itself: each label has a
36243631
* dedicated local for its result if it comes from such a crossing `return`.
36253632
*
3633+
* Those locals must have defaultable types, because they are read outside of
3634+
* the block where they are first ininitialized. If their natural type is not
3635+
* defaultable, we make it defaultable, and cast away nullability when we
3636+
* read them back. See #5165.
3637+
*
36263638
* Two more complications:
36273639
*
36283640
* - If the `finally` block itself contains another `try..finally`, they may
@@ -3850,7 +3862,7 @@ private class FunctionEmitter private (
38503862
_crossInfo.getOrElse {
38513863
val destinationTag = allocateDestinationTag()
38523864
val resultTypes = transformResultType(expectedType)
3853-
val resultLocals = resultTypes.map(addSyntheticLocal(_))
3865+
val resultLocals = resultTypes.map(tpe => addSyntheticLocal(tpe.toDefaultableType))
38543866
val crossLabel = fb.genLabel()
38553867
val info = CrossInfo(destinationTag, resultLocals, crossLabel)
38563868
_crossInfo = Some(info)
@@ -3941,8 +3953,11 @@ private class FunctionEmitter private (
39413953
// Add the `br`, `end` and `local.get` at the current position, as usual
39423954
fb += wa.Br(entry.regularWasmLabel)
39433955
fb += wa.End
3944-
for (local <- resultLocals)
3956+
for ((local, origType) <- resultLocals.zip(ty)) {
39453957
fb += wa.LocalGet(local)
3958+
if (!origType.isDefaultable)
3959+
fb += wa.RefAsNonNull
3960+
}
39463961
}
39473962

39483963
fb += wa.End
@@ -3959,7 +3974,7 @@ private class FunctionEmitter private (
39593974
val entry = new TryFinallyEntry(currentUnwindingStackDepth)
39603975

39613976
val resultType = transformResultType(expectedType)
3962-
val resultLocals = resultType.map(addSyntheticLocal(_))
3977+
val resultLocals = resultType.map(tpe => addSyntheticLocal(tpe.toDefaultableType))
39633978

39643979
markPosition(tree)
39653980

@@ -4074,8 +4089,11 @@ private class FunctionEmitter private (
40744089
} // end block $done
40754090

40764091
// reload the result onto the stack
4077-
for (resultLocal <- resultLocals)
4092+
for ((resultLocal, origType) <- resultLocals.zip(resultType)) {
40784093
fb += wa.LocalGet(resultLocal)
4094+
if (!origType.isDefaultable)
4095+
fb += wa.RefAsNonNull
4096+
}
40794097

40804098
if (expectedType == NothingType)
40814099
fb += wa.Unreachable

linker/shared/src/main/scala/org/scalajs/linker/backend/webassembly/Types.scala

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,24 @@ object Types {
3333
* typing" point of view. It is also the kind of type we manipulate the most
3434
* across the backend, so it also makes sense for it to be the "default".
3535
*/
36-
sealed abstract class Type extends StorageType
36+
sealed abstract class Type extends StorageType {
37+
/** Returns true if and only if this type is defaultable. */
38+
final def isDefaultable: Boolean = this match {
39+
case RefType(nullable, _) => nullable
40+
case _ => true
41+
}
42+
43+
/** Returns a defaultable supertype of this type.
44+
*
45+
* If this type is already defaultable, return `this`. Otherwise, this
46+
* type must be a non-nullable reference type, and this method returns the
47+
* nullable variant.
48+
*/
49+
final def toDefaultableType: Type = this match {
50+
case RefType(false, heapType) => RefType.nullable(heapType)
51+
case _ => this
52+
}
53+
}
3754

3855
/** Convenience superclass for `Type`s that are encoded with a simple opcode. */
3956
sealed abstract class SimpleType(val textName: String, val binaryCode: Byte) extends Type

test-suite/shared/src/test/scala/org/scalajs/testsuite/compiler/TryFinallyTest.scala

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,4 +229,44 @@ class TryFinallyTest {
229229
"in finally 2"
230230
)
231231
}
232+
233+
@Test
234+
def nonDefaultableTryResultType_Issue5165(): Unit = {
235+
test { println =>
236+
// after the optimizer, some has type Some! (a non-nullable reference type)
237+
val some = try {
238+
println("in try")
239+
Some(1)
240+
} finally {
241+
println("in finally")
242+
}
243+
assertEquals(1, some.value)
244+
} (
245+
"in try",
246+
"in finally"
247+
)
248+
}
249+
250+
@Test
251+
def nonDefaultableLabeledResultType_Issue5165(): Unit = {
252+
test { println =>
253+
/* After the optimizer, the result type of the Labeled block that gets
254+
* inlined is a Some! (a non-nullable reference type).
255+
*/
256+
@inline def nonDefaultableLabeledResultTypeInner(): Some[Int] = {
257+
try {
258+
println("in try")
259+
return Some(1)
260+
} finally {
261+
println("in finally")
262+
}
263+
}
264+
265+
val some = nonDefaultableLabeledResultTypeInner()
5531 266+
assertEquals(1, some.value)
267+
} (
268+
"in try",
269+
"in finally"
270+
)
271+
}
232272
}

0 commit comments

Comments
 (0)
0