Skip to content

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

Open
tarrinneal wants to merge 34 commits intoflutter:mainfrom
tarrinneal:effic
Open

[pigeon] Optimize data class equality and hashing in Dart, Kotlin, java, and Swift, adds equality in other languages#11140
tarrinneal wants to merge 34 commits intoflutter:mainfrom
tarrinneal:effic

Conversation

@tarrinneal
Copy link
Contributor

@tarrinneal tarrinneal commented Feb 28, 2026

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.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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 tarrinneal changed the title [pigeon] Optimize data class equality and hashing in Dart, Kotlin, and Swift [pigeon] Optimize data class equality and hashing in Dart, Kotlin, and Swift, adds equality in other languages Mar 4, 2026
@tarrinneal
Copy link
Contributor Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a significant and valuable improvement by optimizing and standardizing data class equality and hashing across all supported languages. The new pigeonDeepEquals and pigeonDeepHashCode (or equivalent) helper functions provide more robust and consistent behavior, especially for complex data types and collections. I've identified a few minor inconsistencies in the handling of signed zero for double values in the C++, GObject, and Swift generators. Addressing these will help fully achieve the goal of cross-platform consistency.

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

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces significant improvements to the equality and hashing logic for data classes generated by Pigeon. It adds custom deepEquals and deepHash helper functions for various languages (Java, Kotlin, Swift, Objective-C, C++, Dart) to ensure consistent behavior across platforms, especially for complex types like lists, maps, and nested objects. The data class equals and hashCode (or equivalent) methods are updated to use these new helpers, making them more efficient and correct. Equality support is also added for languages that were missing it (C++, Objective-C). The changes are extensive, touching both the generated code in examples and the generator logic itself. Overall, this is a very valuable change for improving the robustness of data classes in Pigeon. I've found two minor inconsistencies in the hashing logic for maps in Dart and Swift compared to other platforms, which could affect cross-platform hash-based collection behavior.

@tarrinneal
Copy link
Contributor Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces and optimizes data class equality (==) and hashing (hashCode/hash) across all supported languages, adding helper functions for deep equality and hashing of collections and nested objects to achieve more consistent and efficient implementations. However, a critical security vulnerability exists where the equality/hashing contract is violated for signed zero values (0.0 and -0.0) in the Objective-C and GObject generators; while equality checks consider them equal, their hashing logic produces different results, potentially leading to subtle bugs and logic bypasses in hash-based collections. Furthermore, other general issues have been found concerning the handling of special floating-point values, including NaN, in some of the new logic and tests.

@tarrinneal tarrinneal marked this pull request as draft March 5, 2026 02:54
@tarrinneal tarrinneal changed the title [pigeon] Optimize data class equality and hashing in Dart, Kotlin, and Swift, adds equality in other languages [pigeon] Optimize data class equality and hashing in Dart, Kotlin, java, and Swift, adds equality in other languages Mar 5, 2026
@tarrinneal tarrinneal marked this pull request as ready for review March 5, 2026 09:11
@tarrinneal
Copy link
Contributor Author

@stuartmorgan-g this should be ready for review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +1266 to +1271
size_t result = 0;
for (const auto& kv : v) {
result = result * 31 + PigeonInternalDeepHash(kv.first);
result = result * 31 + PigeonInternalDeepHash(kv.second);
}
return result;

Choose a reason for hiding this comment

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

critical

The hashing implementation for std::map is inconsistent with the other languages. This implementation uses an order-dependent algorithm (result = result * 31 + ...), while other platforms (Dart, Java, Kotlin, Swift, GObject/C) use an order-independent XOR sum of entry hashes (hash(key) ^ hash(value)).

While std::map is ordered, this inconsistency will lead to different hash codes for the same map data across platforms, which defeats a primary goal of this PR. To ensure consistent hashing, please use the XOR sum approach.

Suggested change
size_t result = 0;
for (const auto& kv : v) {
result = result * 31 + PigeonInternalDeepHash(kv.first);
result = result * 31 + PigeonInternalDeepHash(kv.second);
}
return result;
size_t result = 0;
for (const auto& kv : v) {
result += PigeonInternalDeepHash(kv.first) ^ PigeonInternalDeepHash(kv.second);
}
return result;

Comment on lines +1198 to +1205
if (_deepEquals(entryA.key, entryB.key)) {
if (_deepEquals(entryA.value, entryB.value)) {
found = true;
break;
} else {
return false;
}
}

Choose a reason for hiding this comment

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

critical

There's a logic error in the _deepEquals implementation for maps. If a key matches but the value doesn't, it immediately returns false. This is incorrect if another entry in the second map has an equivalent key and a matching value (e.g., if keys are objects that are equal by value but not identical). This could lead to incorrectly reporting two equivalent maps as unequal.

This same issue exists in the Kotlin, Swift, Objective-C, and C++ generators. The else { return false; } block should be removed to allow the loop to continue searching for another matching key-value pair, similar to the implementation in the Java and GObject generators.

Suggested change
if (_deepEquals(entryA.key, entryB.key)) {
if (_deepEquals(entryA.value, entryB.value)) {
found = true;
break;
} else {
return false;
}
}
if (_deepEquals(entryA.key, entryB.key)) {
if (_deepEquals(entryA.value, entryB.value)) {
found = true;
break;
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant