livefront / sealed-enum

A Kotlin annotation processor that makes writing normal enum classes obsolete.
Apache License 2.0
149 stars 7 forks source link

kotlin.NoWhenBranchMatchedException caused by using ordinal inside of a property intiializer #129

Closed kyay10 closed 1 year ago

kyay10 commented 1 year ago

Minimal reproducer

sealed class TestFlag {
    val i: Int = 1 shl ordinal
    @GenSealedEnum companion object
    object Foo: TestFlag()
}

fun main() {
    TestFlag.Foo.i.also(::println)
}

A little context about my use-case: We're using sealed classes as Flags, and so it's common for us to want each object to have an integer representation that is 1 shifted left by its ordinal. This exception happens because during the initialization of the INSTANCEs for the objects, the ordinal extension property is accessed, which internally compares the object (using equality) against the instances of the objects inside the sealed class. A simple workaround for this is to turn i into a computed property, but that makes i get recomputed on every call, which adds the cost of all the equality checks that ordinal does internally. It would be appreciated if an option in the plugin could be provided that, instead of comparing the objects by equality, does is checks inside the ordinal property (so that this issue is prevented). I'm not super familiar with ksp and kotlinpoet, but I can attempt at making a PR that solves this.

alexvanyo commented 1 year ago

Thanks for the detailed use case, issue and PR!

Although in normal code I'd usually prefer to leave out the is in when statements for objects, these two should be overall pretty equivalent:

when (object) {
    FirstObject -> // ...
    SecondObject -> // ...
}

when (object) {
    is FirstObject -> // ...
    is SecondObject -> // ...
}

Instead of having a property useIsChecksForSealedObjectComparison, it might make sense to switch to is checks everywhere in the generated code for situations like this. That should fix your use case without needing an additional argument, and I don't think there'd be any downsides to doing so?

kyay10 commented 1 year ago

It's technically ever so slightly different in that, if a different instance of the sealed object is created through reflection (usually as a result of a serialization framework), it would change the behaviour of code that previously would throw errors (due to NoWhenBranchMatchedException) to not throw anymore. It's surprising behavior that that code would throw in the first place, so I would agree that changing to is checks is the way to go.

kyay10 commented 1 year ago

@alexvanyo Done! I've changed the code to use is checks, and I've updated the tests and docs to reflect that. Please check #130 and suggest any needed changes!

alexvanyo commented 1 year ago

That behavior is surprising, I think the is method is better (or at least is a bit more robust to these edge cases).

I also saw in the updated Kotlin docs on data objects they even have a mention for using == instead of === for similar reasons. I'll take a look at #130 when I have a chance!