C# record constructors are not covered when SkipAutoProps is true

[jlawrence]

Describe the bug
C# record constructors are marked as uncovered whenever SkipAutoProps is true. But if SkipAutoProps is false, the coverage is correct.

To Reproduce

1. Create a file with a simple record
   public record MyRecord(int A);

2. Create a test that constructs the record
   var myRecord = new MyRecord(1);

Minimal Example: https://github.com/jlawrence/coverlet-missing-line-example/tree/main

Expected behavior
The record constructor should be covered regardless of the value of SkipAutoProps.

Actual behavior
Observed behavior: If SkipAutoProps is true, the record constructor will not be covered. If it is false, the record constructor will be covered.

Configuration:
* Coverlet package: "coverlet.collector" Version="6.0.0"
* .NET 8
* Windows 10 x64

[daveMueller]

Thanks for reporting. I didn't find time to look into this in detail but I guess the problem is the primary constructor. When there a new C# language features we often need to adapt to them. Especially if it's a feature where the compiler generates a lot of things like with primary constructors.
@Bertk @MarcoRossignoli I think we should look into primary constructors in general. Besides the primary constructors for records there is now also a variant of them for classes in C# 12.
Probably the easiest solution would be to skip them from coverage as they are just mere method signatures. On the other hand we could try to handle them like e.g. auto properties or so.
What do you think about it?
There is already another issue that also reports issues with primary constructors #1555.

[Bertk]

@daveMueller @MarcoRossignoli I am not sure whether it is possible to always ignore primary constructors. Maybe the skip auto property solution will be possible.

By the way, the current VS2022 preview (Version 17.9.0 Preview 1.1) also shows it as "Not Covered (Lines)"

There is a roslyn issue "Primary constructor properties have sequence points but breakpoints can't be placed" which has some additional information. Maybe we should implement this as default behavior.

5. test coverage should ignore sequence points in compiler generated code

[Bertk]

@jlawrence Could you please use the latest coverlet.collector preview from main branch and add ExcludeByAttribute option (see below).

This will ignore the primary constructor and generate a coverage.opencover.xml with visitedSequencePoints="0" for your repo.
Please send your feedback whether this is acceptable for your code coverage results.

nuget.config:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <!--To inherit the global NuGet package sources remove the <clear/> line below -->
    <clear />
    <add key="nuget" value="https://api.nuget.org/v3/index.json" />
    <add key="coverlet-nightly" value="https://pkgs.dev.azure.com/tonerdo/coverlet/_packaging/coverlet-nightly/nuget/v3/index.json" />
  </packageSources>
</configuration>

coverlet.runsetting:

<?xml version="1.0" encoding="utf-8"?>
<RunSettings>
  <DataCollectionRunSettings>
    <DataCollectors>
      <DataCollector friendlyName="XPlat code coverage">
        <Configuration>
          <SkipAutoProps>true</SkipAutoProps>
          <ExcludeByAttribute>Obsolete,GeneratedCodeAttribute,CompilerGeneratedAttribute</ExcludeByAttribute>
        </Configuration>
      </DataCollector>
    </DataCollectors>
  </DataCollectionRunSettings>
</RunSettings>

[daveMueller]

@daveMueller @MarcoRossignoli I am not sure whether it is possible to always ignore primary constructors. Maybe the skip auto property solution will be possible.

There is a roslyn issue "Primary constructor properties have sequence points but breakpoints can't be placed" which has some additional information. Maybe we should implement this as default behavior.

For records it's probably easy to skip the sequence points in the generated properties but for classes it generates instance fields. I admit I haven't looked into the IL for those two scenarios yet. But I'm interessted and could work on this as vacation time is approaching. Like @Bertk already started I would also look into how the other coverage tools are currently handling this.

[jlawrence]

@Bertk I tried out your solution, but excluding CompilerGeneratedAttribute has the undesired side effect of not covering async methods. Here are steps to reproduce:

1. Clone https://github.com/jlawrence/coverlet-missing-line-example/tree/coverlet-nightly
2. Check out the coverlet-nightly branch.
3. Follow instructions in readme file. You can see that async methods are no longer included in the coverage results.

[Bertk]

@jlawrence I see, So you have to wait until @daveMueller will provide a better solution.

[daveMueller]

OK I found some time to look into this now. It seems we don't have an issue with primary constructors on classes as the compiler only generates fields without any additional methods or assignments. For records we already tackled this issue years back with this PR #1159. But it seems the roslyn team changed something in the generated IL. There now is a second ctor generated that has a sequence point that we instrument erroneously.
I already prototyped a fix for that and will create a PR in the next days.

What I noticed while analyzing this is that there already is a unit test for this scenario that should have failed our CI build (SkipRecordWithProperties). The test also fails when I execute it locally. But the current CI build doesn't report any failed test. Looking into the test report of our latest CI build I found some entries that I can't really interpret. First of all it says this test is skipped, but I couldn't figure out the reason.

TpTrace Verbose: 0 : 5880, 11, 2023/12/22, 08:42:59.352 ... Coverlet.Core.Tests.CoverageTests.SkipRecordWithProperties [SKIP]"}}

And a bit deeper in the test log it says it failed for unknown reason, but the build job still succeeds?

"TestCase": {
    "Id": "f1fa401c-81a1-f20c-97b5-c97033736c40",
    "FullyQualifiedName": "Coverlet.Core.Tests.CoverageTests.SkipRecordWithProperties",
    "DisplayName": "Coverlet.Core.Tests.CoverageTests.SkipRecordWithProperties",
    "ExecutorUri": "executor://xunit/VsTestRunner2/netcoreapp",
    "Source": "D:\\a\\1\\s\\artifacts\\bin\\coverlet.core.tests\\debug\\coverlet.core.tests.dll",
    "CodeFilePath": null,
    "LineNumber": 0,
    "Properties": []
},
"Attachments": [],
"Outcome": 3,
"ErrorMessage": "fails reason unknown (no session debug possible)",
"ErrorStackTrace": null,
"DisplayName": "Coverlet.Core.Tests.CoverageTests.SkipRecordWithProperties",
"Messages": [],
"ComputerName": "fv-az902-962",
"Duration": "00:00:00.0010000",
"StartTime": "2023-12-22T08:42:59.3532843+00:00",
"EndTime": "2023-12-22T08:42:59.3532844+00:00",
"Properties": []

@MarcoRossignoli @Bertk Any idea why this test is skipped on CI build?

[Bertk]

@daveMueller I am not sure why the test is skipped but I noticed failed tests of CoverageTests.SkipRecordWithProperties with PR #1570.
By the way, master branch uses an old version V2.10.0 of Microsoft.CodeAnalysis.CSharp nuget package. The current version is V4.8.0.

[daveMueller]

OK thanks, I'll check this again once I'm finalizing the fix.