New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.13: compatibility with new name-based extractor scheme #253

Open
SethTisue opened this Issue Aug 21, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@SethTisue
Copy link
Member

SethTisue commented Aug 21, 2018

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

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 21, 2018

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

@ashawley

This comment has been minimized.

Copy link
Member

ashawley commented Aug 21, 2018

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

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 21, 2018

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

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 21, 2018

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

@ashawley

This comment has been minimized.

Copy link
Member

ashawley commented Aug 21, 2018

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

This comment has been minimized.

Copy link
Member

ashawley commented Aug 21, 2018

Guess we'll see!

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 21, 2018

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

This comment has been minimized.

Copy link
Member

ashawley commented Aug 22, 2018

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

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 22, 2018

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

This comment has been minimized.

Copy link
Member

lrytz commented Aug 22, 2018

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

This comment has been minimized.

Copy link
Member

ashawley commented Aug 29, 2018

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

This comment has been minimized.

Copy link
Contributor

xuwei-k commented Aug 30, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment