Draft: Fix coverage reporting with for comprehensions #17239
Conversation
vijaykramesh
force-pushed
the
fix_for_comprehension_coverage
branch
from
0f8e529 to
f259dee
Compare
|
seems like a test failure, investigating this. locally running like |
vijaykramesh
force-pushed
the
fix_for_comprehension_coverage
branch
from
f259dee to
4262be0
Compare
|
figured out how to locally use this dotty in another local project (sbt-scoverage-samples, so I can confirm this change addresses the issue at scoverage/sbt-scoverage-samples#28) - BUT then have run into some error when running with coverage on: quite sure it is related to my change, will address this before marking this PR as ready. |
vijaykramesh
changed the title
There was a problem hiding this comment.
Thank you for trying to tackle this problem!
If the problem is that some Select trees are missing, I think that it would be better to find out why canInstrumentParameterless exclude some necessary trees of for-comprehensions. Then, it can be refactored to let them be instrumented, while still excluding some cases that badly interact with the rest of the compiler.
| val transformed = cpy.Select(tree)(transform(tree.qualifier), tree.name) | ||
| if canInstrumentParameterless(sym) then |
There was a problem hiding this comment.
Unfortunately it's not that simple, for two reasons:
- one critical: some
Selecttrees 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 fora.f(x), we get anApply(Select(...), args)where the Select isa.fand the args isx. Here, instrumenting theApplytree is enough.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
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"
There was a problem hiding this comment.
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
|
I've read in vijaykramesh#1 that some |
vijaykramesh
force-pushed
the
fix_for_comprehension_coverage
branch
from
0865aab to
3481f0c
Compare
|
@vijaykramesh @TheElectronWill @ckipp01 Seems we can see what the problem is. What is the next step towards fixing it? Can I help? |
|
Problem is still there in Scala 3.3.0. Any updates on this? |
|
I can confirm the problem still persists on latest main (3.3.2-RC1) |
|
I can also confirm it is still not working in 3.3.1-RC4. |
|
So the problem is not specific to for-comprehensions, I can reproduce it with: def test: Unit =
List(1)
List(2).map(a => a)After phase instrumentCoverage, the argument of @TheElectronWill I'm not sure I understand the logic around using More generally, is it possible to change the percentage of coverage reported to not include stuff we ignore for one reason or another? That would solve the immediate reported in https://contributors.scala-lang.org/t/coverage-broken-why-does-nobody-care/6262v |
|
I've taken a further look and I think the problem is around the handling of lambdas. If I take an example like the below I get the following tasty tree segments.
and the below is a snippet from the decompiled class file From InstrumentCoverage, you can see that I also can't see any handling of LAMBDAs, perhaps that is the key? I think that if LAMBDA |
|
Good call @javax-swing, I think there's two issues involved here, if I start with: val x = List(2).map(a => a)InstrumentCoverages generates code equivalent to: val x = {
scala.runtime.coverage.Invoker.invoked(...)
List(2).map(a => a)
}if I remove the val x = {
scala.runtime.coverage.Invoker.invoked(...)
List(2).map(a =>
scala.runtime.coverage.Invoker.invoked(...)
a
)
}So the lambda is indeed instrumented but the |
|
Nice @smarter I should probably spend some time trying to build the compiler so that I can play with it too! I'm guessing the I'll see if I can spend some time this week coming up with some scenarios and expectations. Perhaps it's better for it to over instrument than under instrument in any case. What I am curious about is the mismatch between the scoverage report and the tasty modifications. Scoverage must be aware of the statements which haven't been instrumented somehow (as well as ones that can be ignored), so perhaps we can align with that, because everything highlighted in red/green looks like sensible things to be instrumented. |
It should work like a regular sbt project but feel free to ask any question on scala-contributors on https://discord.com/invite/scala
In another part of InstrumentCoverage we use
On a second look I think that's not quite correct actually: if the
I believe that's due to the explicit use of |
|
Sorry for my lack of reactivity, I'm currently in the middle of moving. I think you're right, lambdas aren't handled properly! Overinstrumenting vs Underinstrumenting is a difficult trade-off IMO, because we still want to exclude compiler-generated code from the instrumentation, in particular when instrumenting synthetic code can crash the compiler (it happened!). |
|
I believe I have found the issue with this PR: #18420 feel free to nab the commit. It seems that the This was problematic because that After the change code like the following Results in the expected invoke calls, which were otherwise missing |
Hi @javax-swing. Excellent work. What's the next step to get this fixed/merged? |
Based on looking into the issues that: #17239 was trying to solve. It was discovered that `TypeApply` was throwing away parts of the expression tree, which meant that the `coverage` file contained references to things that could never be invoked.
|
Seems like this can be closed now. 👍 |
|
@filipwiech I'm not sure about the changes, they are not the same as the ones already introduced, but they were meant to fix similar things. Regarding the backporting, I'll forward this question to @Kordyjan |
|
@Kordyjan Sorry to bother you, but I see that the backporting process for the 3.3.2 has already begun. Any insight from the maintenance team regarding the question above? 🙂 |
|
@filipwiech |
|
@Kordyjan I hope you mean 3.3.2 ;-) |
|
No problem @Kordyjan, thanks for the update, sounds great! 👍 |
Thanks for the info @Kordyjan , any ETA for Scala 3.3.2 release date? |
I wanted to release 3.3.2-RC1 today. It will probably be on Maven Central tomorrow. The stable release will happen around the last week of January. |
Hi @Kordyjan , Could you please provide the ETA for 3.3.2 stable release. |
February 14th is the likely date. The delay was caused by the regression that was found outside of the Open Community Build projects and CVE in one of scaladoc dependencies (not exploitable, but better to be safe than sorry). |
|
thank you Vijay and reviewers! glad this got sorted out |
This addresses #16634 and all the other sbt-scoverage issues related to it (scoverage/sbt-scoverage#382, scoverage/sbt-scoverage#458)
The main functional change is in InstrumentCoverage, where the tryInstrument for a Select tree should always instrument instead of dropping out to the InstrumentedParts.notCovered call.
I also added a test (see vijaykramesh#1 for some more info) that clearly demonstrates the problem on main.
Without the fix - my new test has all sorts of uncovered code that doesn't make sense:
with the fix - the coverage output make sense:
& then also this changed some of the saved coverage output data - I looked at the output before and after to confirm there were some changes but the actual coverage is still correct and as expected:
before fix :
after fix:
before fix:
after fix:
