Fix 2 bugs in the codegen for pattern matches #2955
Conversation
For some reason, for such a snippet, the `Return` node directly wraps the `LabelDef(matchEnd)`, rather than the `Block` surrounding the entire translated pattern match.
|
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3964/ |
| @@ -2955,9 +2960,10 @@ abstract class GenJSCode extends plugins.PluginComponent | |||
| translateMatch(expr) | |||
|
|
|||
| // Sometimes the pattern matcher casts its final result | |||
| case Apply(TypeApply(Select(expr: LabelDef, nme.asInstanceOf_Ob), _), _) | |||
| case Apply(TypeApply(Select(expr: LabelDef, nme.asInstanceOf_Ob), targs), _) | |||
There was a problem hiding this comment.
Could you replace targs by List(targ) just to be extra prudent? (maybe also the last _ by Nil)?
In fe887a9, we added some code to recognize shapes of pattern matches where the `expr` is a cast surrounding the `matchEnd`. However, that code forgot to actually translate the cast itself, yielding untypeable code. This commit fixes that omission.
|
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3970/ |
| @Test def return_x_match_issue_2928(): Unit = { | ||
| def testNonUnit(x: String): Boolean = { | ||
| return x match { | ||
| case "True" => true |
There was a problem hiding this comment.
@sjrd note that on Scala 2.13.8, matching on a String like this doesn't result in the weird Return(LabelDef(...)) tree — instead you get a normal Return(Match(...) tree, because matching on string constants is now handled differently. I think the PR when this changed is scala/scala#8451 ? So if you want this to actually be testing the change on Scala 2.13, I think you need to e.g. change x: String to x: AnyRef.
I hit this when trying to fix the same bug in my Fortify plugin.
There was a problem hiding this comment.
Ah, good catch! We should change the test to use some other data type.
The good news is that this problem will be entirely gone in 2.13.9+, thanks to scala/scala#10028
There was a problem hiding this comment.
Oh, that's good to know 👍
There was a problem hiding this comment.
Eh eh, looks like we already added a test for that when we added support for Match nodes with strings in
24a22d1
I tried to unfix the fix and it does crash on that test case in 2.13.8.
No idea how I thought about that when I made that commit. 😅
No description provided.