chore: add support for 2.13.15

[ckipp01]

closes #640

[jozic]

🎉

[SethTisue]

==> X scoverage.PluginCoverageTest.scoverage should instrument for-loop guards  0.059s scala.reflect.internal.Positions$ValidateException: Enclosing tree [167] does not include tree [162]

@lrytz @som-snytt any idea what this might be about? I’m not able to dig into it right now

nothing obvious jumps out at me from the 2.13.15 merged PRs

[SethTisue]

I had hoped to use git bisect to locate the scala/scala PR responsible, but it's not trivially doable because when you use a nightly Scala version you get missing dependencies

note that the relevant test case is

  test("scoverage should instrument for-loop guards") {
    val compiler = ScoverageCompiler.default

    compiler.compileCodeSnippet(
      """object A {
        |  def foo(list: List[String]) = for (string: String <- list if string.length > 5)
        |    println(string)
        |} """.stripMargin
    )
    ...

[SethTisue]

note that scalac-scoverage-plugin is in the community build, but we only compile the tests and don't run them. perhaps I could improve that, but not this week. anyway, that's the reason we didn't get early notice of the problem

[lrytz]

Seems it's easy to reproduce with that source file:

object A {
  def foo(list: List[String]) = for (string: String <- list if string.length > 5)
    println(string)
}

compiling with 2.13.14 and -Yvalidate-pos:typer succeeds, and it fails with 2.13.15 with the same error message.

It seems scoverage is running the pos validation, search for validatePositions in https://github.com/scoverage/scalac-scoverage-plugin/blob/main/plugin/src/test/scala/scoverage/ScoverageCompiler.scala.

The regression was caused by scala/scala@060bc32, I tested before and after.

[som-snytt]

Thanks, @lrytz , @SethTisue didn't mention on chat that you had already beaten me to it. That was my guess, so I'll follow up unless you have a fresh PR.

[lrytz]

I didn't do anything more, see my GitHub status :-)

[SethTisue]

@ckipp01 do you have a sense of whether or how badly this affects the usability of scoverage for users? would there just be a minor degradation of coverage report quality for some for comprehensions, or would the coverage report not be produced at all…? or might there be no end user impact at all, and the test is just alerting us to some incorrect positions that we of course should fix but aren’t really going to be affecting users in the meantime?

2.13.15 is already published and ready to be announced, so… unless someone wants to make a case that this problem is bad enough that we should withdraw it and immediately do 2.13.16 instead… I suspect the thing to do is disable the test, go ahead and publish, and if some users are unable to use scoverage as a result, they can stay on 2.13.14 for now, and then we can sit back, see if any other regressions roll in, and depending on how many there are and how many folks are complaining, consider it a reason to do 2.13.16 on a faster cycle

we can add something to the 2.13.15 release notes about it, with a link to a ticket

[ckipp01]

or might there be no end user impact at all, and the test is just alerting us to some incorrect positions that we of course should fix but aren’t really going to be affecting users in the meantime?

@SethTisue so after a bit of testing, I think this one may be it. I published this locally and used the sample that fails and when actually running coverage it doesn't blow up and it does give you the coverage data you expect. I pasted it below to show the data for:

package example

object A {
  def foo(list: List[String]) = for (string: String <- list if string.length > 5)
    println(string)
}

Which produces:

# Coverage data, format version: 3.0
# Statement data:
# - id
# - source path
# - package name
# - class name
# - class type (Class, Object or Trait)
# - full class name
# - method name
# - start offset
# - end offset
# - line number
# - symbol name
# - tree name
# - is branch
# - invocations count
# - is ignored
# - description (can be multi-line)
# '' sign
# ------------------------------------------
1
src/main/scala/example/Hello.scala
example
A
Object
example.A
foo
91
108
4
scala.Int.>
Apply
false
0
false
string.length().>(5)

2
src/main/scala/example/Hello.scala
example
A
Object
example.A
foo
114
129
5
scala.Predef.println
Apply
false
0
false
scala.Predef.println(string)

3
src/main/scala/example/Hello.scala
example
A
Object
example.A
foo
60
129
4
scala.collection.WithFilter.foreach
Apply
false
0
false
list.withFilter(((check$ifrefutable$1: String) => (check$ifrefutable$1: String @unchecked) match {
  case (string @ (_: String)) => true
  case _ => false
})).withFilter(((string: String) => string.length().>(5))).foreach[Unit](((string: String) => scala.Predef.println(string)))


And with then just to be safe as well I produced the same with 2.13.14.

# Coverage data, format version: 3.0
# Statement data:
# - id
# - source path
# - package name
# - class name
# - class type (Class, Object or Trait)
# - full class name
# - method name
# - start offset
# - end offset
# - line number
# - symbol name
# - tree name
# - is branch
# - invocations count
# - is ignored
# - description (can be multi-line)
# '' sign
# ------------------------------------------
1
src/main/scala/example/Hello.scala
example
A
Object
example.A
foo
91
108
4
scala.Int.>
Apply
false
0
false
string.length().>(5)

2
src/main/scala/example/Hello.scala
example
A
Object
example.A
foo
114
129
5
scala.Predef.println
Apply
false
0
false
scala.Predef.println(string)

3
src/main/scala/example/Hello.scala
example
A
Object
example.A
foo
60
129
4
scala.collection.WithFilter.foreach
Apply
false
0
false
list.withFilter(((check$ifrefutable$1: String) => (check$ifrefutable$1: String @unchecked) match {
  case (string @ (_: String)) => true
  case _ => false
})).withFilter(((string: String) => string.length().>(5))).foreach[Unit](((string: String) => scala.Predef.println(string)))

They end up being the same here. So I think it's pretty safe to go ahead and ignore this test and just publish. There might be a chance where this doesn't get instrumented correctly in some cases, but the worst case scenario there would be that you'd see your coverage drop slightly. Thanks a lot for checking this all 🫶

[SethTisue]

Thank you Chris!