From 24a22d1bb43378eb1f6312e2e1a8be2adcfb8c52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Mon, 11 Nov 2019 17:40:30 +0100 Subject: [PATCH] Support `Match` nodes with non-int-literals in the back-end. Since Scala 2.13.2, the pattern matcher will keep `Match` nodes that match on `String`s and `null`s as is, to be desugared later in `cleanup`. This was implemented upstream in https://github.com/scala/scala/pull/8451 We implement a more general translation that will accept any kind of scalac `Literal`. If the scrutinee is an integer and all cases are int literals, we emit a `js.Match` as before. Otherwise, we emit an `if..else` chain with `===` comparisons. --- .../org/scalajs/nscplugin/GenJSCode.scala | 80 ++++++++++++------- .../testsuite/compiler/RegressionTest.scala | 68 +++++++++++++++- 2 files changed, 120 insertions(+), 28 deletions(-) diff --git a/compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala b/compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala index 9f0e11a558..14beae0e31 100644 --- a/compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala +++ b/compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala @@ -3230,15 +3230,13 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G) implicit val pos = tree.pos val Match(selector, cases) = tree - /* We adapt the selector to IntType so that we can use it in a js.Match, - * just like GenBCode does for the JVM. This seems to be redundant, - * though, as anything that comes out of the pattern matching has already - * been adapted to an Int (along with the cases). However, since GenBCode - * adapts, we do the same, to be on the safe side (for example, a - * compiler plugin could generate a Match with other types of - * primitives ...). + /* Although GenBCode adapts the scrutinee and the cases to `int`, only + * true `int`s can reach the back-end, as asserted by the String-switch + * transformation in `cleanup`. Therefore, we do not adapt, preserving + * the `string`s and `null`s that come out of the pattern matching in + * Scala 2.13.2+. */ - val expr = adaptPrimitive(genExpr(selector), jstpe.IntType) + val genSelector = genExpr(selector) val resultType = toIRType(tree.tpe) @@ -3248,7 +3246,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G) body.symbol }.getOrElse(NoSymbol) - var clauses: List[(List[js.IntLiteral], js.Tree)] = Nil + var clauses: List[(List[js.Tree], js.Tree)] = Nil var optElseClause: Option[js.Tree] = None var optElseClauseLabel: Option[js.LabelIdent] = None @@ -3299,16 +3297,9 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G) genStatOrExpr(body, isStat) } - /* value.intValue implicitly adapts the constant value to an Int. This - * is also what GenBCode for the JVM. See also the comment about - * adaptPrimitive at the beginning of this method. - */ - def genLiteral(lit: Literal): js.IntLiteral = - js.IntLiteral(lit.value.intValue)(lit.pos) - pat match { case lit: Literal => - clauses = (List(genLiteral(lit)), genBody(body)) :: clauses + clauses = (List(genExpr(lit)), genBody(body)) :: clauses case Ident(nme.WILDCARD) => optElseClause = Some(body match { case LabelDef(_, Nil, rhs) if hasSynthCaseSymbol(body) => @@ -3319,7 +3310,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G) case Alternative(alts) => val genAlts = { alts map { - case lit: Literal => genLiteral(lit) + case lit: Literal => genExpr(lit) case _ => abort("Invalid case in alternative in switch-like pattern match: " + tree + " at: " + tree.pos) @@ -3341,31 +3332,66 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G) * case is a typical product of `match`es that are full of * `case n if ... =>`, which are used instead of `if` chains for * convenience and/or readability. + * + * When no optimization applies, and any of the case values is not a + * literal int, we emit a series of `if..else` instead of a `js.Match`. + * This became necessary in 2.13.2 with strings and nulls. */ - def buildMatch(selector: js.Tree, - cases: List[(List[js.IntLiteral], js.Tree)], + def buildMatch(cases: List[(List[js.Tree], js.Tree)], default: js.Tree, tpe: jstpe.Type): js.Tree = { + + def isInt(tree: js.Tree): Boolean = tree.tpe == jstpe.IntType + cases match { case Nil => /* Completely remove the Match. Preserve the side-effects of - * `selector`. + * `genSelector`. */ - js.Block(exprToStat(selector), default) + js.Block(exprToStat(genSelector), default) case (uniqueAlt :: Nil, caseRhs) :: Nil => /* Simplify the `match` as an `if`, so that the optimizer has less * work to do, and we emit less code at the end of the day. + * Use `Int_==` instead of `===` if possible, since it is a common + * case. */ - js.If(js.BinaryOp(js.BinaryOp.Int_==, selector, uniqueAlt), - caseRhs, default)(tpe) + val op = + if (isInt(genSelector) && isInt(uniqueAlt)) js.BinaryOp.Int_== + else js.BinaryOp.=== + js.If(js.BinaryOp(op, genSelector, uniqueAlt), caseRhs, default)(tpe) case _ => - js.Match(selector, cases, default)(tpe) + if (isInt(genSelector) && + cases.forall(_._1.forall(_.isInstanceOf[js.IntLiteral]))) { + // We have int literals only: use a js.Match + val intCases = cases.asInstanceOf[List[(List[js.IntLiteral], js.Tree)]] + js.Match(genSelector, intCases, default)(tpe) + } else { + // We have other stuff: generate an if..else chain + val (tempSelectorDef, tempSelectorRef) = genSelector match { + case varRef: js.VarRef => + (js.Skip(), varRef) + case _ => + val varDef = js.VarDef(freshLocalIdent(), genSelector.tpe, + mutable = false, genSelector) + (varDef, varDef.ref) + } + val ifElseChain = cases.foldRight(default) { (caze, elsep) => + val conds = caze._1.map { caseValue => + js.BinaryOp(js.BinaryOp.===, tempSelectorRef, caseValue) + } + val cond = conds.reduceRight[js.Tree] { (left, right) => + js.If(left, js.BooleanLiteral(true), right)(jstpe.BooleanType) + } + js.If(cond, caze._2, elsep)(tpe) + } + js.Block(tempSelectorDef, ifElseChain) + } } } optElseClauseLabel.fold[js.Tree] { - buildMatch(expr, clauses.reverse, elseClause, resultType) + buildMatch(clauses.reverse, elseClause, resultType) } { elseClauseLabel => val matchResultLabel = freshLabelIdent("matchResult") val patchedClauses = for ((alts, body) <- clauses) yield { @@ -3377,7 +3403,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G) } js.Labeled(matchResultLabel, resultType, js.Block(List( js.Labeled(elseClauseLabel, jstpe.NoType, { - buildMatch(expr, patchedClauses.reverse, js.Skip(), jstpe.NoType) + buildMatch(patchedClauses.reverse, js.Skip(), jstpe.NoType) }), elseClause ))) diff --git a/test-suite/shared/src/test/scala/org/scalajs/testsuite/compiler/RegressionTest.scala b/test-suite/shared/src/test/scala/org/scalajs/testsuite/compiler/RegressionTest.scala index ebdd2916c6..85386e07a9 100644 --- a/test-suite/shared/src/test/scala/org/scalajs/testsuite/compiler/RegressionTest.scala +++ b/test-suite/shared/src/test/scala/org/scalajs/testsuite/compiler/RegressionTest.scala @@ -477,7 +477,38 @@ class RegressionTest { assertThrows(classOf[MatchError], bug.bug(2, false)) } - @Test def return_x_match_issue_2928(): Unit = { + @Test def return_x_match_issue_2928_ints(): Unit = { + // scalastyle:off return + + def testNonUnit(x: Int): Boolean = { + return x match { + case 1 => true + case _ => false + } + } + + var r: Option[Boolean] = None + + def testUnit(x: Int): Unit = { + return x match { + case 1 => r = Some(true) + case _ => r = Some(false) + } + } + + assertEquals(true, testNonUnit(1)) + assertEquals(false, testNonUnit(2)) + + testUnit(1) + assertEquals(Some(true), r) + r = None + testUnit(2) + assertEquals(Some(false), r) + + // scalastyle:on return + } + + @Test def return_x_match_issue_2928_strings(): Unit = { // scalastyle:off return def testNonUnit(x: String): Boolean = { @@ -508,6 +539,41 @@ class RegressionTest { // scalastyle:on return } + @Test def return_x_match_issue_2928_lists(): Unit = { + // scalastyle:off return + + def testNonUnit(x: List[String]): Boolean = { + return x match { + case "True" :: Nil => true + case _ => false + } + } + + var r: Option[Boolean] = None + + def testUnit(x: List[String]): Unit = { + return x match { + case "True" :: Nil => r = Some(true) + case _ => r = Some(false) + } + } + + assertEquals(true, testNonUnit("True" :: Nil)) + assertEquals(false, testNonUnit("not true" :: Nil)) + assertEquals(false, testNonUnit("True" :: "second" :: Nil)) + + testUnit("True" :: Nil) + assertEquals(Some(true), r) + r = None + testUnit("not true" :: Nil) + assertEquals(Some(false), r) + r = None + testUnit("True" :: "second" :: Nil) + assertEquals(Some(false), r) + + // scalastyle:on return + } + @Test def null_asInstanceOf_Unit_should_succeed_issue_1691(): Unit = { /* Avoid scalac's special treatment of `.asInstanceOf[X]`. * It does have the benefit to test our constant-folder of that pattern,