scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.79k stars 1.04k forks source link

`-coverage-out` generates coverage information for macro code coming from referenced libraries #15490

Closed arixmkii closed 1 year ago

arixmkii commented 2 years ago

Compiler version

3.2.0-RC1

If you're not sure what version you're using, run print scalaVersion from sbt (if you're running scalac manually, use scalac -version instead).

Minimized code

Macro.scala

package excludeme

import scala.quoted.*

object Macro {

  inline def toUpperS(s: String): String = ${ Macro.toUpperSImpl('s) }

  def toUpperSImpl(s: Expr[String])(using q: Quotes): Expr[String] = '{ ($s.toUpperCase) } // will add coverage info
}

Main.scala

import excludeme.*

@main def main: Unit =
  println(Macro.toUpperS("test"))

Minimized code (using third party library)

Keeping this for the sake of history how it was originally reported.

import io.circe.*
import io.circe.generic.semiauto.*
import io.circe.syntax.*

implicit val basicCodec: Codec[Tuple1[String]] = deriveCodec[Tuple1[String]]

@main def main: Unit =
  println(Tuple1("value").asJson.noSpaces)

Full project is here https://github.com/arixmkii/macro_cov_3.2.0-RC1 I will think about a better minimal example, because circe-generic is a heavy one. May be with 2 source files and using one with macro as pre-compiled class.

Output

scoverage.coverage file contains records of

modules/core/shared/src/main/scala-3/io/circe/Derivation.scala

Expectation

No coverage data for third party libraries as source might not be provided for them.

Originally posted here https://github.com/scoverage/scalac-scoverage-plugin/issues/474 I think that workarounds could be added to report generation to handle such cases (probably giving out a warning by default), but I'm not sure if this is the right approach for dealing with macros from referenced libraries.

arixmkii commented 2 years ago

First try on minimization:

Macro definitions: Macro.scala

package excludeme

sealed abstract class MyToken extends Product with Serializable {}

final case class MyTokenS(s: String) extends MyToken

object MyToken {
  def fromString(s: String): MyToken = MyTokenS(s)
}

import scala.quoted.*

object Macro {
  inline def makeToken(s: String): MyToken = ${ Macro.makeTokenImpl('s) }

  def makeTokenImpl(s: Expr[String])(using q: Quotes): Expr[MyToken] = {
    '{ MyToken.fromString($s) } // will add coverage info
//    '{ MyTokenS.apply($s) } // will not add coverage info
  }
}

Prorgam: Main.scala

import excludeme.*

@main def main: Unit =
  println(Macro.makeToken("test"))

Compile wth -coverage-out enabled and run from sbt.

Scoverage data will have this erroneous record.

0
src/main/scala/Macro.scala
<empty>
Main$package$
Object
<empty>.Main$package$
main
681
709
25
fromString
Apply
false
0
false
MyToken.fromString($strExpr)

Trying to generate coverage data for fromString on MyToken companion.

Expected result - no such record.

If I change last line of a macro to

    '{ MyTokenS.apply($s) }

Directly calling apply on a case class - there will be no coverage info added (as expected).

arixmkii commented 2 years ago

As minimal as I can get it. Macro.scala

package excludeme

import scala.quoted.*

object Macro {

  inline def toUpperS(s: String): String = ${ Macro.toUpperSImpl('s) }

  def toUpperSImpl(s: Expr[String])(using q: Quotes): Expr[String] = '{ ($s.toUpperCase) } // will add coverage info
}

Main.scala

import excludeme.*

@main def main: Unit =
  println(Macro.toUpperS("test"))

Will update the description.

Kordyjan commented 2 years ago

Thanks for the minimalization!

nicolasstucki commented 2 years ago

What is work in the minimized version? What is the expected output? Which command should I use to reproduce the issue?

arixmkii commented 2 years ago
# compile Macro
./scala3-3.2.0-RC1/bin/scalac Macro.scala
# remove source (for clean experiment) and keep only compiled
rm Macro.scala
# compile program with coverage
./scala3-3.2.0-RC1/bin/scalac -coverage-out:. Main.scala
# check coverage data
cat scoverage.coverage | grep -A14 Macro.scala

Outputs

Macro.scala
<empty>
Main$package$
Object
<empty>.Main$package$
main
203
217
8
toUpperCase
Apply
false
0
false

But we removed this source and used only compiled module. So, there should be no coverage info for this. Same is true for macros coming from third party libraries (which then breaks coverage report, because sources can't be found).

I expect that there will be no coverage info generated from code instantiated via macro.

nicolasstucki commented 2 years ago

If I understand correctly you are saying that if we have code for a source that we do not have we should not emit the info in the coverage file.

Wouldn't it be more resilient if the reporting part is resilient to missing files? We also have the situation where the file is deleted after the code is compiled.

arixmkii commented 2 years ago

For me it looks like we are emitting data for code, which comes from macro and referencing the source file, where the Macro was defined.

'{ ($s.toUpperCase) }

I believe that this code should not be instrumented, (especially) when it comes from pre-compiled module (third party library) and we only asked to compile our owned source with instrumentation. The problem is that the source reference for this kind of data will almost always be invalid (theoretically you can find the sources from which it was built, but this doesn't look always reliable).

I'm fine if this is considered not a bug. I discovered it trying to generate scoverage report for the code equivalent, which had this report successfully generated in Scala 2. I know that macros are very different between the two and if scoverage plugin has to adapt to the Scala 3 behavior I will update my issue report in that project.

arixmkii commented 2 years ago

Optionally it could generate coverage info with source erased. Giving it another thought I can see how this could end up in source collision between third party library and own sources and will give (at best) useless output.

/shared/src/main/scala-3/Utils.scala in a library sources defining a macro and any file /shared/src/main/scala-3/Utils.scala in local sources for some random utilities. If library macro will be used anywhere in code it will create data linked to unrelated sources.

lefou commented 1 year ago

I think, generating coverage data for code that comes from macros is a nice thing. In scoverage 1.x, it was not possible at all to get coverage for macro code. (E.g. is was not easily possible, to get coverage data for Mill targets which are written with the help of a surrounding T{ } macro). But the reporter should be a) able to gracefully handle missing source files and b) only count those macros into metrics which are part of the actual code base. As a consequence, we need to distinguish between own and foreign macro code, otherwise we might generate incorrect coverage metrics.

smarter commented 1 year ago

@TheElectronWill is this issue already fixed by https://github.com/lampepfl/dotty/pull/16235/commits/743f3cc1552f92d2b24bdf37e2b2f19298e71ba8 ?

larochef commented 1 year ago

@smarter I have the same issue, if I use scala 3.2.2-RC1 the coverage generation is successful

TheElectronWill commented 1 year ago

I was about to say that we should check that it's fixed by my PR, by trying to reproduce the bug with the latest version of the compiler :smile: It seems that it worked as intended. Thank you @larochef :)