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

Migrate OriginatingElementsHolder and Taggable from JVM to common #2040

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions kotlinpoet/api/kotlinpoet.api
Original file line number Diff line number Diff line change
@@ -1253,6 +1253,9 @@ public final class com/squareup/kotlinpoet/jvm/JvmAnnotations {
public static final fun volatile (Lcom/squareup/kotlinpoet/PropertySpec$Builder;)Lcom/squareup/kotlinpoet/PropertySpec$Builder;
}

public abstract interface annotation class com/squareup/kotlinpoet/jvm/alias/JvmOnlyKotlinPoetApi : java/lang/annotation/Annotation {
}

public final class com/squareup/kotlinpoet/tags/TypeAliasTag {
public fun <init> (Lcom/squareup/kotlinpoet/TypeName;)V
public final fun getAbbreviatedType ()Lcom/squareup/kotlinpoet/TypeName;
Original file line number Diff line number Diff line change
@@ -15,24 +15,26 @@
*/
package com.squareup.kotlinpoet

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an JVM-only API. I don't see a reason to move it.

Copy link
Contributor Author

@ForteScarlet ForteScarlet Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expect/actual mechanism allows actuals on the JVM to implement interfaces not exposed by the expect.

Here's an example in Okio:

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.

Copy link
Contributor Author

@ForteScarlet ForteScarlet Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😢

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that require expect/actuals of FunSpec and co?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good programme!🤩 Please feel free to let me know if there is anything I can do to help!


/** The originating elements of this type. */
public val originatingElements: List<Element>
public val originatingElements: List<JvmElement>

/** The builder analogue to [OriginatingElementsHolder] types. */
@JvmDefaultWithCompatibility
public interface Builder<out T : Builder<T>> {

/** Mutable map of the current originating elements this builder contains. */
public val originatingElements: MutableList<Element>
public val originatingElements: MutableList<JvmElement>

/** Adds an [originatingElement] to this type's list of originating elements. */
@Suppress("UNCHECKED_CAST")
public fun addOriginatingElement(originatingElement: Element): T = apply {
public fun addOriginatingElement(originatingElement: JvmElement): T = apply {
originatingElements += originatingElement
} as T
}
@@ -41,10 +43,10 @@ public interface OriginatingElementsHolder {
internal fun OriginatingElementsHolder.Builder<*>.buildOriginatingElements() =
OriginatingElements(originatingElements.toImmutableList())

internal fun List<Element>.buildOriginatingElements() =
internal fun List<JvmElement>.buildOriginatingElements() =
OriginatingElements(toImmutableList())

@JvmInline
internal value class OriginatingElements(
override val originatingElements: List<Element>,
override val originatingElements: List<JvmElement>,
) : OriginatingElementsHolder
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright (C) 2019 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet

import com.squareup.kotlinpoet.jvm.alias.JvmClass
import com.squareup.kotlinpoet.jvm.alias.JvmDefaultWithCompatibility
import com.squareup.kotlinpoet.jvm.alias.kotlin
import kotlin.jvm.JvmInline
import kotlin.reflect.KClass

/** A type that can be tagged with extra metadata of the user's choice. */
@JvmDefaultWithCompatibility
public interface Taggable {

/** Returns all tags. */
public val tags: Map<KClass<*>, Any> get() = emptyMap()

/** Returns the tag attached with [type] as a key, or null if no tag is attached with that key. */
public fun <T : Any> tag(type: JvmClass<T>): T? = tag(type.kotlin)

/** Returns the tag attached with [type] as a key, or null if no tag is attached with that key. */
public fun <T : Any> tag(type: KClass<T>): T? {
@Suppress("UNCHECKED_CAST")
return tags[type] as T?
}

/** The builder analogue to [Taggable] types. */
@JvmDefaultWithCompatibility
public interface Builder<out T : Builder<T>> {

/** Mutable map of the current tags this builder contains. */
public val tags: MutableMap<KClass<*>, Any>

/**
* Attaches [tag] to the request using [type] as a key. Tags can be read from a
* request using [Taggable.tag]. Use `null` to remove any existing tag assigned for
* [type].
*
* Use this API to attach originating elements, debugging, or other application data to a spec
* so that you may read it in other APIs or callbacks.
*/
public fun tag(type: JvmClass<*>, tag: Any?): T = tag(type.kotlin, tag)

/**
* Attaches [tag] to the request using [type] as a key. Tags can be read from a
* request using [Taggable.tag]. Use `null` to remove any existing tag assigned for
* [type].
*
* Use this API to attach originating elements, debugging, or other application data to a spec
* so that you may read it in other APIs or callbacks.
*/
@Suppress("UNCHECKED_CAST")
public fun tag(type: KClass<*>, tag: Any?): T = apply {
if (tag == null) {
this.tags.remove(type)
} else {
this.tags[type] = tag
}
} as T
}
}

/** Returns the tag attached with [T] as a key, or null if no tag is attached with that key. */
public inline fun <reified T : Any> Taggable.tag(): T? = tag(T::class)

internal fun Taggable.Builder<*>.buildTagMap(): TagMap = TagMap(tags)

@JvmInline
internal value class TagMap private constructor(override val tags: Map<KClass<*>, Any>) : Taggable {
companion object {
operator fun invoke(tags: Map<KClass<*>, Any>): TagMap = TagMap(tags.toImmutableMap())
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

import kotlin.reflect.KClass

/**
* An expected typealias for `java.lang.Class`.
*/
public expect class JvmClass<T : Any>

internal expect val <T : Any> JvmClass<T>.kotlin: KClass<T>
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

/**
* A typealias annotation for `kotlin.jvm.JvmDefaultWithCompatibility`.
*/
@OptIn(ExperimentalMultiplatform::class)
@OptionalExpectation
public expect annotation class JvmDefaultWithCompatibility()
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

/**
* An expected typealias for `javax.lang.model.element.Element`.
*/
public expect interface JvmElement

/**
* An expected typealias for `javax.lang.model.element.TypeElement`.
*/
public expect interface JvmTypeElement : JvmElement

/**
* An expected typealias for `javax.lang.model.element.ExecutableElement`.
*/
public expect interface JvmExecutableElement : JvmElement

/**
* An expected typealias for `javax.lang.model.element.VariableElement`.
*/
public expect interface JvmVariableElement : JvmElement
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

/**
* An expected type for JVM to `typealias` only.
*/
@MustBeDocumented
@Retention(AnnotationRetention.BINARY)
@Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY, AnnotationTarget.TYPEALIAS)
@RequiresOptIn(
level = RequiresOptIn.Level.ERROR,
message = "An expected type for JVM to `typealias` only." +
" Don't implement or use it in non-JVM platforms.",
)
public annotation class JvmOnlyKotlinPoetApi
Original file line number Diff line number Diff line change
@@ -15,63 +15,6 @@
*/
package com.squareup.kotlinpoet

import kotlin.reflect.KClass

/** A type that can be tagged with extra metadata of the user's choice. */
@JvmDefaultWithCompatibility
public interface Taggable {

/** Returns all tags. */
public val tags: Map<KClass<*>, Any> get() = emptyMap()

/** Returns the tag attached with [type] as a key, or null if no tag is attached with that key. */
public fun <T : Any> tag(type: Class<T>): T? = tag(type.kotlin)

/** Returns the tag attached with [type] as a key, or null if no tag is attached with that key. */
public fun <T : Any> tag(type: KClass<T>): T? {
@Suppress("UNCHECKED_CAST")
return tags[type] as T?
}

/** The builder analogue to [Taggable] types. */
@JvmDefaultWithCompatibility
public interface Builder<out T : Builder<T>> {

/** Mutable map of the current tags this builder contains. */
public val tags: MutableMap<KClass<*>, Any>

/**
* Attaches [tag] to the request using [type] as a key. Tags can be read from a
* request using [Taggable.tag]. Use `null` to remove any existing tag assigned for
* [type].
*
* Use this API to attach originating elements, debugging, or other application data to a spec
* so that you may read it in other APIs or callbacks.
*/
public fun tag(type: Class<*>, tag: Any?): T = tag(type.kotlin, tag)

/**
* Attaches [tag] to the request using [type] as a key. Tags can be read from a
* request using [Taggable.tag]. Use `null` to remove any existing tag assigned for
* [type].
*
* Use this API to attach originating elements, debugging, or other application data to a spec
* so that you may read it in other APIs or callbacks.
*/
@Suppress("UNCHECKED_CAST")
public fun tag(type: KClass<*>, tag: Any?): T = apply {
if (tag == null) {
this.tags.remove(type)
} else {
this.tags[type] = tag
}
} as T
}
}

/** Returns the tag attached with [T] as a key, or null if no tag is attached with that key. */
public inline fun <reified T : Any> Taggable.tag(): T? = tag(T::class)

/**
* Attaches [tag] to the request using [T] as a key. Tags can be read from a
* request using [Taggable.tag]. Use `null` to remove any existing tag assigned for
@@ -149,12 +92,3 @@ public inline fun <reified T : Any> TypeAliasSpec.Builder.tag(tag: T?): TypeAlia
*/
public inline fun <reified T : Any> TypeSpec.Builder.tag(tag: T?): TypeSpec.Builder =
tag(T::class, tag)

internal fun Taggable.Builder<*>.buildTagMap(): TagMap = TagMap(tags)

@JvmInline
internal value class TagMap private constructor(override val tags: Map<KClass<*>, Any>) : Taggable {
companion object {
operator fun invoke(tags: Map<KClass<*>, Any>): TagMap = TagMap(tags.toImmutableMap())
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

import kotlin.jvm.kotlin as jvmKotlin
import kotlin.reflect.KClass

public actual typealias JvmClass<T> = Class<T>

internal actual val <T : Any> JvmClass<T>.kotlin: KClass<T>
get() = jvmKotlin
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

public actual typealias JvmDefaultWithCompatibility = kotlin.jvm.JvmDefaultWithCompatibility
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

import javax.lang.model.element.Element
import javax.lang.model.element.ExecutableElement
import javax.lang.model.element.TypeElement
import javax.lang.model.element.VariableElement

public actual typealias JvmElement = Element

public actual typealias JvmTypeElement = TypeElement

public actual typealias JvmExecutableElement = ExecutableElement

public actual typealias JvmVariableElement = VariableElement
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

import kotlin.reflect.KClass

@JvmOnlyKotlinPoetApi
public actual class JvmClass<T : Any> private constructor()

@JvmOnlyKotlinPoetApi
internal actual val <T : Any> JvmClass<T>.kotlin: KClass<T>
get() = throw UnsupportedOperationException()
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.kotlinpoet.jvm.alias

@JvmOnlyKotlinPoetApi
public actual interface JvmElement

@JvmOnlyKotlinPoetApi
public actual interface JvmTypeElement : JvmElement

@JvmOnlyKotlinPoetApi
public actual interface JvmExecutableElement : JvmElement

@JvmOnlyKotlinPoetApi
public actual interface JvmVariableElement : JvmElement