Skip to content

Make coverage more similar to the one in Scala 2 #23722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 54 additions & 18 deletions compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,29 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
val span = pos.span.toSynthetic
invokeCall(statementId, span)

private def transformApplyArgs(trees: List[Tree])(using Context): List[Tree] =
if (allConstArgs(trees)) trees else transform(trees)

private def transformInnerApply(tree: Tree)(using Context): Tree = tree match
case a: Apply if a.fun.symbol == defn.StringContextModule_apply =>
a
case a: Apply =>
cpy.Apply(a)(
transformInnerApply(a.fun),
transformApplyArgs(a.args)
)
case a: TypeApply =>
cpy.TypeApply(a)(
transformInnerApply(a.fun),
transformApplyArgs(a.args)
)
case s: Select =>
cpy.Select(s)(transformInnerApply(s.qualifier), s.name)
case i: (Ident | This) => i
case other => transform(other)

private def allConstArgs(args: List[Tree]) =
args.forall(arg => arg.isInstanceOf[Literal] || arg.isInstanceOf[Ident])
/**
* Tries to instrument an `Apply`.
* These "tryInstrument" methods are useful to tweak the generation of coverage instrumentation,
Expand All @@ -158,10 +181,12 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
// Create a call to Invoker.invoked(coverageDirectory, newStatementId)
val coverageCall = createInvokeCall(tree, tree.sourcePos)

if needsLift(tree) then
// Transform args and fun, i.e. instrument them if needed (and if possible)
val app = cpy.Apply(tree)(transform(tree.fun), tree.args.map(transform))
// Transform args and fun, i.e. instrument them if needed (and if possible)
val app =
if (tree.fun.symbol eq defn.throwMethod) tree
else cpy.Apply(tree)(transformInnerApply(tree.fun), transformApplyArgs(tree.args))

if needsLift(tree) then
// Lifts the arguments. Note that if only one argument needs to be lifted, we lift them all.
// Also, tree.fun can be lifted too.
// See LiftCoverage for the internal working of this lifting.
Expand All @@ -171,11 +196,10 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
InstrumentedParts(liftedDefs.toList, coverageCall, liftedApp)
else
// Instrument without lifting
val transformed = cpy.Apply(tree)(transform(tree.fun), transform(tree.args))
InstrumentedParts.singleExpr(coverageCall, transformed)
InstrumentedParts.singleExpr(coverageCall, app)
else
// Transform recursively but don't instrument the tree itself
val transformed = cpy.Apply(tree)(transform(tree.fun), transform(tree.args))
val transformed = cpy.Apply(tree)(transformInnerApply(tree.fun), transform(tree.args))
InstrumentedParts.notCovered(transformed)

private def tryInstrument(tree: Ident)(using Context): InstrumentedParts =
Expand All @@ -187,9 +211,14 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
else
InstrumentedParts.notCovered(tree)

private def tryInstrument(tree: Literal)(using Context): InstrumentedParts =
val coverageCall = createInvokeCall(tree, tree.sourcePos)
InstrumentedParts.singleExpr(coverageCall, tree)

private def tryInstrument(tree: Select)(using Context): InstrumentedParts =
val sym = tree.symbol
val transformed = cpy.Select(tree)(transform(tree.qualifier), tree.name)
val qual = transform(tree.qualifier).ensureConforms(tree.qualifier.tpe)
val transformed = cpy.Select(tree)(qual, tree.name)
if canInstrumentParameterless(sym) then
// call to a parameterless method
val coverageCall = createInvokeCall(tree, tree.sourcePos)
Expand All @@ -202,6 +231,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
tree match
case t: Apply => tryInstrument(t)
case t: Ident => tryInstrument(t)
case t: Literal => tryInstrument(t)
case t: Select => tryInstrument(t)
case _ => InstrumentedParts.notCovered(transform(tree))

Expand All @@ -223,10 +253,14 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
inContext(transformCtx(tree)) { // necessary to position inlined code properly
tree match
// simple cases
case tree: (Import | Export | Literal | This | Super | New) => tree
case tree: (Import | Export | This | Super | New) => tree
case tree if tree.isEmpty || tree.isType => tree // empty Thicket, Ident (referring to a type), TypeTree, ...
case tree if !tree.span.exists || tree.span.isZeroExtent => tree // no meaningful position

case tree: Literal =>
val rest = tryInstrument(tree).toTree
rest

// identifier
case tree: Ident =>
tryInstrument(tree).toTree
Expand Down Expand Up @@ -280,6 +314,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
case tree: CaseDef =>
transformCaseDef(tree)

case tree: ValDef if tree.symbol.is(Inline) =>
tree // transforming inline vals will result in `inline value must be pure` errors

case tree: ValDef =>
// only transform the rhs
val rhs = transform(tree.rhs)
Expand Down Expand Up @@ -323,13 +360,13 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
)

case tree: Inlined =>
// Ideally, tree.call would provide precise information about the inlined call,
// and we would use this information for the coverage report.
// But PostTyper simplifies tree.call, so we can't report the actual method that was inlined.
// In any case, the subtrees need to be repositioned right now, otherwise the
// coverage statement will point to a potentially unreachable source file.
val dropped = Inlines.dropInlined(tree) // drop and reposition
transform(dropped) // transform the content of the Inlined
// Inlined code contents might come from another file (or project),
// which means that we cannot clearly designate which part of the inlined code
// was run using the API we are given.
// At best, we can show that the Inlined tree itself was reached.
// Additionally, Scala 2's coverage ignores macro calls entirely,
// so let's do that here too, also for regular inlined calls.
tree

// For everything else just recurse and transform
case _ =>
Expand Down Expand Up @@ -559,15 +596,14 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
private def isCompilerIntrinsicMethod(sym: Symbol)(using Context): Boolean =
val owner = sym.maybeOwner
owner.exists && (
owner.eq(defn.AnyClass) ||
owner.isPrimitiveValueClass ||
(owner.eq(defn.AnyClass) && (sym == defn.Any_asInstanceOf || sym == defn.Any_isInstanceOf)) ||
owner.maybeOwner == defn.CompiletimePackageClass
)

object InstrumentCoverage:
val name: String = "instrumentCoverage"
val description: String = "instrument code for coverage checking"
val ExcludeMethodFlags: FlagSet = Synthetic | Artifact | Erased
val ExcludeMethodFlags: FlagSet = Artifact | Erased

/**
* An instrumented Tree, in 3 parts.
Expand Down
80 changes: 57 additions & 23 deletions tests/coverage/pos/Constructor.scoverage.check
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,23 @@ C
Class
covtest.C
x
161
162
13
<none>
Literal
false
0
false
1

6
Constructor.scala
covtest
C
Class
covtest.C
x
153
158
13
Expand All @@ -120,7 +137,7 @@ false
false
def x

6
7
Constructor.scala
covtest
C
Expand All @@ -137,7 +154,7 @@ false
false
f(x)

7
8
Constructor.scala
covtest
C
Expand All @@ -154,7 +171,24 @@ false
false
x

8
9
Constructor.scala
covtest
C
Class
covtest.C
g
188
189
16
<none>
Literal
false
0
false
2

10
Constructor.scala
covtest
C
Expand All @@ -171,7 +205,7 @@ false
false
def g

9
11
Constructor.scala
covtest
O
Expand All @@ -188,7 +222,24 @@ false
false
def g

10
12
Constructor.scala
covtest
O
Object
covtest.O
y
231
232
20
<none>
Literal
false
0
false
1

13
Constructor.scala
covtest
O
Expand All @@ -205,7 +256,7 @@ false
false
def y

11
14
Constructor.scala
covtest
O
Expand All @@ -222,20 +273,3 @@ false
false
g(y)

12
Constructor.scala
covtest
O
Object
covtest.O
<init>
237
238
21
y
Ident
false
0
false
y

43 changes: 30 additions & 13 deletions tests/coverage/pos/ContextFunctions.scoverage.check
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,23 @@ covtest
Imperative
Class
covtest.Imperative
$anonfun
178
184
9
<none>
Literal
false
0
false
"name"

2
ContextFunctions.scala
covtest
Imperative
Class
covtest.Imperative
readName2
121
134
Expand All @@ -52,41 +69,41 @@ false
false
def readName2

2
3
ContextFunctions.scala
covtest
Imperative
Class
covtest.Imperative
readPerson
252
309
14
onError
Apply
243
247
13
<none>
Literal
false
0
false
OnError((e) => readName2(using e)(using s)).onError(None)
null

3
4
ContextFunctions.scala
covtest
Imperative
Class
covtest.Imperative
readPerson
252
295
309
14
<init>
onError
Apply
false
0
false
OnError((e) => readName2(using e)(using s))
OnError((e) => readName2(using e)(using s)).onError(None)

4
5
ContextFunctions.scala
covtest
Imperative
Expand All @@ -103,7 +120,7 @@ false
false
readName2(using e)(using s)

5
6
ContextFunctions.scala
covtest
Imperative
Expand Down
Loading
Loading