From f06b95f1ddd195a3ef8c2e8947b40d8954aeacfb Mon Sep 17 00:00:00 2001 From: Jan Chyb <48855024+jchyb@users.noreply.github.com> Date: Fri, 10 Jan 2025 13:39:53 +0100 Subject: [PATCH] Fix coverage serialization when encountering macro suspension (#22303) Fixes #22247 The fix is simple, as we mainly move the coverage object to a global ContextBase object, which persists it between runs. Initially I thought that appending the newly generated coverage indices would be enough, but if the macro suspends after the InstrumentCoverage phase runs, we end up with duplicate indices. For that reason, when generating indexes for a compilation unit, we also remove the previously generated ones for the same compilation unit. To support having multiple scala files compiled in the coverage tests I had to slightly adjust the suite. While doing that, I noticed that some check files for run tests were ignored, as they were incorrectly named. I added an assertion that throws when `.check` do not exist and renamed the files appropriately (having to add some additional ones as well). --- .../src/dotty/tools/dotc/core/Contexts.scala | 6 + .../dotty/tools/dotc/coverage/Coverage.scala | 12 ++ .../dotc/transform/InstrumentCoverage.scala | 18 +- .../tools/dotc/coverage/CoverageTests.scala | 41 +++-- .../dotty/tools/vulpix/ParallelTesting.scala | 24 ++- .../pos/macro-late-suspend/Test.scala | 13 ++ .../pos/macro-late-suspend/UsesTest.scala | 3 + .../macro-late-suspend/VisitorMacros.scala | 12 ++ .../macro-late-suspend/test.scoverage.check | 88 +++++++++ tests/coverage/run/i16940/i16940.check | 1 + ...0.scoverage.check => test.scoverage.check} | 0 .../coverage/run/i18233-min/i182233-min.check | 2 + ...n.scoverage.check => test.scoverage.check} | 0 tests/coverage/run/i18233/i18233.check | 1 + ...3.scoverage.check => test.scoverage.check} | 0 .../{test.check => JavaObject.check} | 0 tests/coverage/run/macro-suspend/Macro.check | 1 + tests/coverage/run/macro-suspend/Macro.scala | 8 + tests/coverage/run/macro-suspend/Test.scala | 4 + .../run/macro-suspend/test.scoverage.check | 173 ++++++++++++++++++ .../coverage/run/varargs/JavaVarargs_1.check | 3 + .../run/varargs/{test_1.check => test.check} | 0 ...1.scoverage.check => test.scoverage.check} | 0 23 files changed, 378 insertions(+), 32 deletions(-) create mode 100644 tests/coverage/pos/macro-late-suspend/Test.scala create mode 100644 tests/coverage/pos/macro-late-suspend/UsesTest.scala create mode 100644 tests/coverage/pos/macro-late-suspend/VisitorMacros.scala create mode 100644 tests/coverage/pos/macro-late-suspend/test.scoverage.check create mode 100644 tests/coverage/run/i16940/i16940.check rename tests/coverage/run/i16940/{i16940.scoverage.check => test.scoverage.check} (100%) create mode 100644 tests/coverage/run/i18233-min/i182233-min.check rename tests/coverage/run/i18233-min/{i18233-min.scoverage.check => test.scoverage.check} (100%) create mode 100644 tests/coverage/run/i18233/i18233.check rename tests/coverage/run/i18233/{i18233.scoverage.check => test.scoverage.check} (100%) rename tests/coverage/run/java-methods/{test.check => JavaObject.check} (100%) create mode 100644 tests/coverage/run/macro-suspend/Macro.check create mode 100644 tests/coverage/run/macro-suspend/Macro.scala create mode 100644 tests/coverage/run/macro-suspend/Test.scala create mode 100644 tests/coverage/run/macro-suspend/test.scoverage.check create mode 100644 tests/coverage/run/varargs/JavaVarargs_1.check rename tests/coverage/run/varargs/{test_1.check => test.check} (100%) rename tests/coverage/run/varargs/{test_1.scoverage.check => test.scoverage.check} (100%) diff --git a/compiler/src/dotty/tools/dotc/core/Contexts.scala b/compiler/src/dotty/tools/dotc/core/Contexts.scala index 7f5779bb6127..7c54d1392720 100644 --- a/compiler/src/dotty/tools/dotc/core/Contexts.scala +++ b/compiler/src/dotty/tools/dotc/core/Contexts.scala @@ -41,6 +41,7 @@ import util.Store import plugins.* import java.util.concurrent.atomic.AtomicInteger import java.nio.file.InvalidPathException +import dotty.tools.dotc.coverage.Coverage object Contexts { @@ -982,6 +983,11 @@ object Contexts { /** Was best effort file used during compilation? */ private[core] var usedBestEffortTasty = false + /** If coverage option is used, it stores all instrumented statements (for InstrumentCoverage). + * We need this information to be persisted across different runs, so it's stored here. + */ + private[dotc] var coverage: Coverage | Null = null + // Types state /** A table for hash consing unique types */ private[core] val uniques: Uniques = Uniques() diff --git a/compiler/src/dotty/tools/dotc/coverage/Coverage.scala b/compiler/src/dotty/tools/dotc/coverage/Coverage.scala index 98e67178fb69..3061bfa4ee5c 100644 --- a/compiler/src/dotty/tools/dotc/coverage/Coverage.scala +++ b/compiler/src/dotty/tools/dotc/coverage/Coverage.scala @@ -2,15 +2,27 @@ package dotty.tools.dotc package coverage import scala.collection.mutable +import java.nio.file.Path /** Holds a list of statements to include in the coverage reports. */ class Coverage: private val statementsById = new mutable.LongMap[Statement](256) + private var statementId: Int = 0 + + def nextStatementId(): Int = + statementId += 1 + statementId - 1 + + def statements: Iterable[Statement] = statementsById.values def addStatement(stmt: Statement): Unit = statementsById(stmt.id) = stmt + def removeStatementsFromFile(sourcePath: Path) = + val removedIds = statements.filter(_.location.sourcePath == sourcePath).map(_.id.toLong) + removedIds.foreach(statementsById.remove) + /** * A statement that can be invoked, and thus counted as "covered" by code coverage tools. diff --git a/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala b/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala index f5e0f8c63b58..0229284a1b5f 100644 --- a/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala +++ b/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala @@ -38,12 +38,6 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer: override def isEnabled(using ctx: Context) = ctx.settings.coverageOutputDir.value.nonEmpty - // counter to assign a unique id to each statement - private var statementId = 0 - - // stores all instrumented statements - private val coverage = Coverage() - private var coverageExcludeClasslikePatterns: List[Pattern] = Nil private var coverageExcludeFilePatterns: List[Pattern] = Nil @@ -61,12 +55,17 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer: .foreach(_.nn.delete()) end if + // Initialise a coverage object if it does not exist yet + if ctx.base.coverage == null then + ctx.base.coverage = Coverage() + coverageExcludeClasslikePatterns = ctx.settings.coverageExcludeClasslikes.value.map(_.r.pattern) coverageExcludeFilePatterns = ctx.settings.coverageExcludeFiles.value.map(_.r.pattern) + ctx.base.coverage.nn.removeStatementsFromFile(ctx.compilationUnit.source.file.absolute.jpath) super.run - Serializer.serialize(coverage, outputPath, ctx.settings.sourceroot.value) + Serializer.serialize(ctx.base.coverage.nn, outputPath, ctx.settings.sourceroot.value) private def isClassIncluded(sym: Symbol)(using Context): Boolean = val fqn = sym.fullName.toText(ctx.printerFn(ctx)).show @@ -110,8 +109,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer: * @return the statement's id */ private def recordStatement(tree: Tree, pos: SourcePosition, branch: Boolean)(using ctx: Context): Int = - val id = statementId - statementId += 1 + val id = ctx.base.coverage.nn.nextStatementId() val sourceFile = pos.source val statement = Statement( @@ -127,7 +125,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer: treeName = tree.getClass.getSimpleName.nn, branch ) - coverage.addStatement(statement) + ctx.base.coverage.nn.addStatement(statement) id /** diff --git a/compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala b/compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala index 7efa1f6f564e..f6460180cab9 100644 --- a/compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala +++ b/compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala @@ -54,15 +54,27 @@ class CoverageTests: lines end fixWindowsPaths - def runOnFile(p: Path): Boolean = - scalaFile.matches(p) && - (Properties.testsFilter.isEmpty || Properties.testsFilter.exists(p.toString.contains)) + def runOnFileOrDir(p: Path): Boolean = + (scalaFile.matches(p) || Files.isDirectory(p)) + && (p != dir) + && (Properties.testsFilter.isEmpty || Properties.testsFilter.exists(p.toString.contains)) + + Files.walk(dir, 1).filter(runOnFileOrDir).forEach(path => { + // measurement files only exist in the "run" category + // as these are generated at runtime by the scala.runtime.coverage.Invoker + val (targetDir, expectFile, expectMeasurementFile) = + if Files.isDirectory(path) then + val dirName = path.getFileName().toString + assert(!Files.walk(path).filter(scalaFile.matches(_)).toArray.isEmpty, s"No scala files found in test directory: ${path}") + val targetDir = computeCoverageInTmp(path, isDirectory = true, dir, run) + (targetDir, path.resolve(s"test.scoverage.check"), path.resolve(s"test.measurement.check")) + else + val fileName = path.getFileName.toString.stripSuffix(".scala") + val targetDir = computeCoverageInTmp(path, isDirectory = false, dir, run) + (targetDir, path.resolveSibling(s"${fileName}.scoverage.check"), path.resolveSibling(s"${fileName}.measurement.check")) - Files.walk(dir).filter(runOnFile).forEach(path => { - val fileName = path.getFileName.toString.stripSuffix(".scala") - val targetDir = computeCoverageInTmp(path, dir, run) val targetFile = targetDir.resolve(s"scoverage.coverage") - val expectFile = path.resolveSibling(s"$fileName.scoverage.check") + if updateCheckFiles then Files.copy(targetFile, expectFile, StandardCopyOption.REPLACE_EXISTING) else @@ -72,9 +84,6 @@ class CoverageTests: val instructions = FileDiff.diffMessage(expectFile.toString, targetFile.toString) fail(s"Coverage report differs from expected data.\n$instructions") - // measurement files only exist in the "run" category - // as these are generated at runtime by the scala.runtime.coverage.Invoker - val expectMeasurementFile = path.resolveSibling(s"$fileName.measurement.check") if run && Files.exists(expectMeasurementFile) then // Note that this assumes that the test invoked was single threaded, @@ -95,14 +104,20 @@ class CoverageTests: }) /** Generates the coverage report for the given input file, in a temporary directory. */ - def computeCoverageInTmp(inputFile: Path, sourceRoot: Path, run: Boolean)(using TestGroup): Path = + def computeCoverageInTmp(inputFile: Path, isDirectory: Boolean, sourceRoot: Path, run: Boolean)(using TestGroup): Path = val target = Files.createTempDirectory("coverage") val options = defaultOptions.and("-Ycheck:instrumentCoverage", "-coverage-out", target.toString, "-sourceroot", sourceRoot.toString) if run then - val test = compileDir(inputFile.getParent.toString, options) + val path = if isDirectory then inputFile.toString else inputFile.getParent.toString + val test = compileDir(path, options) + test.checkFilePaths.foreach { checkFilePath => + assert(checkFilePath.exists, s"Expected checkfile for $path $checkFilePath does not exist.") + } test.checkRuns() else - val test = compileFile(inputFile.toString, options) + val test = + if isDirectory then compileDir(inputFile.toString, options) + else compileFile(inputFile.toString, options) test.checkCompile() target diff --git a/compiler/test/dotty/tools/vulpix/ParallelTesting.scala b/compiler/test/dotty/tools/vulpix/ParallelTesting.scala index e7e5936a4b29..29e64163b833 100644 --- a/compiler/test/dotty/tools/vulpix/ParallelTesting.scala +++ b/compiler/test/dotty/tools/vulpix/ParallelTesting.scala @@ -258,15 +258,7 @@ trait ParallelTesting extends RunnerOrchestration { self => * For a given test source, returns a check file against which the result of the test run * should be compared. Is used by implementations of this trait. */ - final def checkFile(testSource: TestSource): Option[JFile] = (testSource match { - case ts: JointCompilationSource => - ts.files.collectFirst { - case f if !f.isDirectory => - new JFile(f.getPath.replaceFirst("\\.(scala|java)$", ".check")) - } - case ts: SeparateCompilationSource => - Option(new JFile(ts.dir.getPath + ".check")) - }).filter(_.exists) + final def checkFile(testSource: TestSource): Option[JFile] = (CompilationLogic.checkFilePath(testSource)).filter(_.exists) /** * Checks if the given actual lines are the same as the ones in the check file. @@ -343,6 +335,18 @@ trait ParallelTesting extends RunnerOrchestration { self => } } + object CompilationLogic { + private[ParallelTesting] def checkFilePath(testSource: TestSource) = testSource match { + case ts: JointCompilationSource => + ts.files.collectFirst { + case f if !f.isDirectory => + new JFile(f.getPath.replaceFirst("\\.(scala|java)$", ".check")) + } + case ts: SeparateCompilationSource => + Option(new JFile(ts.dir.getPath + ".check")) + } + } + /** Each `Test` takes the `testSources` and performs the compilation and assertions * according to the implementing class "neg", "run" or "pos". */ @@ -1157,6 +1161,8 @@ trait ParallelTesting extends RunnerOrchestration { self => def this(targets: List[TestSource]) = this(targets, 1, true, None, false, false) + def checkFilePaths: List[JFile] = targets.map(CompilationLogic.checkFilePath).flatten + def copy(targets: List[TestSource], times: Int = times, shouldDelete: Boolean = shouldDelete, diff --git a/tests/coverage/pos/macro-late-suspend/Test.scala b/tests/coverage/pos/macro-late-suspend/Test.scala new file mode 100644 index 000000000000..bf008583c7d9 --- /dev/null +++ b/tests/coverage/pos/macro-late-suspend/Test.scala @@ -0,0 +1,13 @@ +package example + +sealed trait Test + +object Test { + case object Foo extends Test + + val visitorType = mkVisitorType[Test] + + trait Visitor[A] { + type V[a] = visitorType.Out[a] + } +} diff --git a/tests/coverage/pos/macro-late-suspend/UsesTest.scala b/tests/coverage/pos/macro-late-suspend/UsesTest.scala new file mode 100644 index 000000000000..803e93c328c9 --- /dev/null +++ b/tests/coverage/pos/macro-late-suspend/UsesTest.scala @@ -0,0 +1,3 @@ +package example + +val _ = Test.Foo diff --git a/tests/coverage/pos/macro-late-suspend/VisitorMacros.scala b/tests/coverage/pos/macro-late-suspend/VisitorMacros.scala new file mode 100644 index 000000000000..49ada0ff2180 --- /dev/null +++ b/tests/coverage/pos/macro-late-suspend/VisitorMacros.scala @@ -0,0 +1,12 @@ +package example + +import scala.quoted.* + +private def mkVisitorTypeImpl[T: Type](using q: Quotes): Expr[VisitorType[T]] = + '{new VisitorType[T]{}} + +transparent inline def mkVisitorType[T]: VisitorType[T] = ${ mkVisitorTypeImpl[T] } + +trait VisitorType[T] { + type Out[A] +} diff --git a/tests/coverage/pos/macro-late-suspend/test.scoverage.check b/tests/coverage/pos/macro-late-suspend/test.scoverage.check new file mode 100644 index 000000000000..f962705ed2ce --- /dev/null +++ b/tests/coverage/pos/macro-late-suspend/test.scoverage.check @@ -0,0 +1,88 @@ +# 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 +macro-late-suspend/VisitorMacros.scala +example +VisitorMacros$package +Object +example.VisitorMacros$package +mkVisitorTypeImpl +124 +144 +6 +unpickleExprV2 +Apply +false +0 +false +new VisitorType[T]{} + +2 +macro-late-suspend/VisitorMacros.scala +example +VisitorMacros$package +Object +example.VisitorMacros$package +mkVisitorTypeImpl +40 +69 +5 +mkVisitorTypeImpl +DefDef +false +0 +false +private def mkVisitorTypeImpl + +3 +macro-late-suspend/Test.scala +example +Test +Object +example.Test + +102 +121 +8 + +Apply +false +0 +false +mkVisitorType[Test] + +4 +macro-late-suspend/UsesTest.scala +example +UsesTest$package +Object +example.UsesTest$package + +22 +22 +3 + +Literal +true +0 +false + + diff --git a/tests/coverage/run/i16940/i16940.check b/tests/coverage/run/i16940/i16940.check new file mode 100644 index 000000000000..0cfbf08886fc --- /dev/null +++ b/tests/coverage/run/i16940/i16940.check @@ -0,0 +1 @@ +2 diff --git a/tests/coverage/run/i16940/i16940.scoverage.check b/tests/coverage/run/i16940/test.scoverage.check similarity index 100% rename from tests/coverage/run/i16940/i16940.scoverage.check rename to tests/coverage/run/i16940/test.scoverage.check diff --git a/tests/coverage/run/i18233-min/i182233-min.check b/tests/coverage/run/i18233-min/i182233-min.check new file mode 100644 index 000000000000..a4bb477e9e1f --- /dev/null +++ b/tests/coverage/run/i18233-min/i182233-min.check @@ -0,0 +1,2 @@ +List() +List(abc, def) diff --git a/tests/coverage/run/i18233-min/i18233-min.scoverage.check b/tests/coverage/run/i18233-min/test.scoverage.check similarity index 100% rename from tests/coverage/run/i18233-min/i18233-min.scoverage.check rename to tests/coverage/run/i18233-min/test.scoverage.check diff --git a/tests/coverage/run/i18233/i18233.check b/tests/coverage/run/i18233/i18233.check new file mode 100644 index 000000000000..a68818270123 --- /dev/null +++ b/tests/coverage/run/i18233/i18233.check @@ -0,0 +1 @@ +Baz diff --git a/tests/coverage/run/i18233/i18233.scoverage.check b/tests/coverage/run/i18233/test.scoverage.check similarity index 100% rename from tests/coverage/run/i18233/i18233.scoverage.check rename to tests/coverage/run/i18233/test.scoverage.check diff --git a/tests/coverage/run/java-methods/test.check b/tests/coverage/run/java-methods/JavaObject.check similarity index 100% rename from tests/coverage/run/java-methods/test.check rename to tests/coverage/run/java-methods/JavaObject.check diff --git a/tests/coverage/run/macro-suspend/Macro.check b/tests/coverage/run/macro-suspend/Macro.check new file mode 100644 index 000000000000..80a62d1af279 --- /dev/null +++ b/tests/coverage/run/macro-suspend/Macro.check @@ -0,0 +1 @@ +>>> hello <<< diff --git a/tests/coverage/run/macro-suspend/Macro.scala b/tests/coverage/run/macro-suspend/Macro.scala new file mode 100644 index 000000000000..dffe91a9a9b1 --- /dev/null +++ b/tests/coverage/run/macro-suspend/Macro.scala @@ -0,0 +1,8 @@ +import scala.quoted.{Expr, Quotes} + +object Macro: + inline def decorate(inline s: String): String = ${ decorateQuotes('s) } + def decorateQuotes(s: Expr[String])(using Quotes): Expr[String] = '{ ">>> " + $s + " <<<" } + +object Greeting: + def greet() = "hello" diff --git a/tests/coverage/run/macro-suspend/Test.scala b/tests/coverage/run/macro-suspend/Test.scala new file mode 100644 index 000000000000..09cf1c36526c --- /dev/null +++ b/tests/coverage/run/macro-suspend/Test.scala @@ -0,0 +1,4 @@ +object Test: + def main(args: Array[String]): Unit = + println(Macro.decorate(Greeting.greet())) + diff --git a/tests/coverage/run/macro-suspend/test.scoverage.check b/tests/coverage/run/macro-suspend/test.scoverage.check new file mode 100644 index 000000000000..759897eb7747 --- /dev/null +++ b/tests/coverage/run/macro-suspend/test.scoverage.check @@ -0,0 +1,173 @@ +# 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 +# ------------------------------------------ +0 +macro-suspend/Macro.scala + +Macro +Object +.Macro +decorateQuotes +195 +215 +5 +unpickleExprV2 +Apply +false +0 +false +">>> " + $s + " <<<" + +1 +macro-suspend/Macro.scala + +Macro +Object +.Macro +$anonfun +205 +206 +5 +apply +Apply +false +0 +false +s + +2 +macro-suspend/Macro.scala + +Macro +Object +.Macro +decorateQuotes +126 +144 +5 +decorateQuotes +DefDef +false +0 +false +def decorateQuotes + +3 +macro-suspend/Macro.scala + +Greeting +Object +.Greeting +greet +238 +247 +8 +greet +DefDef +false +0 +false +def greet + +4 +macro-suspend/Test.scala + +Test +Object +.Test +main +57 +98 +3 +println +Apply +false +0 +false +println(Macro.decorate(Greeting.greet())) + +5 +macro-suspend/Test.scala + +Test +Object +.Test +main +65 +97 +3 ++ +Apply +false +0 +false +Macro.decorate(Greeting.greet()) + +6 +macro-suspend/Test.scala + +Test +Object +.Test +main +65 +97 +3 ++ +Apply +false +0 +false +Macro.decorate(Greeting.greet()) + +7 +macro-suspend/Test.scala + +Test +Object +.Test +main +80 +96 +3 +greet +Apply +false +0 +false +Greeting.greet() + +8 +macro-suspend/Test.scala + +Test +Object +.Test +main +15 +23 +2 +main +DefDef +false +0 +false +def main + diff --git a/tests/coverage/run/varargs/JavaVarargs_1.check b/tests/coverage/run/varargs/JavaVarargs_1.check new file mode 100644 index 000000000000..24f879b660ff --- /dev/null +++ b/tests/coverage/run/varargs/JavaVarargs_1.check @@ -0,0 +1,3 @@ +first0 +first0 +first3 diff --git a/tests/coverage/run/varargs/test_1.check b/tests/coverage/run/varargs/test.check similarity index 100% rename from tests/coverage/run/varargs/test_1.check rename to tests/coverage/run/varargs/test.check diff --git a/tests/coverage/run/varargs/test_1.scoverage.check b/tests/coverage/run/varargs/test.scoverage.check similarity index 100% rename from tests/coverage/run/varargs/test_1.scoverage.check rename to tests/coverage/run/varargs/test.scoverage.check