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

Stabilize encoding and decoding of value classes #1963

Merged
merged 4 commits into from
Jun 24, 2022

Conversation

qwwdfsad
Copy link
Contributor

@qwwdfsad qwwdfsad commented Jun 4, 2022

No description provided.

@qwwdfsad qwwdfsad requested a review from sandwwraith June 4, 2022 16:28
@qwwdfsad qwwdfsad marked this pull request as ready for review June 4, 2022 16:28
@qwwdfsad
Copy link
Contributor Author

qwwdfsad commented Jun 4, 2022

When we have this sorted out, I'll update the guide

* because inline classes always have one property.
* Calling [Encoder.beginStructure] on returned instance leads to an undefined behavior.
* Note that this function returns [Encoder] instead of the [CompositeEncoder]
* because value classes always have the single property.
Copy link
Contributor

@whyoleg whyoleg Jun 4, 2022

Choose a reason for hiding this comment

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

just interesting: in future value classes will be able to have multiple properties (multi-field value classes KEEP) - how then kx.serialization will work with them?
Even if for now inline class = value class, may be it will be better consider in mind, that it will change.
And, f.e. introduce separate annotation like InlineSerializable which will work not only with inline/value classes, but even with any other ordinary class or even interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how then kx.serialization will work with them

It's hard to tell without an actual design and implementation of multi-field value classes.

If the language will change the notion of value classes, we will of course have to change these methods accordingly

@sandwwraith
Copy link
Member

sandwwraith commented Jun 7, 2022

I'm not sure it is the right approach just to do s/inline/value. For example, there is a statement that now says 'value classes always have one property' — it may become a false statement in the observable future. Moreover, we simply can't encode several properties of value class via decodeInline, so this method would become irrelevant. Inlining in the Kotlin itself is also controlled by @JvmInline annotation, not by value keyword.

In my point of view, we should coin our definition of the words 'inline class' that would mean @JvmInline value class or just that that class' property is 'inlined' into the output without class wrapping — regardless of Kotlin class definition. By doing this, we would be able in the future to serialize regular value classes with any number of properties as regular classes, and, probably, use encodeInline for classes that have one property but are not necessary @JvmInline value classes.

@qwwdfsad
Copy link
Contributor Author

qwwdfsad commented Jun 7, 2022

I'm not sure it is the right approach just to do s/inline/value.

But this is not what this PR does! It is, in fact, already addresses most of your concerns. We still have all methods named inline with the documentation "Returns [Encoder] for encoding an underlying type of a value class in an inline manner."

"an underlying type of a value class in an inline manner" is exactly the terminology I aim for.

there is a statement that now says 'value classes always have one property'

There isn't in our documentation. While we cannot foresee every potential language feature, here we can do an additional safety net and specify "encoding an underlying type of a value class with a single property in an inline manner.". Though, honestly, it may create a false perspective that there are value classes with multiple properties. I would rather add this "single property" clarification at the moment when multi-word value classes are started being worked on.

qwwdfsad added 2 commits June 24, 2022 16:39
# Conflicts:
#	formats/json/commonMain/src/kotlinx/serialization/json/internal/StreamingJsonEncoder.kt
@qwwdfsad qwwdfsad merged commit 3e8331c into dev Jun 24, 2022
@qwwdfsad qwwdfsad deleted the remove-experimental-from-inline branch June 24, 2022 14:41
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.

3 participants