Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix(#16459) xml parse regression #19531

Merged
merged 6 commits into from
Feb 1, 2024

Conversation

i10416
Copy link
Contributor

@i10416 i10416 commented Jan 25, 2024

close #16459

The parser could not parse if expr that contains single-quoted text(s) inside XML literal with newline(s) because followedByToken, which is used to detect do or then token after if, unintentionally consumed XMLSTART symbol, which prevented the parser from delegating parse to XML parser.

The parser could not parse `if expr` that contains single-quoted text(s) inside
XML literal with newline(s) because `followedByToken`, which is used to detect `do` or `then` token after `if`,
unintentionally consumed XMLSTART symbol, which prevented the parser from delegating parse to XML parser.
@i10416
Copy link
Contributor Author

i10416 commented Jan 25, 2024

🤔
scala-xml/shared/src/test/scala/scala/xml/XMLTest.scala:90:24

 [E032] Syntax Error: /__w/dotty/dotty/community-build/community-projects/scala-xml/shared/src/test/scala/scala/xml/XMLTest.scala:90:24 
[error] 90 |      val actual = for (case t @ <book><title>Blabla</title></book> <- NodeSeq.fromSeq(books.child).toList)
[error]    |                        ^^^^
[error]    |                        pattern expected
[error]    |
[error]    | longer explanation available when compiling with `-explain`
[error] -- [E040] Syntax Error: /__w/dotty/dotty/community-build/community-projects/scala-xml/shared/src/test/scala/scala/xml/XMLTest.scala:91:8 
[error] 91 |        yield t
[error]    |        ^^^^^
[error]    |        '<-' expected, but 'yield' found
[error] -- [E040] Syntax Error: /__w/dotty/dotty/community-build/community-projects/scala-xml/shared/src/test/scala/scala/xml/XMLTest.scala:94:4 
[error] 94 |    }
[error]    |    ^
[error]    |    '<-' expected, but '}' found
[error] -- [E007] Type Mismatch Error: /__w/dotty/dotty/community-build/community-projects/scala-xml/shared/src/test/scala/scala/xml/XMLTest.scala:92:0 
[error] 92 |        yield t
[error]    |               ^
[error]    |Found:    (??? : => Nothing)
[error]    |Required: ?{ foreach: ? }
[error]    |Note that implicit conversions were not tried because the result of an implicit conversion
[error]    |must be more specific than ?{ foreach: [applied to (
[error]    |  x$1 =>
[error]    |    x$1 match 
[error]    |      {
[error]    |        case (_root_.scala.Predef.???) =>
[error]    |          _root_.scala.Predef.???.foreach(
[error]    |            x$1 =>
[error]    |              x$1 match 
[error]    |                {
[error]    |                  case _root_.scala.Predef.??? =>
[error]    |                    _root_.scala.Predef.???.foreach(
[error]    |                      x$1 =>
[error]    |                        x$1 match 
[error]    |                          {
[error]    |                            case assertEquals(expected, actual) =>
[error]    |                              _root_.scala.Predef.???
[error]    |                          }
[error]    |                    )
[error]    |                }
[error]    |          )
[error]    |      }
[error]    |) returning <?>] }
[error]    |
[error]    | longer explanation available when compiling with `-explain`

xml literal may appear in LHS of for comprehension.

The previous fix caused a syntax error for the following code.

```scala
val actual: List[Node] = for (case t @ <book><title>Blabla</title></book> <- NodeSeq.fromSeq(books.child).toList)
  yield t
```
I need to come up with good solution for pattern match with `if` guard.
@i10416 i10416 force-pushed the fix/16459-xml-parse-regression branch from 3e83bde to 0423862 Compare January 25, 2024 17:57
Confusing LARROW just after XML pattern breaks the parser.
@i10416 i10416 force-pushed the fix/16459-xml-parse-regression branch from 5d1f0b4 to 4b0fff5 Compare January 25, 2024 19:34
Comment on lines +52 to +53
val auxiliary6 = for (case _ @ <div>empty</div><- Seq(xml)) yield ()
val auxiliary7 = for (case _ @ <div>empty</div><-Seq(xml)) yield ()
Copy link
Contributor Author

@i10416 i10416 Jan 25, 2024

Choose a reason for hiding this comment

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

The left arrow just after xml closing tag broke the parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

tests/run/i16459.scala Outdated Show resolved Hide resolved
Add a patch to cover the cornercase where xml pattern in parens confuse
the parser.

Before this commit, the following code compiles,

```scala
for (case _ @ <div>empty</div> <- Seq(xml)) yield ()
```
but the following resulted in syntax error.

```scala
for (case _ @ <div>empty</div><-Seq(xml)) yield ()
```

Because `followingIsEnclosedGenerators` always comes after `for` and
`(`, I beleive it would not break the parser to early-exit when
`XMLSTART` is found.
@i10416 i10416 force-pushed the fix/16459-xml-parse-regression branch from 53bbb78 to 3d156b6 Compare January 26, 2024 04:46
@i10416 i10416 changed the title Fix/16459 xml parse regression Fix(#16459) xml parse regression Jan 27, 2024
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

@odersky odersky merged commit 5e1f546 into scala:main Feb 1, 2024
19 checks passed
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
WojciechMazur added a commit that referenced this pull request Jul 1, 2024
Backports #19531 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in parsing XML symbols used in old if-else syntax
3 participants