2.13: compatibility with new name-based extractor scheme

[SethTisue]

after scala/scala#7068 was merged, in the 2.13 community build (https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1371/consoleFull) we have:

[scala-xml] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/scala-xml-e1a7e0a64e2727f6b5e8cd3bed61e97ced6504ee/shared/src/main/scala/scala/xml/Utility.scala:47: error during expansion of this match (this is a scalac bug).
[scala-xml] [error] The underlying error was: type mismatch;
[scala-xml] [error]  found   : Seq[scala.xml.Node] (in scala.collection) 
[scala-xml] [error]  required: Seq[scala.xml.Node] (in scala.collection.immutable) 
[scala-xml] [error]   def trim(x: Node): Node = x match {
[scala-xml] [error]                               ^

and a few more such errors.

does this indicate that there is code in scala-xml that needs to be adjusted to the new scheme for name-based extractors? or is there a flaw in scala/scala#7068?

[SethTisue]

(sorting this out is a blocker for releasing 2.13.0-M5)

[ashawley]

Right, if the community build is using the newCollectionsBootstrap branch from #222, then it should be compiling still for 2.13.0-M4.

We hardcoded NodeSeq as a scala.collection.Seq in #210 via 562ea83, which is unreleased but should be in newCollectionsBootstrap.

Is there a test somewhere in the compiler to verify pattern matching with match on scala.collection.Seq works or not?

[SethTisue]

I think the part of scala/scala#7068 that's causing the problem is "This PR changes the type of x @ _* bound variables to scala.Seq" rather than the changes to how name-based extractors work. scala-xml uses def unapplySeq = ... in several places, so we need a few .toSeqs to make sure that we conform to the new expected signature (namely, the last element of the tuple returned needs to be an immutable.Seq and not just a collection.Seq)

[SethTisue]

I'll try this in the community build: scalacommunitybuild/scala-xml@5f67a6f

[ashawley]

Here are the definitions of unapplySeq:

Elem.unapplySeq(n: Node): Option[(String, String, MetaData, NamespaceBinding, Seq[Node])]
Node.unapplySeq(n: Node): Some[(String, MetaData, Seq[Node])]
QNode.unapplySeq(n: Node): Some[(String, String, MetaData, Seq[Node])]

And adding .toSeq should DTRT regardless of Scala version?

[ashawley]

Guess we'll see!

[SethTisue]

as far as I can see that would cross-compile, yes.

but, perhaps there's a more efficient solution if we're concerned about avoiding unnecessary copying? would need a little thought/investigation

[ashawley]

I've tried to write a minimal partest that replicates the Node, NodeSeq type signatures to try and raise this compiler error, but I wasn't successful so there is a lot more cargo cult than might be necessary to reproduce.

import scala.collection.AbstractSeq
import scala.collection.SeqOps
import scala.collection.immutable.StrictOptimizedSeqOps

abstract class CollectionSeq
  extends AbstractSeq[Int]
  with scala.collection.Seq[Int]
  with SeqOps[Int, scala.collection.immutable.Seq, CollectionSeq]
  with StrictOptimizedSeqOps[Int, scala.collection.immutable.Seq, CollectionSeq] 
{
  def first: Int
  def more: scala.collection.Seq[CollectionSeq]
  def iterator: Iterator[Int] = ???
  def apply(i: Int): Int = ???
  def length: Int = ???
}

case class CollSeqCons(first: Int, more: CollectionSeq*) extends CollectionSeq

object CollectionSeq {
  def unapplySeq(x: CollectionSeq) = Some((x.first, x.more))
}

object Test extends App {
  def f(x: CollectionSeq): CollectionSeq = x match {
    case CollSeqCons(x, xs @ _*) => CollSeqCons(x, xs: _*)
  }
}

[SethTisue]

I've tried to write a minimal partest that replicates the Node, NodeSeq type signatures to try and raise this compiler error

attn @lrytz

[lrytz]

I looked into this issue, I didn't see anything unexpected. We should improve the error message though for RC1, as the change breaks existing unapplySeq methods.

Here's a self-contained reproduction:

abstract class Node extends NodeSeq {
  def label: String
  def child: scala.collection.Seq[Node]
}

object Elem {
  def unapplySeq(n: Node): Option[(String, collection.Seq[Node])] = Some((n.label, n.child))
}

abstract class NodeSeq extends collection.immutable.Seq[Node] {
  def theSeq: collection.Seq[Node]
  def length = theSeq.length
  def iterator = theSeq.iterator
  def apply(i: Int): Node = theSeq(i)
}

object NodeSeq {
  import scala.language.implicitConversions
  implicit def seqToNodeSeq(s: collection.Seq[Node]): NodeSeq = new NodeSeq {
    def theSeq = s
  }
}

object Test {
  def foo(n: Node) = n match {
    case Elem(label, child @ _*) => child
  }
}

The error message is because the binding child in child @ _* now gets type immutable.Seq[Node]. In 2.12, it used to get what ever type was produced by unapplySeq. The compiler generates (shown using -Xprint:patmat)

            <synthetic> val o7: Option[(String, scala.collection.Seq[Node])] = Elem.unapplySeq(x1);
            if (o7.isEmpty.unary_!)
              {
                val child: Seq[Node] = o7.get._2;
                matchEnd4(child)

This fails to type-check and produces the error message (error during expansion of this match (this is a scalac bug)).

Note that it's enough to change collection.Seq to collection.immutable.Seq in the return type of the unapplySeq method. Then the implicit conversion seqToNodeSeq kicks in (it's in scope because Node extends NodeSeq). This also avoids copying the collection, which happens in Seth's fix (scalacommunitybuild@5f67a6f). Of course there's a danger wrapping a non-immutable Seq as an immutable Seq in that way, but that's a discussion about the core architecutre of the scala-xml library.

[ashawley]

Did adding .toSeq end up working?

I tried compiling on the latest M5 with Seth's branch:

[error] Test scala.xml.SerializationTest.unmatched failed: expected:<> but was:<List()>, took 0.078 sec
[error] Test scala.xml.SerializationTest.xmlLiteral failed: expected:<<node/>> but was:<List(scala.collection.generic.DefaultSerializationProxy@28ec9fc9)>, took 0.013 sec
[error] Test scala.xml.SerializationTest.implicitConversion failed: expected:<<child></child><child/>> but was:<List(List(scala.collection.generic.DefaultSerializationProxy@5844dad8), List(scala.collection.generic.DefaultSerializationProxy@3204a005))>, took 0.002 sec
[error] Test scala.xml.SerializationTest.empty failed: expected:<> but was:<List()>, took 0.001 sec
[error] Test scala.xml.XMLTestJVM.serializeAttribute failed: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to scala.xml.Attribute, took 0.011 sec
[error]     at scala.xml.XMLTestJVM.serializeAttribute(XMLTest.scala:218)
[error]     ...
[error] Test scala.xml.XMLTestJVM.serializeDocument failed: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to scala.xml.Document, took 0.016 sec
[error]     at scala.xml.XMLTestJVM.serializeDocument(XMLTest.scala:228)
[error]     ...
[error] Test scala.xml.XMLTestJVM.serializeElem failed: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to scala.xml.Elem, took 0.002 sec
[error]     at scala.xml.XMLTestJVM.serializeElem(XMLTest.scala:236)
[error]     ...
[error] Test scala.xml.XMLTestJVM.serializeComplex failed: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to scala.xml.Elem, took 0.082 sec
[error]     at scala.xml.XMLTestJVM.serializeComplex(XMLTest.scala:271)
[error]     ...
[error] Failed: Total 164, Failed 8, Errors 0, Passed 156
[error] Failed tests:
[error] 	scala.xml.XMLTestJVM
[error] 	scala.xml.SerializationTest

When I look in the last passing 2.13 community build from Aug-22 I only see scala-xml getting skipped:

[scala-xml] --== Building scala-xml ==--
[scala-xml] Found cached project build, uuid 4cab6219ea2340373d5bbf922cd70ba828d91bfd
[scala-xml] --== End Building scala-xml ==--

Hopefully, I'm using the right M5:

$ sum ~/.ivy2/cache/org.scala-lang/scala-*/jars/scala-*2.13.0-M5.jar
42388 10055 scala-compiler-2.13.0-M5.jar
58717 3972 scala-library-2.13.0-M5.jar
62224 3567 scala-reflect-2.13.0-M5.jar

[xuwei-k]

• xuwei-k@97bc6b4
• https://github.com/scala/scala/blob/v2.13.0-M5/src/library/scala/collection/Iterable.scala#L42-L46

[xuwei-k]

Should we change scala.Iterable#writeReplace implementation from the viewpoint of compatibility and liskov substitution principle? 🤔
scala/scala#6676