Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Draft: Fix coverage reporting with for comprehensions #17239

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Draft: Fix coverage reporting with for comprehensions #17239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4262be0
Fix coverage reporting with for comprehensions and add some tests to …
vijaykramesh Mar 8, 2023
3481f0c
wip
vijaykramesh Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 38 additions & 17 deletions compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
View file
Open in desktop
Expand Up @@ -2,20 +2,20 @@ package dotty.tools.dotc
package transform

import java.io.File

import ast.tpd.*

import collection.mutable
import core.Flags.*
import core.Contexts.{Context, ctx, inContext}
import core.DenotTransformers.IdentityDenotTransformer
import core.Symbols.{defn, Symbol}
import core.Symbols.{Symbol, defn}
import core.Constants.Constant
import core.NameOps.isContextFunction
import core.StdNames.nme
import core.Types.*
import coverage.*
import typer.LiftCoverage
import util.{SourcePosition, SourceFile}
import util.{SourceFile, SourcePosition}
import util.Spans.Span
import localopt.StringInterpolatorOpt
import inlines.Inlines
Expand Down Expand Up @@ -120,6 +120,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
*/
private def createInvokeCall(tree: Tree, pos: SourcePosition, branch: Boolean = false)(using Context): Apply =
val statementId = recordStatement(tree, pos, branch)
println(s"createInvokeCall: $statementId")
val span = pos.span.toSynthetic
invokeCall(statementId, span)

Expand All @@ -132,6 +133,8 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
* @return instrumentation result, with the preparation statement, coverage call and tree separated
*/
private def tryInstrument(tree: Apply)(using Context): InstrumentedParts =
println(s"tryInstrument Tree: ${tree}")

if canInstrumentApply(tree) then
// Create a call to Invoker.invoked(coverageDirectory, newStatementId)
val coverageCall = createInvokeCall(tree, tree.sourcePos)
Expand All @@ -156,25 +159,33 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
val transformed = cpy.Apply(tree)(transform(tree.fun), transform(tree.args))
InstrumentedParts.notCovered(transformed)

private def tryInstrument(tree: Ident)(using Context): InstrumentedParts =
private def tryInstrument(tree: Ident)(using Context): InstrumentedParts = {
println(s"tryInstrument Ident: ${tree}")


val sym = tree.symbol
if canInstrumentParameterless(sym) then

if (canInstrumentParameterless(sym)) {
// call to a local parameterless method f
val coverageCall = createInvokeCall(tree, tree.sourcePos)
InstrumentedParts.singleExpr(coverageCall, tree)
else
InstrumentedParts.notCovered(tree)
} else {
println(s"tryInstrument Ident: ${tree} not instrumented")
InstrumentedParts.notCovered(tree)
}
}

private def tryInstrument(tree: Select)(using Context): InstrumentedParts =
private def tryInstrument(tree: Select)(using Context): InstrumentedParts = {
val sym = tree.symbol
val transformed = cpy.Select(tree)(transform(tree.qualifier), tree.name)
println(s"tryInstrument Select: ${tree}")
if canInstrumentParameterless(sym) then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it's not that simple, for two reasons:

  • one critical: some Select trees simply cannot be instrumented, because the later phases of the compiler expect some trees to be removed/transformed in some specific ways that aren't compatible with the coverage instrumentation (e.g. asInstanceOf).
  • one ergonomic: method applications are already handled by tryInstrument(Apply), it would be redundant to instrument the select. For instance for a.f(x), we get an Apply(Select(...), args) where the Select is a.f and the args is x. Here, instrumenting the Apply tree is enough.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks these comments make sense, and indeed as I do more testing I see it's not this simple :)

hopefully will report back after tackling this further.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheElectronWill so I'm able to reproduce the issue neatly locally now, and then use my local dotty build in sbt-scoverage-samples in order to clearly see the coverage issue and how my local changes impact it
Screen Shot 2023-04-18 at 7 58 53 AM

I am able to get the right side of the flatMap/for comprehension to show up in the measurements file but not arbitrarily deep (so the right hand side of a <- List(1) will be green but the right hand side of the next line b <- List(2) won't be.

& also it somewhat seems fundamentally wrong to me, because we are getting a statement for the whole Apply flatMap tree around everything -

 for {\n s <- Future.successful("foo")\n t <- Future.successful("barnum")\n } yield {\n "yield me"\n }

and that is properly getting added to the measurements file when it is called.

so I actually think the "correct" solution would be to stop these other statements (that are inside the flatMap/for comprehension statement) from showing up entirely. does that make sense to you?

statement list to show you the "double counting"

Screen Shot 2023-04-18 at 8 02 18 AM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I actually think the "correct" solution would be to stop these other statements (that are inside the flatMap/for comprehension statement) from showing up entirely. does that make sense to you?

But then, how would the coverage check that all of the for-comprehension is executed? The first rhs could fail, and in that case the rest of the for-comprehension must not be 100% covered. For example:

for
  a <- throwException()
  b <- other() // must not be covered
do
  println(b) // must not be covered

// call to a parameterless method
val coverageCall = createInvokeCall(tree, tree.sourcePos)
InstrumentedParts.singleExpr(coverageCall, transformed)
else
InstrumentedParts.notCovered(transformed)

}
/** Generic tryInstrument */
private def tryInstrument(tree: Tree)(using Context): InstrumentedParts =
tree match
Expand Down Expand Up @@ -234,6 +245,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
case TypeApply(fun, args) =>
// Here is where `InstrumentedParts` becomes useful!
// We extract its components and act carefully.
println(s"TypeApply: ${fun}")
val InstrumentedParts(pre, coverageCall, expr) = tryInstrument(fun)

if coverageCall.isEmpty then
Expand Down Expand Up @@ -444,6 +456,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
* should not be changed to {val $x = f(); T($x)}(1) but to {val $x = f(); val $y = 1; T($x)($y)}
*/
private def needsLift(tree: Apply)(using Context): Boolean =
println(s"start needsLift: $tree")
def isShortCircuitedOp(sym: Symbol) =
sym == defn.Boolean_&& || sym == defn.Boolean_||

Expand All @@ -465,8 +478,10 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
case a: Apply => needsLift(a)
case _ => false

nestedApplyNeedsLift ||
val liftMe = nestedApplyNeedsLift ||
!isUnliftableFun(fun) && !tree.args.isEmpty && !tree.args.forall(LiftCoverage.noLift)
println(s"end needsLift: $tree, liftMe: $liftMe, nestedApplyNeedsLift: $nestedApplyNeedsLift, isUnliftableFun: ${isUnliftableFun(fun)}, tree.args.isEmpty: ${tree.args}, tree.args.forall(LiftCoverage.noLift): ${tree.args.forall(LiftCoverage.noLift)}")
liftMe

/** Check if an Apply can be instrumented. Prevents this phase from generating incorrect code. */
private def canInstrumentApply(tree: Apply)(using Context): Boolean =
Expand All @@ -475,10 +490,10 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
case _ => false

val sym = tree.symbol
!sym.isOneOf(ExcludeMethodFlags)
val canI = !sym.isOneOf(ExcludeMethodFlags)
&& !isCompilerIntrinsicMethod(sym)
&& !(sym.isClassConstructor && isSecondaryCtorDelegateCall)
&& (tree.typeOpt match
&& (tree.typeOpt match {
case AppliedType(tycon: NamedType, _) =>
/* If the last expression in a block is a context function, we'll try to
summon its arguments at the current point, even if the expected type
Expand All @@ -501,19 +516,25 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
*/
false
case _ =>
true
)
false
})
println(s"canInstrumentApply ${tree.symbol}: $canI is ${sym.name} is method ${sym.is(Method)} is compiler intrinsic ${isCompilerIntrinsicMethod(sym)} typeOpt ${tree.typeOpt}")
canI


/** 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 = ExcludeMethodFlags)
private def canInstrumentParameterless(sym: Symbol)(using Context): Boolean = {
val isP = (sym.is(Method, butNot = ExcludeMethodFlags)
&& sym.info.isParameterless
&& !isCompilerIntrinsicMethod(sym)
&& !sym.info.typeSymbol.name.isContextFunction // exclude context functions like in canInstrumentApply
&& !sym.info.typeSymbol.name.isContextFunction ) // exclude context functions like in canInstrumentApply)
println(s"canInstrumentParameterless $sym: $isP is ${sym.name} kind string ${sym.kindString} is method ${sym.is(Method)} is parameterless ${sym.info.isParameterless} is compiler intrinsic ${isCompilerIntrinsicMethod(sym)} typeSymbol ${sym.info.typeSymbol} is context function ${sym.info.typeSymbol.name.isContextFunction}")
isP
}

/** Does sym refer to a "compiler intrinsic" method, which only exist during compilation,
* like Any.isInstanceOf?
Expand Down
2 changes: 1 addition & 1 deletion compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ class CoverageTests:

private val scalaFile = FileSystems.getDefault.getPathMatcher("glob:**.scala")
private val rootSrc = Paths.get(userDir, "tests", "coverage")

@Test
def checkCoverageStatements(): Unit =
checkCoverageIn(rootSrc.resolve("pos"), false)
Expand Down
17 changes: 17 additions & 0 deletions tests/coverage/pos/Escaping.scoverage.check
View file
Open in desktop
Expand Up @@ -42,6 +42,23 @@ covtest.\n
Class
covtest.\n.\r\n\f
\r\n\f
69
73
3
\\
Ident
false
0
false
`\\\\`

2
Escaping.scala
covtest.\n
\r\n\f
Class
covtest.\n.\r\n\f
\r\n\f
40
48
3
Expand Down
10 changes: 10 additions & 0 deletions tests/coverage/pos/ForComprehension.scala
View file
Open in desktop
@@ -0,0 +1,10 @@
package covtest

def testForComprehension: Unit =
for {
a <- List(1)
b <- List(1)
if b > 1
c = a + b
} yield (a, b, c)

190 changes: 190 additions & 0 deletions tests/coverage/pos/ForComprehension.scoverage.check
View file
Open in desktop
@@ -0,0 +1,190 @@
# 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
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
testForComprehension
52
138
3
flatMap
Apply
false
0
false
for {\n a <- List(1)\n b <- List(1)\n if b > 1\n c = a + b\n } yield (a, b, c)

1
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
testForComprehension
67
74
4
apply
Apply
false
0
false
List(1)

2
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
testForComprehension
67
71
4
List
Ident
false
0
false
List

3
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
$anonfun
79
138
5
map
Apply
false
0
false
b <- List(1)\n if b > 1\n c = a + b\n } yield (a, b, c)

4
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
$anonfun
79
118
5
map
Apply
false
0
false
b <- List(1)\n if b > 1\n c = a + b

5
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
$anonfun
79
104
5
withFilter
Apply
false
0
false
b <- List(1)\n if b > 1

6
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
$anonfun
84
91
5
apply
Apply
false
0
false
List(1)

7
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
$anonfun
84
88
5
List
Ident
false
0
false
List

8
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
$anonfun
79
110
5
apply
Apply
false
0
false
b <- List(1)\n if b > 1\n c

9
ForComprehension.scala
covtest
ForComprehension$package$
Object
covtest.ForComprehension$package$
testForComprehension
17
41
2
testForComprehension
DefDef
false
0
false
def testForComprehension