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.
Part of #304, #1959
Migrate
OriginatingElementsHolderandTaggablefrom JVM to common