fix(#16458): regression in xml syntax parsing #19522

i10416 · 2024-01-23T22:03:56Z

close #16458
xLiteral mistakenly assumed that the element just after < is always non-special element, but it is not true. It could be xCharData, comment, xProcInstr.

xLiteral mistakenly assumed that the element just after `<` is always non-special element, but it is not true. It could be xCharData, comment, xProcInstr.

i10416 · 2024-01-23T22:05:01Z

tests/pos/i16458.scala

@@ -0,0 +1,37 @@
+def x = <div>FooBar</div><!-- /.modal-content -->
+
+package scala.xml {


I copy-pasted and modifed code from tests/run/xml.scala. Without this, the test fails due to missing xml module.

som-snytt · 2024-01-23T23:06:59Z

compiler/src/dotty/tools/dotc/parsing/xml/MarkupParsers.scala

@@ -371,10 +371,17 @@ object MarkupParsers {
        // parse more XML?
        if (charComingAfter(xSpaceOpt()) == '<') {
          while {
-            xSpaceOpt()
-            nextch()
-            ts.append(element)


I don't understand why my previous fix 624a6e0 used ts.append(element) instead of content_LT(ts) which is what scala 2 has. (I would expect the codebases not to diverge much.)

I don't know why too, but I suspect it was just a mistake as element cannot consume XML special nodes.

By the way, current Scala 2 also uses content_LT instead of element
https://github.com/scala/scala/blob/8d598d102f0c2f23616ccd732e3ae93765da497a/src/compiler/scala/tools/nsc/ast/parser/MarkupParsers.scala#L392

Yes, that is what I was comparing. oh, my previous comment says just that.

It was a quick fix, but the scala 2 pr was also my quick fix, so I have no idea what happened that day. Thanks for following it up.

som-snytt · 2024-01-23T23:26:20Z

I copied your test and just changed ts.append(element) to content_LT(ts) and it passed.

I didn't put in any brain power, but I see content_LT handles the empty element.

--- a/compiler/src/dotty/tools/dotc/parsing/xml/MarkupParsers.scala
+++ b/compiler/src/dotty/tools/dotc/parsing/xml/MarkupParsers.scala
@@ -373,7 +373,8 @@ object MarkupParsers {
           while {
             xSpaceOpt()
             nextch()
-            ts.append(element)
+            //ts.append(element)
+            content_LT(ts)
             charComingAfter(xSpaceOpt()) == '<'

Just replacing element with content_LT, it works. See scala#19522 (comment)

i10416 · 2024-01-24T04:00:56Z

I copied your test and just changed ts.append(element) to content_LT(ts) and it passed.

Thank you. I checked it and it works!

I added some precondition check in the previous commit because I suspected the xSpaceOpt();nextch();content_LT(ts); would mistakenly parse < !-- comment -->, but it does not. It properly fails with in XML literal: name expected, but char ' ' cannot start a name.

Just replacing element with content_LT, it works. See scala#19522 (comment)

i10416 · 2024-01-24T05:24:06Z

tests/run/i16458.scala

+object Test {
+  import scala.xml.*
+  def main(args: Array[String]): Unit = {
+    val xml =  <div>FooBar</div><!-- /.modal-content -->


Replaced tests/pos/i16458.scala with tests/run/i16458.scala to make sure we get expected parse result.

To check parsing properly, it is better to run a test and assert parse result.

odersky

I know very little about XML parsing but judging from the discussion everybody agrees this is OK.

Just replacing element with content_LT, it works. See #19522 (comment) [Cherry-picked 27046fe]

Backports #19522 to the LTS branch. PR submitted by the release tooling. [skip ci]

fix(scala#16458): regression in xml syntax parsing

77aa363

xLiteral mistakenly assumed that the element just after `<` is always non-special element, but it is not true. It could be xCharData, comment, xProcInstr.

i10416 commented Jan 23, 2024

View reviewed changes

som-snytt reviewed Jan 23, 2024

View reviewed changes

i10416 added a commit to i10416/dotty that referenced this pull request Jan 24, 2024

improve: just use content_LT to fix 16458

2a88851

Just replacing element with content_LT, it works. See scala#19522 (comment)

improve: just use content_LT to fix 16458

27046fe

Just replacing element with content_LT, it works. See scala#19522 (comment)

i10416 force-pushed the fix/16458-xml-syntax-parse-regressions branch from 2a88851 to 27046fe Compare January 24, 2024 04:13

i10416 commented Jan 24, 2024

View reviewed changes

i10416 force-pushed the fix/16458-xml-syntax-parse-regressions branch from 788fc43 to 8bdbe77 Compare January 24, 2024 05:24

tidy: use run test instead of compile test

9de9d57

To check parsing properly, it is better to run a test and assert parse result.

i10416 force-pushed the fix/16458-xml-syntax-parse-regressions branch from 8bdbe77 to 9de9d57 Compare January 24, 2024 05:25

i10416 requested a review from som-snytt January 24, 2024 07:50

nicolasstucki assigned odersky Feb 1, 2024

odersky approved these changes Feb 15, 2024

View reviewed changes

odersky merged commit 50d62f7 into scala:main Feb 15, 2024
19 checks passed

Kordyjan added this to the 3.4.2 milestone Mar 28, 2024

WojciechMazur pushed a commit that referenced this pull request Jul 1, 2024

improve: just use content_LT to fix 16458

4e69d79

Just replacing element with content_LT, it works. See #19522 (comment) [Cherry-picked 27046fe]

WojciechMazur mentioned this pull request Jul 1, 2024

Backport "fix(#16458): regression in xml syntax parsing" to LTS #20920

Merged

WojciechMazur pushed a commit that referenced this pull request Jul 2, 2024

improve: just use content_LT to fix 16458

42941b0

Just replacing element with content_LT, it works. See #19522 (comment) [Cherry-picked 27046fe]

WojciechMazur added a commit that referenced this pull request Jul 2, 2024

Backport "fix(#16458): regression in xml syntax parsing" to LTS (#20920)

94dbe78

Backports #19522 to the LTS branch. PR submitted by the release tooling. [skip ci]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#16458): regression in xml syntax parsing #19522

fix(#16458): regression in xml syntax parsing #19522

i10416 commented Jan 23, 2024 •

edited

Loading

i10416 Jan 23, 2024

som-snytt Jan 23, 2024

i10416 Jan 24, 2024

i10416 Jan 25, 2024

som-snytt Jan 25, 2024 •

edited

Loading

som-snytt commented Jan 23, 2024 •

edited

Loading

i10416 commented Jan 24, 2024 •

edited

Loading

i10416 Jan 24, 2024 •

edited

Loading

odersky left a comment

		@@ -0,0 +1,37 @@
		def x = <div>FooBar</div><!-- /.modal-content -->

		package scala.xml {

fix(#16458): regression in xml syntax parsing #19522

fix(#16458): regression in xml syntax parsing #19522

Conversation

i10416 commented Jan 23, 2024 • edited Loading

i10416 Jan 23, 2024

Choose a reason for hiding this comment

som-snytt Jan 23, 2024

Choose a reason for hiding this comment

i10416 Jan 24, 2024

Choose a reason for hiding this comment

i10416 Jan 25, 2024

Choose a reason for hiding this comment

som-snytt Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

som-snytt commented Jan 23, 2024 • edited Loading

i10416 commented Jan 24, 2024 • edited Loading

i10416 Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

odersky left a comment

Choose a reason for hiding this comment

i10416 commented Jan 23, 2024 •

edited

Loading

som-snytt Jan 25, 2024 •

edited

Loading

som-snytt commented Jan 23, 2024 •

edited

Loading

i10416 commented Jan 24, 2024 •

edited

Loading

i10416 Jan 24, 2024 •

edited

Loading