Add a new analyzer for checking record equality implementations

[jonathanou]

Justification

For C# record types, which are meant to represent a value type, the compiler will automatically generate Equals and GetHashCode such that the members of the record are compared for equality—this contrasts to class where the default Equals behavior is to compare reference equality of the class instance.

However, if the record contains members of IEnumerable, the default behavior of the generated Equals implementation is to compare the reference equality of the enumerable member, not the more expected value equality of each enumerable element.

Thus for any record that contains enumerables, it is important to manually implement Equals and GetHashCode such that the enumerables are compared via SequenceEqual if ordering matters, or UnorderedEquals—a helper method provided by NationalInstruments.Core—if not.

In the past, we've relied on code reviews to enforce this requirement, but it can get tedious and easy to miss. Additionally, it does not allow other teams that are not aware of the default record behavior to learn about the requirement.

By providing a custom analyzer, we can auto enforce that all record types with enumerables must provide a manually implemented Equals and GetHashCode, making code reviews easier and educates others about why the default record behavior doesn't work for enumerables.

Implementation

• Add a new analyzer that checks for a record type's properties to see if there are enumerables.
• Raise a new NI1019 warning if the record has enumerable properties but doesn't implement Equals.

Testing

Added several unit tests to verify the new warning fires or doesn't fire for various scenarios.

Built the analyzer and tested it in ASW codebase, made sure it shows the warning properly:

Encouragingly, the analyzer also found existing record violations in ASW that I will work on fixing later.

[pakdev]

Did you intentionally not check for a custom GetHashCode implementation? I assume someone already decided that exposing enumerables in records is fine in the first place (it seems like there could be issues beyond value equality)?

[jonathanou]

Did you intentionally not check for a custom GetHashCode implementation?

Yes. There is already a built-in C# warning CS0659 that emits a warning if you implement Equals but not GetHashCode, so I did not check for it (FYI this is also documented in the code as a comment).

I assume someone already decided that exposing enumerables in records is fine in the first place

That's definitely possible. I know some developers like to use record for its concise syntax shortcut (i.e. primary constructor syntax) to quickly create data structures that hold a bunch of variables, e.g:

internal record MyRecord(int MyInt, string MyString);

In those cases, it's true that equality isn't the main concern. However, starting in .NET 8, C# 12 supports primary constructor syntax for classes and structs too—so using records should now be reserved for truly wanting a value type class where equality works as expected.

And in my experience in code reviews, most are unaware of the enumerable equality problem (or forgot about it). For my team's code, equality is critically important, and we've had bugs in the past due to failure to override equality.

it seems like there could be issues beyond value equality?

The issue you referenced seems to stem from creating a custom enumerable type that performs value equality internally in its Equals implementation, so that the default record equality implementation will work out of the box.

That's a different approach than what my team's project has done. My team's project also uses standard immutable collection types in records heavily for json serialization/deserialization, and we've not run into the serialization issues mentioned in stackoverflow.

[pakdev]

Overall great job and I (we) appreciate the contribution - thanks for educating me on this issue!