scoverage / scalac-scoverage-plugin

Scoverage Scala Code Coverage Core Libs
https://github.com/scoverage
Apache License 2.0
428 stars 126 forks source link

Non-printable characters in symbols should be properly escaped #468

Open armanbilge opened 2 years ago

armanbilge commented 2 years ago

Code like this:

    if (buffer.startsWith(Shared.`\r\n`.toArray)) {

https://github.com/http4s/http4s/blob/f9533ec5b499c277f915c7f275be5fdb3af20fac/ember-core/shared/src/main/scala/org/http4s/ember/core/ChunkedEncoding.scala#L123

Generates a scoverage.coverage like this:

30
/workspace/http4s/ember-core/shared/src/main/scala/org/http4s/ember/core/ChunkedEncoding.scala
org.http4s.ember.core
ChunkedEncoding
Object
org.http4s.ember.core.ChunkedEncoding
go
2366
2372
72
org.http4s.ember.core.Shared.

Select
false
0
false
Shared.



Which makes great sadness like this:

[error] java.lang.IllegalArgumentException: For input string: "Select"
[error]         at scala.collection.immutable.StringLike.parseBoolean(StringLike.scala:327)
[error]         at scala.collection.immutable.StringLike.toBoolean(StringLike.scala:289)
[error]         at scala.collection.immutable.StringLike.toBoolean$(StringLike.scala:289)
[error]         at scala.collection.immutable.StringOps.toBoolean(StringOps.scala:33)
[error]         at scoverage.Serializer$.toStatement$1(Serializer.scala:115)
[error]         at scoverage.Serializer$.deserialize(Serializer.scala:144)
[error]         at scoverage.Serializer$.deserialize(Serializer.scala:89)
[error]         at scoverage.ScoverageSbtPlugin$.loadCoverage(ScoverageSbtPlugin.scala:318)
[error]         at scoverage.ScoverageSbtPlugin$.$anonfun$coverageReport0$1(ScoverageSbtPlugin.scala:148)
[error]         at scoverage.ScoverageSbtPlugin$.$anonfun$coverageReport0$1$adapted(ScoverageSbtPlugin.scala:139)
armanbilge commented 2 years ago

I can make a PR to fix this. Is this data format specced anywhere?

armanbilge commented 2 years ago

Also, I can't really grasp whether this is an issue with newlines in particular, or backslashes in general πŸ€”

ckipp01 commented 2 years ago

I can make a PR to fix this. Is this data format specced anywhere?

It's versioned and sort of defined in here, since it needs to both deserialize 2 stuff that this plugin created and also 3 that was generated by the Scala 3 compiler.

https://github.com/scoverage/scalac-scoverage-plugin/blob/7eba8e1fda8ba63b3299c6d0328d65ebd7181f35/serializer/src/main/scala/scoverage/serialize/Serializer.scala#L62-L86

armanbilge commented 2 years ago

since it needs to both deserialize 2 stuff that this plugin created and also 3 that was generated by the Scala 3 compiler.

Hum, I'm confused about this. It seems to only be capable of deserializing v3, if I understanding correctly. https://github.com/scoverage/scalac-scoverage-plugin/blob/7eba8e1fda8ba63b3299c6d0328d65ebd7181f35/serializer/src/main/scala/scoverage/serialize/Serializer.scala#L169-L173

ckipp01 commented 2 years ago

Hum, I'm confused about this. It seems to only be capable of deserializing v3, if I understanding correctly.

Sorry, I wasn't clear. Yes, only V3. By 2, I mean Scala 2 coverage and Scala 3 coverage.

TheElectronWill commented 2 years ago

Also, I can't really grasp whether this is an issue with newlines in particular, or backslashes in general πŸ€”

The parser excepts the name to be on one line, because it uses new lines as a value separator, so I'd say that newlines are the biggest problem. Any character should be fine, as long as it doesn't break this expectation. Are there other characters that mess up lines? I know that unicode can be weird sometimes πŸ˜„

armanbilge commented 2 years ago

Thanks, I think you are right! I was wondering whether if you define a method:

def `\n` = ???

whether the symbol should be a "newline" instead of \\n backslash-n. But I guess the compiler determines that.

Are there other characters that mess up lines? I know that unicode can be weird sometimes

Not sure tbh. I think JSON has a similar restriction and these are the characters they escape.

https://github.com/circe/circe/blob/92ab397a45f08c685bcb41527321b18db4e48c68/modules/core/shared/src/main/scala/io/circe/Printer.scala#L322-L331

TheElectronWill commented 2 years ago

Thanks, I think you are right! I was wondering whether if you define a method:

def `\n` = ???

whether the symbol should be a "newline" instead of \\n backslash-n. But I guess the compiler determines that.

A quick test with scalac (dotty version) shows that the compiler keeps \n as a newline (it even shows up as a real newline in the trees with -Vprint:all :smile:) and escape it (fortunately!) when producing bytecode. The def gets compiled to:

public Nothing$ $u000A() {
    return Predef$.MODULE$.$qmark$qmark$qmark();
}
TheElectronWill commented 2 years ago

edit: for scoverage, we should escape it to \\n (i.e. write two chars: \ and n), it's simpler. Of course it means that all \ must now be escaped. All like JSON

armanbilge commented 2 years ago

Yes, I think we should follow JSON here πŸ‘