Matching strings makes switches in bytecode

[hrhino]

Switchable matches with string-typed scrutinee survive the pattern matcher in the same way as those on integer types do: as a series of CaseDefs with empty guard and literal pattern.

Cleanup collates them by hash code and emits a switch on that. No sooner, so scala.js can emit a more JS-friendly implementation.

Labels were used to avoid a proliferation of throw new MatchError.

Works with nulls. Works with Unit.

Enclosed "pos" test stands for positions, not positivity.

Fixes scala/bug#11740.

[hrhino]

Darn, I missed the chance to use the shiny "WIP" feature.

@sjrd: the positions test is implicitly a scala.js-compatibility test. The trees coming out of patmat are hopefully not too difficult for the SJS backend to handle. Maybe this is a good time for me to get started over there.

[sjrd]

Cool :) Thanks for the separation in cleanup.

Maybe this is a good time for me to get started over there.

Sure, why not? :) If you do, you'll want to work in the 0.6.x branch. We forward-merge 0.6.x into master every now and then.

[retronym]

The tests fail when the compiler has been bootstrapped through this change. A pattern match in ScalaVersion$.<init> now throws a MatchError.

Minimized:

import annotation.switch

object Test {
  def test(s: String): Int = {
    (s : @switch) match {
      case null | "" => 0
      case _ => s.toInt
    }
  }

  def main(args: Array[String]): Unit = {
    println(test("2"))
  }
}

% qscalac sandbox/test.scala && qscala Test
scala.MatchError: 2 (of class java.lang.String)
	at Test$.test(test.scala:5)

      case <synthetic> val x1: String = (s: String);
      {
        matchEnd1(if (x1.eq(null))
          0
        else
          x1.hashCode() match {
          case 0 => if ("".equals(x1))
            0
          else
            matchEnd2()
          case 3392903 => if ("null".equals(x1))
            0
          else
            matchEnd2()
          case _ => matchEnd2()
        });
        matchEnd2(){
          throw new MatchError(x1)
        };
        matchEnd1(x$1: Int){
          x$1
        }
      }
    };

[retronym]

There appear to be two separate problems.

First, if we want to make this work for null patterns, we need special care in Cleanup to avoid the fact that Constant(null).stringValue == "null" (rather than == null). We also need to avoid a NPE by using null eq value rather than null.equals(value).

The MatchError above does not rely on null, though, as

import annotation.switch

object Test {
  def test(s: String): Int = {
    (s : @switch) match {
      case "" => 0
      case _ => s.toInt
    }
  }

  def main(args: Array[String]): Unit = {
    println(test("2"))
  }
}

Also fails.

[retronym]

I push-f-ed fixes for these problems.

[hrhino]

Thanks! I'm also not correct in my handling of Alternative... Right now it copies the body of the case for each pattern. I'll push a fix for that.

[hrhino]

I can't remember why I thought I couldn't use the synth default case from patmat. This should never be used (the Ident(WILDCARD, _, body) case should always be hit but it seems safer.

[hrhino]

I used to copy the body of a case if there were alternatives, so

x match { case "a" | "b" => body }

would introduce tree sharing.

[hrhino]

Thanks... I can't believe I forgot this

[retronym]

Probably worth a test to show that the scrutinee is only evaluated once. Cleanup.transformSwitch relies on the fact that patmat stabilizes the scrutinee into temp val.

I've manually tested with the following.

object Test {

  def test(s: String, expected: Int): Unit = {
    val it = Iterator(s)
    val r = (it.next(): @annotation.switch) match {
        // collision
        case "AaAa" => 1
        case "BBBB" => 2
      
        // collision with "FB"
        case "Ea"   => 3

        case _      => 4
     }
     assert(expected == r, (expected, r))
  }

  def main(args: Array[String]): Unit = {
    test("AaAa", 1)
    test("BBBB", 2)
    test("Ea", 3)
    test("FB", 4)
    test("XXX", 4)
    test(null, 4)
  }
}

[retronym]

Why "goto matchSuccess" here? All cases of the match branch so it seems like a no-op to me.

[hrhino]

Nice catch. I had it before I realized the problem with Alternatives

[retronym]

Comparing javac and scalac:

retronym/string-switch@c2820bb

class Test {
    int test(String s) {
        return switch(s) {
            case "AaAa" -> 1;
            case "BBBB" -> 2;
            case "Ea"   -> 3;
            case "xx"   -> 4;
            default     -> 5;
        };
    }
}

class Test {

  def test(s: String, expected: Int) = {
     s match {
        case "AaAa" => 1
        case "BBBB" => 2
        case "Ea"   => 3
        case "xx"   => 4
        case _      => 5
     }
  }
}

[hrhino]

That's interesting. Seems like Java emits two switch instructions: one to get a number representing what code block to run, and one to run it. I'm not sure I see a benefit there compared to this.

[hrhino]

I'm honored as always, @SethTisue

[martijnhoekstra]

There is a comment in the javac source that they do it to keep fall through simpler, so that doesn't apply to match.

[hrhino]

Nice find; thanks!

[retronym]

Another advantage the double-lookup would be the opportunity to use an invokedynamic based implementation like is being added to JDK 14 (https://github.com/openjdk/amber/blob/pattern-runtime/src/java.base/share/classes/java/lang/runtime/PatternHandles.java). That's not something we're targetting right now as we obviously can't depend on JDK14, but something for later on.

[retronym]

I guess the cleanup phase is de-facto part of the JVM backend as ScalaJS doesn't run it, but it does seem a little strange (but not a blocker for the change) to desugar match-on-int and match-on-string in different phases.

[sjrd]

it does seem a little strange (but not a blocker for the change) to desugar match-on-int and match-on-string in different phases.

The way I see it, match-on-string is desugared into match-on-int, but the latter is compiled as is to a JVM instruction. Match-on-int is never really desugared.

[lrytz]

LGTM!

[sjrd]

@hrhino Are you interested in trying to make the required changes in the Scala.js compiler backend, or should I do them?

[hrhino]

@sjrd I'll give it a go.

[lrytz]

Nice work! @sjrd should we wait with the merge until Scala.js is ready?

[sjrd]

should we wait with the merge until Scala.js is ready?

Yes please, otherwise the community build will break. @hrhino told me he intended to do it, but he needs some approval.

[sjrd]

So ... it turns out that in Scala.js 0.6.x, this actually just works out of the box 😆 The thing is that in 0.6.x, our IR supports any kind of literal in our Match nodes (equivalent to JVM switches), and our entire pipeline is ready to accept constant strings and nulls in there, and emit them as is in JS, which is valid JS. Even though none of it was ever exercised nor tested before, it just worked 😎

Now, ironically, Scala.js 1.x crashes and burns with this change, because in 1.x we restricted our Match nodes to support only Ints (like the JVM). So I'll have to work on that.

[sjrd]

scala-js/scala-js#3842

[sjrd]

OK we've got a PR for Scala.js with green CI and review, ready to be merged. So feel free to merge this PR in Scala, and we'll follow suit in Scala.js.