Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

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

Closed
arixmkii opened this issue Jun 21, 2022 · 12 comments
Labels
area:coverage Code coverage, see https://dotty.epfl.ch/docs/internals/coverage.html itype:bug

Comments

@arixmkii
Copy link
Contributor

arixmkii commented Jun 21, 2022

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 scoverage/scalac-scoverage-plugin#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 arixmkii added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jun 21, 2022
@KacperFKorban KacperFKorban added stat:needs minimization Needs a self contained minimization area:coverage Code coverage, see https://dotty.epfl.ch/docs/internals/coverage.html and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jun 21, 2022
@arixmkii
Copy link
Contributor Author

arixmkii commented Jun 21, 2022

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
Copy link
Contributor Author

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 Kordyjan removed the stat:needs minimization Needs a self contained minimization label Jun 22, 2022
@Kordyjan
Copy link
Contributor

Thanks for the minimalization!

@nicolasstucki
Copy link
Contributor

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

@arixmkii
Copy link
Contributor Author

# 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 added a commit to dotty-staging/dotty that referenced this issue Jun 22, 2022
These methods will not generate code and therefore do not need to be
instrumented. Note that a retained inline method will have a `$retained`
variant that will be instrumented.

Found this issue while looking into scala#15490.
@nicolasstucki
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

TheElectronWill pushed a commit to TheElectronWill/dotty that referenced this issue Jul 2, 2022
These methods will not generate code and therefore do not need to be
instrumented. Note that a retained inline method will have a `$retained`
variant that will be instrumented.

Found this issue while looking into scala#15490.
bishabosha pushed a commit to dotty-staging/dotty that referenced this issue Oct 18, 2022
These methods will not generate code and therefore do not need to be
instrumented. Note that a retained inline method will have a `$retained`
variant that will be instrumented.

Found this issue while looking into scala#15490.
@lefou
Copy link

lefou commented Oct 20, 2022

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.

@rochala rochala removed their assignment Nov 22, 2022
@smarter
Copy link
Member

smarter commented Nov 22, 2022

@TheElectronWill is this issue already fixed by 743f3cc ?

@larochef
Copy link

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

@TheElectronWill
Copy link
Contributor

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 😄
It seems that it worked as intended. Thank you @larochef :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:coverage Code coverage, see https://dotty.epfl.ch/docs/internals/coverage.html itype:bug
Projects
None yet
Development

No branches or pull requests

9 participants