If we go ahead with this PR we'll need a new minor release (2.4.0).

There is more API with return type collection.Seq that probably always returns an immutable.Seq, e.g.

• Metadata.value
• NodeSeq.theSeq
• MetaData.apply / MetaData.get / Node.attribute
• TextBuffer.toText
• BasicTransformer.transform

I'm happy take a look at those as well; I only did child for now because that's what is affecting the 2.13 migration I'm working on.

For the record, there is an implicit conversion collection.Seq[Node] => NodeSeq and NodeSeq is a subclass of immutable.Seq on 2.13.

So the following works already without this PR

scala> val n: Node = <a><b/></a>
val n: scala.xml.Node = <a><b/></a>

scala> val c: collection.immutable.Seq[Node] = n.child
val c: Seq[scala.xml.Node] = Seq(<b/>)

But this fails:

scala> val c: collection.immutable.Seq[String] = n.child.map(_.toString)
                                                            ^
       error: type mismatch;
        found   : Seq[String] (in scala.collection)
        required: Seq[String] (in scala.collection.immutable)

probably always returns an immutable.Seq

I took a closer look.

MarkupParser passes a NodeBuffer – I'll change that.

The compiler uses a NodeBuffer for XML literals and passes it directly to Elem($buf). That worked on 2.12, so child is actually a mutable collection. On 2.13 the implicit seqToNodeSeq makes it compile. Details here: scala/scala#11065

I also changed Node.attribute and the related MetaData.apply / MetaData.value / MetaData.get methods to return immutable.Seq.

And I changed NodeSeq.theSeq, even though this method is probably not used often as part of the public API. The reason is that NodeSeq extends immutable.Seq, and if theSeq is not immutable, it is essentially an unsafe immutable wrapper around a generic collection:

scala> val b = new NodeBuffer() += <b/>

scala> val x: NodeSeq = Elem(null, "a", Null, TopScope, true, NodeSeq.fromSeq(b): _*)
val x: scala.xml.NodeSeq = <a><b/></a>

scala> b += <c/>

scala> x
val res0: scala.xml.NodeSeq = <a><b/><c/></a>

I didn't change BasicTransformer.transform because that API is expected to be subclassed, changes would likely cause source incompatibilities.

This is ready, if you'd like to take a look @dubinsky.

Besides mima, I also used jardiff to compare the changes and it looks as expected.

One minor comment -- it might be more efficient to use VectorBuilder rather than NodeBuffer (which extends ArrayBuffer adding the utility method &+).

Right.. We have to keep NodeBuffer extends ArrayBuffer though, because of source and binary compatibility. So the code emitted by the compiler for literals will keep using ArrayBuffer.

We could use a VectorBuilder within scala-xml on 2.13, but it would involve changing public signatures, e.g. MarkupParser.content1(NamespaceBinding, NodeBuffer): Unit.

So it's probably not worth the effort...

Agreed. If we were maximising performance from XML literals we'd be better to use an immutable ArraySeq builder with a size hint as the compiler knows how many elements there are. But maximising performance isn't really a design goal of scala-xml IMO, so the small overhead of ArrayBuffer => Vector is okay.