SI-3368 CDATA gets a Node by som-snytt · Pull Request #4306 · scala/scala

som-snytt · 2015-02-10T07:01:09Z

XML Parser uses scala.xml.PCData.

A compiler flag -Yxml:coalescing, analogous to
DocumentBuilderFactory.setCoalescing, turns PCData
nodes into Text nodes and coalesces sibling text nodes.

This change also fixes parse errors such as rejecting a
sequence of CDATA sections.

A sequence of "top level" nodes is not coalesced.

scala> <a><b/>start<![CDATA[hi & bye]]><c/>world<d/>stuff<![CDATA[red & black]]></a>
res0: scala.xml.Elem = <a><b/>start<![CDATA[hi & bye]]><c/>world<d/>stuff<![CDATA[red & black]]></a>

scala> :replay -Yxml:coalescing
Replaying: <a><b/>start<![CDATA[hi & bye]]><c/>world<d/>stuff<![CDATA[red & black]]></a>
res0: scala.xml.Elem = <a><b/>starthi &amp; bye<c/>world<d/>stuffred &amp; black</a>

adriaanm · 2015-02-19T00:17:46Z

Since the 2.11.6 deadline is extremely nigh, I've tentatively postponed this PR (along with other recent ones) to 2.11.7. I'll make a pass through reviewed 2.11.7 PRs on Friday and move them back to 2.11.6 where feasible.

In the mean time, feel free to let me know if you'd like some help getting this reviewed by then.

lrytz · 2015-03-23T20:44:24Z

src/compiler/scala/tools/nsc/settings/ScalaSettings.scala

It would be nice to have a help text here, like we do in -language

lrytz · 2015-03-24T09:53:48Z

looks great! just the few minor things mentioned above.

som-snytt · 2015-03-24T15:50:56Z

@lrytz Thx! I'll break out the can of polish.

XML Parser uses `scala.xml.PCData`. A compiler flag `-Yxml:coalescing`, analogous to `DocumentBuilderFactory.setCoalescing`, turns `PCData` nodes into `Text` nodes and coalesces sibling text nodes. This change also fixes parse errors such as rejecting a sequence of CDATA sections. A sequence of "top level" nodes are not coalesced. ``` scala> <a><b/>start<![CDATA[hi & bye]]><c/>world<d/>stuff<![CDATA[red & black]]></a> res0: scala.xml.Elem = <a><b/>start<![CDATA[hi & bye]]><c/>world<d/>stuff<![CDATA[red & black]]></a> scala> :replay -Yxml:coalescing Replaying: <a><b/>start<![CDATA[hi & bye]]><c/>world<d/>stuff<![CDATA[red & black]]></a> res0: scala.xml.Elem = <a><b/>starthi & bye<c/>world<d/>stuffred & black</a> ```

Update the test slightly to use the rig it inspired.

As long as Scala does XML literals, there is probably parsing behavior worth configuring. Therefore, the umbrella option is promoted to `-Xxml`. It was tempting to make it `-XML`, but we resisted.

som-snytt · 2015-04-08T16:52:29Z

Rebased, so it lost previous greenies.

adriaanm · 2015-04-08T18:30:38Z

The (non-deterministic?) fail in b60de05 seems to imply we regressed somewhere (in the pattern matcher) -- diagnosing! Sorry about loss of green. Rebuilding to see whether it's stubborn.

adriaanm · 2015-04-08T19:16:15Z

ff2c0bb seems to have a legit fail, though:

fail - run/t5699.scala  [output differs]% scalac t5699.scala
% /usr/lib/jvm/jdk1.6.0_45/jre/bin/java \
  -Dfile.encoding=UTF-8 \
  -server \
  -XX:+AggressiveOpts \
  -XX:+UseParNewGC \
  -Xmx2G \
  -Xss1M \
  -XX:MaxPermSize=512M \
  -XX:ReservedCodeCacheSize=128M \
  -Dpartest.threads=4 \
  -classpath \
  /home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run/t5699-run.obj:/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-library.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-reflect.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-compiler.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scalap.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-actors.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/m2repo/org/scala-lang/modules/scala-partest_2.11/1.0.6/scala-partest_2.11-1.0.6.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/m2repo/com/googlecode/java-diff-utils/diffutils/1.3.0/diffutils-1.3.0.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/m2repo/org/scala-sbt/test-interface/1.0/test-interface-1.0.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/m2repo/org/scala-lang/modules/scala-xml_2.11/1.0.3/scala-xml_2.11-1.0.3.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/m2repo/org/scala-lang/modules/scala-parser-combinators_2.11/1.0.3/scala-parser-combinators_2.11-1.0.3.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/m2repo/org/scalacheck/scalacheck_2.11/1.11.4/scalacheck_2.11-1.11.4.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-partest-extras.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-partest-javaagent.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/lib/annotations.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/lib/enums.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/lib/genericNest.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/lib/jsoup-1.3.1.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/lib/macro210.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/lib/methvsfield.jar:/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/lib/nest.jar \
  -Dfile.encoding=UTF-8 \
  -Djava.library.path=/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run \
  -Dpartest.output=/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run/t5699-run.obj \
  -Dpartest.lib=/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-library.jar \
  -Dpartest.reflect=/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-reflect.jar \
  -Dpartest.comp=/home/jenkins/workspace/scala-2.11.x-validate-test/build/pack/lib/scala-compiler.jar \
  -Dpartest.cwd=/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run \
  -Dpartest.test-path=/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run/t5699.scala \
  -Dpartest.testname=t5699 \
  -Djavacmd=/usr/lib/jvm/jdk1.6.0_45/jre/bin/java \
  -Djavaccmd=javac \
  -Duser.language=en \
  -Duser.country=US \
  scala.tools.nsc.MainGenericRunner \
  -usejavacp \
  Test \
  jvm > t5699-run.log
% diff /home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run/t5699-run.log /home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run/t5699.check
--- a/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run/t5699-run.log
+++ b/home/jenkins/workspace/scala-2.11.x-validate-test/test/files/run/t5699.check
@@ -1,3 +1,11 @@
 [[syntax trees at end of                    parser]] // annodef.java
-<empty>
+package <empty> {
+  object MyAnnotation extends  {
+    def <init>() = _
+  };
+  class MyAnnotation extends scala.annotation.Annotation with _root_.java.lang.annotation.Annotation with scala.annotation.ClassfileAnnotation {
+    def <init>() = _;
+    def value(): String
+  }
+}

som-snytt · 2015-04-08T19:17:47Z

I just commented above (outdatedly):

Owner
lrytz added a note 16 days ago

also redundant now
@som-snytt
som-snytt added a note 3 hours ago

It will stop after parser; assume it would proceed successfully thru typer.
@som-snytt
som-snytt added a note 8 minutes ago

Or, maybe it won't print anything and fail the test.

som-snytt · 2015-04-08T20:37:00Z

OK, next theory, the prodigal test is compiling Java code. How does that interact with phasing? he asked himself.

I left the test overriding extra settings with -Ystop-after:namer. I came close to -Ystop-before:packageobjects because neither stop-before nor packageobjects get enough attention.

Verbose option help and test tweak.

som-snytt · 2015-04-08T23:03:16Z

/rebuild

adriaanm · 2015-04-10T00:00:59Z

Let's 🚀 ✨ :so_much_emoji_everywhere!:

SI-3368 CDATA gets a Node

adriaanm · 2015-04-13T21:24:46Z

Is this test failure expected? https://scala-ci.typesafe.com/job/scala-2.11.x-integrate-bootstrap/112/artifact/logs/builds/*view*/

Test method: https://github.com/scala/scala-xml/blob/master/src/test/scala/scala/xml/XMLTest.scala#L441

Tracked as scala/scala-xml#51

som-snytt · 2015-04-13T22:12:43Z

I'll take a look. I guess it doesn't community-build per commit yet.

adriaanm · 2015-04-13T23:09:28Z

Thank you! Unfortunately, this wasn't caught until late at night (in some timezone anyway).

som-snytt · 2015-04-13T23:17:41Z

I commented on the other ticket that presumably -Xxml:coalescing should default for 2.11.x. With a dash of -Xfuture.

Or, possibly, the previous behavior was just a bug, and the only people using scala-xml are the only people who want this feature to default to the correct behavior.

adriaanm · 2015-04-13T23:18:50Z

I unfortunately can't speak for one or the other, knowing nothing of xml's knotty gritty except that it was one of Scala's hippest features at one point.

som-snytt · 2015-04-13T23:20:46Z

I saw the slide show. OK, I'll re-open this ticket to preserve behavior, and add an issue to change it in 2.12.

adriaanm · 2015-04-13T23:22:15Z

Much obliged </.>

scala-jenkins added this to the 2.11.6 milestone Feb 10, 2015

som-snytt mentioned this pull request Feb 10, 2015

SI-3368 CDATA gets a Node #4218

Closed

adriaanm modified the milestones: 2.11.6, 2.11.7 Feb 19, 2015

lrytz reviewed Mar 23, 2015
View reviewed changes

src/compiler/scala/tools/nsc/settings/ScalaSettings.scala Outdated

Copy link

Member

lrytz Mar 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a help text here, like we do in -language

som-snytt closed this Mar 25, 2015

som-snytt added 3 commits April 8, 2015 09:20

SI-5699 Use ParserTest

b60de05

Update the test slightly to use the rig it inspired.

SI-3368 Promote xml option to -Xxml

996d066

As long as Scala does XML literals, there is probably parsing behavior worth configuring. Therefore, the umbrella option is promoted to `-Xxml`. It was tempting to make it `-XML`, but we resisted.

som-snytt reopened this Apr 8, 2015

som-snytt force-pushed the issue/3368-b branch from 4df81a8 to ff2c0bb Compare April 8, 2015 16:51

SI-3368 Review

ef7b552

Verbose option help and test tweak.

som-snytt force-pushed the issue/3368-b branch from ff2c0bb to ef7b552 Compare April 8, 2015 20:41

adriaanm added a commit that referenced this pull request Apr 10, 2015

Merge pull request #4306 from som-snytt/issue/3368-b

3cf7c95

SI-3368 CDATA gets a Node

adriaanm merged commit 3cf7c95 into scala:2.11.x Apr 10, 2015

som-snytt deleted the issue/3368-b branch April 12, 2015 21:40

adriaanm mentioned this pull request Apr 13, 2015

CDATA test broken scala/scala-xml#51

Closed

som-snytt mentioned this pull request Apr 16, 2015

SI-3368 Default to coalescing for 2.11 #4451

Merged

scabug mentioned this pull request Apr 7, 2017

Preserve CDATA sections, don't convert them to generic Text nodes. scala/bug#3368

Closed

som-snytt mentioned this pull request Feb 7, 2020

Add more XML to partest suite #8138

Merged

Conversation

som-snytt commented Feb 10, 2015

Uh oh!

adriaanm commented Feb 19, 2015

Uh oh!

lrytz Mar 23, 2015

Choose a reason for hiding this comment

Uh oh!

lrytz commented Mar 24, 2015

Uh oh!

som-snytt commented Mar 24, 2015

Uh oh!

som-snytt commented Apr 8, 2015

Uh oh!

adriaanm commented Apr 8, 2015

Uh oh!

adriaanm commented Apr 8, 2015

Uh oh!

som-snytt commented Apr 8, 2015

Uh oh!

som-snytt commented Apr 8, 2015

Uh oh!

som-snytt commented Apr 8, 2015

Uh oh!

adriaanm commented Apr 10, 2015

Uh oh!

adriaanm commented Apr 13, 2015

Uh oh!

som-snytt commented Apr 13, 2015

Uh oh!

adriaanm commented Apr 13, 2015

Uh oh!

som-snytt commented Apr 13, 2015

Uh oh!

adriaanm commented Apr 13, 2015

Uh oh!

som-snytt commented Apr 13, 2015

Uh oh!

adriaanm commented Apr 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants