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

Add support for mutable messages in Wire's Kotlin Generator. #3217

Merged
merged 6 commits into from
Dec 24, 2024

Conversation

tikurahul
Copy link
Contributor

@tikurahul tikurahul commented Dec 17, 2024

  • Sometimes its very useful to avoid needing to create new object graph instances when all we need to do is to serialize the object graph into a ProtoSink.
  • Mutable messages solves for this, for code that is particularly performance sensitive where a large number of objects would have otherwise been created which adds unnecesssary GC pressure.
  • Mutable messages should be used very carefully. There are no guarantees of safety whatsoever.
  • Mutable messages don't use Builders and so newBuilder() throws an UnsupportedOperationException().
  • Mutable messages don't support copy(...).
  • When using mutable messages, hashCode() is no longer cached.
  • Mutable messages don't support redact().

Test: Added a unit test in KotlinGeneratorTest.

tikurahul and others added 6 commits December 20, 2024 13:00
* Sometimes its very useful to avoid needing to create new object graph instances when all we need to do is to serialize the object graph into a `ProtoSink`.
* Mutable messages solves for this, for code that is particularly performance sensitive where a large number of objects would have otherwise been created which adds unnecesssary GC pressure.
* Mutable messages should be **used very carefully**. There are **no guarantees of safety whatsoever**.
* When using mutable messages, `hashCode()` is no longer cached.
* Mutable messages don't support `redact()`.

Test: Added a unit test in KotlinGeneratorTest
* `newBuilder()` throws an `UnsupportedOperationException` instead. Thanks @oldergod.
* Updated the `KotlinGeneratorTest` to use the renamed option in the `KotlinGenerator`.
* Update goldens to include the new error message.
* Update API files for `wire-compiler`.
* `Mutable` messsages now have access to mutable `unknownFields`.

Test: Updated KotlinGeneratorTest and the golden tests.
* `Mutable` messsages now have access to mutable `unknownFields`.

Test: Updated KotlinGeneratorTest and the golden tests.
* Regenerate golden files.
* Update API files for `wire-gradle-plugin`.
* Update API files for `wire-kotlin-generator`.
* Spotless checks.
)
override fun newBuilder(): Nothing = throw UnsupportedOperationException("newBuilder() is unsupported for mutable message types")

override fun equals(other: Any?): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be by-identity rather than by-value? Equals is weird on mutable things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean that we should only support referential equality ?

I think it's also okay for us to also compare the values. Otherwise it might just end up being confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Contributor

@squarejesse squarejesse left a comment

Choose a reason for hiding this comment

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

This is rad

@JakeWharton JakeWharton merged commit 54e032b into square:master Dec 24, 2024
11 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.

4 participants