[Bug fix]Incorrect coverage for statement lambda function

[MarcoRossignoli]

fixes #343

I try to explain what happened.
When we use anonymous delegate compiler generate custom class and passes it as delegate.
If inside this method there are branches this hits are counted for "anonymous" compiler generated class.
This lead to missing "branches" from "actual" class(the method of the class that contains delegate).
Fortunately line of branches inside anonymous compiler generated class are correct, so my stategy is move all "anonymous" branches to "actual" class and after remove generated class from report to avoid noise(however it doesn't show because generated class has got 0 lines but only branches list).
In case of @driseley repro seems work well.

It's not possible write a test, or rather we could in future for this type of test generate a project and run real coverage and check "reports"(similar to BDN), but it's not easy/fast work.

@driseley I ask to you if kindly can clone build my fork/branch and test with your actual project. If you need help to setup let me know we could chat.

@sharwell I IIRW you work for Roslyn, please can you trust my "strategy" and help with review?

/cc @tonerdo @petli

[codecov]

Codecov Report

Merging #369 into master will increase coverage by 3.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   86.96%   89.99%   +3.03%     
==========================================
  Files          16       16              
  Lines        2178     2229      +51     
==========================================
+ Hits         1894     2006     +112     
+ Misses        284      223      -61

[driseley]

Hi @MarcoRossignoli

Thanks for looking at this - I think have been able to build and test your branch ok, as I was able to reproduce the fixed coverage on my demo project. However when I applied the fix to my real project, it did not seem to address the issue.

Here is a screenshot of the coverage of the named function:

And here is the lambda function:

As you can see the results are still quite different for the same set of tests.

If you were to subtract 1 from the coverage of each line of the lambda, it's be close...

Unfortunately I can't share the whole code , but let me know if there is some more information I can give you to help fully reproduce this.

Kind Regards
Dave

[sharwell]

Interesting. I would expect coverage of the named function to show partially covered for each of the branches, but for some reason they seem to be showing as fully covered.

[sharwell]

@MarcoRossignoli I don't understand this case well enough right now to figure out the difference, but I may be able to help with setting up a test harness.

[MarcoRossignoli]

@sharwell I thought also to add a verbose file output(maybe with switch i.e. /p:CoverletDebug=true) and generate a file with detailed operation/class/line/branch/hit maybe this could be used by @driseley without too much disclosure, only namespace/class/method name if possible, could be possible @driseley?

[sharwell]

Have you tried to reproduce this with an async lambda? I find those sometimes have different results in code coverage tools.

[MarcoRossignoli]

Have you tried to reproduce this with an async lambda? I find those sometimes have different results in code coverage tools.

Actually not, I used repro, maybe the issue is there.

[sharwell]

Yep, I noticed reply above confirmed that you fixed the original scenario, but the new screenshot shows async code.

[MarcoRossignoli]

Maybe related to different generated code I searched for specific compiler attribute(CompilerGeneratedAttribute), maybe we're missing async state machine and his interals branches.

[driseley]

I'll try to abstract the code that I have in the screenshots above to my reproducer repo, as that might help things. Should be ready in a couple of hours if I can untangle it.

[MarcoRossignoli]

I'll try to abstract the code that I have in the screenshots above to my reproducer repo, as that might help things. Should take a couple of hours if can untangle it.

Great, the code should be very close but with async lambda.

[driseley]

I have added a new class and test case here:

driseley/coverlet-issue-343@afb84e0

that reproduces the issue in the screenshots above

[MarcoRossignoli]

I did quick investigation and the issue seems unrelated to this PR, maybe related to nested async state machine where we remove one of two branch(because expected not covered), but removed branch is not the correct one(not related to sync/async execution).
I'll come back...

[MarcoRossignoli]

I tried another way to solve this.
Issue is not related to new features(report lambda methods branches) but to the way we remove async branch in async state machine MoveNext() method.
New strategy is to find async branch pattern(looking for call instance bool [System.Runtime]System.Runtime.CompilerServices.TaskAwaiter``1<!T>::get_IsCompleted()) and avoid to instrument it, because inside this branch there is no interesting code, it only enqueue async callback.
In case of sync code this code will never be covered, old strategy remove non covered branches in MoveNext() method, but it's not correct because inside this method we could have "application logic"(for nested async/await code) and we cannot simply "remove non covered branches".

@driseley can you try this new version in your prod code?

/cc @tonerdo

[driseley]

@MarcoRossignoli - thanks for the updates, however they don't appear to have made any difference to the test case (or my prod code). Could you confirm that the test case is now reporting correctly for you?

[MarcoRossignoli]

@driseley you should see a missed branch on if, btw you should see again covered line inside if block.

[MarcoRossignoli]

This is not a complete solution of your problem...I don't know if it's solvable at the moment 😞

TL;DR;

The issue is that compiler generate 2 state machine and one state machine create and run other.
Coverlet combines "hit"(coveret point) with "c# source range lines" to undestand what is covered, for instance hit number x cover lines from y-z and does this using DebugInformation
In your case when we call method.DebugInformation.GetSequencePoint(instruction) for outermost async machine state the range returned is from start method to end method.
This lead to "false positive" because we "cover" and "hit" that point on runtime but during "the count" we count +1 hit for every "line of code associated" you can see alg here but actually it's false, the issue here is that we cannot change the behaviour of DebugInformation API.

My current update is to "change" the strategy to avoid false "negative" on async branch of machine state, and works well , old PR #158 to almost see correct "branch" results(uncovered branch on ifs in your case, better than nothing you can see on reports)

But I have no idea at the moment how we can fix this use case, I'm trying to understand if it's possible find out "end line of branch" for now we've only the start, in that way if we found uncovered branch we can "override" false "positive"(because branch win on others).

I ask to @tonerdo @sharwell if they've some idea or other point of view of the problem.
/cc also @SteveGilham that maybe has already ecountered this issue.

[SteveGilham]

The algorithm I use is derived from OpenCover's process for finding branch offsetend and offsetchain values, and is implemented in the AltCover Visitor.getJumps function

This is what OpenCover essentially does --

For each conditional branch instruction get final destination:
get end of containing sequence point: step until the next instruction is either null (end of method) or starts another sequence point
for each possible next location from the conditional branch
label:
if the target is an unconditional branch, add to the chain for this target and recurse to label
if the target is after the end of the original sequence point, or is a conditional branch, terminate recursion
step to next instruction and recurse to label

I embellish by adding --

For non-switch conditional branches, if the range of instructions from the next instruction (branch not taken) to the branch target inclusive contains neither a sequence point boundary nor a return instruction (as in a ternary return expression), then I drop the branch from the report as being something compiler generated within the sequence point.

[MarcoRossignoli]

@driseley can you re-try with new updates?On my local seem fixed, take a look also if all other coverage is ok

Fix: the issue as explained above is that for nested async/away/lambda there is a chance that one state machine create another and pdb report "large block" as covered, but it's a false positive i.e.
Source.cs--> StateMachineA.MoveNext()---Lines 10-50
Source.cs--> StateMachineB.MoveNext()---Lines 20-23 <-- non covered
StateMachineA creates StateMachineB
If we instrument StateMachineA and we cover that point(before new StateMachineB) we count 10-50 as covered lines, but actually the "real" user code is in StateMachineB.MoveNext().
This update fix counting resetting hits for lines that are "non covered" by every covered line registered.
The idea should work because we expect that if we have more than one "copy" of line in covered lines if covered should be covered in "all" copies, pseudo code:

nonCoveredLines = CountHits(lines, hitfile);
for nonCoveredline in nonCoveredLines 
for line in lines
if line == nonCoveredLine then line.hits = 0

[driseley]

@MarcoRossignoli - Thanks for keeping working on this. My local testing matches your results, so it seems to largely address the coverage issue. There is a remaining issue - the coverage counts still seem to be one higher than expected:

In my repo - there is only one test case that covers the code - but some lines are shown as being covered twice. I can live with it - but is that something you want to continue to look at?

[MarcoRossignoli]

but is that something you want to continue to look at?

I think so, I understood the problem now...I'll give it a try...BTW other coverage is ok(other branch, lines unrelated to this)?
If you confirm that other all ok, my idea is to split this PR in more than one(for simple review by @tonerdo) and land it to master.
Here we've 3 fix

1. Better handling of async/await state machine coverage
2. Fix lambda coverage
3. Fix async/await/lambda false positive accounting

[driseley]

Thanks , the only other problem I can spot is in the sync lambda coverage - which again has the double counting:

otherwise it all looks good to me.

[SteveGilham]

@driseley have you checked the raw coverage file for visit counts? ReportGenerator's assessment of whether a line is covered can be optimistic when sequence points span many lines -- it gives you comfy reports with rounded up numbers that will make management feel warm fuzzies.

[driseley]

@SteveGilham - for the screenshot above:

        "System.Boolean DemoLibrary.DemoClass::InvokeAnonymous()": {
          "Lines": {
            "36": 1,
            "37": 1,
            "38": 2,
            "39": 2,
            "40": 0,
            "41": 0,
            "42": 0,
            "43": 2,
            "44": 2,
            "45": 2,
            "46": 1,
            "47": 1
          },

And OpenCover:

            <Method cyclomaticComplexity="4" nPathComplexity="0" sequenceCoverage="75" branchCoverage="75" isConstructor="False" isGetter="False" isSetter="False" isStatic="True">
              <Summary numSequencePoints="12" visitedSequencePoints="9" numBranchPoints="4" visitedBranchPoints="3" sequenceCoverage="75" branchCoverage="75" maxCyclomaticComplexity="4" minCyclomaticComplexity="4" visitedClasses="0" numClasses="0" visitedMethods="1" numMethods="1" />
              <MetadataToken />
              <Name>System.Boolean DemoLibrary.DemoClass::InvokeAnonymous()</Name>
              <FileRef uid="2" />
              <SequencePoints>
                <SequencePoint vc="1" uspid="36" ordinal="0" sl="36" sc="1" el="36" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="1" uspid="37" ordinal="1" sl="37" sc="1" el="37" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="2" uspid="38" ordinal="2" sl="38" sc="1" el="38" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="2" uspid="39" ordinal="3" sl="39" sc="1" el="39" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="0" uspid="40" ordinal="4" sl="40" sc="1" el="40" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="0" uspid="41" ordinal="5" sl="41" sc="1" el="41" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="0" uspid="42" ordinal="6" sl="42" sc="1" el="42" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="2" uspid="43" ordinal="7" sl="43" sc="1" el="43" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="2" uspid="44" ordinal="8" sl="44" sc="1" el="44" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="2" uspid="45" ordinal="9" sl="45" sc="1" el="45" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="1" uspid="46" ordinal="10" sl="46" sc="1" el="46" ec="2" bec="0" bev="0" fileid="2" />
                <SequencePoint vc="1" uspid="47" ordinal="11" sl="47" sc="1" el="47" ec="2" bec="0" bev="0" fileid="2" />
              </SequencePoints>
              <BranchPoints>
                <BranchPoint vc="1" uspid="37" ordinal="0" path="0" offset="8" offsetend="10" sl="37" fileid="2" />
                <BranchPoint vc="1" uspid="37" ordinal="1" path="1" offset="8" offsetend="33" sl="37" fileid="2" />
                <BranchPoint vc="0" uspid="39" ordinal="0" path="0" offset="4" offsetend="6" sl="39" fileid="2" />
                <BranchPoint vc="1" uspid="39" ordinal="1" path="1" offset="4" offsetend="37" sl="39" fileid="2" />
              </BranchPoints>
              <MethodPoint vc="9" uspid="0" p8:type="SequencePoint" ordinal="3" offset="3" sc="0" sl="36" ec="1" el="47" bec="0" bev="0" fileid="2" xmlns:p8="xsi" />
            </Method>

[MarcoRossignoli]

@driseley @SteveGilham the issue is clear to me...I cleaned up false positive on line but "other lines" are accounted again, the greatest problem here is understand how to exclude instrumentation of async lambda it's similar to async state machine(it has got MoveNext()) but inside there isn't user code, but at the moment I don't have idea how to exclude without remove user code.
To better explain the issue is "async lambda that call async code".

[driseley]

Hi Marco - just to be clear, in the example above - that was for sync code, not async. So could the issue be with lambda calls - not sync/async?

[MarcoRossignoli]

So could the issue be with lambda calls...

I think so the issue should be the same...I'll take a look again, should be more simple to fix if the pattern of issue is the same.