Varargs and unapplySeq don't allow mutable Seqs anymore

[xuwei-k]

scala/scala#6962 (comment)

object A {
  def unapplySeq(a: Int): Option[scala.collection.mutable.Seq[Int]] =
    Some(scala.collection.mutable.ArraySeq(a))
}

object Main {
  def main(args: Array[String]): Unit = {
    val seq = 2 match {
      case A(xs @ _*) => xs
    }
  }
}

[info] Running Main 
[error] (run-main-2) java.lang.ClassCastException: scala.collection.mutable.ArraySeq$ofInt cannot be cast to scala.collection.immutable.Seq
[error] java.lang.ClassCastException: scala.collection.mutable.ArraySeq$ofInt cannot be cast to scala.collection.immutable.Seq
[error] 	at Main$.main(A.scala:9)
[error] 	at Main.main(A.scala)

[szeiger]

I'm hijacking this ticket instead of opening a new one because it is already about what looks to me like the core of the problem.

We changed the scala.Seq alias in M4 to resolve to scala.collection.immutable.Seq instead of scala.collection.Seq. This alias is used by varargs so now they are forced to be immutable, too. Since we have to interoperate with Java varargs, which use mutable arrays, varargs arrays coming from Java are wrapped with s.c.i.ArraySeq.unsafeWrapArray.

This is from an email by @julienrf where he sums up the situation and proposes a solution:

It seems that the fact that scala.Seq is now (in 2.13.x) an alias to
scala.collection.immutable.Seq causes problems in the case of varargs
methods and unapplySeq:

• previous code that used to pass a mutable.Seq as a varargs parameter
  does not compile anymore
• it’s possible to pass an Array, which is seen as an immutable
  Sequence from the method implementation side (see
  Disallow passing arrays for repeated arguments #11024)
• it’s possible to pattern match on the elements on a mutable sequence
  and see them as an immutable sequence (see
  [WIP] fix Array.unapplySeq scala#6962)

One solution suggested by @sjrd was to change the language
specification to define that varargs (and unapplySeq) should have type
scala.VarArgs, which would be an alias to scala.collection.Seq (as
before).

I'm not a fan of this solution. I think restricting varargs to immutable Seqs is acceptable. Enforcing immutability is generally a good thing. Passing arrays as varargs in #11024 can be solved in the same way that we are already using when passing arrays to methods that expect a scala.Seq: Allow it for now but make a defensive copy of the array and print a deprecation warning that tells users to either make that defensive copy explicitly or use unsafeWrapArray. I have implemented this in scala/scala#6970.

As @retronym pointed out, if you really need to treat a mutable Seq as a vararg, you can write a wrapper that pretends that it is immutable (which is what s.c.i.ArraySeq does for arrays).

If we stick with immutable scala.Seq and immutable varargs, the remaining piece of the puzzle is unapplySeq which is used in pattern matching as a dual of varargs constructors. As @allanrenucci points out in scala/scala#6962 (comment) it is not acceptable for performance reasons to require that unapplySeq return a scala.Seq. When used on a mutable collection or array we do not want to create a defensive copy. It would be unnecessary in most cases (e.g. case Array(1,2,3)) but the Seq can escape the pattern match (case Array(xs: _*)) so we can't just use a wrapper like s.c.i.ArraySeq without copying.

I propose that we sidestep this issue with name-based extractors. Scala has already supported them unofficially for a few versions now and they are implemented and properly specified in Dotty. The error that prompted this ticket actually appears to be a bug in the handling of name-based extractors by the pattern matcher. It already allows unapplySeq methods to return types other than scala.Seq as long as they support the required methods (apply, length and drop AFAIR) but erroneously casts the result to scala.Seq in cases where a _* match escapes. This could be fixed in one of two ways:

• Either remove the cast and use the inferred type instead. This would remove the need for any defensive copying in generated pattern matching code. If a _* match returns a mutable Seq it is properly typed as such and the user has to convert it to an immutable Seq if necessary.
• Or check the type and emit a .toSeq call if it does not conform to scala.Seq. This keeps _* extractors aligned with varargs and enforces immutability. Defensive would only be created for escaping sequence values but not for any that are only used internally.

I think we could justify keeping name-based extractors unofficial for another release but using them internally for Array and mutable.Seq extractors in the standard library. A proper specification based on Dotty's implementation could be done in 2.14 or 3.0.

[Ichoran]

Writing a wrapper should not be left as an exercise for the user for two reasons:

1. It is a lot of work for someone who is a user not a library implementor, and
2. If people do rise to the challenge, it will result in a profusion of wrappers, making it impossible to pattern-match out the mutable collections in case it is necessary for performance (e.g. if you want to use indexing).

Thus if we're going to go this route, we should write a collection.immutable.SeqSeq or somesuch.

[leolin479]

I don't see the merit of "restricting varargs to immutable Seqs". "varargs", as the syntax is suggesting, should be treated as if the collection is already been decomposed to a parameter list. In such sense, it shouldn't matter at all the collection itself is mutable or immutable.

It seems dogmatic to me that the decision was made purely based on "Enforcing immutability is generally a good thing" but nothing else.

The suggested workarounds, one of which is using "unsafeWrapArray". Would rather confuse the function caller. Why the caller has to do something "unsafe" while the function itself has nothing to depend on immutability?

[SethTisue]

If you're interested in exploring the thinking behind these decisions — which is a lot broader and stretches back a lot farther than just Stefan's one comment on this one ticket — I'd suggest asking about it on https://users.scala-lang.org