Skip to content

Commit

Permalink
Fix coverage instrumentation of java and parameterless methods
Browse files Browse the repository at this point in the history 
1. Select and TypeApply trees weren't properly handled for JavaDefined symbols, leading to a crash when selecting a static method with parameter types.
2. Select and Ident trees weren't properly handled, and many occurences of parameterless methods were missed.
3. Some methods like asInstanceOf and isInstanceOf must not be instrumented, otherwise it crashes in Erasure.
  • Loading branch information
@TheElectronWill
TheElectronWill committed Jul 12, 2022
1 parent 9cfe76e commit 3fbbbd9
Show file tree
Hide file tree
Showing 27 changed files with 1,028 additions and 902 deletions.
84 changes: 46 additions & 38 deletions compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ import core.Flags.*
import core.Contexts.{Context, ctx, inContext}
import core.DenotTransformers.IdentityDenotTransformer
import core.Symbols.{defn, Symbol}
import core.Decorators.{toTermName, i}
import core.Constants.Constant
import core.NameOps.isContextFunction
import core.Types.*
import typer.LiftCoverage
import util.{SourcePosition, Property}
import util.Spans.Span
import coverage.*
import localopt.StringInterpolatorOpt.isCompilerIntrinsic
import localopt.StringInterpolatorOpt

/** Implements code coverage by inserting calls to scala.runtime.coverage.Invoker
* ("instruments" the source code).
Expand Down Expand Up @@ -66,7 +65,16 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
tree match
// simple cases
case tree: (Import | Export | Literal | This | Super | New) => tree
case tree if tree.isEmpty || tree.isType => tree // empty Thicket, Ident, TypTree, ...
case tree if tree.isEmpty || tree.isType => tree // empty Thicket, Ident (referring to a type), TypeTree, ...

// identifier
case tree: Ident =>
val sym = tree.symbol
if canInstrumentParameterless(sym) then
// call to a local parameterless method f
instrument(tree)
else
tree

// branches
case tree: If =>
Expand All @@ -82,20 +90,6 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
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
if needsLift(tree) then
val transformed = cpy.Apply(tree)(transformedFun, args) // args will be transformed in instrumentLifted
instrumentLifted(transformed)
else
val transformed = transformApply(tree, transformedFun)
instrument(transformed)
else
transformApply(tree, transformedFun)

// f(args)
case tree: Apply =>
if canInstrumentApply(tree) then
Expand All @@ -106,24 +100,19 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
else
transformApply(tree)

// (f(x))[args]
case TypeApply(fun: Apply, args) =>
// (fun)[args]
case TypeApply(fun, args) =>
cpy.TypeApply(tree)(transform(fun), args)

// a.b
case Select(qual, name) =>
if qual.symbol.exists && qual.symbol.is(JavaDefined) then
//Java class can't be used as a value, we can't instrument the
//qualifier ({<Probe>;System}.xyz() is not possible !) instrument it
//as it is
instrument(tree)
val transformed = cpy.Select(tree)(transform(qual), name)
val sym = tree.symbol
if canInstrumentParameterless(sym) then
// call to a parameterless method
instrument(transformed)
else
val transformed = cpy.Select(tree)(transform(qual), name)
if transformed.qualifier.isDef then
// instrument calls to methods without parameter list
instrument(transformed)
else
transformed
transformed

case tree: CaseDef => instrumentCaseDef(tree)
case tree: ValDef =>
Expand All @@ -142,7 +131,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
val rhs = transform(tree.rhs)
val finalRhs =
if canInstrumentDefDef(tree) then
// Ensure that the rhs is always instrumented, if possible
// Ensure that the rhs is always instrumented, if possible.
// 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`), the method won't show up as covered.
instrumentBody(tree, rhs)
else
rhs
Expand All @@ -162,7 +153,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.
* Note that if only one arg needs to be lifted, we just lift everything (see LiftCoverage).
*/
private def instrumentLifted(tree: Apply)(using Context) =
// lifting
Expand All @@ -178,10 +169,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
)

private inline def transformApply(tree: Apply)(using Context): Apply =
transformApply(tree, transform(tree.fun))

private inline def transformApply(tree: Apply, transformedFun: Tree)(using Context): Apply =
cpy.Apply(tree)(transformedFun, transform(tree.args))
cpy.Apply(tree)(transform(tree.fun), transform(tree.args))

private inline def instrumentCases(cases: List[CaseDef])(using Context): List[CaseDef] =
cases.map(instrumentCaseDef)
Expand Down Expand Up @@ -292,7 +280,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
* they shouldn't be lifted.
*/
val sym = fun.symbol
sym.exists && (isShortCircuitedOp(sym) || isCompilerIntrinsic(sym))
sym.exists && (isShortCircuitedOp(sym) || StringInterpolatorOpt.isCompilerIntrinsic(sym))
end

val fun = tree.fun
Expand All @@ -312,7 +300,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:

/** 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
val sym = tree.symbol
!sym.isOneOf(Synthetic | Artifact) && // no need to instrument synthetic apply
!isCompilerIntrinsicMethod(sym) &&
(tree.typeOpt match
case AppliedType(tycon: NamedType, _) =>
/* If the last expression in a block is a context function, we'll try to
Expand All @@ -339,6 +329,24 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
true
)

/** Is this the symbol of a parameterless method that we can instrument?
* Note: it is crucial that `asInstanceOf` and `isInstanceOf`, among others,
* do NOT get instrumented, because that would generate invalid code and crash
* in post-erasure checking.
*/
private def canInstrumentParameterless(sym: Symbol)(using Context): Boolean =
sym.is(Method, butNot = Synthetic | Artifact) &&
sym.info.isParameterless &&
!isCompilerIntrinsicMethod(sym)

/** Does sym refer to a "compiler intrinsic" method, which only exist during compilation,
* like Any.isInstanceOf?
* If this returns true, the call souldn't be instrumented.
*/
private def isCompilerIntrinsicMethod(sym: Symbol)(using Context): Boolean =
val owner = sym.maybeOwner
owner.eq(defn.AnyClass) || owner.isPrimitiveValueClass

object InstrumentCoverage:
val name: String = "instrumentCoverage"
val description: String = "instrument code for coverage checking"
40 changes: 37 additions & 3 deletions tests/coverage/pos/Constructor.scoverage.check
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,23 @@ C
Class
covtest.C
<init>
62
63
5
x
Select
false
0
false
x

3
Constructor.scala
covtest
C
Class
covtest.C
<init>
60
64
5
Expand All @@ -69,7 +86,7 @@ false
false
f(x)

3
4
Constructor.scala
covtest
O$
Expand All @@ -86,7 +103,7 @@ false
false
def g

4
5
Constructor.scala
covtest
O$
Expand All @@ -103,7 +120,24 @@ false
false
def y

5
6
Constructor.scala
covtest
O$
Object
covtest.O$
<init>
112
113
10
y
Ident
false
0
false
y

7
Constructor.scala
covtest
O$
Expand Down
104 changes: 1 addition & 103 deletions tests/coverage/pos/ContextFunctions.scoverage.check
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,108 +58,6 @@ covtest
Imperative
Class
covtest.Imperative
$anonfun
267
294
13
apply
Apply
false
0
false
readName2(using e)(using s)

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

4
ContextFunctions.scala
covtest
Imperative
Class
covtest.Imperative
readPerson
252
295
13
invoked
Apply
false
0
false
OnError((e) => readName2(using e)(using s))

5
ContextFunctions.scala
covtest
Imperative
Class
covtest.Imperative
$anonfun
267
294
13
invoked
Apply
false
0
false
readName2(using e)(using s)

6
ContextFunctions.scala
covtest
Imperative
Class
covtest.Imperative
$anonfun
267
294
13
apply
Apply
false
0
false
readName2(using e)(using s)

7
ContextFunctions.scala
covtest
Imperative
Class
covtest.Imperative
readPerson
252
295
13
<init>
Apply
false
0
false
OnError((e) => readName2(using e)(using s))

8
ContextFunctions.scala
covtest
Imperative
Class
covtest.Imperative
readPerson
252
309
Expand All @@ -171,7 +69,7 @@ false
false
OnError((e) => readName2(using e)(using s)).onError(None)

9
3
ContextFunctions.scala
covtest
Imperative
Expand Down
Loading

0 comments on commit 3fbbbd9

Please sign in to comment.