Ensure that cases and DefDefs are always instrumented for coverage

Author: TheElectronWill

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -1437,6 +1437,7 @@ object desugar {
           ValDef(param.name, param.tpt, selector(idx))
             .withSpan(param.span)
             .withAttachment(UntupledParam, ())
+            .withFlags(Synthetic)
       }
     Function(param :: Nil, Block(vdefs, body))
   }
@@ -1693,7 +1694,7 @@ object desugar {
             case (p, n) => makeSyntheticParameter(n + 1, p).withAddedFlags(mods.flags)
           }
           RefinedTypeTree(polyFunctionTpt, List(
-            DefDef(nme.apply, applyTParams :: applyVParams :: Nil, res, EmptyTree)
+            DefDef(nme.apply, applyTParams :: applyVParams :: Nil, res, EmptyTree).withFlags(Synthetic)
           ))
         }
         else {

compiler/src/dotty/tools/dotc/ast/MainProxies.scala

@@ -105,12 +105,14 @@ object MainProxies {
         .filterNot(_.matches(defn.MainAnnot))
         .map(annot => insertTypeSplices.transform(annot.tree))
       val mainMeth = DefDef(nme.main, (mainArg :: Nil) :: Nil, TypeTree(defn.UnitType), body)
-        .withFlags(JavaStatic)
+        .withFlags(JavaStatic | Synthetic)
         .withAnnotations(annots)
       val mainTempl = Template(emptyConstructor, Nil, Nil, EmptyValDef, mainMeth :: Nil)
       val mainCls = TypeDef(mainFun.name.toTypeName, mainTempl)
         .withFlags(Final | Invisible)
-      if (!ctx.reporter.hasErrors) result = mainCls.withSpan(mainAnnotSpan.toSynthetic) :: Nil
+
+      if (!ctx.reporter.hasErrors)
+        result = mainCls.withSpan(mainAnnotSpan.toSynthetic) :: Nil
     }
     result
   }

compiler/src/dotty/tools/dotc/ast/Trees.scala

@@ -610,7 +610,7 @@ object Trees {
     override def toString = s"InlineMatch($selector, $cases)"
   }
 
-  /** case pat if guard => body; only appears as child of a Match */
+  /** case pat if guard => body */
   case class CaseDef[-T >: Untyped] private[ast] (pat: Tree[T], guard: Tree[T], body: Tree[T])(implicit @constructorOnly src: SourceFile)
     extends Tree[T] {
     type ThisTree[-T >: Untyped] = CaseDef[T]

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

@@ -23,12 +23,5 @@ case class Statement(
     symbolName: String,
     treeName: String,
     branch: Boolean,
-    var count: Int = 0,
     ignored: Boolean = false
-):
-  /** Records that this statement has been invoked one more time. */
-  def invoked(): Unit =
-    count += 1
-
-  def isInvoked: Boolean =
-    count > 0
+)

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

@@ -58,6 +58,7 @@ object Serializer:
                       |""".stripMargin)
 
     def writeStatement(stmt: Statement, writer: Writer): Unit =
+      // Note: we write 0 for the count because we have not measured the actual coverage at this point
       writer.write(s"""${stmt.id}
                       |${getRelativePath(stmt.location.sourcePath)}
                       |${stmt.location.packageName}
@@ -71,7 +72,7 @@ object Serializer:
                       |${stmt.symbolName}
                       |${stmt.treeName}
                       |${stmt.branch}
-                      |${stmt.count}
+                      |0
                       |${stmt.ignored}
                       |${stmt.desc}
                       |\f

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

@@ -60,27 +60,13 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
 
   /** Transforms trees to insert calls to Invoker.invoked to compute the coverage when the code is called */
   private class CoverageTransformer 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 ctx: Context): Tree =
       inContext(transformCtx(tree)) { // necessary to position inlined code properly
         tree match
           // simple cases
-          case tree: (Import | Export | This | Super | New) => tree
+          case tree: (Import | Export | Literal | 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)(
@@ -92,32 +78,32 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
             cpy.Try(tree)(
               expr = instrument(transform(tree.expr), branch = true),
               cases = instrumentCases(tree.cases),
-              finalizer = instrument(transform(tree.finalizer), true)
+              finalizer = instrument(transform(tree.finalizer), branch = 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 canInstrumentApply(tree) then
-              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)
+                instrumentLifted(transformed)
               else
-                val transformed = cpy.Apply(tree)(transformedFun, transform(args)(using ignoreLiteralsContext))
-                instrument(transformed)(using ignoreLiteralsContext)
+                val transformed = transformApply(tree, transformedFun)
+                instrument(transformed)
             else
-              tree
+              transformApply(tree, transformedFun)
 
           // f(args)
           case tree: Apply =>
             if canInstrumentApply(tree) then
               if needsLift(tree) then
-                instrumentLifted(tree)(using ignoreLiteralsContext) // see comment about Literals
+                instrumentLifted(tree)
               else
-                instrument(super.transform(tree)(using ignoreLiteralsContext))
+                instrument(transformApply(tree))
             else
-              tree
+              transformApply(tree)
 
           // (f(x))[args]
           case TypeApply(fun: Apply, args) =>
@@ -142,15 +128,19 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
           case tree: ValDef =>
             // only transform the rhs
             val rhs = transform(tree.rhs)
-            cpy.ValDef(tree)(rhs=rhs)
+            cpy.ValDef(tree)(rhs = rhs)
 
           case tree: DefDef =>
-            // only transform the params (for the default values) and the rhs
-            // force instrumentation of literals and other small trees in the rhs,
-            // to ensure that the method call are recorded
+            // Only transform the params (for the default values) and the rhs.
             val paramss = transformParamss(tree.paramss)
             val rhs = transform(tree.rhs)
-            cpy.DefDef(tree)(tree.name, paramss, tree.tpt, rhs)
+            val finalRhs =
+              if canInstrumentDefDef(tree) then
+                // Ensure that the rhs is always instrumented, if possible
+                instrumentBody(tree, rhs)
+              else
+                rhs
+            cpy.DefDef(tree)(tree.name, paramss, tree.tpt, finalRhs)
 
           case tree: PackageDef =>
             // only transform the statements of the package
@@ -168,7 +158,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
     /** Lifts and instruments an application.
       * Note that if only one arg needs to be lifted, we just lift everything.
       */
-    def instrumentLifted(tree: Apply)(using Context) =
+    private def instrumentLifted(tree: Apply)(using Context) =
       // lifting
       val buffer = mutable.ListBuffer[Tree]()
       val liftedApply = LiftCoverage.liftForCoverage(buffer, tree)
@@ -179,40 +169,93 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
       Block(
         instrumentedArgs,
         instrumentedApply
-      ).withSpan(instrumentedApply.span)
+      )
 
-    def instrumentCases(cases: List[CaseDef])(using Context): List[CaseDef] =
-      cases.map(instrumentCaseDef)
+    private inline def transformApply(tree: Apply)(using Context): Apply =
+      transformApply(tree, transform(tree.fun))
 
-    def instrumentCaseDef(tree: CaseDef)(using Context): CaseDef =
-      cpy.CaseDef(tree)(tree.pat, transform(tree.guard), transform(tree.body))
+    private inline def transformApply(tree: Apply, transformedFun: Tree)(using Context): Apply =
+      cpy.Apply(tree)(transformedFun, transform(tree.args))
 
-    def instrument(tree: Tree, branch: Boolean = false)(using Context): Tree =
+    private inline def instrumentCases(cases: List[CaseDef])(using Context): List[CaseDef] =
+      cases.map(instrumentCaseDef)
+
+    private def instrumentCaseDef(tree: CaseDef)(using Context): CaseDef =
+      val pat = tree.pat
+      val guard = tree.guard
+      val friendlyEnd = if guard.span.exists then guard.span.end else pat.span.end
+      val pos = tree.sourcePos.withSpan(tree.span.withEnd(friendlyEnd)) // user-friendly span
+      // ensure that the body is always instrumented by inserting a call to Invoker.invoked at its beginning
+      val instrumentedBody = instrument(transform(tree.body), pos, false)
+      cpy.CaseDef(tree)(tree.pat, transform(tree.guard), instrumentedBody)
+
+    /** Records information about a new coverable statement. Generates a unique id for it.
+      * @return the statement's id
+      */
+    private def recordStatement(tree: Tree, pos: SourcePosition, branch: Boolean)(using ctx: Context): Int =
+      val id = statementId
+      statementId += 1
+      val statement = new Statement(
+        source = ctx.source.file.name,
+        location = Location(tree),
+        id = id,
+        start = pos.start,
+        end = pos.end,
+        line = pos.line,
+        desc = tree.source.content.slice(pos.start, pos.end).mkString,
+        symbolName = tree.symbol.name.toSimpleName.toString,
+        treeName = tree.getClass.getSimpleName.nn,
+        branch
+      )
+      coverage.addStatement(statement)
+      id
+
+    private inline def syntheticSpan(pos: SourcePosition): Span = pos.span.toSynthetic
+
+    /** Shortcut for instrument(tree, tree.sourcePos, branch) */
+    private inline 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 =
+    /** Instruments a statement, if it has a position. */
+    private def instrument(tree: Tree, pos: SourcePosition, branch: Boolean)(using Context): Tree =
       if pos.exists && !pos.span.isZeroExtent then
-        statementId += 1
-        val id = statementId
-        val statement = new Statement(
-          source = ctx.source.file.name,
-          location = Location(tree),
-          id = id,
-          start = pos.start,
-          end = pos.end,
-          line = pos.line,
-          desc = tree.source.content.slice(pos.start, pos.end).mkString,
-          symbolName = tree.symbol.name.toSimpleName.toString(),
-          treeName = tree.getClass.getSimpleName.nn,
-          branch
-        )
-        coverage.addStatement(statement)
-        val span = Span(pos.start, pos.end) // synthetic span
-        Block(List(invokeCall(id, span)), tree).withSpan(span)
+        val statementId = recordStatement(tree, pos, branch)
+        insertInvokeCall(tree, pos, statementId)
       else
         tree
 
-    def invokeCall(id: Int, span: Span)(using Context): Tree =
+    /** Instruments the body of a DefDef. Handles corner cases. */
+    private def instrumentBody(parent: DefDef, body: Tree)(using Context): Tree =
+      /* recurse on closures, so that we insert the call at the leaf:
+
+         def g: (a: Ta) ?=> (b: Tb) = {
+           // nothing here             <-- not here!
+           def $anonfun(using a: Ta) =
+             Invoked.invoked(id, DIR)  <-- here
+             <userCode>
+           closure($anonfun)
+         }
+      */
+      body match
+        case b @ Block((meth: DefDef) :: Nil, closure: Closure)
+        if meth.symbol == closure.meth.symbol && defn.isContextFunctionType(body.tpe) =>
+          val instr = cpy.DefDef(meth)(rhs = instrumentBody(parent, meth.rhs))
+          cpy.Block(b)(instr :: Nil, closure)
+        case _ =>
+          // compute user-friendly position to highlight more text in the coverage UI
+          val namePos = parent.namePos
+          val pos = namePos.withSpan(namePos.span.withStart(parent.span.start))
+          // record info and insert call to Invoker.invoked
+          val statementId = recordStatement(parent, pos, false)
+          insertInvokeCall(body, pos, statementId)
+
+    /** Returns the tree, prepended by a call to Invoker.invoker */
+    private def insertInvokeCall(tree: Tree, pos: SourcePosition, statementId: Int)(using Context): Tree =
+      val callSpan = syntheticSpan(pos)
+      Block(invokeCall(statementId, callSpan) :: Nil, tree).withSpan(callSpan.union(tree.span))
+
+    /** Generates Invoked.invoked(id, DIR) */
+    private def invokeCall(id: Int, span: Span)(using Context): Tree =
       val outputPath = ctx.settings.coverageOutputDir.value
       ref(defn.InvokedMethodRef).withSpan(span)
         .appliedToArgs(
@@ -229,7 +272,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
      * ```
      * should not be changed to {val $x = f(); T($x)}(1) but to {val $x = f(); val $y = 1; T($x)($y)}
      */
-    def needsLift(tree: Apply)(using Context): Boolean =
+    private def needsLift(tree: Apply)(using Context): Boolean =
       def isBooleanOperator(fun: Tree) =
         // We don't want to lift a || getB(), to avoid calling getB if a is true.
         // Same idea with a && getB(): if a is false, getB shouldn't be called.
@@ -249,9 +292,17 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
       nestedApplyNeedsLift ||
       !isBooleanOperator(fun) && !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
 
-    def canInstrumentApply(tree: Apply)(using Context): Boolean =
-      val tpe = tree.typeOpt
-      tpe match
+    /** Check if the body of a DefDef can be instrumented with instrumentBody. */
+    private def canInstrumentDefDef(tree: DefDef)(using Context): Boolean =
+      // No need to force the instrumentation of synthetic definitions
+      // (it would work, but it looks better without).
+      !tree.symbol.isOneOf(Accessor | Synthetic | Artifact) &&
+      !tree.rhs.isEmpty
+
+    /** Check if an Apply can be instrumented. Prevents this phase from generating incorrect code. */
+    private def canInstrumentApply(tree: Apply)(using Context): Boolean =
+      !tree.symbol.isOneOf(Synthetic | Artifact) && // no need to instrument synthetic apply
+      (tree.typeOpt match
         case AppliedType(tycon: NamedType, _) =>
           /* If the last expression in a block is a context function, we'll try to
              summon its arguments at the current point, even if the expected type
@@ -267,11 +318,15 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
           */
           !tycon.name.isContextFunction
         case m: MethodType =>
-          // def f(a: Ta)(b: Tb)
-          // f(a)(b) cannot be rewritten to {invoked();f(a)}(b)
+          /* def f(a: Ta)(b: Tb)
+             f(a)(b)
+
+             Here, f(a)(b) cannot be rewritten to {invoked();f(a)}(b)
+          */
           false
         case _ =>
           true
+      )
 
 object InstrumentCoverage:
   val name: String = "instrumentCoverage"

docs/_docs/usage/coverage.md

@@ -15,7 +15,8 @@ In Scala 2, all these steps were performed by external tools. In particular, ste
 
 In Scala 3, the compiler itself takes care of step 1. To use this feature, add the compile option `-coverage-out:DIR`, where `DIR` is the destination of the measurement files.
 
-You can also set `-coverage-sourceroot:PATHS_ROOT` to customize how the path of your source files are resolved.
+You can also set `-sourceroot:PATHS_ROOT` to customize how the path of your source files are resolved.
+Note that `-sourceroot` also sets the root path of the SemanticDB files.
 
 ## How-to with sbt