Skip to content
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

NodeSeq ++ results in List[Node] in 2.13 #392

Closed
fanf opened this issue Jan 20, 2020 · 9 comments
Closed

NodeSeq ++ results in List[Node] in 2.13 #392

fanf opened this issue Jan 20, 2020 · 9 comments
Milestone

Comments

@fanf
Copy link

fanf commented Jan 20, 2020

I see the following change in behavior between scala 2.12 and 2.13:
Scala 2.12:

scala> import scala.xml._
import scala.xml._

scala> val a: NodeSeq = <a>Hello</a>
a: scala.xml.NodeSeq = <a>Hello</a>

scala> a ++ <b>hi</b>
res0: scala.xml.NodeSeq = NodeSeq(<a>Hello</a>, <b>hi</b>)

Scala 2.13:

scala> a ++ <b>hi</b>
res0: Seq[scala.xml.Node] = List(<a>Hello</a>, <b>hi</b>)

That change has a very deep reach for us, because Liftweb doesn't interpret both type in the same way: it applies template transformation one time for a NodeSeq (it's only one template), and N times for List[Node] (it's a list of template). Our web application is totally broken, and it's almost impossible to find all places which would need a type ascription (and it would bloat a lot the resulting code.

Is there a way to get back the old behavior? Perhaps we are missing a new implicit? Perhaps the implicit precedence in NodeSeq changed and it could be adpated?
Any insight would be appreciated.

@ashawley ashawley added this to the 1.3.0 milestone Jan 20, 2020
@ashawley ashawley changed the title Scala 2.13 NodeSeq ++ results in List[Node] vs NodeSeq in 2.12 NodeSeq ++ results in List[Node] in 2.13 Jan 20, 2020
@ashawley
Copy link
Member

Yeah, the code base for 2.13 is new as a result of the changes to the collections library updates, so the code for NodeSeq here is also different for 2.12.

If we're going to fix this and make a release, we should make the change to 1.x branch. It will be easy to cherry-pick to master for a 2.0.0-M2 release.

@ashawley
Copy link
Member

Here's a first attempt at writing a unit test:

package scala.xml

import org.junit.Test
import org.junit.Assert.assertEquals
import org.junit.Assert.fail

class NodeSeqTest {

  @Test
  def testAppend: Unit = {
    val a: NodeSeq = <a>Hello</a>
    val b = <b>Hi</b>
    val res: NodeSeq = a ++ b
    assertEquals(NodeSeq.fromSeq(Seq(<a>Hello</a>, <b>Hi</b>)), res)
    a ++ <b>Hi</b> match {
      case _: NodeSeq => assertEquals(2, res.size) // 2.12 and earlier
      case _: List[Node] => fail("Should be NodeSeq") // 2.13
    }
  }
}

@ashawley
Copy link
Member

Since there were a lot of changes to the build during 2.13 milestones, it's difficult to bisect this. My first pass at it suggests switching to 2.13.0-M4 in 4c7a945 is the issue. That raises different questions than I epxected, though. I'd need to dig further in to this to confirm.

@jtjeferreira
Copy link

I think NodeSeq needs to override ++ to return NodeSeq as explained in https://docs.scala-lang.org/overviews/core/custom-collections.html

To address this shortcoming, you need to overload the methods that return an IndexedSeq[B] for the case where B is known to be Base, to return an RNA2 instead.

@fanf

This comment has been minimized.

@SethTisue

This comment has been minimized.

@ashawley
Copy link
Member

I think NodeSeq needs to override ++ to return NodeSeq

Good catch!

I've tried to cargo cult that example code in #396. It seemed to fix the specific issue raised here.

@fanf

This comment has been minimized.

@SethTisue
Copy link
Member

@fanf thanks for reporting this! @ashawley has now published 1.3.0 with the fix

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

No branches or pull requests

4 participants