Skip to content

Commit

Permalink
Make RuleTranformer fully recursive [scala#257]
Browse files Browse the repository at this point in the history
RuleTransformer for the past eleven years or more has first recursed,
then applied the rewrite rules as it backed out of the recursion.

That prevented rewrite rules from acting on changes of previous rules
unless they recursed themselves.

The workaround has always been chain rule transformers instead of
calling one rule transformer with all the rules. This change basically
re-implements RuleTransformer as that workaround, and introduces
a NestingTransformer which does the nesting.

Clearly, if you have N rules you'll now recurse N times instead of
once, though each rule is still only applied once for each element.

On the other hand, a RewriteRule that recursed would incur in
exponential times, and now they don't need to.

The original behavior has no reason to be. It didn't prevent
rules from seeing each other changes, nor was it particularly
concerned with performance.  With API changes coming on 2.0, I
believe this is the right time to introduce this change.
  • Loading branch information
dcsobral committed Mar 21, 2020
1 parent 226c1af commit c17cd10
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 3 deletions.
19 changes: 19 additions & 0 deletions shared/src/main/scala/scala/xml/transform/NestingTransformer.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/* __ *\
** ________ ___ / / ___ Scala API **
** / __/ __// _ | / / / _ | (c) 2002-2020, LAMP/EPFL **
** __\ \/ /__/ __ |/ /__/ __ | (c) 2011-2020, Lightbend, Inc. **
** /____/\___/_/ |_/____/_/ | | http://scala-lang.org/ **
** |/ **
\* */

package scala
package xml
package transform

import scala.collection.Seq

class NestingTransformer(rule: RewriteRule) extends BasicTransformer {
override def transform(n: Node): Seq[Node] = {
rule.transform(super.transform(n))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ package transform
import scala.collection.Seq

class RuleTransformer(rules: RewriteRule*) extends BasicTransformer {
override def transform(n: Node): Seq[Node] =
rules.foldLeft(super.transform(n)) { (res, rule) => rule transform res }
private val transformers = rules.map(new NestingTransformer(_))
override def transform(n: Node): Seq[Node] = {
if (transformers.isEmpty) n
else transformers.tail.foldLeft(transformers.head.transform(n)) { (res, transformer) => transformer.transform(res) }
}
}
17 changes: 16 additions & 1 deletion shared/src/test/scala-2.x/scala/xml/TransformersTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class TransformersTest {
@Test
def preserveReferentialComplexityInLinearComplexity = { // SI-4528
var i = 0

val xmlNode = <a><b><c><h1>Hello Example</h1></c></b></a>

new RuleTransformer(new RewriteRule {
Expand All @@ -77,4 +77,19 @@ class TransformersTest {

assertEquals(1, i)
}

@Test
def appliesRulesRecursivelyOnPreviousChanges = { // #257
def add(outer: Elem, inner: Node) = new RewriteRule {
override def transform(n: Node): Seq[Node] = n match {
case e: Elem if e.label == outer.label => e.copy(child = e.child ++ inner)
case other => other
}
}

def transformer = new RuleTransformer(add(<element/>, <new/>), add(<new/>, <thing/>))

assertEquals(<element><new><thing/></new></element>, transformer(<element/>))
}
}

0 comments on commit c17cd10

Please sign in to comment.