Fix #25624: Scoverage and separation checking failures (#25625)

Fixes #25624

Separation checking for captures relies on `unsafeAssumeSeparate` to opt
out of separation checking. To detect such code, `SepCheck` uses
structural matching on the trees. Coverage phase may lift some trees,
breaking the assumptions about the structure.

This PR:
- Opts `unsafeAssumeSeparate` out of coverage instrumentation - we
aren't really interested in coverage info on it anyway, only the body is
interesting
- On lifting time, marks coverage-lifted synthetic trees with an
explicit attachment
- On `SepCheck` time, opts-out synthetic trees created by coverage from
separation checking - rationalle being we have already checked their rhs
for separation on assignment time

## How much have you relied on LLM-based tools in this contribution?

Moderately.

## How was the solution tested?

- Ran the full bootstrapped `testCompilation --enable-coverage-phase`
suite
- Run the compiler `pos` suite
- Reproduced failing tests individually before and after the fix

## Additional notes

I am not familiar with the captures codebase, so can use help of someone
familiar with it to review this PR.

Author: anatoliykmetyuk

compiler/src/dotty/tools/dotc/cc/SepCheck.scala

@@ -10,6 +10,7 @@ import CaptureSet.{Refs, emptyRefs, HiddenSet}
 import NameKinds.WildcardParamName
 import config.Printers.capt
 import StdNames.nme
+import transform.LiftCoverage
 import util.{SimpleIdentitySet, EqHashMap, SrcPos}
 import tpd.*
 import reflect.ClassTag
@@ -599,13 +600,22 @@ class SepCheck(checker: CheckCaptures.CheckerAPI) extends tpd.TreeTraverser:
       case _ =>
   end checkAssign
 
+  /** Is `tree` a coverage-lifted local temp or a reference to one?
+   *  These aliases are identified through the attachment set by `LiftCoverage`,
+   *  not by broad synthetic checks.
+   */
+  private def isCoverageLiftedTemp(tree: Tree)(using Context): Boolean = tree match
+    case tree: ValDef => tree.symbol.exists && LiftCoverage.isCoverageLiftedTemp(tree.symbol)
+    case tree: Ident => tree.symbol.exists && LiftCoverage.isCoverageLiftedTemp(tree.symbol)
+    case _ => false
+
   /** 1. Check that the capabilities used at `tree` don't overlap with
    *     capabilities hidden by a previous definition.
    *  2. Also check that none of the used capabilities was consumed before.
    */
   def checkUse(tree: Tree)(using Context): Unit =
     val used = tree.markedFree.elems
-    if !used.isEmpty then
+    if !used.isEmpty && !isCoverageLiftedTemp(tree) then
       capt.println(i"check use $tree: $used")
       val usedPeaks = used.allPeaks
       if !defsShadow.allPeaks.sharedPeaks(usedPeaks).isEmpty then
@@ -994,13 +1004,16 @@ class SepCheck(checker: CheckCaptures.CheckerAPI) extends tpd.TreeTraverser:
   /** Check (result-) type of `tree` for separation conditions using `checkType`.
    *  Excluded are parameters and definitions that have an =unsafeAssumeSeparate
    *  application as right hand sides.
+   *  Also excluded are local temps marked by `LiftCoverage`, which are aliases
+   *  introduced solely to preserve coverage evaluation order.
    *  Hidden sets of checked definitions are added to `defsShadow`.
    */
   def checkValOrDefDef(tree: ValOrDefDef)(using Context): Unit =
     val sym = tree.symbol
     if !sym.isOneOf(TermParamOrAccessor)
        && !sym.needsResultRefinement
        && !isUnsafeAssumeSeparate(tree.rhs)
+       && !isCoverageLiftedTemp(tree)
     then
       checkType(tree.tpt, sym)
       capt.println(i"sep check def $sym: ${tree.tpt} with ${spanCaptures(tree.tpt).transHiddenSet.directFootprint}")

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

@@ -4,6 +4,7 @@ package transform
 import java.io.File
 import java.nio.file.{Files, Path}
 
+import ast.tpd
 import ast.tpd.*
 import collection.mutable
 import core.Comments.Comment
@@ -18,14 +19,75 @@ import core.StdNames.nme
 import core.Types.*
 import core.Decorators.*
 import coverage.*
-import typer.LiftCoverage
-import util.{SourcePosition, SourceFile}
+import typer.LiftImpure
+import util.{Property, SourcePosition, SourceFile}
 import util.Spans.Span
 import localopt.StringInterpolatorOpt
 import inlines.Inlines
 import scala.util.matching.Regex
 import java.util.regex.Pattern
 
+/** Lift impure + lift the prefixes for coverage instrumentation. */
+object LiftCoverage extends LiftImpure:
+
+  // Property indicating whether we're currently lifting the arguments of an application
+  private val LiftingArgs = new Property.Key[Boolean]
+  val CoverageLiftedTemp = Property.StickyKey[Unit]()
+
+  private inline def liftingArgs(using Context): Boolean =
+    ctx.property(LiftingArgs).contains(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
+   */
+  private def noLiftArg(arg: tpd.Tree)(using Context): Boolean =
+    arg match
+      case arg if isUnsafeAssumeSeparate(arg) => true
+      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)
+
+  def isUnsafeAssumeSeparate(tree: tpd.Tree)(using Context): Boolean = tree match
+    case tree: tpd.Apply =>
+      tree.symbol == defn.Caps_unsafeAssumeSeparate
+      || isUnsafeAssumeSeparate(tree.fun)
+    case tpd.Select(qual, _) => isUnsafeAssumeSeparate(qual)
+    case tpd.Block(_, expr) => isUnsafeAssumeSeparate(expr)
+    case tpd.Inlined(_, _, expr) => isUnsafeAssumeSeparate(expr)
+    case tpd.Typed(expr, _) => isUnsafeAssumeSeparate(expr)
+    case _ => false
+
+  def isCoverageLiftedTemp(sym: Symbol)(using Context): Boolean =
+    sym.defTree.hasAttachment(CoverageLiftedTemp)
+
+  override protected def onLiftedDef(tree: tpd.Tree)(using Context): Unit =
+    tree.putAttachment(CoverageLiftedTemp, ())
+
+  override def noLift(expr: tpd.Tree)(using Context) =
+    if liftingArgs then noLiftArg(expr)
+    else isUnsafeAssumeSeparate(expr) || super.noLift(expr)
+
+  /** Preserve singleton precision for lifted coverage temps when the underlying value is a
+   *  compile-time constant (same notion ConstFold uses), so constant re-folding after lifting
+   *  still matches the original inferred singleton type. Everything else uses the base widen.
+   */
+  override protected def liftedExprType(expr: tpd.Tree)(using Context): Type =
+    val dealiased = expr.tpe.dealias.deskolemized
+    dealiased.widenTermRefExpr.normalized.simplified match
+      case _: ConstantType => dealiased
+      case _ => super.liftedExprType(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 liftingArgsContext)
+    tpd.cpy.Apply(tree)(liftedFun, liftedArgs)
+
 /** 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.
@@ -261,7 +323,10 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
       * @return instrumentation result, with the preparation statement, coverage call and tree separated
       */
     private def tryInstrument(tree: Apply)(using Context): InstrumentedParts =
-      if canInstrumentApply(tree) then
+      if LiftCoverage.isUnsafeAssumeSeparate(tree) then
+        val transformed = cpy.Apply(tree)(transformInnerApply(tree.fun), transformApplyArgs(tree.args, erasedParamStatuses(tree)))
+        InstrumentedParts.notCovered(transformed)
+      else if canInstrumentApply(tree) then
         // Create a call to Invoker.invoked(coverageDirectory, newStatementId)
         val coverageCall = createInvokeCall(tree, tree.sourcePos)
 
@@ -501,7 +566,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
             tree.rhs
           else if sym.isClassConstructor then
             instrumentSecondaryCtor(tree)
-          else if !sym.isOneOf(Accessor | Artifact | Synthetic) then
+          else if !sym.isOneOf(Accessor | Artifact | Synthetic)
+               && !LiftCoverage.isUnsafeAssumeSeparate(tree.rhs)
+          then
             // If the body can be instrumented, do it (i.e. insert a "coverage call" at the beginning)
             // This is useful because methods can be stored and called later, or called by reflection,
             // and if the rhs is too simple to be instrumented (like `def f = this`),

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

@@ -11,7 +11,6 @@ import Symbols.*
 import Names.*
 import NameKinds.UniqueName
 import util.Spans.*
-import util.Property
 import collection.mutable
 import Trees.*
 
@@ -52,6 +51,9 @@ abstract class Lifter {
   protected def liftedExprType(expr: Tree)(using Context): Type =
     expr.tpe.widen.deskolemized
 
+  /** Hook for lifters that need to record or mark freshly created lifted defs. */
+  protected def onLiftedDef(tree: Tree)(using Context): Unit = ()
+
   private def lift(defs: mutable.ListBuffer[Tree], expr: Tree, prefix: TermName = EmptyTermName)(using Context): Tree =
     if (noLift(expr)) expr
     else {
@@ -63,10 +65,12 @@ abstract class Lifter {
         // Lifted definitions will be added to a local block, so they need to be
         // at a higher nesting level to prevent leaks. See tests/pos/i15174.scala
         nestingLevel = ctx.nestingLevel + 1)
-      defs += liftedDef(lifted, expr)
+      val liftedTree = liftedDef(lifted, expr)
         .withSpan(expr.span)
         .changeNonLocalOwners(lifted)
         .setDefTree
+      onLiftedDef(liftedTree)
+      defs += liftedTree
       ref(lifted.termRef).withSpan(expr.span.focus)
     }
 
@@ -187,53 +191,6 @@ class LiftComplex extends Lifter {
 }
 object LiftComplex extends LiftComplex
 
-/** Lift impure + lift the prefixes */
-object LiftCoverage extends LiftImpure {
-
-  // Property indicating whether we're currently lifting the arguments of an application
-  private val LiftingArgs = new Property.Key[Boolean]
-
-  private inline def liftingArgs(using Context): Boolean =
-    ctx.property(LiftingArgs).contains(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) =
-    if liftingArgs then noLiftArg(expr) else super.noLift(expr)
-
-  /** Preserve singleton precision for lifted coverage temps when the underlying value is a
-   *  compile-time constant (same notion ConstFold uses), so constant re-folding after lifting
-   *  still matches the original inferred singleton type. Everything else uses the base widen.
-   */
-  override protected def liftedExprType(expr: tpd.Tree)(using Context): Type =
-    val dealiased = expr.tpe.dealias.deskolemized
-    dealiased.widenTermRefExpr.normalized.simplified match
-      case _: ConstantType => dealiased
-      case _ => super.liftedExprType(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 liftingArgsContext)
-    tpd.cpy.Apply(tree)(liftedFun, liftedArgs)
-  }
-}
-
 /** Lifter for eta expansion */
 object EtaExpansion extends LiftImpure {
   import tpd.*

compiler/test/dotc/scoverage-ignore.excludelist

@@ -8,7 +8,6 @@
 applied_constructor_types.scala
 capt1.scala
 capture.scala
-colltest5
 gadt-ycheck.scala
 help.scala
 i10889.scala
@@ -35,12 +34,10 @@ i8900a3.scala
 i9228.scala
 lazyVals_c3.0.0.scala
 lazyVals_c3.1.0.scala
-minicheck.scala
 minicheck-toplevel.scala
 mt-scrutinee-widen3.scala
 null.scala
 pos_valueclasses
-skolems2.scala
 spurious-overload.scala
 tailrec.scala
 traitParams.scala