Fix coverage serialization when encountering macro suspension (scala#22303)

Fixes scala#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).

Author: jchyb

compiler/src/dotty/tools/dotc/core/Contexts.scala

@@ -40,6 +40,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 {
 
@@ -958,6 +959,11 @@ object Contexts {
     val sources: util.HashMap[AbstractFile, SourceFile] = util.HashMap[AbstractFile, SourceFile]()
     val files: util.HashMap[TermName, AbstractFile] = util.HashMap()
 
+    /** 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()

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.

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
 
     /**

compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala

@@ -45,15 +45,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
@@ -63,9 +75,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,
@@ -86,14 +95,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
 

compiler/test/dotty/tools/vulpix/ParallelTesting.scala

@@ -250,15 +250,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.
@@ -335,6 +327,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".
    */
@@ -1087,6 +1091,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,

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]
+  }
+}

tests/coverage/pos/macro-late-suspend/UsesTest.scala

@@ -0,0 +1,3 @@
+package example
+
+val _ = Test.Foo

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]
+}

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
+<init>
+102
+121
+8
+<init>
+Apply
+false
+0
+false
+mkVisitorType[Test]
+
+4
+macro-late-suspend/UsesTest.scala
+example
+UsesTest$package
+Object
+example.UsesTest$package
+<init>
+22
+22
+3
+<none>
+Literal
+true
+0
+false
+
+

tests/coverage/run/i16940/i16940.check

@@ -0,0 +1 @@
+2