Repository: scalameta/scalameta
Pull request #4053 (2024-12-03 21:25:32)

improvement: Don't use format since it seems to break at some chars
Related to https://github.com/scalameta/metals/issues/7003

Looks like there is at least 6 stack traces formatting error for InvalidToken and this looks like a problem.

We get things like:
```
	at java.util.regex.Pattern$BmpCharProperty.match(java.base@11.0.24/Pattern.java:3966)
	at java.util.regex.Pattern$GroupHead.match(java.base@11.0.24/Pattern.java:4807)
	at java.util.regex.Pattern$Branch.match(java.base@11.0.24/Pattern.java:4752)
	at java.util.regex.Pattern$Branch.match(java.base@11.0.24/Pattern.java:4750)
	at java.util.regex.Pattern$BranchConn.match(java.base@11.0.24/Pattern.java:4716)
	at java.util.regex.Pattern$GroupTail.match(java.base@11.0.24/Pattern.java:4866)
	at java.util.regex.Pattern$BmpCharPropertyGreedy.match(java.base@11.0.24/Pattern.java:4347)
	at java.util.regex.Pattern$GroupHead.match(java.base@11.0.24/Pattern.java:4807)
	at java.util.regex.Pattern$Branch.match(java.base@11.0.24/Pattern.java:4752)
	at java.util.regex.Pattern$BranchConn.match(java.base@11.0.24/Pattern.java:4716)
	at java.util.regex.Pattern$GroupTail.match(java.base@11.0.24/Pattern.java:4866)
	at java.util.regex.Pattern$BmpCharPropertyGreedy.match(java.base@11.0.24/Pattern.java:4347)
	at java.util.regex.Pattern$GroupHead.match(java.base@11.0.24/Pattern.java:4807)
	at java.util.regex.Pattern$Branch.match(java.base@11.0.24/Pattern.java:4752)
	at java.util.regex.Pattern$Branch.match(java.base@11.0.24/Pattern.java:4750)
	at java.util.regex.Pattern$BmpCharProperty.match(java.base@11.0.24/Pattern.java:3967)
	at java.util.regex.Pattern$Start.match(java.base@11.0.24/Pattern.java:3622)
	at java.util.regex.Matcher.search(java.base@11.0.24/Matcher.java:1729)
	at java.util.regex.Matcher.find(java.base@11.0.24/Matcher.java:773)
	at java.util.Formatter.parse(java.base@11.0.24/Formatter.java:2702)
	at java.util.Formatter.format(java.base@11.0.24/Formatter.java:2655)
```

and we should be as efficient as possible when just printing an error. Especially if we have a lot of errors then this might become inefficient.

tgodzik: > could it be that metals is a bit too aggressive constantly parsing too many files at once? only one is presumably active at any given time.

This is a possibility, but what surprised me is that every stack trace I saw was at `reportIllegalCharacter` which might mean this is what takes longest. If there is a lot of such characters then it will tokenize a longer time.

> also, if metals is capable of pointing out the location of the illegal character in a given file, can the user share what exactly was being parsed there which triggered the error? should we change something in the scanner to help identify that location?

So the problem is that we don't get any errors and I am not sure why. We don't also have access to the codebase, so figure I will try first here and see if that helps.

> is there perhaps a bug in the scanner that leads to valid code being interpreted as invalid in that user's case?

Might be, but we don't know which file that is since we use the scanner not only for open files bu dependencies as well.
tgodzik: > Looks like we might need to add a test or two, including those which take more than 16 bits.
> 
> ChatGPT says the first 16 bits are occupied by Basic Multilingual Plane, and shows examples from others:
> 
> Supplementary Multilingual Plane (SMP, U+010000–U+01FFFF)
> 
> * U+1F600: 😀 (Grinning Face Emoji)
> * U+1F4A9: 💩 (Pile of Poo Emoji)
> * U+1D11E: 𝄞 (Musical Symbol G Clef)
> * U+1F680: 🚀 (Rocket Emoji, SMP)
> * U+1F9D1: 🧑 (Person Emoji, SMP)

Curiously, all those are legal as identifiers. I am not sure now why `。` is not :sweat_smile: 
tgodzik: > Curiously, all those are legal as identifiers. I am not sure now why `。` is not 😅

And it's not possible to write \u with more than 4 characters.

What if we drop all that and just toString that character?


kitbellew: > > Curiously, all those are legal as identifiers. I am not sure now why `。` is not 😅
> 
> And it's not possible to write \u with more than 4 characters.
> 
> What if we drop all that and just toString that character?

`toString` is likely not so good since it show that little circle (for all invalid-surrogate codepoints).

I think the simplest solution is:
```
  curr.setInvalidToken("illegal unicode codepoint: 0x" + ch.toHexString)
```

if you don't like `\u` with more than 4 characters :)
tgodzik: I think the compiler itself only deals with 4 

https://github.com/scala/scala/blob/06d9be6e31c62c3ddeb03e8b802daef250d0dbc1/src/compiler/scala/tools/nsc/ast/parser/Scanners.scala#L846
tgodzik: > > I think the compiler itself only deals with 4
> > https://github.com/scala/scala/blob/06d9be6e31c62c3ddeb03e8b802daef250d0dbc1/src/compiler/scala/tools/nsc/ast/parser/Scanners.scala#L846
> 
> are you referring to `%04x` again? if so, what i remember, it means "print hex, but at least 4, pad with zeros if there are fewer than 4".

ach, that's quite magical. I changed to ` curr.setInvalidToken("illegal unicode codepoint: 0x" + ch.toHexString)` solution. What do you think?
kitbellew: > > > I think the compiler itself only deals with 4
> > > https://github.com/scala/scala/blob/06d9be6e31c62c3ddeb03e8b802daef250d0dbc1/src/compiler/scala/tools/nsc/ast/parser/Scanners.scala#L846
> > 
> > 
> > are you referring to `%04x` again? if so, what i remember, it means "print hex, but at least 4, pad with zeros if there are fewer than 4".
> 
> ach, that's quite magical. I changed to ` curr.setInvalidToken("illegal unicode codepoint: 0x" + ch.toHexString)` solution. What do you think?

i think it's good... but you didn't upload a new patch.
tgodzik: > i think it's good... but you didn't upload a new patch.

My bad, I was sure I did it!
kitbellew: looks good

Patch:
diff --git a/scalameta/tokenizers/shared/src/main/scala/scala/meta/internal/tokenizers/LegacyScanner.scala b/scalameta/tokenizers/shared/src/main/scala/scala/meta/internal/tokenizers/LegacyScanner.scala
index 352a71ca1..cdee7976f 100644
--- a/scalameta/tokenizers/shared/src/main/scala/scala/meta/internal/tokenizers/LegacyScanner.scala
+++ b/scalameta/tokenizers/shared/src/main/scala/scala/meta/internal/tokenizers/LegacyScanner.scala
@@ -234,7 +234,7 @@ class LegacyScanner(input: Input, dialect: Dialect) {
 
     @inline
     def reportIllegalCharacter(): Unit = curr
-      .setInvalidToken(s"illegal character '\\u${"%04x".format(ch)}'")
+      .setInvalidToken("illegal unicode codepoint: 0x" + ch.toHexString)
 
     (ch: @switch) match {
       case ' ' =>


Test patch:
diff --git a/tests/shared/src/test/scala/scala/meta/tests/tokenizers/TokenizerSuite.scala b/tests/shared/src/test/scala/scala/meta/tests/tokenizers/TokenizerSuite.scala
index 1c3c7bbc9..a1fcaf31c 100644
--- a/tests/shared/src/test/scala/scala/meta/tests/tokenizers/TokenizerSuite.scala
+++ b/tests/shared/src/test/scala/scala/meta/tests/tokenizers/TokenizerSuite.scala
@@ -2136,6 +2136,7 @@ class TokenizerSuite extends BaseTokenizerSuite {
          |""".stripMargin.replace("'''", "\"\"\"").nl2lf
     assertTokenizedAsStructureLines(code, struct)
   }
+
   test("unexpected-character") {
     val code = """|
                   |val 。 = 123
@@ -2145,7 +2146,7 @@ class TokenizerSuite extends BaseTokenizerSuite {
     val struct = s"""|BOF [0..0)
                      |LF [0..1)
                      |KwVal [1..4)
-                     |Invalid(illegal character '\\u3002') [5..6)
+                     |Invalid(illegal unicode codepoint: 0x3002) [5..6)
                      |MultiHS(3) [4..7)
                      |Equals [7..8)
                      |Space [8..9)