coverlet icon indicating copy to clipboard operation
coverlet copied to clipboard

Treat records equal to classes

Open daveMueller opened this issue 1 year ago • 6 comments

This is a proposed design change related to records. As records in the IL are just classes with some generated methods we should also treat them like classes. With issue #1139 we implemented records (with primary constructor) to be completely excluded from instrumentation when auto properties are skipped. This is somehow troublesome because in the IL there is no difference to classes or records with auto properties. As an example, when we SkipAutoProps the instrumentation of a record with a primary consturctor is completely skipped and looks like this:

grafik

If we now compare this to an equivalent class with the same coverage parameters, the assignement in the constructor is still beeing instrumented (even with SkipAutoProps).

grafik

I would suggest to do the same for records. As assignement in the constructor and definition of properties is all part of the primary constructor, it also should be instrument even if auto properties are skipped.

grafik

daveMueller avatar Jan 02 '24 00:01 daveMueller

I guess the same would apply for records with default constructor, like this:

public record SomeType
{
    public string? Optional { get; init; }
    public required string Required { get; init; }
}

If I change to class, I get 100 % coverage, but with record I get 66% coverage.

tengl avatar Jan 31 '24 14:01 tengl

@tengl This is another issue we already fixed with https://github.com/coverlet-coverage/coverlet/pull/1575. The roslyn team changed something in the compiler with the release of net8. They emit additional sequence points for records to get a better debugging experience etc... You can try to consume our nightly build to get coverage back to 100%.

grafik

daveMueller avatar Feb 03 '24 21:02 daveMueller

@daveMueller that's great, I'll wait for the NuGet update.

tengl avatar Feb 04 '24 18:02 tengl

@MarcoRossignoli @bert2 what do you think about this. I think we would have less issues with records if we would undo the changes we did for #1139.

daveMueller avatar Feb 28 '24 12:02 daveMueller

Fine to me

MarcoRossignoli avatar Feb 28 '24 12:02 MarcoRossignoli

This issue is stale because it has been open for 3 months with no activity.

github-actions[bot] avatar Jun 02 '24 01:06 github-actions[bot]