Skip to content

[Analyzer Proposal]: Don't include non-IEquatable structs in records #102593

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

Open
Youssef1313 opened this issue May 23, 2024 · 2 comments
Open

[Analyzer Proposal]: Don't include non-IEquatable structs in records #102593

Youssef1313 opened this issue May 23, 2024 · 2 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@Youssef1313
Copy link
Member

Youssef1313 commented May 23, 2024

Background and motivation

Using records in C# can be tricky (performance-wise) if the developer isn't aware of some implementation details. The compiler generates the equality implementation using EqualityComparer<T>.Default, which is ObjectEqualityComparer for a struct that is not IEquatable<T>.

Using ObjectEqualityComparer will cause boxing for every equality operation being done.

API Proposal

An analyzer that warns if a record primary constructor has a parameter which is a value type (struct) that doesn't implement IEquatable<T> where T is the same value type.

For example:

using System.Diagnostics.CodeAnalysis;

var bad1 = new BadRecord(default);
var bad2 = new BadRecord(default);

Console.WriteLine(bad1 == bad2); // boxing because `ObjectEqualityComparer` is being used

public struct MyPoint
{
	public int X { get; }
	public int Y { get; }

	public MyPoint(int x, int y)
	{
		X = x;
		Y = y;
	}

    public override bool Equals([NotNullWhen(true)] object? obj)
    {
        return obj is MyPoint point &&
               this == point;
    }

    public static bool operator ==(MyPoint left, MyPoint right)
    {
        return left.X == right.X && left.Y == right.Y;
    }

    public static bool operator !=(MyPoint left, MyPoint right)
    {
        return !(left == right);
    }

    public override int GetHashCode()
    {
        return HashCode.Combine(X, Y);
    }
}

public record struct BadRecord(MyPoint Point); // Produce warning here

API Usage

// Fancy the value
var c = new MyFancyCollection<int>();
c.Fancy(42);

// Getting the values out
foreach (var v in c)
    Console.WriteLine(v);

Alternative Designs

No response

Risks

No response

@Youssef1313 Youssef1313 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 23, 2024
@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 23, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 23, 2024
@huoyaoyuan huoyaoyuan added the code-analyzer Marks an issue that suggests a Roslyn analyzer label May 23, 2024
@EgorBo
Copy link
Member

EgorBo commented May 23, 2024

Using ObjectEqualityComparer will cause boxing for every equality operation being done.

I think it's rather an unfortunate RyuJIT's limitation (mainly, #9118) and should be fixed in future, so should we introduce temporarily analyzers?

What I guess makes sense to do is to warn if a struct doesn't override Equals at all probably (if it's not already a warning/suggestion).

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@vcsjones vcsjones removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 24, 2024
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Jul 2, 2024
@stephentoub stephentoub added this to the Future milestone Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

No branches or pull requests

6 participants