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

Exhaustive checks in oneof fields #2749

Open
scmorse opened this issue Dec 13, 2023 · 8 comments
Open

Exhaustive checks in oneof fields #2749

scmorse opened this issue Dec 13, 2023 · 8 comments

Comments

@scmorse
Copy link

scmorse commented Dec 13, 2023

Is there a way to configure Wire to generate either an Enum / Sealed Class for oneof fields, to provide Kotlin developers with an exhaustive API for selecting the oneof field? It would be very helpful for detecting new cases at compile-time, rather then as errors at run-time. 🙏

I see that the sealed classes approach used to exist, but was removed, and several developers have asked for similar "exhaustive check" support in this issue: #998. I have never contributed to this project before, but I would be willing to invest some time into this feature if it will be well-received.

@scmorse
Copy link
Author

scmorse commented Dec 20, 2023

@oldergod Do you think this could work with the boxOneOfsMinSize feature from #1866?

@scmorse
Copy link
Author

scmorse commented Dec 27, 2023

CC @Egorand Since I saw you had a PR to remove Sealed Classes in #1002

@oldergod
Copy link
Member

We never had sealed classes for that, and currently have no plan to.

boxed oneofs will provide a set of all entries but that's it.

@scmorse
Copy link
Author

scmorse commented Jan 17, 2024

@oldergod thanks for your reply!

It wouldn't have to be a sealed class necessarily, but I think exhaustive checks that are verified at compile-time are a very strong feature, and worth some consideration. Because if someone adds a new option to a oneof, any clients of that proto would like to be asked how they want to handle the new case, rather than defaulting to some else -> throw case, crashing at runtime. (The else -> case need not even be written when the compiler can guarantee the check is exhaustive, as I'm sure you know)

@scmorse
Copy link
Author

scmorse commented Feb 9, 2024

@oldergod If sealed classes aren't palatable, what would you think declaring an enum where the value corresponds to which one of the oneof cases is provided? For example, if a field is declared like this:

oneof my_field {
  MessageOne message_one = 1;
  MessageTwo message_two = 2;
}

Then in addition to the message_one and message_two properties on the generated classes, there would also be a new my_field_case property, having an enum generated value so that we could check the case like:

when (obj.my_field_case) {
  MyFieldCase.MESSAGE_ONE -> handle(obj.message_one!!)
  MyFieldCase.MESSAGE_TWO -> handle(obj.message_two!!)
}

Which is an exhaustive check, so that we can handle new oneof cases at compile time rather than failing at runtime!

@oldergod
Copy link
Member

We have no plan to add that at the moment.
This is however something you could build yourself for your project by injecting a custom handler which would generate extension members for your messages.

@friscoMad
Copy link

I am also struggling with this, not only for exhaustiveness in message handling but also for making it clear for devs which ones are oneOf fields and enforcing a single oneOf value is set during message construction.
I see that the custom handler can be used to create some extension functions for the map proposed by @scmorse but if I am not wrong there is no way to change constructor and getters.

For reference, all other alternative protoc compilers for Kotlin that I know do use sealed interfaces for oneOfs (ProtoKt, KrotoDC and PBAndK)

@lily-es
Copy link

lily-es commented Aug 29, 2024

Generating enums for oneof fields would allow for exhaustive type checking while maintaining backwards compatibility
Would be nice to have support for this

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

No branches or pull requests

4 participants