[pigeon] Optimize data class equality and hashing in Dart, Kotlin, java, and Swift, adds equality in other languages#11140
[pigeon] Optimize data class equality and hashing in Dart, Kotlin, java, and Swift, adds equality in other languages#11140tarrinneal wants to merge 34 commits intoflutter:mainfrom
Conversation
There was a problem hiding this comment.
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.
|
/gemini review |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
/gemini review |
There was a problem hiding this comment.
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.
packages/pigeon/platform_tests/shared_test_plugin_code/test/equality_test.dart
Show resolved
Hide resolved
packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/AllDatatypesTests.swift
Show resolved
Hide resolved
...lugin/android/src/test/java/com/example/alternate_language_test_plugin/AllDatatypesTest.java
Outdated
Show resolved
Hide resolved
|
@stuartmorgan-g this should be ready for review |
There was a problem hiding this comment.
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.
| size_t result = 0; | ||
| for (const auto& kv : v) { | ||
| result = result * 31 + PigeonInternalDeepHash(kv.first); | ||
| result = result * 31 + PigeonInternalDeepHash(kv.second); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
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.
| 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; |
| if (_deepEquals(entryA.key, entryB.key)) { | ||
| if (_deepEquals(entryA.value, entryB.value)) { | ||
| found = true; | ||
| break; | ||
| } else { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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; | |
| } | |
| } |
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.