Skip to content

Commit 6c29374

Browse files
Scoverage: instrument chained calls correctly (#26166)
Fixes #26088 ## Root Issue The issue in question is actually a manifestation of a larger issue: ```scala addMode(foo()) ``` Will correctly get instrumented by coverage to roughly: ```scala val $1$: T = { Invoker.invoked("foo"); foo() } Invoker.invoked("addMode") addMode($1$) ``` But: ```scala addMode(foo()).setA(...) ``` Will incorrectly get instrumented to roughly: ```scala val $1$ = addMode(foo()) Invoker.invoked("setA") $1.setA(...) ``` So, in case of chained calls `foo(x).bar(y).baz(z)`, only the last call `baz(z)` will get proper instrumentation that respects argument evaluation order. ## Reported Issue Impact Consider code: ```scala class C { def setA(a: Int): this.type = this } def addMode(c: C): c.type = c val x = addMode(identity(new C()).setA(???)).setA(???) ``` Its line: ```scala val x = addMode(identity(new C()).setA(???)).setA(???) ``` Coverage instrumentation of that line: ```scala val $2$: (?1 : C) = addMode( { val $1$: C = identity[C]({ Invoker.invoked(...); new C() }) val a$1: Nothing = ??? Invoker.invoked(...) $1$.setA(a$1) } ) ``` `Ycheck` then fails because the result type is tied to the method parameter placeholder `?1`, while the block result is tied to the lifted local `$1$`: ```scala assertion failed: Type Mismatch Found: ($1$ : C) Required: (?1 : C) ``` ## Solution Ensure call chains such as `foo(x).bar(y).baz(z)` have all their calls lifted and instrumented properly to preserve evaluation order. In the case of the issue in question, this will have the following effect: ```scala val c$1: C = { val x$1: C = { Invoker.invoked(...); new C() } val $1$: C = identity[C](x$1) val a$1: Nothing = ??? Invoker.invoked(...) $1$.setA(a$1) } Invoker.invoked(...) val $2$: (c$1 : C) = addMode(c$1) ``` `c.type` is preserved in `addMode`, and `addMode(...)` is traced the same way in `addMode(...).setA(...)` as it is in `addMode(...)`. ## How much have you relied on LLM-based tools in this contribution? Moderately, for minimization, codebase analysis, tracing, test generation. ## How was the solution tested? New automated tests. Added `tests/pos-custom-args/captures/coverage-freshcontext-addmode.scala` and `tests/coverage/run/chained-apply/test.scala`. The coverage run test checks direct, single-chain, multi-chain, and receiver-dependent chained applications. ```bash sbt "testCompilation --enable-coverage-phase coverage-freshcontext-addmode" sbt "testCompilation coverage-freshcontext-addmode" sbt "testCompilation --coverage" ```
1 parent e2d1809 commit 6c29374

9 files changed

Lines changed: 803 additions & 7 deletions

File tree

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

Lines changed: 81 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import java.nio.file.{Files, Path}
66

77
import ast.tpd
88
import ast.tpd.*
9+
import ast.desugar.TrailingForMap
910
import collection.mutable
1011
import core.Comments.Comment
1112
import core.Flags.*
@@ -32,6 +33,7 @@ object LiftCoverage extends LiftImpure:
3233

3334
// Property indicating whether we're currently lifting the arguments of an application
3435
private val LiftingArgs = new Property.Key[Boolean]
36+
private val SelectedReceiverApply = Property.StickyKey[tpd.Apply]()
3537
val CoverageLiftedTemp = Property.StickyKey[Unit]()
3638

3739
private inline def liftingArgs(using Context): Boolean =
@@ -68,6 +70,12 @@ object LiftCoverage extends LiftImpure:
6870
def isCoverageLiftedTemp(sym: Symbol)(using Context): Boolean =
6971
sym.defTree.hasAttachment(CoverageLiftedTemp)
7072

73+
def selectedReceiverApply(tree: tpd.Tree)(using Context): Option[tpd.Apply] =
74+
tree.getAttachment(SelectedReceiverApply)
75+
76+
def markSelectedReceiverApply(tree: tpd.Apply)(using Context): Unit =
77+
tree.putAttachment(SelectedReceiverApply, tree)
78+
7179
override protected def onLiftedDef(tree: tpd.Tree)(using Context): Unit =
7280
tree.putAttachment(CoverageLiftedTemp, ())
7381

@@ -93,10 +101,33 @@ object LiftCoverage extends LiftImpure:
93101
case _ if valueType.existsPart(_.typeSymbol == defn.TypeBox_CAP) => valueType
94102
case _ => super.liftedExprType(expr)
95103

104+
private def markSelectedReceiverDef(
105+
defs: mutable.ListBuffer[tpd.Tree],
106+
from: Int,
107+
selectedReceiver: tpd.Apply
108+
)(using Context): Unit =
109+
if defs.length > from then
110+
defs.last match
111+
case stat: tpd.ValDef => stat.putAttachment(SelectedReceiverApply, selectedReceiver)
112+
case _ => ()
113+
96114
def liftForCoverage(defs: mutable.ListBuffer[tpd.Tree], tree: tpd.Apply)(using Context) =
97-
val liftedFun = liftApp(defs, tree.fun)
98-
val liftedArgs = liftArgs(defs, tree.fun.tpe, tree.args)(using liftingArgsContext)
99-
tpd.cpy.Apply(tree)(liftedFun, liftedArgs)
115+
def recur(tree: tpd.Apply): tpd.Tree =
116+
val liftedFun = tree.fun match
117+
case sel @ tpd.Select(app: tpd.Apply, name) if selectedReceiverApply(app).nonEmpty =>
118+
val selectedReceiver = tpd.cpy.Select(sel)(recur(app), name)
119+
val defsBeforeReceiver = defs.length
120+
val liftedReceiver = liftApp(defs, selectedReceiver)
121+
markSelectedReceiverDef(defs, defsBeforeReceiver, app)
122+
liftedReceiver
123+
case _ =>
124+
liftApp(defs, tree.fun)
125+
val liftedArgs = liftArgs(defs, tree.fun.tpe, tree.args)(using liftingArgsContext)
126+
tpd.cpy.Apply(tree)(liftedFun, liftedArgs)
127+
end recur
128+
129+
recur(tree)
130+
end liftForCoverage
100131

101132
/** Implements code coverage by inserting calls to scala.runtime.coverage.Invoker
102133
* ("instruments" the source code).
@@ -324,6 +355,15 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
324355

325356
private def allConstArgs(args: List[Tree]) =
326357
args.forall(arg => arg.isInstanceOf[Literal] || arg.isInstanceOf[Ident])
358+
359+
private def withSelectedReceiverProbes(stats: List[Tree])(using Context): List[Tree] =
360+
stats.flatMap: stat =>
361+
LiftCoverage.selectedReceiverApply(stat) match
362+
case Some(app) =>
363+
createInvokeCall(app, app.sourcePos) :: stat :: Nil
364+
case _ =>
365+
stat :: Nil
366+
327367
/**
328368
* Tries to instrument an `Apply`.
329369
* These "tryInstrument" methods are useful to tweak the generation of coverage instrumentation,
@@ -345,21 +385,25 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
345385
if tree.fun.symbol eq defn.throwMethod then tree
346386
else cpy.Apply(tree)(transformInnerApply(tree.fun), transformApplyArgs(tree.args, erasedParamStatuses(tree)))
347387

348-
if needsLift(tree) then
388+
if needsLift(app) then
349389
// Lifts the arguments. Note that if only one argument needs to be lifted, we lift them all.
350390
// Also, tree.fun can be lifted too.
351391
// See LiftCoverage for the internal working of this lifting.
352392
val liftedDefs = mutable.ListBuffer[Tree]()
353393
val liftedApp = LiftCoverage.liftForCoverage(liftedDefs, app)
394+
val prefix =
395+
if tree.hasAttachment(TrailingForMap) then liftedDefs.toList
396+
else withSelectedReceiverProbes(liftedDefs.toList)
354397

355-
InstrumentedParts(liftedDefs.toList, coverageCall, liftedApp)
398+
InstrumentedParts(prefix, coverageCall, liftedApp)
356399
else
357400
// Instrument without lifting
358401
InstrumentedParts.singleExpr(coverageCall, app)
359402
else
360403
// Transform recursively but don't instrument the tree itself
361404
val transformed = cpy.Apply(tree)(transformInnerApply(tree.fun), transformApplyArgs(tree.args, erasedParamStatuses(tree)))
362405
InstrumentedParts.notCovered(transformed)
406+
end tryInstrument
363407

364408
private def tryInstrument(tree: Ident)(using Context): InstrumentedParts =
365409
val sym = tree.symbol
@@ -730,6 +774,32 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
730774
* should not be changed to {val $x = f(); T($x)}(1) but to {val $x = f(); val $y = 1; T($x)($y)}
731775
*/
732776
private def needsLift(tree: Apply)(using Context): Boolean =
777+
def hasSelectedApply(fun: Tree): Boolean = fun match
778+
case Select(app: Apply, _) =>
779+
val nestedNeedsProbe = hasSelectedApply(app.fun)
780+
val needsProbe = selectedReceiverNeedsProbe(app)
781+
if needsProbe then LiftCoverage.markSelectedReceiverApply(app)
782+
nestedNeedsProbe || needsProbe
783+
case TypeApply(fn, _) => hasSelectedApply(fn)
784+
case _ => false
785+
end hasSelectedApply
786+
787+
def applicationEvaluationNeedsLift(tree: Apply): Boolean =
788+
val fun = tree.fun
789+
val nestedApplyNeedsLift = fun match
790+
case a: Apply => applicationEvaluationNeedsLift(a)
791+
case _ => false
792+
793+
nestedApplyNeedsLift ||
794+
!isUnliftableFun(fun) && !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
795+
end applicationEvaluationNeedsLift
796+
797+
def selectedReceiverNeedsProbe(tree: Apply): Boolean =
798+
!LiftCoverage.isUnsafeAssumeSeparate(tree)
799+
&& canInstrumentApply(tree)
800+
&& applicationEvaluationNeedsLift(tree)
801+
end selectedReceiverNeedsProbe
802+
733803
def isShortCircuitedOp(sym: Symbol) =
734804
sym == defn.Boolean_&& || sym == defn.Boolean_||
735805

@@ -757,7 +827,12 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
757827
case _ => false
758828

759829
nestedApplyNeedsLift ||
760-
!isUnliftableFun(fun) && !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
830+
!isUnliftableFun(fun)
831+
&& (
832+
!tree.hasAttachment(TrailingForMap) && hasSelectedApply(fun)
833+
|| !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
834+
)
835+
end needsLift
761836

762837
private def isContextFunctionApply(fun: Tree)(using Context): Boolean =
763838
fun match

tests/coverage/pos/DefaultArgs.scoverage.check

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,40 @@ DefaultArgs
399399
Object
400400
covtest.DefaultArgs
401401
staticCaller
402+
706
403+
720
404+
28
405+
staticMethod
406+
Apply
407+
false
408+
0
409+
false
410+
staticMethod()
411+
412+
23
413+
DefaultArgs.scala
414+
covtest
415+
DefaultArgs
416+
Object
417+
covtest.DefaultArgs
418+
staticCaller
419+
706
420+
738
421+
28
422+
+
423+
Apply
424+
false
425+
0
426+
false
427+
staticMethod() + staticMethod(5)
428+
429+
24
430+
DefaultArgs.scala
431+
covtest
432+
DefaultArgs
433+
Object
434+
covtest.DefaultArgs
435+
staticCaller
402436
676
403437
692
404438
27

tests/coverage/pos/i21695/test.scoverage.check

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,23 @@ A
5959
Trait
6060
example.A
6161
create
62+
94
63+
111
64+
8
65+
addService
66+
Apply
67+
false
68+
0
69+
false
70+
x1.addService(x2)
71+
72+
3
73+
i21695/A.scala
74+
example
75+
A
76+
Trait
77+
example.A
78+
create
6279
69
6380
79
6481
7
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
foo
2+
addMode
3+
foo
4+
addMode
5+
bar
6+
foo
7+
addMode
8+
argBar
9+
memberAddMode
10+
bar
11+
depFoo
12+
depAddMode
13+
dep.bar
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
32
2+
20
3+
8
4+
6
5+
7
6+
19
7+
13
8+
12
9+
22
10+
23
11+
21
12+
3
13+
2
14+
25
15+
27
16+
26
17+
11
18+
9
19+
10
20+
28
21+
1
22+
0
23+
24
24+
30
25+
16
26+
14
27+
15
28+
31
29+
18
30+
17
31+
29
32+
5
33+
4
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
class C:
2+
def addMode(c: C): C =
3+
println("memberAddMode")
4+
this
5+
6+
def bar(): C =
7+
println("bar")
8+
this
9+
10+
class D:
11+
def bar(): this.type =
12+
println("dep.bar")
13+
this
14+
15+
def foo(): C =
16+
println("foo")
17+
C()
18+
19+
def bar(): C =
20+
println("argBar")
21+
C()
22+
23+
def addMode(c: C): C =
24+
println("addMode")
25+
c
26+
27+
def depFoo(): D =
28+
println("depFoo")
29+
D()
30+
31+
def depAddMode(d: D): d.type =
32+
println("depAddMode")
33+
d
34+
35+
@main
36+
def Test: Unit =
37+
addMode(foo())
38+
addMode(foo()).bar()
39+
addMode(foo()).addMode(bar()).bar()
40+
depAddMode(depFoo()).bar()

0 commit comments

Comments
 (0)