Skip to content

Fix Equals and GetHashCode for types containing Lists and Arrays in C# #2710

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

Merged
merged 20 commits into from
May 12, 2025

Conversation

kazimuth
Copy link
Contributor

@kazimuth kazimuth commented May 6, 2025

Description of Changes

Addresses clockworklabs/com.clockworklabs.spacetimedbsdk#313 .

We used to rely on List.Equals when comparing lists for equality. However, this is wrong: List.Equals uses reference equality in C#, not sequence equality. Similarly for GetHashCode, and for both methods on arrays.

I wanted to do something reflection-y to generate EqualityComparers for any Lists we need at runtime. However, I realized that with IL2CPP, this may be very slow, especially with nested lists. Equals and GetHashCode are now in the hot path of various SDK operations so I want them to be fast.

So, I bit the bullet and just started generating nested loops whenever I see an array or list in a type marked [SpacetimeDB.Type]. This is efficient and should generate fast code with IL2CPP. However, it's kind of zany, and wants careful review.

I also made our null checking more conservative and started checking for nulls even for reference types that aren't explicitly marked nullable.

API and ABI breaking changes

N/A

Expected complexity level and risk

2: this is fairly complicated generated code that needs to be correct and fast.

Testing

  • Test suite
  • Added some more proptests that randomly generate types containing nested lists.
  • Blackholio
  • Bitcraft

@kazimuth kazimuth force-pushed the jgilles/fix-list-equals branch from b0fb980 to 63dcdc4 Compare May 7, 2025 18:47
Copy link
Contributor

@rekhoff rekhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is complex, but well documented. The snapshot examples help in seeing the resulting behavior. I also tested against the Blackholio repo and verified regenerated bindings behave correctly.
Approved!

@kazimuth kazimuth closed this May 9, 2025
@kazimuth kazimuth force-pushed the jgilles/fix-list-equals branch from 31dcc37 to 2699c03 Compare May 9, 2025 17:19
@kazimuth kazimuth reopened this May 9, 2025
@kazimuth
Copy link
Contributor Author

kazimuth commented May 9, 2025

Whoops

@kazimuth kazimuth force-pushed the jgilles/fix-list-equals branch from c27bf8a to 0c741a6 Compare May 12, 2025 17:19
@kazimuth kazimuth added this pull request to the merge queue May 12, 2025
Merged via the queue into master with commit 74ad4a1 May 12, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants