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

Kotlin sealed classes #274

Closed
dcow opened this issue Mar 24, 2017 · 7 comments
Closed

Kotlin sealed classes #274

dcow opened this issue Mar 24, 2017 · 7 comments

Comments

@dcow
Copy link

dcow commented Mar 24, 2017

I just wrote a custom converter for a sealed class. I have something like:

class BaseConverter : ParcelConverter<Base> {
    override fun fromParcel(parcel: android.os.Parcel?): Base? {
        return Parcels.unwrap(parcel?.readParcelable(Base::class.java.classLoader))
    }

    override fun toParcel(input: Base?, parcel: android.os.Parcel?) {
        val out = parcel ?: return
        val obj = input ?: return
        when (obj) {
            is Base.A -> out.writeParcelable(Parcels.wrap(obj), 0)
            is Base.B -> out.writeParcelable(Parcels.wrap(obj), 0)
            // exhausted because Base is a sealed class
        }
    }
}

@Parcel(converter = BaseConverter::class)
sealed class Base
{
    @Parcel
    data class A @ParcelConstructor constructor(
            @ParcelProperty("prop1") val prop1: String,
            @ParcelProperty("prop2") val prop2: SomeOtherParcelClass
    ) : Base()

    @Parcel
    class B : Base()
}

This works fine, but it strikes me as something Parceler could support considering that we could generate the when clause at compile time. In Kotlin it's colloquial to use a pattern of sealed base class and sub-classes of different types to associate unrelated data structures into some enumerable form. I'm unsure how or if the Kotlin compiler chooses to optimize a when clause on sealed classes, but I think we'd leave that up to the compiler to decide when or how to do so.

I haven't looked into this further. I don't know if you can get all types of a sealed class using kapt, but I figure it's worth investigation.

@johncarl81
Copy link
Owner

Would you mind putting together a quick project with this example in it so I can work from it? This seems like something we should be able to handle, and hopefully in a more elegant way.

@dcow
Copy link
Author

dcow commented Mar 24, 2017

@johncarl81 sure! I'm happy to help investigate.

@johncarl81
Copy link
Owner

I've been meaning to add Kotlin collection support as well: #264

@dcow
Copy link
Author

dcow commented Mar 28, 2017

@johncarl81 here is a sample-repo.

dcow pushed a commit to dcow/kotlin-sealed-class-parceler-sample that referenced this issue Mar 28, 2017
@johncarl81
Copy link
Owner

Great! I'll try to jump on it this evening.

@johncarl81
Copy link
Owner

Thanks again @dcow for the example project. Looks like all the generated classes are being created and the issue is dealing with inheritance when handling Screen's subtypes (same issue here: #249). I see two options here. One, we begin to support inheritance and subtypes fully (It would make this guy a bit less unhappy: https://twitter.com/joaomc/status/703636309190057984 🤣). Someone would need to do some analysis around the reflection overhead of looking up types and their associated parcelable classes. Two, we could use some heuristics on Kotlin generated stubs to determine if the class is a sealed class. If it is we could automatically generate the switch statement you referenced to parcel the given type. I'm leaning more towards the reflection option as it will solve a number of other issues, but I'm weary of the overhead possibility.

Thoughts?

@dcow
Copy link
Author

dcow commented Apr 6, 2017

I haven't had tons of time to mull this over, but my initial impression is that it's okay to not support sub-classes of sealed classes unless the compiler also supports such a scenario. For instance, if class Foo extends either of sealed classes A or B or C, is Foo considered one of the cases for a when clause? I'm guessing it would be? If not, I would expect to need a custom converter if I want to go from sealed -> not-sealed.

If you do go the full blown support for inheritance route, then my impression is that you'd want to provide such compatibility as an add on module or a configurable option. That way people concerned with it who are willing to pay the performance penalty and don't like serializable for other reasons can still use this library, but people who really need to eek out the most performance can still be happy with custom converters.

From a Kotlin angle, using the is keyword is pretty natural and per my initial commentary, I think at this point in time, it's okay to use that level of reflection because I think the Kotlin compiler is at liberty to optimize basic type inspection vs the full blown kotlin-reflection support--which I'd avoid at runtime but certainly leverage for code generation.

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

2 participants