Cut coverage tests in small parts and avoid lifting literals

Author: TheElectronWill

compiler/src/dotty/tools/dotc/config/ScalaSettings.scala

@@ -117,7 +117,7 @@ trait CommonScalaSettings:
   val unchecked: Setting[Boolean] = BooleanSetting("-unchecked", "Enable additional warnings where generated code depends on assumptions.", initialValue = true, aliases = List("--unchecked"))
   val language: Setting[List[String]] = MultiStringSetting("-language", "feature", "Enable one or more language features.", aliases = List("--language"))
   /* Coverage settings */
-  val coverageOutputDir = PathSetting("-coverage", "Destination for coverage classfiles and instrumentation data.", "")
+  val coverageOutputDir = PathSetting("-coverage-out", "Destination for coverage classfiles and instrumentation data.", "")
   val coverageSourceroot = PathSetting("-coverage-sourceroot", "An alternative root dir of your sources used to relativize.", ".")
 
   /* Other settings */

compiler/src/dotty/tools/dotc/coverage/Coverage.scala

@@ -4,7 +4,7 @@ package coverage
 import scala.collection.mutable
 
 class Coverage:
-  private val statementsById = mutable.Map[Int, Statement]()
+  private val statementsById = new mutable.LongMap[Statement](256)
 
   def statements: Iterable[Statement] = statementsById.values
 

compiler/src/dotty/tools/dotc/coverage/Serializer.scala

@@ -1,9 +1,9 @@
 package dotty.tools.dotc
 package coverage
 
-import java.io._
-
-import scala.io.Source
+import java.nio.file.{Path, Paths, Files}
+import java.io.Writer
+import scala.language.unsafeNulls
 
 /**
  * Serializes scoverage data.
@@ -16,19 +16,22 @@ object Serializer:
 
   /** Write out coverage data to the given data directory, using the default coverage filename */
   def serialize(coverage: Coverage, dataDir: String, sourceRoot: String): Unit =
-    serialize(coverage, coverageFile(dataDir), new File(sourceRoot))
+    serialize(coverage, Paths.get(dataDir, CoverageFileName).toAbsolutePath, Paths.get(sourceRoot).toAbsolutePath)
 
-  /** Write out coverage data to given file. */
-  def serialize(coverage: Coverage, file: File, sourceRoot: File): Unit =
-    val writer = BufferedWriter(FileWriter(file))
-    serialize(coverage, writer, sourceRoot)
-    writer.close()
+  /** Write out coverage data to a file. */
+  def serialize(coverage: Coverage, file: Path, sourceRoot: Path): Unit =
+    val writer = Files.newBufferedWriter(file)
+    try
+      serialize(coverage, writer, sourceRoot)
+    finally
+      writer.close()
 
-  def serialize(coverage: Coverage, writer: Writer, sourceRoot: File): Unit =
+  /** Write out coverage data (info about each statement that can be covered) to a writer.
+   */
+  def serialize(coverage: Coverage, writer: Writer, sourceRoot: Path): Unit =
 
     def getRelativePath(filePath: String): String =
-      val base = sourceRoot.getCanonicalFile().toPath()
-      val relPath = base.relativize(File(filePath).getCanonicalFile().toPath())
+      val relPath = sourceRoot.relativize(Paths.get(filePath).toAbsolutePath)
       relPath.toString
 
     def writeHeader(writer: Writer): Unit =
@@ -78,6 +81,3 @@ object Serializer:
     coverage.statements.toSeq
       .sortBy(_.id)
       .foreach(stmt => writeStatement(stmt, writer))
-
-  def coverageFile(dataDir: File): File = coverageFile(dataDir.getAbsolutePath)
-  def coverageFile(dataDir: String): File = File(dataDir, CoverageFileName)

compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala

@@ -15,12 +15,10 @@ import dotty.tools.dotc.coverage.Location
 import dotty.tools.dotc.core.Symbols.defn
 import dotty.tools.dotc.core.Symbols.Symbol
 import dotty.tools.dotc.core.Decorators.toTermName
-import dotty.tools.dotc.util.SourcePosition
+import dotty.tools.dotc.util.{SourcePosition, Property}
 import dotty.tools.dotc.core.Constants.Constant
 import dotty.tools.dotc.typer.LiftCoverage
 
-import scala.quoted
-
 /** Implements code coverage by inserting calls to scala.runtime.Invoker
   * ("instruments" the source code).
   * The result can then be consumed by the Scoverage tool.
@@ -51,29 +49,40 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
     val dataDir = new File(outputPath)
     val newlyCreated = dataDir.mkdirs()
 
-    if (!newlyCreated) {
+    if !newlyCreated then
       // If the directory existed before, let's clean it up.
-      dataDir.listFiles
-        .filter(_.getName.startsWith("scoverage"))
-        .foreach(_.delete)
-    }
-
+      dataDir.listFiles.nn
+        .filter(_.nn.getName.nn.startsWith("scoverage"))
+        .foreach(_.nn.delete())
+    end if
     super.run
 
     Serializer.serialize(coverage, outputPath, ctx.settings.coverageSourceroot.value)
 
   override protected def newTransformer(using Context) = CoverageTransormer()
 
+  /** Transforms trees to insert calls to Invoker.invoked to compute the coverage when the code is called */
   private class CoverageTransormer extends Transformer:
+    private val IgnoreLiterals = new Property.Key[Boolean]
+
+    private def ignoreLiteralsContext(using ctx: Context): Context =
+      ctx.fresh.setProperty(IgnoreLiterals, true)
 
-    override def transform(tree: Tree)(using Context): Tree =
-      println(tree.show + tree.toString)
+    override def transform(tree: Tree)(using ctx: Context): Tree =
       tree match
         // simple cases
-        case tree: (Literal | Import | Export) => tree
-        case tree: (New | This | Super) => instrument(tree)
+        case tree: (Import | Export | This | Super | New) => tree
         case tree if (tree.isEmpty || tree.isType) => tree // empty Thicket, Ident, TypTree, ...
 
+        // Literals must be instrumented (at least) when returned by a def,
+        // otherwise `def d = "literal"` is not covered when called from a test.
+        // They can be left untouched when passed in a parameter of an Apply.
+        case tree: Literal =>
+          if ctx.property(IgnoreLiterals).contains(true) then
+            tree
+          else
+            instrument(tree)
+
         // branches
         case tree: If =>
           cpy.If(tree)(
@@ -88,28 +97,42 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
             finalizer = instrument(transform(tree.finalizer), true)
           )
 
+        // a.f(args)
+        case tree @ Apply(fun: Select, args) =>
+          // don't transform the first Select, but do transform `a.b` in `a.b.f(args)`
+          val transformedFun = cpy.Select(fun)(transform(fun.qualifier), fun.name)
+          if needsLift(tree) then
+            val transformed = cpy.Apply(tree)(transformedFun, args) // args will be transformed in instrumentLifted
+            instrumentLifted(transformed)(using ignoreLiteralsContext)
+          else
+            val transformed = cpy.Apply(tree)(transformedFun, transform(args))
+            instrument(transformed)(using ignoreLiteralsContext)
+
         // f(args)
         case tree: Apply =>
           if needsLift(tree) then
-            liftApply(tree)
+            instrumentLifted(tree)(using ignoreLiteralsContext) // see comment about Literals
           else
-            super.transform(tree)
+            instrument(super.transform(tree)(using ignoreLiteralsContext))
 
         // (f(x))[args]
-        case tree @ TypeApply(fun: Apply, args) =>
+        case TypeApply(fun: Apply, args) =>
           cpy.TypeApply(tree)(transform(fun), args)
 
         // a.b
-        case Select(qual, _) if (qual.symbol.exists && qual.symbol.is(JavaDefined)) =>
-          //Java class can't be used as a value, we can't instrument the
-          //qualifier ({<Probe>;System}.xyz() is not possible !) instrument it
-          //as it is
-          instrument(tree)
-        case tree: Select =>
-          if tree.qualifier.isInstanceOf[New] then
+        case Select(qual, name) =>
+          if qual.symbol.exists && qual.symbol.is(JavaDefined) then
+            //Java class can't be used as a value, we can't instrument the
+            //qualifier ({<Probe>;System}.xyz() is not possible !) instrument it
+            //as it is
             instrument(tree)
           else
-            cpy.Select(tree)(transform(tree.qualifier), tree.name)
+            val transformed = cpy.Select(tree)(transform(qual), name)
+            if transformed.qualifier.isDef then
+              // instrument calls to methods without parameter list
+              instrument(transformed)
+            else
+              transformed
 
         case tree: CaseDef => instrumentCaseDef(tree)
         case tree: ValDef =>
@@ -120,6 +143,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
           // only transform the statements of the package
           cpy.PackageDef(tree)(tree.pid, transform(tree.stats))
         case tree: Assign =>
+          // only transform the rhs
           cpy.Assign(tree)(tree.lhs, transform(tree.rhs))
         case tree: Template =>
           // Don't instrument the parents (extends) of a template since it
@@ -131,13 +155,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
 
         // For everything else just recurse and transform
         case _ =>
-          report.warning(
-            "Unmatched: " + tree.getClass + " " + tree.symbol,
-             tree.sourcePos
-          )
           super.transform(tree)
 
-    def liftApply(tree: Apply)(using Context) =
+    def instrumentLifted(tree: Apply)(using Context) =
       val buffer = mutable.ListBuffer[Tree]()
       // NOTE: that if only one arg needs to be lifted, we just lift everything
       val lifted = LiftCoverage.liftForCoverage(buffer, tree)
@@ -170,10 +190,10 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
           id = id,
           start = pos.start,
           end = pos.end,
-          line = ctx.source.offsetToLine(pos.point),
+          line = pos.line,
           desc = tree.source.content.slice(pos.start, pos.end).mkString,
           symbolName = tree.symbol.name.toSimpleName.toString(),
-          treeName = tree.getClass.getSimpleName,
+          treeName = tree.getClass.getSimpleName.nn,
           branch
         )
         coverage.addStatement(statement)
@@ -209,8 +229,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
       val fun = tree.fun
 
       fun.isInstanceOf[Apply] || // nested apply
-      !isBooleanOperator(fun) ||
-      !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
+      !isBooleanOperator(fun) && !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
 
 object InstrumentCoverage:
   val name: String = "instrumentCoverage"

compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala

@@ -161,7 +161,7 @@ object LiftCoverage extends LiftComplex {
 
   private val LiftEverything = new Property.Key[Boolean]
 
-  private def liftEverything(using Context): Boolean =
+  private inline def liftEverything(using Context): Boolean =
     ctx.property(LiftEverything).contains(true)
 
   private def liftEverythingContext(using Context): Context =

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

@@ -1,75 +1,63 @@
 package dotty.tools.dotc.coverage
 
-import java.util.stream.Collectors
-import org.junit.Assert._
-import dotty.BootstrappedOnlyTests
-import org.junit.experimental.categories.Category
 import org.junit.Test
-import java.nio.file.Files
+import org.junit.Assert.*
+import org.junit.experimental.categories.Category
+
+import dotty.BootstrappedOnlyTests
 import dotty.tools.dotc.Main
-import scala.jdk.CollectionConverters._
 
-import java.io.File
+import java.nio.file.Files
 import java.nio.file.Path
 import java.nio.file.FileSystems
 import java.nio.file.Paths
-import java.nio.charset.StandardCharsets
-import scala.io.Source
-import java.io.BufferedOutputStream
-import java.io.FileOutputStream
+import java.nio.charset.StandardCharsets.UTF_8
+import java.nio.file.StandardCopyOption
 
 @main def updateExpect =
-  CoverageTests().runExpectTest(updateExpectFiles = true)
+  CoverageTests().runExpectTest(updateCheckfiles = true)
 
 @Category(Array(classOf[BootstrappedOnlyTests]))
-class CoverageTests {
+class CoverageTests:
 
-  val scalaFile = FileSystems.getDefault.getPathMatcher("glob:**.scala")
-  val rootSrc = Paths.get(System.getProperty("dotty.tools.dotc.coverage.test"))
-  val expectSrc = rootSrc.resolve("expect")
+  private val scalaFile = FileSystems.getDefault.nn.getPathMatcher("glob:**.scala").nn
+  private val rootSrc = Paths.get(System.getProperty("dotty.tools.dotc.coverage.test")).nn
+  private val expectDir = rootSrc.resolve("expect").nn
 
   @Category(Array(classOf[dotty.SlowTests]))
   @Test def expectTests: Unit =
-    if (!scala.util.Properties.isWin) runExpectTest(updateExpectFiles = false)
-
-  def runExpectTest(updateExpectFiles: Boolean): Unit = {
-    val target = generateCoverage(updateExpectFiles)
-    val input = Source.fromFile(new File(target.toString, "scoverage.coverage"))
-    val expectFile = new File(expectSrc.resolveSibling("scoverage.coverage.expect").toUri)
-
-    if (updateExpectFiles) {
-      val outputStream = new BufferedOutputStream(new FileOutputStream(expectFile))
-      try {
-        input.foreach(outputStream.write(_))
-      } finally outputStream.close
-    } else {
-      val expected = new String(Files.readAllBytes(expectFile.toPath), StandardCharsets.UTF_8)
-      val obtained = input.mkString
-
-      assertEquals(expected, obtained)
-    }
-  }
-
-  def inputFiles(): List[Path] = {
-    val ls = Files.walk(expectSrc)
-    val files =
-      try ls.filter(p => scalaFile.matches(p)).collect(Collectors.toList).asScala
-      finally ls.close()
-
-    files.toList
-  }
-
-  def generateCoverage(updateExpectFiles: Boolean): Path = {
+    runExpectTest(dotty.Properties.testsUpdateCheckfile)
+
+  /** Runs the tests */
+  def runExpectTest(updateCheckfiles: Boolean): Unit =
+    val sourceRoot = if updateCheckfiles then "../" else "."
+
+    Files.walk(expectDir).nn.filter(scalaFile.matches).nn.forEach(p => {
+      val path = p.nn
+      val fileName = path.getFileName.nn.toString.nn.stripSuffix(".scala")
+      val targetDir = computeCoverageInTmp(Seq(path), sourceRoot).nn
+      val targetFile = targetDir.resolve(s"scoverage.coverage").nn
+      val expectFile = expectDir.resolve(s"$fileName.scoverage.check").nn
+
+      if updateCheckfiles then
+        Files.copy(targetFile, expectFile, StandardCopyOption.REPLACE_EXISTING)
+      else
+        val expected = new String(Files.readAllBytes(expectFile), UTF_8)
+        val obtained = new String(Files.readAllBytes(targetFile), UTF_8)
+        assertEquals(expected, obtained)
+
+    })
+
+  /** Generates the coverage report for the given input file, in a temporary directory. */
+  def computeCoverageInTmp(inputFiles: Seq[Path], sourceRoot: String): Path =
     val target = Files.createTempDirectory("coverage")
     val args = Array(
-      "-coverage",
+      "-coverage-out",
       target.toString,
       "-coverage-sourceroot",
-      if (updateExpectFiles) "../" else ".",
+      sourceRoot,
       "-usejavacp"
-    ) ++ inputFiles().map(_.toString)
+    ) ++ inputFiles.map(_.toString)
     val exit = Main.process(args)
-    assertFalse(s"dotc errors: ${exit.errorCount}", exit.hasErrors)
-    target
-  }
-}
+    assertFalse(s"Compilation failed, ${exit.errorCount} errors", exit.hasErrors)
+    target.nn

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

@@ -666,7 +666,7 @@ trait ParallelTesting extends RunnerOrchestration { self =>
         if (didFail) {
           reportFailed()
           failedTestSources.toSet.foreach(addFailedTest)
-          reproduceInstructions.iterator.foreach(addReproduceInstruction)
+          reproduceInstructions.foreach(addReproduceInstruction)
         }
         else reportPassed()
       }
@@ -980,7 +980,7 @@ trait ParallelTesting extends RunnerOrchestration { self =>
       cleanup()
 
       if (!shouldFail && test.didFail) {
-        fail(s"Expected no errors when compiling, failed for the following reason(s):\n${ reasonsForFailure(test) }")
+        fail(s"Expected no errors when compiling, failed for the following reason(s):\n${reasonsForFailure(test)}\n")
       }
       else if (shouldFail && !test.didFail) {
         fail("Pos test should have failed, but didn't")
@@ -1077,14 +1077,16 @@ trait ParallelTesting extends RunnerOrchestration { self =>
     /** Extract `Failure` set and render from `Test` */
     private def reasonsForFailure(test: Test): String = {
       val failureReport =
-        if (test.failureCount == 0) ""
-        else s"\n  - encountered ${test.failureCount} test failures(s)"
+        if test.failureCount == 0 then ""
+        else s"encountered ${test.failureCount} test failure(s):\n"
 
       failureReport + test.failureReasons.collect {
         case test.TimeoutFailure(title) =>
           s"  - test '$title' timed out"
         case test.JavaCompilationFailure(msg) =>
           s"  - java compilation failed with:\n${ msg.linesIterator.map("      " + _).mkString("\n") }"
+        case test.Generic =>
+           "  - generic failure (see test output)"
       }.mkString("\n")
     }