SI-12499 performance regression in patmat.

[paulp]

Introduced in 6eaf217. ListSets are slow.

Fixes scala/bug#12499

[Ichoran]

I don't think this is enough because Set isn't ordered, and the previous commit specifically stated that a stable order was important (iteration order?).

I don't know the code well enough to know why, and we don't have a performant ordered set in the collections library, so I'm not sure what to do.

[paulp]

/rebuild

[paulp]

It doesn't say it's important, it says it's "to stabilise the results" which is unlikely to mean anything beyond how the error is printed. The test which failed in CI doesn't fail that way for me, so I'm assuming for now it's spurious.

[paulp]

Failure isn't spurious but doesn't make sense to me. I guess the orderedness of the set is being relied upon for correctness. Still weird that it doesn't fail on my machine.

[paulp]

Oh... Oh... Oh.

Not every day you see one test case make three appearances in the comments

[paulp]

The error as it is lists a lot of unnecessary cases:

  List(5) match {
    case 1 :: Nil | 2 :: Nil  =>
    case (x@(4 | 5 | 6)) :: Nil =>
    case 7 :: Nil  =>
    case Nil =>
  }

t7020.scala:5: warning: match may not be exhaustive.
It would fail on the following inputs: List((x: Int forSome x not in (1, 2, 4, 5, 6, 7))), List((x: Int forSome x not in (1, 2, 4, 5, 6, 7)), _), List(1, _), List(2, _), List(4, _), List(5, _), List(6, _), List(7, _), List(_, _)
  List(5) match {
      ^

The last case List(_, _) subsumes all the others except for the first.

[paulp]

The comments make it seem like it was actually going for that output? I think it's that pathology where people start writing code to protect whatever the test case says the output should be. I changed it not to include patterns which cover other patterns and it went straight to the correct output.

test/files/neg/t7020.scala:5: warning: match may not be exhaustive.
It would fail on the following inputs: List((x: Int forSome x not in (1, 2, 4, 5, 6, 7))), List(_, _)
  List(5) match {
      ^
test/files/neg/t7020.scala:12: warning: match may not be exhaustive.
It would fail on the following inputs: List((x: Int forSome x not in (1, 2, 4, 5, 6, 7))), List(_, _)
  List(5) match {
      ^
test/files/neg/t7020.scala:19: warning: match may not be exhaustive.
It would fail on the following inputs: List((x: Int forSome x not in (1, 2, 4, 5, 6, 7))), List(_, _)
  List(5) match {
      ^
test/files/neg/t7020.scala:26: warning: match may not be exhaustive.
It would fail on the following inputs: List((x: Int forSome x not in (1, 2, 4, 5, 6, 7))), List(_, _)
  List(5) match {
      ^
4 warnings

[paulp]

I don't know how much I trust its ideas about exhaustiveness.

object Test {
  def f(xs: List[Int]) = xs match {
    case List(_) =>
  }
}

% scala -version
Scala code runner version 2.13.8 -- Copyright 2002-2021, LAMP/EPFL and Lightbend, Inc.

% scalac /tmp/test.scala
/tmp/test.scala:4: warning: match may not be exhaustive.
It would fail on the following inputs: List(_), Nil
  def f(xs: List[Int]) = xs match {
                         ^
1 warning

[paulp]

I think you guys are out of luck. Have to touch too many places to rescue the performance while preserving existing behaviors.

[SethTisue]

Note that the Scala 2.13 collections introduced SeqMap and VectorMap but we didn't get SeqSet and VectorSet at the same time. (reference: scala/scala-library-next#22)

Another contributor might be interested in trying to find a way forward with whatever is available.