Move coverage phase to before FirstTransform + some cleanup

Author: TheElectronWill

Diff for: compiler/src/dotty/tools/dotc/Compiler.scala

@@ -46,7 +46,6 @@ class Compiler {
     List(new sjs.PrepJSInterop) ::  // Additional checks and transformations for Scala.js (Scala.js only)
     List(new sbt.ExtractAPI) ::     // Sends a representation of the API of classes to sbt via callbacks
     List(new SetRootTree) ::        // Set the `rootTreeOrProvider` on class symbols
-    List(new CoverageTransformMacro) :: // Perform instrumentation for coverage transform (if -coverage is present)
     Nil
 
   /** Phases dealing with TASTY tree pickling and unpickling */
@@ -60,6 +59,7 @@ class Compiler {
 
   /** Phases dealing with the transformation from pickled trees to backend trees */
   protected def transformPhases: List[List[Phase]] =
+    List(new InstrumentCoverage) ::  // Perform instrumentation for code coverage (if -coverage setting is set)
     List(new FirstTransform,         // Some transformations to put trees into a canonical form
          new CheckReentrant,         // Internal use only: Check that compiled program has no data races involving global vars
          new ElimPackagePrefixes,    // Eliminate references to package prefixes in Select nodes

Diff for: compiler/src/dotty/tools/dotc/coverage/Coverage.scala

@@ -3,13 +3,12 @@ package coverage
 
 import scala.collection.mutable
 
-class Coverage {
+class Coverage:
   private val statementsById = mutable.Map[Int, Statement]()
 
-  def statements = statementsById.values
+  def statements: Iterable[Statement] = statementsById.values
 
-  def addStatement(stmt: Statement): Unit = statementsById.put(stmt.id, stmt)
-}
+  def addStatement(stmt: Statement): Unit = statementsById(stmt.id) = stmt
 
 case class Statement(
     source: String,
@@ -24,7 +23,9 @@ case class Statement(
     branch: Boolean,
     var count: Int = 0,
     ignored: Boolean = false
-) {
-  def invoked(): Unit = count = count + 1
-  def isInvoked = count > 0
-}
+):
+  def invoked(): Unit =
+    count += 1
+
+  def isInvoked: Boolean =
+    count > 0

Diff for: compiler/src/dotty/tools/dotc/coverage/Location.scala

@@ -20,8 +20,8 @@ final case class Location(
     sourcePath: String
 )
 
-object Location {
-  def apply(tree: Tree)(using ctx: Context): Location = {
+object Location:
+  def apply(tree: Tree)(using ctx: Context): Location =
 
     val packageName = ctx.owner.denot.enclosingPackageClass.name.toSimpleName.toString()
     val className = ctx.owner.denot.enclosingClass.name.toSimpleName.toString()
@@ -34,5 +34,3 @@ object Location {
       ctx.owner.denot.enclosingMethod.name.toSimpleName.toString(),
       ctx.source.file.absolute.toString()
     )
-  }
-}

Diff for: compiler/src/dotty/tools/dotc/coverage/Serializer.scala

@@ -5,30 +5,29 @@ import java.io._
 
 import scala.io.Source
 
-object Serializer {
+object Serializer:
 
-  val coverageFileName = "scoverage.coverage"
-  val coverageDataFormatVersion = "3.0"
-  // Write out coverage data to the given data directory, using the default coverage filename
+  private val coverageFileName = "scoverage.coverage"
+  private val coverageDataFormatVersion = "3.0"
+
+  /** 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))
 
-  // Write out coverage data to given file.
-  def serialize(coverage: Coverage, file: File, sourceRoot: File): Unit = {
+  /** Write out coverage data to given file. */
+  def serialize(coverage: Coverage, file: File, sourceRoot: File): Unit =
     val writer = new BufferedWriter(new FileWriter(file))
     serialize(coverage, writer, sourceRoot)
     writer.close()
-  }
 
-  def serialize(coverage: Coverage, writer: Writer, sourceRoot: File): Unit = {
+  def serialize(coverage: Coverage, writer: Writer, sourceRoot: File): Unit =
 
-    def getRelativePath(filePath: String): String = {
+    def getRelativePath(filePath: String): String =
       val base = sourceRoot.getCanonicalFile().toPath()
       val relPath = base.relativize(new File(filePath).getCanonicalFile().toPath())
       relPath.toString
-    }
 
-    def writeHeader(writer: Writer): Unit = {
+    def writeHeader(writer: Writer): Unit =
       writer.write(s"""# Coverage data, format version: $coverageDataFormatVersion
                       |# Statement data:
                       |# - id
@@ -50,8 +49,8 @@ object Serializer {
                       |# '\f' sign
                       |# ------------------------------------------
                       |""".stripMargin)
-    }
-    def writeStatement(stmt: Statement, writer: Writer): Unit = {
+
+    def writeStatement(stmt: Statement, writer: Writer): Unit =
       writer.write(s"""${stmt.id}
                       |${getRelativePath(stmt.location.sourcePath)}
                       |${stmt.location.packageName}
@@ -70,14 +69,11 @@ object Serializer {
                       |${stmt.desc}
                       |\f
                       |""".stripMargin)
-    }
 
     writeHeader(writer)
     coverage.statements.toVector
       .sortBy(_.id)
       .foreach(stmt => writeStatement(stmt, writer))
-  }
 
   def coverageFile(dataDir: File): File = coverageFile(dataDir.getAbsolutePath)
-  def coverageFile(dataDir: String): File = new File(dataDir, coverageFileName)
-}
+  def coverageFile(dataDir: String): File = File(dataDir, coverageFileName)

Diff for: compiler/src/dotty/tools/dotc/transform/CoverageTransformMacro.scala renamed to compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala

@@ -21,53 +21,54 @@ import dotty.tools.dotc.typer.LiftCoverage
 
 import scala.quoted
 
-/** Phase that implements code coverage, executed when the "-coverage
- *  OUTPUT_PATH" is added to the compilation.
- */
-class CoverageTransformMacro extends MacroTransform with IdentityDenotTransformer {
+/** Implements code coverage by inserting calls to scala.runtime.Invoker
+  * ("instruments" the source code).
+  * The result can then be consumed by the Scoverage tool.
+  */
+class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
   import ast.tpd._
 
-  override def phaseName = "coverage"
+  override def phaseName = InstrumentCoverage.name
 
-  // Atomic counter used for assignation of IDs to difference statements
-  val statementId = new AtomicInteger(0)
+  override def description = InstrumentCoverage.description
 
-  var outputPath = ""
+  // Enabled by argument "-coverage OUTPUT_DIR"
+  override def isEnabled(using ctx: Context) =
+    ctx.settings.coverageOutputDir.value.nonEmpty
 
-  // Main class used to store all instrumented statements
-  val coverage = new Coverage
+  // Atomic counter used for assignation of IDs to difference statements
+  private val statementId = AtomicInteger(0)
 
-  override def run(using ctx: Context): Unit = {
+  private var outputPath = ""
 
-    if (ctx.settings.coverageOutputDir.value.nonEmpty) {
-      outputPath = ctx.settings.coverageOutputDir.value
+  // Main class used to store all instrumented statements
+  private val coverage = Coverage()
 
-      // Ensure the dir exists
-      val dataDir = new File(outputPath)
-      val newlyCreated = dataDir.mkdirs()
+  override def run(using ctx: Context): Unit =
+    outputPath = ctx.settings.coverageOutputDir.value
 
-      if (!newlyCreated) {
-        // If the directory existed before, let's clean it up.
-        dataDir.listFiles
-          .filter(_.getName.startsWith("scoverage"))
-          .foreach(_.delete)
-      }
+    // Ensure the dir exists
+    val dataDir = new File(outputPath)
+    val newlyCreated = dataDir.mkdirs()
 
-      super.run
+    if (!newlyCreated) {
+      // If the directory existed before, let's clean it up.
+      dataDir.listFiles
+        .filter(_.getName.startsWith("scoverage"))
+        .foreach(_.delete)
+    }
 
+    super.run
 
-      Serializer.serialize(coverage, outputPath, ctx.settings.coverageSourceroot.value)
-    }
-  }
+    Serializer.serialize(coverage, outputPath, ctx.settings.coverageSourceroot.value)
 
-  protected def newTransformer(using Context): Transformer =
-    new CoverageTransormer
+  override protected def newTransformer(using Context) = CoverageTransormer()
 
-  class CoverageTransormer extends Transformer {
+  class CoverageTransormer extends Transformer:
     var instrumented = false
 
-    override def transform(tree: Tree)(using Context): Tree = {
-      tree match {
+    override def transform(tree: Tree)(using Context): Tree =
+      tree match
         case tree: If =>
           cpy.If(tree)(
             cond = transform(tree.cond),
@@ -139,10 +140,8 @@ class CoverageTransformMacro extends MacroTransform with IdentityDenotTransforme
              tree.sourcePos
           )
           super.transform(tree)
-      }
-    }
 
-    def liftApply(tree: Apply)(using Context) = {
+    def liftApply(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)
@@ -156,22 +155,18 @@ class CoverageTransformMacro extends MacroTransform with IdentityDenotTransforme
           false
         )
       )
-    }
 
-    def instrumentCasees(cases: List[CaseDef])(using Context): List[CaseDef] = {
+    def instrumentCasees(cases: List[CaseDef])(using Context): List[CaseDef] =
       cases.map(instrumentCaseDef)
-    }
 
-    def instrumentCaseDef(tree: CaseDef)(using Context): CaseDef = {
+    def instrumentCaseDef(tree: CaseDef)(using Context): CaseDef =
       cpy.CaseDef(tree)(tree.pat, transform(tree.guard), transform(tree.body))
-    }
 
-    def instrument(tree: Tree, branch: Boolean = false)(using Context): Tree = {
+    def instrument(tree: Tree, branch: Boolean = false)(using Context): Tree =
       instrument(tree, tree.sourcePos, branch)
-    }
 
-    def instrument(tree: Tree, pos: SourcePosition, branch: Boolean)(using ctx: Context): Tree = {
-      if (pos.exists && !pos.span.isZeroExtent && !tree.isType) {
+    def instrument(tree: Tree, pos: SourcePosition, branch: Boolean)(using ctx: Context): Tree =
+      if (pos.exists && !pos.span.isZeroExtent && !tree.isType)
         val id = statementId.incrementAndGet()
         val statement = new Statement(
           source = ctx.source.file.name,
@@ -187,18 +182,16 @@ class CoverageTransformMacro extends MacroTransform with IdentityDenotTransforme
         )
         coverage.addStatement(statement)
         Block(List(invokeCall(id)), tree)
-      } else {
+      else
         tree
-      }
-    }
 
-    def invokeCall(id: Int)(using Context): Tree = {
+    def invokeCall(id: Int)(using Context): Tree =
       ref(defn.InvokerModuleRef)
         .select("invoked".toTermName)
         .appliedToArgs(
           List(Literal(Constant(id)), Literal(Constant(outputPath)))
         )
-    }
-  }
 
-}
+object InstrumentCoverage:
+  val name: String = "instrumentCoverage"
+  val description: String = "instrument code for coverage cheking"

Diff for: compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala

@@ -158,7 +158,7 @@ object LiftComplex extends LiftComplex
 /** Lift complex + lift the prefixes */
 object LiftCoverage extends LiftComplex {
 
-  var liftEverything = false
+  private var liftEverything = false
 
   /** Return true if the apply needs a lift in the coverage phase
     Return false if the args are empty, if one or more will be lifter by a