[pigeon] Optimize data class equality and hashing in Dart, Kotlin, java, and Swift, adds equality in other languages

[tarrinneal]

This PR optimizes the generated equality (==) and hashing (hashCode/hash) logic for data classes across All languages. These have been bothering me for a while, and I wanted to break out a new pr before the NI code is in review.

[gemini-code-assist]

Code Review

This pull request introduces significant performance optimizations for data class equality and hashing in Dart, Kotlin, and Swift by avoiding the creation of intermediate lists. The changes are well-implemented and include necessary updates to helper utilities like deepEquals and deepHash for robust recursive comparisons. The addition of generator tests for each language is a great way to ensure the new logic is correct. I have a couple of minor suggestions to improve the generator code's maintainability.

[tarrinneal]

/gemini review

[tarrinneal]

@stuartmorgan-g this should be ready for review

[gemini-code-assist]

Code Review

This pull request introduces a significant and wide-ranging change to optimize and standardize equality and hashing for data classes across all supported languages. It adds new helper functions for deep equality and hashing, which is a great step towards cross-platform consistency. However, I've found a critical logic bug in the new map equality implementation across several language generators (Dart, Kotlin, Swift, Objective-C, C++) that could lead to incorrect results. Additionally, the C++ map hashing implementation is inconsistent with other platforms, which undermines the goal of uniform hashing. These issues need to be addressed before merging.

_{Note: Security Review did not run due to the size of the PR.}

[stuartmorgan-g]

Capturing for posterity, this has the same inefficiency as the previous iteration, where we are feeding known-type values through a big type-inspection chain instead of dispatching to specific-type equality checkers.

Not something we need to address now since we still mostly expect these to be used in tests where efficiency doesn't matter, but something to keep in mind later if it performance ever comes up.

[tarrinneal]

This doesn't actually, any non-collections well return first before any type checking. Even collections are only checking for collection types.

[tarrinneal]

You're probably thinking about the codec, which I intend to change so known types bypass the type checking soon(tm)

[stuartmorgan-g]

This doesn't actually, any non-collections well return first before any type checking.

I'm not following. The first line in this statement is calling pigeonDeepEquals with two strings. We know at compile time they are strings. But we are calling pigeonDeepEquals for them which will do runtime checks for whether they are:

• byte arrays
• int arrays
• long arrays
• double arrays
• Lists
• Maps
• Doubles
• Floats

and only then, after all of that, check that the strings are equal.

[tarrinneal]

the first thing pigeon deep equals does is return if identical

[stuartmorgan-g]

Right, but that's only pointer equality for most types. For two instances of "Foo", a == b at the top is false, only a.equals(b) at the very end is true, after all the type checks. Similarly, two Map instance variables that aren't actually the same map, but will evaluate to the same under the deep equality evaluation, will go through 5 pointless type checks first. Similarly with List. Similarly with Double and Float.

Basically, in any case where we actually need all this logic in the first place and can't just rely on == alone, we are doing a bunch of type checks.

[tarrinneal]

For two instances of "Foo", a == b at the top is false, only a.equals(b) at the very end is true

That is a detail I missed. I've resolved this in every language (that I can).

My future work for changing the codec calls to skip the type checking (this will be relevant once native interop lands) will change this to avoid type checking also.

[stuartmorgan-g]

I don't think the quick fix of moving the non-trivial equality check to the top is something we want to do, because it may be non-trivially expensive, and duplicative of specific checks. E.g., I'm almost positive this will compare non-equal strings twice in Obj-C instead of once, which is potentially more expensive than the thing I was describing in the initial comment.

The fix here (which again, doesn't need to be done now, I just want to capture it for later reference) isn't to change how pigeonDeepEquals works, it's to not generate calls to pigeonDeepEquals in the first place when we already know the type. Instead, all the specific checks should be helpers (pigeonMapDeepEquals, etc.), pigeonDeepEquals should be implemented using those helpers, and the generator for class equality should be calling the correct helper for each field instead of the generic helper.

[tarrinneal]

That is true, reverted.

[tarrinneal]

@stuartmorgan-g your comments are addressed, I'm still planning on reading up a bit on things before landing.

[stuartmorgan-g]

Minor notes (mostly orthodontia), but otherwise LGTM.

[stuartmorgan-g]

This doesn't actually, any non-collections well return first before any type checking.

I'm not following. The first line in this statement is calling pigeonDeepEquals with two strings. We know at compile time they are strings. But we are calling pigeonDeepEquals for them which will do runtime checks for whether they are:

• byte arrays
• int arrays
• long arrays
• double arrays
• Lists
• Maps
• Doubles
• Floats

and only then, after all of that, check that the strings are equal.

[tarrinneal]

(mostly orthodontia)

Can we add a gobject lint tool?

[tarrinneal]

@stuartmorgan-g done done

[tarrinneal]

@stuartmorgan-g While I technically need your review. Gemini said: "You have essentially achieved the holy grail of cross-platform plugin design. You maintained strict, identical behavioral output while perfectly adapting to the efficiency constraints and memory safety rules of each specific compiler."

So I think I'm set.

[stuartmorgan-g]

LGTM