Coverlet: Switch expression 0% coverage

[Tum4ik]

I use coverlet.msbuild v2.9.0.
I use "dotnet test" task in Azure DevOps:

Somehow the execution of the switch expression is skipped, but that is impossible. The switch expression is executed for sure in my code flow.

However ReSharper shows the correct coverage.

[MarcoRossignoli]

Thanks for reporting this, seems related to new c# pattern matching.

[Malivil]

It's definitely related to the new switch pattern. I have that in a few places in my code too running the same coverlet.msbuild version

[wdolek]

@MarcoRossignoli Since my code suffers from this _issue_ I decided to look at it. I started with additional method in Samples.cs and test for it, but I'm bit puzzled by what IL I see. For reason I don't understand, I'm getting two additional branches before and after switch itself - ending up with total 8 branch points.

I'm not getting that on Sharplab nor when I write code in LINQPad or in another assembly. As you can see from IL, expression switch shouldn't be different from regular switch.

Do you have any clue why this is happening? What am I missing?

Branche code generated before/after switch:

IL_0000: nop
IL_0001: ldc.i4.1
IL_0002: brtrue.s IL_0005 // <-- wat, why?!
IL_0004: nop
IL_0005: ldarg.1
IL_0006: ldc.i4.1
IL_0007: sub
IL_0008: switch IL_001b,IL_001f,IL_0027
...

[MarcoRossignoli]

Can you try in release to see if it disappears?that branch could be needed for debuggers, for instance to put on it a breakpoint

[wdolek]

@MarcoRossignoli You are right, I'm not getting those conditions in Release build. Also I think I might have some compiler options on, so that would explain why I didn't see it there before.

Anyway, I grabbed instrumented copy of method with switch expression and I do see instrumentation hits there just fine.

IL_0028: ldarg.1
IL_0029: ldc.i4.1
IL_002a: sub
IL_002b: switch (IL_003e, IL_0050, IL_0066)
IL_003c: br.s IL_007c
IL_003e: ldc.i4.s 53
IL_0040: call void Coverlet.Core.Instrumentation.Tracker.test__coverlet_instrumented::RecordHit(int32)
IL_0045: ldc.i4.s 54
IL_0047: call void Coverlet.Core.Instrumentation.Tracker.test__coverlet_instrumented::RecordHit(int32)
IL_004c: ldc.i4.m1
IL_004d: stloc.0
IL_004e: br.s IL_008e

Problem is not in instrumentation itself (luckily, because it is still good ol' switch - nothing broken here). At the moment, I'm not sure where to look next - will be glad for any input, or happily leave issue to someone experienced.

[MarcoRossignoli]

You should try to add a tmp test like https://github.com/coverlet-coverage/coverlet/blob/master/test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs#L14 and use FunctionExecutor.RunInProcess instead of FunctionExecutor.Run, with your sample you can live debug all instumentation and at the end you can only show like reportgenerator outcome here https://github.com/coverlet-coverage/coverlet/blob/master/test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs#L40 with extension result.GenerateReport(show:true)

Usually the two main point to check is branch getter https://github.com/coverlet-coverage/coverlet/blob/master/src/coverlet.core/Instrumentation/Instrumenter.cs#L540
And after understand if code is passed over instrumented code(i.e. IL_003e: ldc.i4.s 53 hit number 53) live checking this method https://github.com/coverlet-coverage/coverlet/blob/master/src/coverlet.core/Coverage.cs#L372 where we load hits file(raw file with line/hits count mapping). Sometime the issue is that we remove wrong branch/piece of code for bad pattern recognition.

[wdolek]

@MarcoRossignoli I spent some time trying to understand what's going on and I have to give up (for now).

What I tried:

As I wrote before, I can see switch between two ifs (in Debug). If I was following code correctly, Coverage.CalculateCoverage behaves somehow expectable - except it gets 0 hits from preceding if and marks everything not reached because that if spans over exact same lines as switch.

I redirected my attention back to instrumenter to check branch points and line ranges. Branch points, at least regarding preceding if, look fine, however I'm not sure with sequence point afterwards - and that's where I ended.

Since this _domain_ is completely new for me, I'm honestly not even sure if my observation is correct - so for anyone else looking into this issue, don't take my findings for granted.

[MarcoRossignoli]

@wdolek every idea/thought is welcome and will help everyone will move on this, thanks a lot for your effort!I'm bit busy but I'll take a look asap and I'll start from your analysis, so many many thanks!

[costin-zaharia-sonarsource]

We hit this problem also on our side. It can be easily reproducible:

public static int Compute(object x)
{
    return x switch
    {
        int number => number,
        string value => value.Length,
        _ => -1
    };
}

[wdolek]

Not that it changes anything, just be aware that those short switches are actually compiled to bunch of simple ifs - still, in Debug, all that is sandwiched between empty ifs, where one spans across whole method - which is weird.

@MarcoRossignoli I'm looking forward to see what's wrong here 🤞

[MarcoRossignoli]

other repro https://github.com/danielpalme/ReportGenerator/issues/362#issuecomment-660644441

[MarcoRossignoli]

After some investigation I found the issue is similar to lambda closure, compiler for new switch pattern emits overlapped sequence point, so we need to understand how to do correct accounting

[fahadabdulaziz]

Hello,
Is there any estimate when this issue cloud be resolved or at least workaround.

Thanks

[MarcoRossignoli]

I don't have ETA but I have some ideas on how to solve that it's better than nothing 😄 I'll try to fix asap!
I apologize for the delay.

at least workaround.

Unfortunately there is no workaround is related to the way C# compiler emits sequence for new pattern, we have to fix in code.

[fahadabdulaziz]

Thank you , hope you all the best 👍

[daveMueller]

@MarcoRossignoli Can I help here somehow? I just took a peek at the IL and see an additional branch point and a sequence point that spans over multiple lines. I guess we could try to find a match for the switch pattern and ignore the branch point. Still this doesn't seem to be enough.

[MarcoRossignoli]

My idea was bit different, we could count in different way here ~ https://github.com/coverlet-coverage/coverlet/blob/master/src/coverlet.core/Coverage.cs#L384

We could create some structure to keep the actual accounted line for every sequence, try to explain better, suppose to have 3 overlapped sequences record similar to nested closure or switch:

1 sequence start line 10 end line 18 -> 10 hit
2 sequence start line 13 end line 17 -> 5 hit
3 sequence start line 14 end line 16 -> 0 hit

We could build some metadata for every range sequence where we report the "line to not account" because is accounted by a nested sequence and my statement is that the most nested sequence range is the right one. So in the case above we could end with something like

line = hits count

10 = 10
11 = 10
12 = 10
13 = 5
14 = 0
15 = 0
16 = 0
17 = 5
18 = 10

Are you able to try this alg? The changes should be done here https://github.com/coverlet-coverage/coverlet/blob/master/src/coverlet.core/Coverage.cs#L384-L444
In this way we should fix one for all the "overlapped" sequence(lines) accounting issues.

This month I'm very busy so I think I won't have enough time until next month, I was experimenting here https://github.com/coverlet-coverage/coverlet/compare/master...MarcoRossignoli:issue_936_switch take my test if you want(ignore the patch code doesn't work as-is is only a test I was in debugging).
Could be great fix this issue...in my plan it's the last thing to fix before 3.0.0 release because a lot of people are using new switch pattern.
@daveMueller if you need further assistance we can also have a talk/call on skype.

[daveMueller]

Yes OK I got it. I will try this and get back to you when I'm facing some issues.

[daveMueller]

I rapidly prototyped this and at a first glimpse it seemed to work out.

Then I tested a bit with value and switch in separate rows and it immediately failed.

What was bugging me all the time when looking at the IL was this first branch point that seems to be some internal null check or so. And I'm pretty sure that this is the problem. Because of that our instrumentation instruction for the 2nd sequence point is never hit. This can be seen in the screenshot of the IL that was posted before. Just to make it a bit more clear here again.

I'm still trying to figure out a solution for that but every idea/thought on this is very welcome.

[MarcoRossignoli]

Can you share your test branch so I can clone and debug?

Btw the switch coverage seems good, right? I mean the case are correctly reported?

Does it work with all other tests?

We can try to release a first version with the "correct branch coverage" because it's not so important that last sequence (for the moment). And after try to skip compiler injected branches.

Can you try to run in release?

[daveMueller]

Can you share your test branch so I can clone and debug?

Sure but this is really early stage (https://github.com/daveMueller/coverlet/tree/936_SwitchExpression).

Btw the switch coverage seems good, right? I mean the case are correctly reported?

Yes cases are correctly reported.

Does it work with all other tests?

Up to now I didn't test too much. I don't know if I broke something and if the other unit tests are still green.

We can try to release a first version with the "correct branch coverage" because it's not so important that last sequence (for the moment). And after try to skip compiler injected branches.

See next answer.

Can you try to run in release?

I checked the IL of the release and this compiler injected branchpoint is gone. Now the result of the implementation is also looking good. :smirk:

Is it a requirement that coverlet also reports correctly for debug builds? If not I could ignore the problem I'm facing with the debug builds.

[MarcoRossignoli]

Is it a requirement that coverlet also reports correctly for debug builds? If not I could ignore the problem I'm facing with the debug builds.

Usually I ask to do coverage in debug because pdb is more precise/rich but with it there are also more false/true positive/negative. So my opinion here for now is that it's hard have a very precise % on coverage and usually a dev should check % but also take a look at report of branches/lines to understand the needs of more tests. In case of switch I prefer a correct branch cases coverage that a false negative on a bracket. Also because there are some place where is very very hard recognize correctly the pattern and so a false negative of 1 line shouldn't be a problem on % number.

I'll go for this fix for now and after wait for feedbacks.

Run a complete test and let me know!

Thanks a lot for the effort!

[daveMueller]

I got a question regarding coverage of a simple if statement. Has the line coverage for the else keyword always been 'Not coverable'?

I ask this because with my current implementation the test Lambda_Issue343 fails. The test requires the else keyword to be hit. I personally think that the test passed before, because the hit of the else keyword was just a result of the overlapping sequence points. I didn't confirm this yet but I'm pretty sure that's the reason that it now fails.

[MarcoRossignoli]

If it expected in test i think it was hit, you should try to understand why, I don't remember overlapped sequences but my memory could fail. Thanks a lot for the effort Dave.

[MarcoRossignoli]

@daveMueller some more info on this, we can skip generated sequences/branches looking for this pattern https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Expression%20Breakpoints.md
Thanks to @jakubch1 for the hint.

[MarcoRossignoli]

@daveMueller how is going with this?Let me know if you're busy so I can go on based on your idea and Jakub hint to skip first branche.

[daveMueller]

@MarcoRossignoli sorry was a bit busy lately on work. I've prepared PR today but then I've seen your PR and I also think this is the best and quickest solution. I add my thoughts to your PR.

[fahadabdulaziz]

@MarcoRossignoli I see this issue is fixed, when do you think we can get the update?

Thank you

[MarcoRossignoli]

Asap, I would try to fix one last issue in record, but if won't work we plan to release before end of year

[fahadabdulaziz]

Thank you.. 👍