Pattern matching with alternatives is compiled to "throw null" with Scala.js

[devkat]

Minimized code

https://github.com/scala-js/scala-js/blob/master/test-suite/shared/src/test/scala/org/scalajs/testsuite/javalib/util/FormatterTest.scala#L626

  @Test def leftAlignWithoutWidthThrows(): Unit = {
    for (conversion <- "bBhHsHcCdoxXeEgGf%") {
      val fmt = "ab%-" + conversion + "cd"
      val arg = conversion match {
        case 'e' | 'E' | 'g' | 'G' | 'f' => 5.5
        case _ => 5
      }
      val e =
        expectFormatterThrows(classOf[MissingFormatWidthException], fmt, arg)
      assertEquals(fmt, "%-" + conversion, e.getFormatSpecifier)
    }
  }

Output

Scala.js IR:

  @hints(1) private def leftAlignWithoutWidthThrows$$anonfun$1;C;V(conversion: char) {
    val fmt: java.lang.String = (("ab%-" +[string] conversion) +[string] "cd");
    val arg: double = matchResult1[double]: {
      val x1: char = conversion;
      throw null
    };
    val e: java.util.MissingFormatWidthException = this.expectFormatterThrows;Ljava.lang.Class;Ljava.lang.String;Lscala.collection.immutable.Seq;Ljava.lang.Throwable(classOf[java.util.MissingFormatWidthException], fmt, mod:scala.runtime.ScalaRunTime$.genericWrapArray;Ljava.lang.Object;Lscala.collection.immutable.ArraySeq(java.lang.Object[](arg))).asInstanceOf[java.util.MissingFormatWidthException];
    mod:org.junit.Assert$.assertEquals;Ljava.lang.String;Ljava.lang.Object;Ljava.lang.Object;V(fmt, ("%-" +[string] conversion), e.getFormatSpecifier;Ljava.lang.String())
  }

JavaScript:

  leftAlignWithoutWidthThrows__V() {
    const len = $uI("bBhHsHcCdoxXeEgGf%".length);
    let i = 0;
    while ((i < len)) {
      const index = i;
      $uI("bBhHsHcCdoxXeEgGf%".charCodeAt(index));
      throw null
    }
  };

Expectation

This (equivalent?) match expression:

      val arg = conversion match {
        case _: ('e' | 'E' | 'g' | 'G' | 'f') => 5.5
        case _ => 5
      }

is compiled to the following IR:

    val arg: double = matchResult1[double]: {
      val x1: char = conversion;
      if ((((int)'e') ==[int] ((int)x1)) || (((int)'E') ==[int] ((int)x1)) || (((int)'g') ==[int] ((int)x1)) || (((int)'G') ==[int] ((int)x1)) || (((int)'f') ==[int] ((int)x1))) {
        return@matchResult1 5.5d
      };
      return@matchResult1 5.0d
    };

and the following JavaScript code:

const arg = ((((((arg1 === 101) || (arg1 === 69)) || (arg1 === 103)) || (arg1 === 71)) || (arg1 === 102)) ? 5.5 : 5.0);

[smarter]

@sjrd what could cause the scalajs backend to emit "throw null" here?

[sjrd]

This:
https://github.com/lampepfl/dotty/blob/baba3f0a610bf971045d69a29d7f9e9c1ce2c8ff/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala#L1176-L1180
😆

[smarter]

Ah I see :). Would you recommend a straight port of genMatch from the scala 2 plugin? According to https://github.com/scala-js/scala-js/blob/c0b8f67c91a0c94796c8b8df94119c60902199e9/compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala#L3434-L3487 there's some complexity related to handling guards, does that also apply to dotty?

[sjrd]

I do recommend a straight port of genMatch from the Scala 2 plugin, minus all the LabelDef-related stuff. There should be no complexity to handling guards in Dotty since patmat already takes care of emitting Labeled blocks, which are directly translated to Scala.js Labeled blocks.

[smarter]

Great thanks, @devkat: feel free to give this a go :).

[devkat]

case 'e' | 'E' | 'g' | 'G' | 'f' => 5.5

CaseDef(Alternative(List(Literal(Constant(e)), Literal(Constant(E)), Literal(Constant(g)), Literal(Constant(G)), Literal(Constant(f)))),EmptyTree,Literal(Constant(5.5)))

I didn't realize at first that the working code is a simple type test for a union type. Apparently this is straightforward enough to emit an If tree in PatternMatcher:

case _: ('e' | 'E' | 'g' | 'G' | 'f') => 5.5

CaseDef(Typed(Ident(_),TypeTree[OrType(OrType(OrType(OrType(ConstantType(Constant(e)),ConstantType(Constant(E))),ConstantType(Constant(g))),ConstantType(Constant(G))),ConstantType(Constant(f)))]),EmptyTree,Literal(Constant(5.5)))

I'll try to find the time to look into genMatch and port it to Dotty.

[devkat]

Now the JS looks good at first glance, but strangely the match expression is never true and the result is always 5.0:

      const arg = ((($bC(arg1) === $bC(101)) || (($bC(arg1) === $bC(69)) || (($bC(arg1) === $bC(103)) || (($bC(arg1) === $bC(71)) || ($bC(arg1) === $bC(102)))))) ? 5.5 : 5.0);

[sjrd]

That code is trying to compare boxed chars with an identity comparison. That will always be false. In a switch, the scrutinee and the cases must all be converted to Ints.

Also I encourage you to look at the IR as the point of reference. Not at the generated JS.

[devkat]

The IR looks like this:

      if ((x1 === 'e') || (x1 === 'E') || (x1 === 'g') || (x1 === 'G') || (x1 === 'f')) {
        return@matchResult1 5.5d
      } else {
        return@matchResult1 5.0d
      }

I'm not sure if the boxed chars comparison is in the scope of this ticket, but I'll take a closer look.

[sjrd]

=== is not the correct operator to use here. The correct one would be Int_==, after having converted the chars into ints using UnaryOp(UnaryOp.CharToInt, c).

[devkat]

Here's a first draft:

https://github.com/devkat/dotty/blob/scalajs-tests/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala#L3128-L3782

This is basically a copy of the genMatch method and its dependencies from GenJSCode with minor modifications to make it compile.

Some open issues:

• The issue mentioned above (comparison in alternatives) isn't addressed yet.
• The genMatchEnd method (https://github.com/devkat/dotty/blob/scalajs-tests/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala#L3559) used to take a LabelDef which has params. I have no idea how to handle this with Labeled (see FIXME on line 3572).
• Some of the code can probably be removed or simplified, particularly the code which was required due to issues with the 2.x compiler.

[sjrd]

Can you already submit a PR with what you have? It's easier to discuss the details via line comments in a PR.

[devkat]

Done – thanks a lot in advance for reviewing!