Skip to content

Fix #17042: Preserve the shape of secondary ctors in instrumentCoverage. #17111

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

Merged
merged 1 commit into from
Mar 30, 2023
Merged
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
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