From bcb2053147db516e8179063036a0e0cce3d08ef8 Mon Sep 17 00:00:00 2001 From: Kacper Korban Date: Mon, 21 Aug 2023 13:24:58 +0200 Subject: [PATCH] Coverage: mark case bodies as branches; don't ignore branches with synthetic spans Marking bodies of CaseDefs as branches seems like an uncontroversial change, it was probably an oversight. Not ignoring synthetic spans when creating coverage calls in branches seems like a good trade off. There might be some auto-generated `else ()` interpreted as branches, but Scala introduces quite a lot of synthetic trees that wrap non-synthetic trees (e.g. implicit classes). Also, it looks like Scala 2 includes those compiler-generated `else` branches in coverage. (Another possibility here would be to also check if the span is zero extent, but that approach would be different to the Scala 2 one) partial fix for lampepfl#16634 --- .../dotc/transform/InstrumentCoverage.scala | 11 +- tests/coverage/pos/Inlined.scoverage.check | 71 +++++- .../pos/MatchCaseClasses.scoverage.check | 106 ++++----- .../coverage/pos/MatchNumbers.scoverage.check | 24 +- .../pos/SimpleMethods.scoverage.check | 33 ++- .../coverage/pos/scoverage-samples-case.scala | 28 +++ .../scoverage-samples-case.scoverage.check | 173 +++++++++++++++ .../scoverage-samples-implicit-class.scala | 14 ++ ...age-samples-implicit-class.scoverage.check | 207 ++++++++++++++++++ 9 files changed, 576 insertions(+), 91 deletions(-) create mode 100644 tests/coverage/pos/scoverage-samples-case.scala create mode 100644 tests/coverage/pos/scoverage-samples-case.scoverage.check create mode 100644 tests/coverage/pos/scoverage-samples-implicit-class.scala create mode 100644 tests/coverage/pos/scoverage-samples-implicit-class.scoverage.check diff --git a/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala b/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala index 3e5925899848..a371c7c98fca 100644 --- a/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala +++ b/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala @@ -188,12 +188,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer: * If the tree is empty, return itself and don't instrument. */ private def transformBranch(tree: Tree)(using Context): Tree = - import dotty.tools.dotc.core.Decorators.{show,i} - if tree.isEmpty || tree.span.isSynthetic then + if tree.isEmpty then // - If t.isEmpty then `transform(t) == t` always hold, // so we can avoid calling transform in that case. - // - If tree.span.isSynthetic then the branch has been generated - // by the frontend phases, so we don't want to instrument it. tree else val transformed = transform(tree) @@ -353,10 +350,8 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer: // recursively transform the guard, but keep the pat val transformedGuard = transform(guard) - // ensure that the body is always instrumented by inserting a call to Invoker.invoked at its beginning - val coverageCall = createInvokeCall(tree.body, pos) - val transformedBody = transform(tree.body) - val instrumentedBody = InstrumentedParts.singleExprTree(coverageCall, transformedBody) + // ensure that the body is always instrumented as a branch + val instrumentedBody = transformBranch(tree.body) cpy.CaseDef(tree)(pat, transformedGuard, instrumentedBody) diff --git a/tests/coverage/pos/Inlined.scoverage.check b/tests/coverage/pos/Inlined.scoverage.check index 85393db11a8f..dccee7d0026b 100644 --- a/tests/coverage/pos/Inlined.scoverage.check +++ b/tests/coverage/pos/Inlined.scoverage.check @@ -59,6 +59,23 @@ Inlined$package$ Object covtest.Inlined$package$ testInlined +330 +330 +10 + +Literal +true +0 +false + + +3 +Inlined.scala +covtest +Inlined$package$ +Object +covtest.Inlined$package$ +testInlined 155 162 6 @@ -69,7 +86,7 @@ false false List(l) -3 +4 Inlined.scala covtest Inlined$package$ @@ -86,7 +103,7 @@ false false List -4 +5 Inlined.scala covtest Inlined$package$ @@ -103,7 +120,7 @@ false false List(l).length -5 +6 Inlined.scala covtest Inlined$package$ @@ -120,7 +137,7 @@ false false scala.runtime.Scala3RunTime.assertFailed() -6 +7 Inlined.scala covtest Inlined$package$ @@ -137,7 +154,24 @@ true false scala.runtime.Scala3RunTime.assertFailed() -7 +8 +Inlined.scala +covtest +Inlined$package$ +Object +covtest.Inlined$package$ +testInlined +330 +330 +10 + +Literal +true +0 +false + + +9 Inlined.scala covtest Inlined$package$ @@ -154,7 +188,7 @@ false false List(l) -8 +10 Inlined.scala covtest Inlined$package$ @@ -171,7 +205,7 @@ false false List -9 +11 Inlined.scala covtest Inlined$package$ @@ -188,7 +222,7 @@ false false List(l).length -10 +12 Inlined.scala covtest Inlined$package$ @@ -205,7 +239,7 @@ false false scala.runtime.Scala3RunTime.assertFailed() -11 +13 Inlined.scala covtest Inlined$package$ @@ -222,7 +256,24 @@ true false scala.runtime.Scala3RunTime.assertFailed() -12 +14 +Inlined.scala +covtest +Inlined$package$ +Object +covtest.Inlined$package$ +testInlined +330 +330 +10 + +Literal +true +0 +false + + +15 Inlined.scala covtest Inlined$package$ diff --git a/tests/coverage/pos/MatchCaseClasses.scoverage.check b/tests/coverage/pos/MatchCaseClasses.scoverage.check index 5440c2e3098d..5f52604d0d58 100644 --- a/tests/coverage/pos/MatchCaseClasses.scoverage.check +++ b/tests/coverage/pos/MatchCaseClasses.scoverage.check @@ -25,23 +25,6 @@ MatchCaseClasses$ Object covtest.MatchCaseClasses$ f -135 -147 -7 - -Block -false -0 -false -case Pat1(0) - -1 -MatchCaseClasses.scala -covtest -MatchCaseClasses$ -Object -covtest.MatchCaseClasses$ -f 151 163 7 @@ -52,24 +35,24 @@ false false println("a") -2 +1 MatchCaseClasses.scala covtest MatchCaseClasses$ Object covtest.MatchCaseClasses$ f -168 -180 -8 +148 +163 +7 Block -false +true 0 false -case Pat1(_) +=> println("a") -3 +2 MatchCaseClasses.scala covtest MatchCaseClasses$ @@ -86,24 +69,24 @@ false false println("b") -4 +3 MatchCaseClasses.scala covtest MatchCaseClasses$ Object covtest.MatchCaseClasses$ f -201 -221 -9 +181 +196 +8 Block -false +true 0 false -case p @ Pat2(1, -1) +=> println("b") -5 +4 MatchCaseClasses.scala covtest MatchCaseClasses$ @@ -120,24 +103,24 @@ false false println("c") -6 +5 MatchCaseClasses.scala covtest MatchCaseClasses$ Object covtest.MatchCaseClasses$ f -242 -265 -10 +222 +237 +9 Block -false +true 0 false -case Pat2(_, y: String) +=> println("c") -7 +6 MatchCaseClasses.scala covtest MatchCaseClasses$ @@ -154,7 +137,7 @@ false false println(y) -8 +7 MatchCaseClasses.scala covtest MatchCaseClasses$ @@ -171,24 +154,24 @@ false false println("d") -9 +8 MatchCaseClasses.scala covtest MatchCaseClasses$ Object covtest.MatchCaseClasses$ f -309 -321 -13 +275 +304 +11 Block -false +true 0 false -case p: Pat2 +println(y)\n println("d") -10 +9 MatchCaseClasses.scala covtest MatchCaseClasses$ @@ -205,24 +188,24 @@ false false println("e") -11 +10 MatchCaseClasses.scala covtest MatchCaseClasses$ Object covtest.MatchCaseClasses$ f -342 -348 -14 +322 +337 +13 Block -false +true 0 false -case _ +=> println("e") -12 +11 MatchCaseClasses.scala covtest MatchCaseClasses$ @@ -239,6 +222,23 @@ false false println("other") +12 +MatchCaseClasses.scala +covtest +MatchCaseClasses$ +Object +covtest.MatchCaseClasses$ +f +349 +368 +14 + +Block +true +0 +false +=> println("other") + 13 MatchCaseClasses.scala covtest diff --git a/tests/coverage/pos/MatchNumbers.scoverage.check b/tests/coverage/pos/MatchNumbers.scoverage.check index 0421cf77002a..a101e0aaa9f9 100644 --- a/tests/coverage/pos/MatchNumbers.scoverage.check +++ b/tests/coverage/pos/MatchNumbers.scoverage.check @@ -25,15 +25,15 @@ MatchNumbers$ Object covtest.MatchNumbers$ f -106 -126 +127 +132 6 Block -false +true 0 false -case x: Int if x < 0 +=> -1 1 MatchNumbers.scala @@ -42,15 +42,15 @@ MatchNumbers$ Object covtest.MatchNumbers$ f -137 -148 +149 +153 7 Block -false +true 0 false -case x: Int +=> x 2 MatchNumbers.scala @@ -59,15 +59,15 @@ MatchNumbers$ Object covtest.MatchNumbers$ f -158 -170 +171 +181 8 Block -false +true 0 false -case y: Long +=> y.toInt 3 MatchNumbers.scala diff --git a/tests/coverage/pos/SimpleMethods.scoverage.check b/tests/coverage/pos/SimpleMethods.scoverage.check index dc68258f9a66..ecb2774bed5b 100644 --- a/tests/coverage/pos/SimpleMethods.scoverage.check +++ b/tests/coverage/pos/SimpleMethods.scoverage.check @@ -195,6 +195,23 @@ C Class covtest.C partialCond +273 +273 +18 + +Literal +true +0 +false + + +11 +SimpleMethods.scala +covtest +C +Class +covtest.C +partialCond 229 244 17 @@ -205,7 +222,7 @@ false false def partialCond -11 +12 SimpleMethods.scala covtest C @@ -222,7 +239,7 @@ false false def new1 -12 +13 SimpleMethods.scala covtest C @@ -239,24 +256,24 @@ true false () -13 +14 SimpleMethods.scala covtest C Class covtest.C tryCatch -349 -366 +367 +371 25 Block -false +true 0 false -case e: Exception +=> 1 -14 +15 SimpleMethods.scala covtest C diff --git a/tests/coverage/pos/scoverage-samples-case.scala b/tests/coverage/pos/scoverage-samples-case.scala new file mode 100644 index 000000000000..78f13da70401 --- /dev/null +++ b/tests/coverage/pos/scoverage-samples-case.scala @@ -0,0 +1,28 @@ +// minimized example from sbt-scoverage-samples +package org.scoverage.samples + +import scala.concurrent.duration._ +import scala.concurrent.ExecutionContext.Implicits.global + +case object StartService +case object StopService + +class PriceEngine() { + + var cancellable: String = _ + + cancellable = "abc" + + def receive: Any => Unit = { + case StartService => + stop() + + case StopService => + stop() + } + + def stop(): Unit = { + if (cancellable != null) + println("stop") + } +} diff --git a/tests/coverage/pos/scoverage-samples-case.scoverage.check b/tests/coverage/pos/scoverage-samples-case.scoverage.check new file mode 100644 index 000000000000..173296932299 --- /dev/null +++ b/tests/coverage/pos/scoverage-samples-case.scoverage.check @@ -0,0 +1,173 @@ +# Coverage data, format version: 3.0 +# Statement data: +# - id +# - source path +# - package name +# - class name +# - class type (Class, Object or Trait) +# - full class name +# - method name +# - start offset +# - end offset +# - line number +# - symbol name +# - tree name +# - is branch +# - invocations count +# - is ignored +# - description (can be multi-line) +# ' ' sign +# ------------------------------------------ +0 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +$anonfun +362 +368 +17 +stop +Apply +false +0 +false +stop() + +1 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +$anonfun +353 +368 +16 + +Block +true +0 +false +=>\n stop() + +2 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +$anonfun +400 +406 +20 +stop +Apply +false +0 +false +stop() + +3 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +$anonfun +391 +406 +19 + +Block +true +0 +false +=>\n stop() + +4 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +receive +302 +313 +15 +receive +DefDef +false +0 +false +def receive + +5 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +stop +470 +485 +25 +println +Apply +false +0 +false +println("stop") + +6 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +stop +470 +485 +25 +println +Apply +true +0 +false +println("stop") + +7 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +stop +485 +485 +25 + +Literal +true +0 +false + + +8 +scoverage-samples-case.scala +org.scoverage.samples +PriceEngine +Class +org.scoverage.samples.PriceEngine +stop +414 +422 +23 +stop +DefDef +false +0 +false +def stop + diff --git a/tests/coverage/pos/scoverage-samples-implicit-class.scala b/tests/coverage/pos/scoverage-samples-implicit-class.scala new file mode 100644 index 000000000000..98a0e0976b89 --- /dev/null +++ b/tests/coverage/pos/scoverage-samples-implicit-class.scala @@ -0,0 +1,14 @@ +// minimized example from sbt-scoverage-samples +package org.scoverage.samples + +implicit class StringOpssssss(s: String) { + def ! (str: String): Unit = println(s + "!" + str) +} + +class CreditEngine { + def receive: Int => Unit = { req => + if (req < 2000) + "if 1" ! "xd" + else println("else 1") + } +} diff --git a/tests/coverage/pos/scoverage-samples-implicit-class.scoverage.check b/tests/coverage/pos/scoverage-samples-implicit-class.scoverage.check new file mode 100644 index 000000000000..5da8a482c685 --- /dev/null +++ b/tests/coverage/pos/scoverage-samples-implicit-class.scoverage.check @@ -0,0 +1,207 @@ +# Coverage data, format version: 3.0 +# Statement data: +# - id +# - source path +# - package name +# - class name +# - class type (Class, Object or Trait) +# - full class name +# - method name +# - start offset +# - end offset +# - line number +# - symbol name +# - tree name +# - is branch +# - invocations count +# - is ignored +# - description (can be multi-line) +# ' ' sign +# ------------------------------------------ +0 +scoverage-samples-implicit-class.scala +org.scoverage.samples +CreditEngine +Class +org.scoverage.samples.CreditEngine +$anonfun +263 +276 +10 +! +Apply +false +0 +false +"if 1" ! "xd" + +1 +scoverage-samples-implicit-class.scala +org.scoverage.samples +CreditEngine +Class +org.scoverage.samples.CreditEngine +$anonfun +263 +269 +10 +StringOpssssss +Apply +false +0 +false +"if 1" + +2 +scoverage-samples-implicit-class.scala +org.scoverage.samples +CreditEngine +Class +org.scoverage.samples.CreditEngine +$anonfun +263 +276 +10 +! +Apply +true +0 +false +"if 1" ! "xd" + +3 +scoverage-samples-implicit-class.scala +org.scoverage.samples +CreditEngine +Class +org.scoverage.samples.CreditEngine +$anonfun +286 +303 +11 +println +Apply +false +0 +false +println("else 1") + +4 +scoverage-samples-implicit-class.scala +org.scoverage.samples +CreditEngine +Class +org.scoverage.samples.CreditEngine +$anonfun +286 +303 +11 +println +Apply +true +0 +false +println("else 1") + +5 +scoverage-samples-implicit-class.scala +org.scoverage.samples +CreditEngine +Class +org.scoverage.samples.CreditEngine +receive +201 +212 +8 +receive +DefDef +false +0 +false +def receive + +6 +scoverage-samples-implicit-class.scala +org.scoverage.samples +StringOpssssss +Class +org.scoverage.samples.StringOpssssss +! +152 +174 +4 +println +Apply +false +0 +false +println(s + "!" + str) + +7 +scoverage-samples-implicit-class.scala +org.scoverage.samples +StringOpssssss +Class +org.scoverage.samples.StringOpssssss +! +160 +173 +4 ++ +Apply +false +0 +false +s + "!" + str + +8 +scoverage-samples-implicit-class.scala +org.scoverage.samples +StringOpssssss +Class +org.scoverage.samples.StringOpssssss +! +160 +167 +4 ++ +Apply +false +0 +false +s + "!" + +9 +scoverage-samples-implicit-class.scala +org.scoverage.samples +StringOpssssss +Class +org.scoverage.samples.StringOpssssss +! +124 +129 +4 +! +DefDef +false +0 +false +def ! + +10 +scoverage-samples-implicit-class.scala +org.scoverage.samples +scoverage-samples-implicit-class$package$ +Object +org.scoverage.samples.scoverage-samples-implicit-class$package$ +StringOpssssss +79 +108 +3 +StringOpssssss +DefDef +false +0 +false +implicit class StringOpssssss +