Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat records equal to classes #1576

Open
daveMueller opened this issue Jan 2, 2024 · 5 comments
Open

Treat records equal to classes #1576

daveMueller opened this issue Jan 2, 2024 · 5 comments
Labels
discussion Generic discussion on something

Comments

@daveMueller
Copy link
Collaborator

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

@github-actions github-actions bot added the untriaged To be investigated label Jan 2, 2024
@tengl
Copy link

tengl commented Jan 31, 2024

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.

@daveMueller
Copy link
Collaborator Author

daveMueller commented Feb 3, 2024

@tengl This is another issue we already fixed with #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

@tengl
Copy link

tengl commented Feb 4, 2024

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

@daveMueller daveMueller added discussion Generic discussion on something and removed untriaged To be investigated labels Feb 6, 2024
@daveMueller
Copy link
Collaborator Author

@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.

@MarcoRossignoli
Copy link
Collaborator

Fine to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Generic discussion on something
Projects
None yet
Development

No branches or pull requests

3 participants