From 5fb69be4eaac058d1d19fb72eb79e04bb8fe7d93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Wed, 15 Mar 2023 16:38:00 +0100 Subject: [PATCH] Fix #17042: Preserve the shape of secondary ctors in instrumentCoverage. --- .../dotc/transform/InstrumentCoverage.scala | 30 +++- tests/coverage/pos/Constructor.scala | 10 ++ .../coverage/pos/Constructor.scoverage.check | 145 ++++++++++++++---- 3 files changed, 154 insertions(+), 31 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala b/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala index c69b342b9a01..29572a4ae30d 100644 --- a/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala +++ b/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala @@ -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 @@ -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`), @@ -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 @@ -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 diff --git a/tests/coverage/pos/Constructor.scala b/tests/coverage/pos/Constructor.scala index 251370ec8e6e..536bfa26f386 100644 --- a/tests/coverage/pos/Constructor.scala +++ b/tests/coverage/pos/Constructor.scala @@ -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 diff --git a/tests/coverage/pos/Constructor.scoverage.check b/tests/coverage/pos/Constructor.scoverage.check index 678da472fd4c..6a6742c9118d 100644 --- a/tests/coverage/pos/Constructor.scoverage.check +++ b/tests/coverage/pos/Constructor.scoverage.check @@ -24,10 +24,78 @@ covtest C Class covtest.C -f + 28 -33 +36 3 + +DefDef +false +0 +false +def this + +1 +Constructor.scala +covtest +C +Class +covtest.C + +69 +72 +5 +g +Apply +false +0 +false +g() + +2 +Constructor.scala +covtest +C +Class +covtest.C + +80 +88 +8 + +DefDef +false +0 +false +def this + +3 +Constructor.scala +covtest +C +Class +covtest.C + +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 @@ -35,16 +103,16 @@ false false def f -1 +5 Constructor.scala covtest C Class covtest.C x -48 -53 -4 +153 +158 +12 x DefDef false @@ -52,16 +120,16 @@ false false def x -2 +6 Constructor.scala covtest C Class covtest.C -60 -64 -5 +165 +169 +13 f Apply false @@ -69,16 +137,16 @@ false false f(x) -3 +7 Constructor.scala covtest C Class covtest.C -62 -63 -5 +167 +168 +13 x Select false @@ -86,16 +154,33 @@ false 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 @@ -103,16 +188,16 @@ false false def g -5 +10 Constructor.scala covtest O$ Object covtest.O$ y -98 -103 -9 +223 +228 +19 y DefDef false @@ -120,16 +205,16 @@ false false def y -6 +11 Constructor.scala covtest O$ Object covtest.O$ -110 -114 -10 +235 +239 +20 g Apply false @@ -137,16 +222,16 @@ false false g(y) -7 +12 Constructor.scala covtest O$ Object covtest.O$ -112 -113 -10 +237 +238 +20 y Ident false