Skip to content

Use toVector for XML literal sequences #23221

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented May 21, 2025

Forward-port of scala/scala#11065

In <a><b/><c/><a>, a NodeBuffer (which extends ArrayBuffer) is used to accumulate the children. The buffer is passed to new Elem($buf: _*), which only works thanks to the implicit collection.Seq[Node] => NodeSeq declared in scala-xml.

With -Vprint:typer:

scala> <a><b/></a>
[[syntax trees at end of                     typer]] // rs$line$1

    val res0: scala.xml.Elem =
      {
        new _root_.scala.xml.Elem(null, "a", _root_.scala.xml.Null,
          scala.xml.TopScope, false,
          {
            val $buf: scala.xml.NodeBuffer = new _root_.scala.xml.NodeBuffer()
            $buf.&+(
              {
                {
                  new _root_.scala.xml.Elem(null, "b", _root_.scala.xml.Null,
                    scala.xml.TopScope, true, [ : scala.xml.Node]*)
                }
              }
            )
            scala.xml.NodeSeq.seqToNodeSeq($buf)
          }*
        )
      }

The implicit was not inserted in 2.12 because the varargs parameter of Elem accepted a collection.Seq.

@lrytz lrytz changed the title Use toVector` for XML literal sequences Use toVector for XML literal sequences May 21, 2025
@lrytz
Copy link
Member Author

lrytz commented May 21, 2025

I have a related PR to scala-xml which changes the API to return immutable.Seq instead of collection.Seq scala/scala-xml#760.

The current scala.xml.NodeSeq is only pretending to be immutable :)

$ scala repl -S 3 --dep org.scala-lang.modules::scala-xml:2.3.0

scala> import scala.xml._

scala> val x: collection.immutable.Seq[Node] = <a><b/></a>
val x: Seq[scala.xml.Node] = <a><b/></a>

scala> x.asInstanceOf[scala.xml.Node].child.asInstanceOf[scala.xml.NodeSeq].theSeq.asInstanceOf[NodeBuffer] += <c/>

scala> x
val res1: Seq[scala.xml.Node] = <a><b/><c/></a>

In `<a><b/><c/><a>`, a `NodeBuffer` (which extends `ArrayBuffer`) is
used to accumulate the children. The buffer is passed to
`new Elem($buf: _*)`, which only works thanks to the implicit
`collection.Seq[Node] => NodeSeq` declared in scala-xml.

With `-Vprint:typer`:

```scala
scala> <a><b/></a>
[[syntax trees at end of                     typer]] // rs$line$1

    val res0: scala.xml.Elem =
      {
        new _root_.scala.xml.Elem(null, "a", _root_.scala.xml.Null,
          scala.xml.TopScope, false,
          {
            val $buf: scala.xml.NodeBuffer = new _root_.scala.xml.NodeBuffer()
            $buf.&+(
              {
                {
                  new _root_.scala.xml.Elem(null, "b", _root_.scala.xml.Null,
                    scala.xml.TopScope, true, [ : scala.xml.Node]*)
                }
              }
            )
            scala.xml.NodeSeq.seqToNodeSeq($buf)
          }*
        )
      }
```

The implicit was not inserted in 2.12 because the varargs parameter of
Elem accepted a `collection.Seq`.
@lrytz
Copy link
Member Author

lrytz commented May 22, 2025

There is code that relies on XML sequences <a/><b/> having type NodeBuffer

https://github.com/scalatest/scalatest/blob/release-3.2.19/jvm/core/src/main/scala/org/scalatest/tools/HtmlReporter.scala#L828

<table>...</table><table>...</table> &+ getHTMLForCause(cause)

Method &+ is defined in NodeBuffer.

I narrowed the change so that plain XML sequences keep type NodeBuffer. Other XML sequences (children passed to Elem, children of <xml:group> passed to Group, attribute values with entity references passed to UnprefixedAttribute / PrefixedAttribute ) are converted toVector.

@som-snytt
Copy link
Contributor

I know you have your reasons. But the dust you raise makes me sneeze. Oh it almost rhymes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants