Skip to content

Commit

Permalink
Fix scala#17042: Preserve the shape of secondary ctors in instrumentC…
Browse files Browse the repository at this point in the history
…overage.
  • Loading branch information
sjrd committed Mar 15, 2023
1 parent 6e5be23 commit 5fb69be
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 31 deletions.
30 changes: 29 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import core.DenotTransformers.IdentityDenotTransformer
import core.Symbols.{defn, Symbol}
import core.Constants.Constant
import core.NameOps.isContextFunction
import core.StdNames.nme
import core.Types.*
import coverage.*
import typer.LiftCoverage
Expand Down Expand Up @@ -325,7 +326,11 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
// Only transform the params (for the default values) and the rhs, not the name and tpt.
val transformedParamss = transformParamss(tree.paramss)
val transformedRhs =
if !sym.isOneOf(Accessor | Artifact | Synthetic) && !tree.rhs.isEmpty then
if tree.rhs.isEmpty then
tree.rhs
else if sym.isClassConstructor then
instrumentSecondaryCtor(tree)
else if !sym.isOneOf(Accessor | Artifact | Synthetic) 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`),
Expand Down Expand Up @@ -410,6 +415,24 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
val coverageCall = createInvokeCall(parent, pos)
InstrumentedParts.singleExprTree(coverageCall, body)

/** Instruments the body of a secondary constructor DefDef.
*
* We must preserve the delegate constructor call as the first statement of
* the rhs Block, otherwise `HoistSuperArgs` will not be happy (see #17042).
*/
private def instrumentSecondaryCtor(ctorDef: DefDef)(using Context): Tree =
// compute position like in instrumentBody
val namePos = ctorDef.namePos
val pos = namePos.withSpan(namePos.span.withStart(ctorDef.span.start))
val coverageCall = createInvokeCall(ctorDef, pos)

ctorDef.rhs match
case b @ Block(delegateCtorCall :: stats, expr: Literal) =>
cpy.Block(b)(transform(delegateCtorCall) :: coverageCall :: stats.mapConserve(transform), expr)
case rhs =>
cpy.Block(rhs)(transform(rhs) :: coverageCall :: Nil, unitLiteral)
end instrumentSecondaryCtor

/**
* Checks if the apply needs a lift in the coverage phase.
* In case of a nested application, we have to lift all arguments
Expand Down Expand Up @@ -447,9 +470,14 @@ 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 =
def isSecondaryCtorDelegateCall: Boolean = tree.fun match
case Select(This(_), nme.CONSTRUCTOR) => true
case _ => false

val sym = tree.symbol
!sym.isOneOf(ExcludeMethodFlags)
&& !isCompilerIntrinsicMethod(sym)
&& !(sym.isClassConstructor && isSecondaryCtorDelegateCall)
&& (tree.typeOpt match
case AppliedType(tycon: NamedType, _) =>
/* If the last expression in a block is a context function, we'll try to
Expand Down
10 changes: 10 additions & 0 deletions tests/coverage/pos/Constructor.scala
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
package covtest

class C:
def this(arg: String) = {
this()
g()
}

def this(x: Int) =
this(x.toString() + "foo")

def f(x: Int) = x
def x = 1
f(x)

def g(): Int = 2

object O:
def g(y: Int) = y
def y = 1
Expand Down
145 changes: 115 additions & 30 deletions tests/coverage/pos/Constructor.scoverage.check
Original file line number Diff line number Diff line change
Expand Up @@ -24,129 +24,214 @@ covtest
C
Class
covtest.C
f
<init>
28
33
36
3
<init>
DefDef
false
0
false
def this

1
Constructor.scala
covtest
C
Class
covtest.C
<init>
69
72
5
g
Apply
false
0
false
g()

2
Constructor.scala
covtest
C
Class
covtest.C
<init>
80
88
8
<init>
DefDef
false
0
false
def this

3
Constructor.scala
covtest
C
Class
covtest.C
<init>
108
128
9
+
Apply
false
0
false
x.toString() + "foo"

4
Constructor.scala
covtest
C
Class
covtest.C
f
133
138
11
f
DefDef
false
0
false
def f

1
5
Constructor.scala
covtest
C
Class
covtest.C
x
48
53
4
153
158
12
x
DefDef
false
0
false
def x

2
6
Constructor.scala
covtest
C
Class
covtest.C
<init>
60
64
5
165
169
13
f
Apply
false
0
false
f(x)

3
7
Constructor.scala
covtest
C
Class
covtest.C
<init>
62
63
5
167
168
13
x
Select
false
0
false
x

4
8
Constructor.scala
covtest
C
Class
covtest.C
g
173
178
15
g
DefDef
false
0
false
def g

9
Constructor.scala
covtest
O$
Object
covtest.O$
g
78
83
8
203
208
18
g
DefDef
false
0
false
def g

5
10
Constructor.scala
covtest
O$
Object
covtest.O$
y
98
103
9
223
228
19
y
DefDef
false
0
false
def y

6
11
Constructor.scala
covtest
O$
Object
covtest.O$
<init>
110
114
10
235
239
20
g
Apply
false
0
false
g(y)

7
12
Constructor.scala
covtest
O$
Object
covtest.O$
<init>
112
113
10
237
238
20
y
Ident
false
Expand Down

0 comments on commit 5fb69be

Please sign in to comment.