Migrate OriginatingElementsHolder and Taggable from JVM to common #2040
Conversation
| import javax.lang.model.element.TypeElement | ||
| import javax.lang.model.element.VariableElement | ||
|
|
||
| /** |
There was a problem hiding this comment.
nit: docs in this file feel redundant, they more or less just restate what the code says.
| message = "An expected type for JVM to `typealias` only." + | ||
| " Don't implement or use it in non-JVM platforms.", | ||
| ) | ||
| public annotation class JvmTypeAliasKotlinPoetApi |
There was a problem hiding this comment.
Should we maybe call it JvmOnlyKotlinPoetApi? IMO this makes it a bit more clear that this type only exists in KotlinPoet for JVM.
| public expect class JvmClass<T : Any> | ||
|
|
||
| /** | ||
| * Convert [JvmClass] to [KClass]. |
| public actual typealias JvmClass<T> = Class<T> | ||
|
|
||
| /** | ||
| * Convert [JvmClass] to [KClass]. |
| import kotlin.reflect.KClass | ||
|
|
||
| /** | ||
| * An expected typealias for [Class]. |
| public actual class JvmClass<@Suppress("unused") | ||
| T : Any,> private constructor() |
There was a problem hiding this comment.
nit:
| public actual class JvmClass<@Suppress("unused") | |
| T : Any,> private constructor() | |
| public actual class JvmClass<@Suppress("unused") T : Any> private constructor() |
| package com.squareup.kotlinpoet.jvm.alias | ||
|
|
||
| /** | ||
| * A typealias annotation for [kotlin.jvm.JvmDefaultWithCompatibility]. |
| /** | ||
| * Convert [JvmClass] to [KClass]. | ||
| */ | ||
| public expect val <T : Any> JvmClass<T>.kotlin: KClass<T> |
There was a problem hiding this comment.
Should we make this val internal? It shouldn't really be used outside of the library.
There was a problem hiding this comment.
Indeed, it doesn't seem to be used outside. I would change it to internal .
| import javax.lang.model.element.Element | ||
| import com.squareup.kotlinpoet.jvm.alias.JvmDefaultWithCompatibility | ||
| import com.squareup.kotlinpoet.jvm.alias.JvmElement | ||
| import kotlin.jvm.JvmInline | ||
|
|
||
| /** A type that can have originating [elements][Element]. */ | ||
| public interface OriginatingElementsHolder { |
There was a problem hiding this comment.
This is an JVM-only API. I don't see a reason to move it.
There was a problem hiding this comment.
It is implemented by many other types, e.g. FunSpec, TypeSpec, PropertySpec. I think even though it's JVM-only, it still needs to be in the common
There was a problem hiding this comment.
The expect/actual mechanism allows actuals on the JVM to implement interfaces not exposed by the expect.
Here's an example in Okio:
- Expect: https://github.com/square/okio/blob/b6b4d52e5f0099328a5bc288249058e573dc19ff/okio/src/commonMain/kotlin/okio/Sink.kt#L45
- Actual: https://github.com/square/okio/blob/b6b4d52e5f0099328a5bc288249058e573dc19ff/okio/src/jvmMain/kotlin/okio/Sink.kt#L22
We should not expose JVM-only APIs in common. The need to create a ton of dummy typealiases to JVM types is a good indicator for an API that should not be in common.
There was a problem hiding this comment.
In the case of FunSpec, for example, if you want to use the expect/actual mechanism to make FunSpec in the JVM implement OriginatingElementsHolder in a way that other platforms don't, then FunSpec itself must be expect/actual (unless the extra in the interface is not abstract).
This can create a lot of duplicate logic or generate a lot of extra work. That's what I use typealias for: to reduce the amount of work due to compatibility.
If we don't accept any JVM-only typealias exposed in common, then almost all major types will become expect/actual, even though their logic doesn't really differ.
This is the same problem you mentioned in #304 (comment), and I'm trying to get around it with typealias.
Exposing JVM-only types in common I think is a compromise for compatibility and maybe acceptable? 🤔 If it's unacceptable, that's fine too, except that later migrations to those major types will become more difficult.
There doesn't seem to be any best of both worlds until KT-20427 is resolved? 😢
There was a problem hiding this comment.
Would that require expect/actuals of FunSpec and co?
There was a problem hiding this comment.
If an interface is expect/actual and there is an extra abstract method in the JVM, then FunSpec must also be expect/actual. Otherwise, in common it can't know about that extra part of the JVM and thus can't implement it.
There was a problem hiding this comment.
The primary reason this work hasn't been done is because of this problem. I don't love the idea of exposing a large, JVM-based API surface that is effectively useless to other platforms.
I would rather we either wait for the language to evolve, or explore alternative ways of simplifying the duplication which comes from doing it properly today. For example, we could pull out sealed abstract base classes which contain the common code of each type, and then expect/actual a mostly-empty subtype which then would allow retaining platform-specific API surfaces.
There was a problem hiding this comment.
This is indeed tricky. Even more painful is the fact that KT-20427 has made little visible progress to date. 😭
(Come on, Jetbrains! ✊)
But it seems that most of the forced ‘compatibility’ is related to DelicateKotlinPoetApi or javax.lang.model. Do they have an obvious role in the Kotlin API? Or do they need to be kept? If they are not planned to be retained in the future (3.0, for example), then some of the current interim measures might be acceptable? Personal opinion.
There was a problem hiding this comment.
K2 blocked all significant language features for nearly three years. They're only starting to catch up to addressing things, but I still wouldn't hold my breath for that feature for a while.
Delicate APIs are still stable APIs–we can't break them. The usage of Java mirrors and elements are tied to annotation processors which generate Kotlin. While kapt is set to be deprecated, we need to retain those APIs for existing tools.
We could attempt to pursue a 3.x by developing a multiplatform variant of KotlinPoet in common from scratch in the app.cash.kotlinpoet package. We would develop this over time, allowing people to try it out in the 2.x releases since it would sit as a sibling. Then, when we're happy, we would delete the com.squareup.kotlinpoet API in its entirety, change the Maven groupId, and release 3.0. This would ensure that 2.x users never see a breakage, while allowing 3.x to be iterated on and tested.
It would allow fixing design mistakes both in regard to how the JVM-specific APIs are integrated, but also the pervasive use of builders instead of something more appropriate to Kotlin such as fully mutable types that get captured into immutable specs, with helpers for things like moving the mutable types into the receiver position of a function.
There was a problem hiding this comment.
Sounds like a good programme!🤩 Please feel free to let me know if there is anything I can do to help!
Part of #304, #1959
Migrate
OriginatingElementsHolderandTaggablefrom JVM to common