Merge pull request scala#760 from lrytz/imm-child

Change Node.child and Node.attribute return type to immutable.Seq on 2.13+

Author: lrytz

build.sbt

@@ -1,4 +1,5 @@
-import sbtcrossproject.CrossPlugin.autoImport.{crossProject, CrossType}
+import com.typesafe.tools.mima.core._
+import sbtcrossproject.CrossPlugin.autoImport.{CrossType, crossProject}
 
 publish / skip := true  // root project
 
@@ -68,6 +69,51 @@ lazy val xml = crossProject(JSPlatform, JVMPlatform, NativePlatform)
       //import com.typesafe.tools.mima.core.{}
       //import com.typesafe.tools.mima.core.ProblemFilters
       Seq( // exclusions for all Scala versions
+        // new method in `Node` with return type `immutable.Seq`
+        ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.xml.Node.child"),
+
+        // these used to be declared methods, but are now bridges without generic signature
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Node.nonEmptyChildren"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Group.child"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.SpecialNode.child"),
+
+        // new methods with return type immutable.Seq
+        ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.xml.Attribute.apply"),
+        ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.xml.Attribute.value"),
+        ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.xml.MetaData.apply"),
+        ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.xml.MetaData.value"),
+        ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.xml.NodeSeq.theSeq"),
+
+        // Synthetic static accessors (for Java interop) have a changed return type
+        ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.xml.Null.apply"),
+        ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.xml.Null.value"),
+        ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.xml.Utility.parseAttributeValue"),
+        ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.xml.Utility.trimProper"),
+
+        // used to be a declared method, now a bridge without generic signature
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.MetaData.apply"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Null.apply"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Null.value"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.PrefixedAttribute.apply"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.PrefixedAttribute.value"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.UnprefixedAttribute.apply"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.UnprefixedAttribute.value"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Document.theSeq"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Group.theSeq"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Node.theSeq"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.TextBuffer.toText"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Utility.trimProper"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Utility.parseAttributeValue"),
+
+        // Option[c.Seq] => Option[i.Seq] results in a changed generic signature
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.MetaData.get"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Null.get"),
+        ProblemFilters.exclude[IncompatibleSignatureProblem]("scala.xml.Node.attribute"),
+
+        // trait Attribute now extends trait ScalaVersionSpecificMetaData to ensure the previous signatures
+        // with return type `collection.Seq` remain valid.
+        // (trait Attribute extends MetaData, but that parent is not present in bytecode because it's a class.)
+        ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("scala.xml.Attribute.apply"),
       ) ++ (CrossVersion.partialVersion(scalaVersion.value) match {
         case Some((3, _)) => Seq( // Scala 3-specific exclusions
         )

jvm/src/test/scala/scala/xml/SerializationTest.scala

@@ -25,7 +25,7 @@ class SerializationTest {
   def implicitConversion(): Unit = {
     val parent: Elem = <parent><child></child><child/></parent>
     val children: Seq[Node] = parent.child
-    val asNodeSeq: NodeSeq = children
+    val asNodeSeq: NodeSeq = children // implicit seqToNodeSeq
     assertEquals(asNodeSeq, JavaByteSerialization.roundTrip(asNodeSeq))
   }
 }

shared/src/main/scala-2.12/scala/xml/ScalaVersionSpecific.scala

@@ -22,7 +22,8 @@ private[xml] object ScalaVersionSpecific {
     override def apply(from: Coll): mutable.Builder[Node, NodeSeq] = NodeSeq.newBuilder
     override def apply(): mutable.Builder[Node, NodeSeq] = NodeSeq.newBuilder
   }
-  type SeqNodeUnapplySeq = scala.collection.Seq[Node]
+  type SeqOfNode = scala.collection.Seq[Node]
+  type SeqOfText = scala.collection.Seq[Text]
 }
 
 private[xml] trait ScalaVersionSpecificNodeSeq extends SeqLike[Node, NodeSeq] { self: NodeSeq =>
@@ -33,3 +34,11 @@ private[xml] trait ScalaVersionSpecificNodeSeq extends SeqLike[Node, NodeSeq] {
 private[xml] trait ScalaVersionSpecificNodeBuffer { self: NodeBuffer =>
   override def stringPrefix: String = "NodeBuffer"
 }
+
+private[xml] trait ScalaVersionSpecificNode { self: Node => }
+
+private[xml] trait ScalaVersionSpecificMetaData { self: MetaData => }
+
+private[xml] trait ScalaVersionSpecificTextBuffer { self: TextBuffer => }
+
+private[xml] trait ScalaVersionSpecificUtility { self: Utility.type => }

shared/src/main/scala-2.13+/scala/xml/ScalaVersionSpecific.scala

@@ -24,7 +24,8 @@ private[xml] object ScalaVersionSpecific {
     def newBuilder(from: Coll): Builder[Node, NodeSeq] = NodeSeq.newBuilder
     def fromSpecific(from: Coll)(it: IterableOnce[Node]): NodeSeq = (NodeSeq.newBuilder ++= from).result()
   }
-  type SeqNodeUnapplySeq = scala.collection.immutable.Seq[Node]
+  type SeqOfNode = scala.collection.immutable.Seq[Node]
+  type SeqOfText = scala.collection.immutable.Seq[Text]
 }
 
 private[xml] trait ScalaVersionSpecificNodeSeq
@@ -48,8 +49,34 @@ private[xml] trait ScalaVersionSpecificNodeSeq
     fromSpecific(new View.Map(this, f))
   def flatMap(f: Node => IterableOnce[Node]): NodeSeq =
     fromSpecific(new View.FlatMap(this, f))
+
+  def theSeq: scala.collection.Seq[Node]
 }
 
 private[xml] trait ScalaVersionSpecificNodeBuffer { self: NodeBuffer =>
   override def className: String = "NodeBuffer"
 }
+
+private[xml] trait ScalaVersionSpecificNode { self: Node =>
+  // These methods are overridden in Node with return type `immutable.Seq`. The declarations here result
+  // in a bridge method in `Node` with result type `collection.Seq` which is needed for binary compatibility.
+  def child: scala.collection.Seq[Node]
+  def nonEmptyChildren: scala.collection.Seq[Node]
+}
+
+private[xml] trait ScalaVersionSpecificMetaData { self: MetaData =>
+  def apply(key: String): scala.collection.Seq[Node]
+  def apply(namespace_uri: String, owner: Node, key: String): scala.collection.Seq[Node]
+  def apply(namespace_uri: String, scp: NamespaceBinding, k: String): scala.collection.Seq[Node]
+
+  def value: scala.collection.Seq[Node]
+}
+
+private[xml] trait ScalaVersionSpecificTextBuffer { self: TextBuffer =>
+  def toText: scala.collection.Seq[Text]
+}
+
+private[xml] trait ScalaVersionSpecificUtility { self: Utility.type =>
+  def trimProper(x: Node): scala.collection.Seq[Node]
+  def parseAttributeValue(value: String): scala.collection.Seq[Node]
+}

shared/src/main/scala-2/scala/xml/ScalaVersionSpecificReturnTypes.scala

@@ -28,7 +28,6 @@ private[xml] object ScalaVersionSpecificReturnTypes { // should be
   type NullNext = scala.Null
   type NullKey = scala.Null
   type NullValue = scala.Null
-  type NullApply1 = scala.collection.Seq[Node]        // scala.Null
   type NullApply3 = scala.Null
   type NullRemove = Null.type
   type SpecialNodeChild = Nil.type

shared/src/main/scala-3/scala/xml/ScalaVersionSpecificReturnTypes.scala

@@ -19,18 +19,17 @@ package scala.xml
  What should have been specified explicitly is given in the comments;
  next time we break binary compatibility the types should be changed in the code and this class removed.
  */
-private[xml] object ScalaVersionSpecificReturnTypes { // should be
-  type ExternalIDAttribute = MetaData                 // Null.type
-  type NoExternalIDId = String                        // scala.Null
-  type NodeNoAttributes = MetaData                    // Null.type
-  type NullFilter = MetaData                          // Null.type
-  type NullGetNamespace = String                      // scala.Null
-  type NullNext = MetaData                            // scala.Null
-  type NullKey = String                               // scala.Null
-  type NullValue = scala.collection.Seq[Node]         // scala.Null
-  type NullApply1 = scala.collection.Seq[Node]        // scala.Null
-  type NullApply3 = scala.collection.Seq[Node]        // scala.Null
-  type NullRemove = MetaData                          // Null.type
-  type SpecialNodeChild = scala.collection.Seq[Node]  // Nil.type
-  type GroupChild = scala.collection.Seq[Node]        // Nothing
+private[xml] object ScalaVersionSpecificReturnTypes {          // should be
+  type ExternalIDAttribute = MetaData                          // Null.type
+  type NoExternalIDId = String                                 // scala.Null
+  type NodeNoAttributes = MetaData                             // Null.type
+  type NullFilter = MetaData                                   // Null.type
+  type NullGetNamespace = String                               // scala.Null
+  type NullNext = MetaData                                     // scala.Null
+  type NullKey = String                                        // scala.Null
+  type NullValue = scala.collection.immutable.Seq[Node]        // scala.Null
+  type NullApply3 = scala.collection.immutable.Seq[Node]       // scala.Null
+  type NullRemove = MetaData                                   // Null.type
+  type SpecialNodeChild = scala.collection.immutable.Seq[Node] // Nil.type
+  type GroupChild = scala.collection.immutable.Seq[Node]       // Nothing
 }

shared/src/main/scala/scala/xml/Attribute.scala

@@ -53,14 +53,14 @@ object Attribute {
  *
  *  @author  Burak Emir
  */
-trait Attribute extends MetaData {
+trait Attribute extends MetaData with ScalaVersionSpecificMetaData {
   def pre: String // will be null if unprefixed
   override val key: String
-  override val value: Seq[Node]
+  override val value: ScalaVersionSpecific.SeqOfNode
   override val next: MetaData
 
-  override def apply(key: String): Seq[Node]
-  override def apply(namespace: String, scope: NamespaceBinding, key: String): Seq[Node]
+  override def apply(key: String): ScalaVersionSpecific.SeqOfNode
+  override def apply(namespace: String, scope: NamespaceBinding, key: String): ScalaVersionSpecific.SeqOfNode
   override def copy(next: MetaData): Attribute
 
   override def remove(key: String): MetaData =

shared/src/main/scala/scala/xml/Document.scala

@@ -36,7 +36,7 @@ class Document extends NodeSeq with Serializable {
    *  excluded. If there is a document type declaration, the list also
    *  contains a document type declaration information item.
    */
-  var children: Seq[Node] = _
+  var children: Seq[Node] = _ // effectively an `immutable.Seq`, not changed due to binary compatibility
 
   /** The element information item corresponding to the document element. */
   var docElem: Node = _
@@ -96,7 +96,7 @@ class Document extends NodeSeq with Serializable {
 
   // methods for NodeSeq
 
-  override def theSeq: Seq[Node] = this.docElem
+  override def theSeq: ScalaVersionSpecific.SeqOfNode = this.docElem
 
   override def canEqual(other: Any): Boolean = other match {
     case _: Document => true

shared/src/main/scala/scala/xml/Elem.scala

@@ -27,10 +27,10 @@ object Elem {
   def apply(prefix: String, label: String, attributes: MetaData, scope: NamespaceBinding, minimizeEmpty: Boolean, child: Node*): Elem =
     new Elem(prefix, label, attributes, scope, minimizeEmpty, child: _*)
 
-  def unapplySeq(n: Node): Option[(String, String, MetaData, NamespaceBinding, ScalaVersionSpecific.SeqNodeUnapplySeq)] =
+  def unapplySeq(n: Node): Option[(String, String, MetaData, NamespaceBinding, ScalaVersionSpecific.SeqOfNode)] =
     n match {
       case _: SpecialNode | _: Group => None
-      case _                         => Some((n.prefix, n.label, n.attributes, n.scope, n.child.toSeq))
+      case _                         => Some((n.prefix, n.label, n.attributes, n.scope, n.child))
     }
 }
 
@@ -104,7 +104,7 @@ class Elem(
     scope: NamespaceBinding = this.scope,
     minimizeEmpty: Boolean = this.minimizeEmpty,
     child: Seq[Node] = this.child
-  ): Elem = Elem(prefix, label, attributes, scope, minimizeEmpty, child: _*)
+  ): Elem = Elem(prefix, label, attributes, scope, minimizeEmpty, child.toSeq: _*)
 
   /**
    * Returns concatenation of `text(n)` for each child `n`.

shared/src/main/scala/scala/xml/Group.scala

@@ -22,7 +22,11 @@ import scala.collection.Seq
  */
 // Note: used by the Scala compiler.
 final case class Group(nodes: Seq[Node]) extends Node {
-  override def theSeq: Seq[Node] = nodes
+  // Ideally, the `immutable.Seq` would be stored as a field.
+  // But evolving the case class and remaining binary compatible is very difficult
+  // Since `Group` is used rarely, call `toSeq` on the field.
+  // In practice, it should not matter - the `nodes` field anyway contains an `immutable.Seq`.
+  override def theSeq: ScalaVersionSpecific.SeqOfNode = nodes.toSeq
 
   override def canEqual(other: Any): Boolean = other match {
     case _: Group => true