Skip to content
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

[pigeon] Implement equals for Java data classes #6992

Merged

Conversation

stuartmorgan
Copy link
Contributor

Adds implementations of equals and hashCode to Java data classes. This is frequently useful for native unit tests of plugins using Pigeon (e.g., when using a mock FlutterApi implementation to check that the expected call is being made with the right arguments).

Part of flutter/flutter#118087

Pre-launch Checklist

@@ -168,7 +179,7 @@ public void hasValues() {
.setBoolList(Arrays.asList(new Boolean[] {true, false}))
.setDoubleList(Arrays.asList(new Double[] {0.5, 0.25, 1.5, 1.25}))
.setIntList(Arrays.asList(new Long[] {1l, 2l, 3l, 4l}))
.setList(Arrays.asList(new int[] {1, 2, 3, 4}))
.setList(Arrays.asList(genericList))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is because an inherent limitation in Java equals implementations is that List equality only works if each element in the list has a useful implementation of equals, and (as demonstrated in the new generator code) arrays don't have that. So having the generic list's value be a nested array makes equality not work. Fixing that would require a bunch of special code generation—we would have to do custom equality checks of all collections to call out to Arrays.equals for certain types—for a case that I'm skeptical will ever matter in practice, and is a common behavior of the language anyway.

Since the specific type here didn't actually matter, I opted for just changing to something with a useful equals so that the test will pass.

@tarrinneal
Copy link
Contributor

Looks like you need to gen/format a file

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

a questionable nit, make the gen code cleaner, but the generator messier?

Comment on lines +367 to +370
__pigeon_result = 31 * __pigeon_result + Arrays.hashCode(aByteArray);
__pigeon_result = 31 * __pigeon_result + Arrays.hashCode(a4ByteArray);
__pigeon_result = 31 * __pigeon_result + Arrays.hashCode(a8ByteArray);
__pigeon_result = 31 * __pigeon_result + Arrays.hashCode(aFloatArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this a helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing it inline like this seems to be the stand way I've seen it in other Java code. We'd still need the assignment, so we'd be adding a function call just to abstract away 31 * __pigeon_result + which seems like a lot of overhead.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 26, 2024
@auto-submit auto-submit bot merged commit 50ad6ee into flutter:main Jun 27, 2024
74 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2024
@stuartmorgan stuartmorgan deleted the pigeon-java-data-object-equality branch June 27, 2024 16:16
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 28, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 28, 2024
flutter/packages@03f5f6d...412ec46

2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.6 to 3.25.10 (flutter/packages#6924)
2024-06-27 [email protected] [video_player] Exposes VideoPlayerWebOptions. (flutter/packages#6990)
2024-06-27 [email protected] [ci] Add Wasm compilation to all_packages web app. (flutter/packages#6981)
2024-06-27 [email protected] [interactive_media_ads] Updates `README` with a usage section and fix some interface docs (flutter/packages#6988)
2024-06-27 [email protected] [tools] Fix vm test requirement (flutter/packages#6995)
2024-06-27 [email protected] Update `web_benchmarks` package to properly support wasm. (flutter/packages#6970)
2024-06-27 [email protected] [pigeon] Implement equals for Java data classes (flutter/packages#6992)
2024-06-25 [email protected] Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. (flutter/packages#6982)
2024-06-25 [email protected] [pigeon] Update testing and docs (flutter/packages#6984)
2024-06-25 [email protected] [various] Update flutter.dev links to more reliable destinations (flutter/packages#6963)
2024-06-25 [email protected] [interactive_media_ads] Adds initial Android implementation (flutter/packages#6733)
2024-06-25 [email protected] Roll Flutter from 6c06abb to e726eb4 (51 revisions) (flutter/packages#6987)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
flutter/packages@03f5f6d...412ec46

2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.6 to 3.25.10 (flutter/packages#6924)
2024-06-27 [email protected] [video_player] Exposes VideoPlayerWebOptions. (flutter/packages#6990)
2024-06-27 [email protected] [ci] Add Wasm compilation to all_packages web app. (flutter/packages#6981)
2024-06-27 [email protected] [interactive_media_ads] Updates `README` with a usage section and fix some interface docs (flutter/packages#6988)
2024-06-27 [email protected] [tools] Fix vm test requirement (flutter/packages#6995)
2024-06-27 [email protected] Update `web_benchmarks` package to properly support wasm. (flutter/packages#6970)
2024-06-27 [email protected] [pigeon] Implement equals for Java data classes (flutter/packages#6992)
2024-06-25 [email protected] Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. (flutter/packages#6982)
2024-06-25 [email protected] [pigeon] Update testing and docs (flutter/packages#6984)
2024-06-25 [email protected] [various] Update flutter.dev links to more reliable destinations (flutter/packages#6963)
2024-06-25 [email protected] [interactive_media_ads] Adds initial Android implementation (flutter/packages#6733)
2024-06-25 [email protected] Roll Flutter from 6c06abb to e726eb4 (51 revisions) (flutter/packages#6987)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: pigeon platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants