Merge pull request #15530 from TheElectronWill/fix-coverage-1

Fix lifting of arguments with -coverage-out

Author: smarter

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

@@ -124,17 +124,21 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
       tp
 
   override def transformApply(tree: Apply)(using Context): Tree =
-    val args = tree.args.mapConserve {
-      case arg: Typed if isWildcardStarArg(arg) =>
+    val args = tree.args.mapConserve { arg =>
+      if isWildcardStarArg(arg) then
+        val expr = arg match
+          case t: Typed => t.expr
+          case _ => arg // if the argument has been lifted it's not a Typed (often it's an Ident)
+
         val isJavaDefined = tree.fun.symbol.is(JavaDefined)
-        val tpe = arg.expr.tpe
         if isJavaDefined then
-          adaptToArray(arg.expr)
-        else if tpe.derivesFrom(defn.ArrayClass) then
-          arrayToSeq(arg.expr)
+          adaptToArray(expr)
+        else if expr.tpe.derivesFrom(defn.ArrayClass) then
+          arrayToSeq(expr)
         else
-          arg.expr
-      case arg => arg
+          expr
+      else
+        arg
     }
     cpy.Apply(tree)(tree.fun, args)
 
@@ -287,9 +291,9 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
     val array = tp.translateFromRepeated(toArray = true) // Array[? <: T]
     val element = array.elemType.hiBound // T
 
-
     if element <:< defn.AnyRefType
       || ctx.mode.is(Mode.SafeNulls) && element.stripNull <:< defn.AnyRefType
-      || element.typeSymbol.isPrimitiveValueClass then array
+      || element.typeSymbol.isPrimitiveValueClass
+    then array
     else defn.ArrayOf(TypeBounds.upper(AndType(element, defn.AnyRefType))) // Array[? <: T & AnyRef]
 }

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

@@ -4,6 +4,7 @@ package transform
 import java.io.File
 import java.util.concurrent.atomic.AtomicInteger
 
+import ast.tpd.*
 import collection.mutable
 import core.Flags.*
 import core.Contexts.{Context, ctx, inContext}
@@ -17,13 +18,13 @@ import typer.LiftCoverage
 import util.{SourcePosition, Property}
 import util.Spans.Span
 import coverage.*
+import localopt.StringInterpolatorOpt.isCompilerIntrinsic
 
 /** Implements code coverage by inserting calls to scala.runtime.coverage.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 = InstrumentCoverage.name
 
@@ -60,7 +61,7 @@ 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:
-    override def transform(tree: Tree)(using ctx: Context): Tree =
+    override def transform(tree: Tree)(using Context): Tree =
       inContext(transformCtx(tree)) { // necessary to position inlined code properly
         tree match
           // simple cases
@@ -278,24 +279,29 @@ 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)}
      */
     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.
-        val sym = fun.symbol
-        sym.exists &&
+      def isShortCircuitedOp(sym: Symbol) =
         sym == defn.Boolean_&& || sym == defn.Boolean_||
 
-      def isContextual(fun: Apply): Boolean =
-        val args = fun.args
-        args.nonEmpty && args.head.symbol.isAllOf(GivenOrImplicit)
+      def isUnliftableFun(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.
+         *
+         * On top of that, the `s`, `f` and `raw` string interpolators are special-cased
+         * by the compiler and will disappear in phase StringInterpolatorOpt, therefore
+         * they shouldn't be lifted.
+         */
+        val sym = fun.symbol
+        sym.exists && (isShortCircuitedOp(sym) || isCompilerIntrinsic(sym))
+      end
 
       val fun = tree.fun
       val nestedApplyNeedsLift = fun match
         case a: Apply => needsLift(a)
         case _ => false
 
       nestedApplyNeedsLift ||
-      !isBooleanOperator(fun) && !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
+      !isUnliftableFun(fun) && !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
 
     /** Check if the body of a DefDef can be instrumented with instrumentBody. */
     private def canInstrumentDefDef(tree: DefDef)(using Context): Boolean =

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

@@ -31,7 +31,7 @@ class StringInterpolatorOpt extends MiniPhase:
     tree match
       case tree: RefTree =>
         val sym = tree.symbol
-        assert(sym != defn.StringContext_raw && sym != defn.StringContext_s && sym != defn.StringContext_f,
+        assert(!StringInterpolatorOpt.isCompilerIntrinsic(sym),
           i"$tree in ${ctx.owner.showLocated} should have been rewritten by phase $phaseName")
       case _ =>
 
@@ -162,3 +162,9 @@ class StringInterpolatorOpt extends MiniPhase:
 object StringInterpolatorOpt:
   val name: String = "interpolators"
   val description: String = "optimize s, f, and raw string interpolators"
+
+  /** Is this symbol one of the s, f or raw string interpolator? */
+  def isCompilerIntrinsic(sym: Symbol)(using Context): Boolean =
+    sym == defn.StringContext_s ||
+    sym == defn.StringContext_f ||
+    sym == defn.StringContext_raw

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

@@ -159,23 +159,39 @@ class LiftComplex extends Lifter {
 }
 object LiftComplex extends LiftComplex
 
-/** Lift complex + lift the prefixes */
-object LiftCoverage extends LiftComplex {
+/** Lift impure + lift the prefixes */
+object LiftCoverage extends LiftImpure {
 
-  private val LiftEverything = new Property.Key[Boolean]
+  // Property indicating whether we're currently lifting the arguments of an application
+  private val LiftingArgs = new Property.Key[Boolean]
 
-  private inline def liftEverything(using Context): Boolean =
-    ctx.property(LiftEverything).contains(true)
+  private inline def liftingArgs(using Context): Boolean =
+    ctx.property(LiftingArgs).contains(true)
 
-  private def liftEverythingContext(using Context): Context =
-    ctx.fresh.setProperty(LiftEverything, true)
+  private def liftingArgsContext(using Context): Context =
+    ctx.fresh.setProperty(LiftingArgs, true)
+
+  /** Variant of `noLift` for the arguments of applications.
+   *  To produce the right coverage information (especially in case of exceptions), we must lift:
+   *  - all the applications, except the erased ones
+   *  - all the impure arguments
+   *
+   * There's no need to lift the other arguments.
+   */
+  private def noLiftArg(arg: tpd.Tree)(using Context): Boolean =
+    arg match
+      case a: tpd.Apply => a.symbol.is(Erased) // don't lift erased applications, but lift all others
+      case tpd.Block(stats, expr) => stats.forall(noLiftArg) && noLiftArg(expr)
+      case tpd.Inlined(_, bindings, expr) => noLiftArg(expr)
+      case tpd.Typed(expr, _) => noLiftArg(expr)
+      case _ => super.noLift(arg)
 
   override def noLift(expr: tpd.Tree)(using Context) =
-    !liftEverything && super.noLift(expr)
+    if liftingArgs then noLiftArg(expr) else super.noLift(expr)
 
   def liftForCoverage(defs: mutable.ListBuffer[tpd.Tree], tree: tpd.Apply)(using Context) = {
     val liftedFun = liftApp(defs, tree.fun)
-    val liftedArgs = liftArgs(defs, tree.fun.tpe, tree.args)(using liftEverythingContext)
+    val liftedArgs = liftArgs(defs, tree.fun.tpe, tree.args)(using liftingArgsContext)
     tpd.cpy.Apply(tree)(liftedFun, liftedArgs)
   }
 }

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

@@ -69,10 +69,11 @@ class CoverageTests:
   def computeCoverageInTmp(inputFile: Path, 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)
-    val test = compileFile(inputFile.toString, options)
     if run then
+      val test = compileDir(inputFile.getParent.toString, options)
       test.checkRuns()
     else
+      val test = compileFile(inputFile.toString, options)
       test.checkCompile()
     target
 

tests/coverage/pos/ContextFunctions.scoverage.check

@@ -92,16 +92,16 @@ covtest
 Imperative
 Class
 covtest.Imperative
-$anonfun
-267
-294
+readPerson
+252
+295
 13
 invoked
 Apply
 false
 0
 false
-readName2(using e)(using s)
+OnError((e) => readName2(using e)(using s))
 
 5
 ContextFunctions.scala
@@ -113,7 +113,7 @@ $anonfun
 267
 294
 13
-apply
+invoked
 Apply
 false
 0
@@ -126,16 +126,16 @@ covtest
 Imperative
 Class
 covtest.Imperative
-readPerson
-252
-295
+$anonfun
+267
+294
 13
-invoked
+apply
 Apply
 false
 0
 false
-OnError((e) => readName2(using e)(using s))
+readName2(using e)(using s)
 
 7
 ContextFunctions.scala

tests/coverage/run/erased/test.check

@@ -0,0 +1,3 @@
+foo(a)(b)
+identity(idem)
+foo(a)(idem)

tests/coverage/run/erased/test.scala

@@ -0,0 +1,15 @@
+import scala.language.experimental.erasedDefinitions
+
+erased def e(x: String): String = "x"
+def foo(erased a: String)(b: String): String =
+  println(s"foo(a)($b)")
+  b
+
+def identity(s: String): String =
+  println(s"identity($s)")
+  s
+
+@main
+def Test: Unit =
+  foo(e("a"))("b")
+  foo(e("a"))(identity("idem"))