-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Comments
First try on minimization: Macro definitions: 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: import excludeme.*
@main def main: Unit =
println(Macro.makeToken("test")) Compile wth Scoverage data will have this erroneous record.
Trying to generate coverage data for 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). |
As minimal as I can get it. 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. |
Thanks for the minimalization! |
What is work in the minimized version? What is the expected output? Which command should I use to reproduce the issue? |
# 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
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. |
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.
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. |
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. |
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.
|
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.
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.
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 |
@TheElectronWill is this issue already fixed by 743f3cc ? |
@smarter I have the same issue, if I use scala 3.2.2-RC1 the coverage generation is successful |
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 😄 |
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
Main.scala
Minimized code (using third party library)
Keeping this for the sake of history how it was originally reported.
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 ofExpectation
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.
The text was updated successfully, but these errors were encountered: