ical.net Implement GetHashCode and Equals in terms of the type hierarchy

Extending some of the ideas from #271, calendar components should delegate to their parents the hashing and equality-checking for the properties that their parent types own, and simplify down to the properties that the child type owns.

For example, Event does its own thing, even though there is a type hierarchy above it that it completely ignores. It should probably use the type hierarchy instead. This way, bugfixes and feature enhancements will propagate to the other child types as well.

This may require spending some time understanding the relationship between UniqueComponent and RecurringComponent, and moving some of the hashing and equality logic to other places.

Apr 28 '17 15:04 rianjs

C.f. #393

May 24 '18 13:05 rianjs

Here we have classes overriding Equals().

The equality comparison is quite a burden for the bigger classes. When it comes to GetHashCode there are many of them using mutable fields, which limits the usability in hash-based collections anyway. Often it's tricky to decide which fields should be included in the comparison, and which should not.

For which of the classes do we see a practical advantage to override Equals()? I have the low-level classes in mind as candidates.

@minichma @rianjs What do you think?

Code File Line Column

[ ] public override bool Equals(object obj) | Calendar.cs 93 4
[ ] public override bool Equals(object obj) | CalendarCollection.cs 95 4
[ ] public override bool Equals(object? obj) | CalendarComponents\CalendarEvent.cs 310 4
[ ] public override bool Equals(object obj) | CalendarComponents\Journal.cs 39 4
[ ] public override bool Equals(object obj) | CalendarComponents\RecurringComponent.cs 221 4
[ ] public override bool Equals(object obj) | CalendarComponents\UniqueComponent.cs 98 4
[ ] public override bool Equals(object obj) | CalendarComponents\VTimeZone.cs 368 4
[ ] public override bool Equals(object? obj) | CalendarObject.cs 59 4
[ ] public override bool Equals(object obj) | DataTypes\AlarmOccurrence.cs 54 4
[ ] public override bool Equals(object obj) | DataTypes\Attachment.cs 104 4
[ ] public override bool Equals(object obj) | DataTypes\Attendee.cs 282 4
[X] public override bool Equals(object? obj) | DataTypes\CalDateTime.cs 241 4
[ ] public override bool Equals(object obj) | DataTypes\GeographicLocation.cs 55 4
[ ] public override bool Equals(object obj) | DataTypes\Occurrence.cs 30 4
[ ] public override bool Equals(object? obj) | DataTypes\Organizer.cs 93 4
[X] public override bool Equals(object? obj) | DataTypes\Period.cs 122 4
[ ] public override bool Equals(object? obj) | DataTypes\PeriodList.cs 90 4
[ ] public override bool Equals(object? obj) | DataTypes\RecurrencePattern.cs 164 4
[X] public override bool Equals(object obj) | DataTypes\RequestStatus.cs 72 4
[X] public override bool Equals(object obj) | DataTypes\StatusCode.cs 67 4
[ ] public override bool Equals(object obj) | DataTypes\Trigger.cs 101 4
[X] public override bool Equals(object obj) | DataTypes\UTCOffset.cs 48 4
[X] public override bool Equals(object obj) | DataTypes\WeekDay.cs 43 4
[ ] public override bool Equals(object obj) | VTimeZoneInfo.cs 50 4

Jan 19 '25 14:01 axunonb

@axunonb Agree that Equals doesn't make too much sense in many classes. Agree to your list. Maybe double-check which types we use HashSet, Dictionary, OrderBy, ... (ask CoPilot which others) on (i.e. those that use Equals/GetHashCode under the hoods). Sometimes it might even be better to have an external Comarator or EqualityComparator for a specific use than implementing Equals where there is no single natural equality.

Jan 20 '25 20:01 minichma

Here is the revised list after checking manually whether Equals or GetHashCode are in use, and how.

[ ] public override bool Equals(object obj) | Calendar.cs | equality tests
[ ] public override bool Equals(object obj) | CalendarCollection.cs | equality tests
[ ] public override bool Equals(object? obj) | CalendarComponents\CalendarEvent.cs | equality tests
[ ] public override bool Equals(object obj) | CalendarComponents\Journal.cs | equality tests
[ ] public override bool Equals(object obj) | CalendarComponents\RecurringComponent.cs | equality tests
[ ] public override bool Equals(object obj) | CalendarComponents\UniqueComponent.cs | not used at all
[ ] public override bool Equals(object obj) | CalendarComponents\VTimeZone.cs | equality tests
[X] public override bool Equals(object? obj) | CalendarObject.cs | IN USE
[ ] public override bool Equals(object obj) | DataTypes\AlarmOccurrence.cs | not used at all
[ ] public override bool Equals(object obj) | DataTypes\Attachment.cs | equality tests
[ ] public override bool Equals(object obj) | DataTypes\Attendee.cs | equality tests
[X] public override bool Equals(object? obj) | DataTypes\CalDateTime.cs | IN USE
[ ] public override bool Equals(object obj) | DataTypes\GeographicLocation.cs | equality tests
[X] public override bool Equals(object obj) | DataTypes\Occurrence.cs | IN USE
[ ] public override bool Equals(object? obj) | DataTypes\Organizer.cs | not used at all
[X] public override bool Equals(object? obj) | DataTypes\Period.cs | IN USE
[ ] public override bool Equals(object? obj) | DataTypes\PeriodList.cs | equality tests
[ ] public override bool Equals(object? obj) | DataTypes\RecurrencePattern.cs | via CalendarEvent, equality tests
[X] public override bool Equals(object obj) | DataTypes\RequestStatus.cs | not used at all
[X] public override bool Equals(object obj) | DataTypes\StatusCode.cs | not used at all
[ ] public override bool Equals(object obj) | DataTypes\Trigger.cs | equality tests
[X] public override bool Equals(object obj) | DataTypes\UTCOffset.cs | via Calendar, equality tests
[X] public override bool Equals(object obj) | DataTypes\WeekDay.cs | via RecurrencePattern and Attachment, equality tests
[ ] public override bool Equals(object obj) | VTimeZoneInfo.cs | not used at all

Jan 21 '25 18:01 axunonb

For types that are inherently mutable (such as Calendar, whose events, todos, or properties can change), implementing IEquatable<T> and IComparable<T> should be avoided or better removed to ensure more reliable behavior. We can keep it for immutable types.
In general, a custom interface like IEquivalent<T> with an IsEquivalentTo(other) method can be useful for comparisons, especially if it provides detailed information about the locations of any differences.
Compare-Net-Objects may be a suitable option for users looking for deep comparison when performance is not a primary concern, even though it is primarily designed for unit testing scenarios.

May 31 '25 09:05 axunonb

It would be nice to have some sort of comparer for something like a Recurrence Pattern. Pre v5 we would compare patterns to know if they changed and relied on the Equals implementation. An external comparer for some of the classes like that could be useful. At present we are just going to implement a custom comparer type using the previous implementation

Jun 18 '25 15:06 pinkfloydx33

@pinkfloydx33 Maybe just serialize both and compare the strings?

Jun 18 '25 15:06 minichma

Oh... hadn't considered that! It looks like the serializer also normalizes the string (reorders the sub-components) which is precisely what we are hoping for... so that would be something we could definitely do. But I assume that's less performant than a manual comparison algorithm (like the code removed). hrmm 🤔

Jun 18 '25 15:06 pinkfloydx33

The reason why they have been removed is that its not always obvious what equality means. In your example of RecurrencePattern: are two instances still equal if any of the BY arrays is reordered? Not sure, really depends on the context. The produced recurrences would be the same but while interacting with it on the UI it might be considered different. So when trying to keep the API clean and understandable, we thought it would be better to let the user decide on their own.

Regarding the performance, I wouldn't expect it to be a noticeable difference, except under very high load. If you think this is an issue, please do some measurements and let us know what the real-life impact would be in your case.

Jun 18 '25 15:06 minichma

Sure, makes total sense. In our case re-ordering of the arrays would technically be considered equal... but we also don't expect them to ever be anything but ascending. We control both sides of the problem here, but we have multiple consumers using the appropriate ICAL library for their ecosystem. The biggest issue we have is the sub-components being out of order. I think otherwise the BY arrays are always ascending. With that said I think your solution of serializing and comparing is probably the route to go

Thanks

Jun 18 '25 16:06 pinkfloydx33