KE2: Upgrade to Kotlin 2.1.0; restore basic type parameter and type argument extraction #18215

smowton · 2024-12-04T23:08:21Z

Kotlin 2.1.0

Needed to upgrade this due to finding compiler crashes when dealing with Java classes that declare type parameters against constructors.

Two snags with this:

Kotlin analysis API now requires Caffeine, a caching framework, but this isn't a dependency of either of the kotlin packages we depend on (guess: they're intended to run in a context that already has caffeine on the classpath -- perhaps IntelliJ already does, for example?), and I can't find an appropriate Kotlin package to retarget. Just adding it as an extra dependency suffices for now, but if there is a more appropriate Maven dependency to target that will pull it in for us, we should do that.
Probably relatedly, since Caffeine schedules background work to clean up caches periodically, the extractor no longer terminates without a hard System.exit(0) once all our work is done. Ideally we'd work out (a) is it actually Caffeine keeping the JVM awake (snork), and (b) if it is, how should it be asked to make an orderly shutdown.

Generics extraction

Done here:

Enabled extraction of type parameters
Enabled using type arguments in TRAP labels and function signatures
Enabled extracting wildcard types

Not done yet:

Actually extract specialised classes (e.g. Map<String, String>) -- requires re-enabling the logic to emit a class outline into the current trap file, and type substitution
File classes / other special class names, like companion objects and such
Any of the Java/Kotlin equivalence stuff -- so f(Int, Float?) will extract is kotlin.Int, kotlin.Float not int, java.lang.Float as it ought to for J/K compatibility.

smowton · 2024-12-04T23:08:44Z

java/kotlin-extractor2/src/main/kotlin/KotlinExtractor.kt


    val session = buildStandaloneAnalysisAPISession {
-        registerProjectService(KotlinLifetimeTokenProvider::class.java, KotlinAlwaysAccessibleLifetimeTokenProvider())
+        registerProjectService(KotlinLifetimeTokenFactory::class.java, KotlinAlwaysAccessibleLifetimeTokenFactory())


Changed in Kotlin 2.1.0

smowton · 2024-12-04T23:11:15Z

java/kotlin-extractor2/src/main/kotlin/KotlinUsesExtractor.kt

-        // The Kotlin compiler internal representation of Outer<A, B>.Inner<C, D>.InnerInner<E,
-        // F>.someFunction<G, H>.LocalClass<I, J> is LocalClass<I, J, G, H, E, F, C, D, A, B>. This
-        // function returns [A, B, C, D, E, F, G, H, I, J].
-        private fun orderTypeArgsLeftToRight(


Pleasingly this could die, because the Kotlin Analysis API represents this as [[A, B], [C, D], [E, F], [G, H], [I, K]] via the KaClassType.qualifiers property, which is a much more useful than the IR representation.

smowton · 2024-12-04T23:12:21Z

java/kotlin-extractor2/src/main/kotlin/entities/Class.kt

    cBeforeReplacement: KaClassSymbol,
-    argsIncludingOuterClassesBeforeReplacement: List<Nothing>?
-): Pair<KaClassSymbol, List<Nothing>?> {
+    argsIncludingOuterClassesBeforeReplacement: List<List<KaTypeProjection>>?


Note this List<List<KaTypeProjection>> format occurs quite a lot, and represents the type-args of outer classes, if any.

smowton · 2024-12-04T23:14:16Z

java/kotlin-extractor2/src/main/kotlin/entities/Function.kt

        val functionSyntax = f.psi as? KtDeclarationWithBody
        val locId = functionSyntax?.let {
-            tw.getLocation(functionSyntax ?: TODO())
+            tw.getLocation(functionSyntax)


Incidental cleanup -- the ?: was unreachable

smowton · 2024-12-04T23:16:08Z

java/kotlin-extractor2/src/main/kotlin/entities/TypeArgument.kt

+    // Note this function doesn't return a signature because type arguments are never
+    // incorporated into function signatures.
+    return when (arg) {
+        is KaStarTypeProjection -> {


This is all basically unchanged from the IR format -- the representation of variances is essentially the same. Basic translation to wildcards (out T == ? extends T, in T == ? super T) was likewise restored.

smowton · 2024-12-04T23:17:19Z

java/kotlin-extractor2/src/main/kotlin/entities/TypeParameter.kt

+        }
+
+        // extractAnnotations(tp, id)
+        // TODO: introduce annotations once they can be disambiguated from bounds, which are


Note this is an old TODO; this was commented out in KE1

smowton · 2024-12-04T23:17:52Z

java/kotlin-extractor2/src/main/kotlin/entities/Types.kt

    }

-context(KaSession)
-fun KotlinUsesExtractor.getTypeParameterLabel(param: KaTypeParameterType): String {


Gone to TypeParameter.kt

smowton · 2024-12-04T23:19:22Z

java/kotlin-extractor2/src/main/kotlin/utils/TypeResults.kt

-        val shortName: String
-    */
+    // OLD: KE1: val signature: SignatureType?,
+    val shortName: String


Passing shortName around is restored (used to generate names for wildcard types, like "? extends Blah" where "Blah" is the shortName of the underlying type). I haven't looked into how we're using signature yet so the relevant arguments are still behind OLD/KE1 comments.

ben-manes · 2024-12-05T06:37:18Z

Probably relatedly, since Caffeine schedules background work to clean up caches periodically, the extractor no longer terminates without a hard System.exit(0) once all our work is done. Ideally we'd work out (a) is it actually Caffeine keeping the JVM awake (snork), and (b) if it is, how should it be asked to make an orderly shutdown.

Caffeine doesn’t create any threads directly or default to use any non-daemon threads indirectly. It will defers anything that can be async and calls foreign user code to an executor, like eviction and its listener. That by default is ForkJoinPool.commonPool() which uses daemon threads. Users can set a custom pool, many use Runnable::run to execute on the caller since caffeine’s own logic is inexpensive.

You can use jstack to find the blocking thread. If it’s related to the cache then it would be custom configuration.

smowton · 2024-12-05T11:59:42Z

Confirmed the thread in question doesn't belong to Caffeine; rather it is a thread associated with com.intellij.util.concurrency.AppScheduledExecutorService, which we indirectly activate via KaSessionProvider.getInstance -> KaFirSessionProvider's init block -> scheduledCacheMaintenance = AppExecutorUtil.getAppScheduledExecutorService().scheduleWithFixedDelay.

An hour of digging later I am not much closer to identifying what the ownership structure for that is supposed to be, and while there are various shutdown functions I can't see any paths from disposing of standalone analysis sessions or similar that would clean up the thread pool.

Bleeding-edge Kotlin provides https://github.com/JetBrains/kotlin/blob/196ae1b0d009f633000910da5d5e6e50433cdb96/analysis/analysis-api-standalone/src/org/jetbrains/kotlin/analysis/api/standalone/StandaloneAnalysisAPISessionBuilder.kt#L275 but warns about how calling that shouldn't be necessary. It isn't present in Kotlin 2.1.0, and neither is the destroy function that it calls despite that having been in IntelliJ for years.

In summary, I suggest we shrug and continue to simply hard-kill the JVM when we're done.

igfoo · 2024-12-09T12:06:32Z

MODULE.bazel

+        "org.jetbrains.kotlin:%s:2.1.0" % kotlin_lib
        for kotlin_lib in ("kotlin-annotation-processing", "kotlin-compiler")
-    ],
+    ] + [ "com.github.ben-manes.caffeine:caffeine:2.9.3" ]  ,


I think it would be worth adding a comment like

We need the version specified in https://github.com/JetBrains/kotlin/blob/master/gradle/libs.versions.toml

so we don't upgrade to the latest version without thinking.

Depending on what happens with https://youtrack.jetbrains.com/issue/KT-73751/Analysis-API-Caffeine-dependency we might be able to do better here in the future.

igfoo · 2024-12-09T12:08:17Z

java/kotlin-extractor2/src/main/kotlin/KotlinExtractor.kt

        )
        dtw.flush()
        loggerBase.close()
+        System.exit(0) // TODO: figure out what's keeping the JVM awake


Filed a ticket for this here: https://youtrack.jetbrains.com/issue/KT-73753/Analysis-API-doesnt-terminate

You might want to include the context at #18215 (comment) identifying thread-pool cleanup as the cause

igfoo · 2024-12-09T12:10:40Z

java/kotlin-extractor2/src/main/kotlin/entities/Class.kt

 private fun KotlinUsesExtractor.getClassLabel(
    c: KaClassSymbol,
-    argsIncludingOuterClasses: List<Nothing>?
+    argsIncludingOuterClasses: List<List<KaTypeProjection>>?


I think it would be worth including a comment with a small example explaining what this list-of-lists means.

igfoo · 2024-12-09T12:25:52Z

java/kotlin-extractor2/src/main/kotlin/entities/TypeArgument.kt

+            }
+        }
+        else -> {
+            logger.error("Unexpected type argument.")


Can we include the .javaClass in the error please?

igfoo · 2024-12-09T12:28:48Z

java/kotlin-extractor2/src/main/kotlin/entities/TypeParameter.kt

+    javaTypeParameter: JavaTypeParameter? */
+): Label<out DbTypevariable>? {
+    with("type parameter", tp) {
+        val parentId = getTypeParameterParentLabel(tp) ?: return null


Should we log an error in the null case?

The callee already logs, but I've added error-cascading per this and the comment below.

igfoo · 2024-12-09T12:31:16Z

java/kotlin-extractor2/src/main/kotlin/entities/TypeParameter.kt

+    // ensuring that e.g. a method-scoped type variable declared on kotlin.String.transform<R> gets
+    // a different name to the corresponding java.lang.String.transform<R>, even though
+    // useFunction will usually replace references to one function with the other.
+    val parentLabel = getTypeParameterParentLabel(param)


I think this wants to fail, or possibly return some other best-attempt string, if parentLabel is null

igfoo · 2024-12-09T12:50:16Z

java/kotlin-extractor2/src/main/kotlin/entities/Types.kt

-        tw.getLabelFor<DbKt_class_type>("@\"kt_class;{$classId}\"") {
-            tw.writeKt_class_types(it, classId)
+        tw.getLabelFor<DbKt_class_type>("@\"kt_class;{${javaResult.id}}\"") {
+            tw.writeKt_class_types(it, javaResult.id)


I don't think this refactoring is going to do the right thing in the future, e.g. for Int, where the Java ID might be for int but the Kotlin ID should be for Int

Added a TODO -- I wasn't intending to address this here, only to keep the existing behaviour.

igfoo · 2024-12-09T12:52:55Z

java/kotlin-extractor2/src/main/kotlin/entities/Types.kt

                                val kotlinAliasTypeId =
                                    tw.getLabelFor<DbKt_type_alias>("@\"kt_type_alias;{$classId};{$kotlinBaseTypeId}\"") {
-                                        tw.writeKt_type_aliases(it, classId, kotlinBaseTypeId)
+                                        tw.writeKt_type_aliases(it, classId.id, kotlinBaseTypeId)


classId should probably be renamed classResult or similar.

smowton · 2024-12-11T15:59:13Z

@igfoo comments addressed

smowton added 2 commits December 4, 2024 22:38

KE2: Upgrade to Kotlin 2.1.0

2cc2d93

KE2: Restore the basics of type-parameter and type-argument extraction

784a63b

smowton requested review from a team as code owners December 4, 2024 23:08

github-actions bot added the Kotlin label Dec 4, 2024

smowton commented Dec 4, 2024

View reviewed changes

igfoo reviewed Dec 9, 2024

View reviewed changes

smowton added 6 commits December 11, 2024 15:29

Add comment noting Caffeiene dependency

43576a1

Add comment noting structure of argsIncludingOuterClasses

448d368

Note class of unexpected type arguments

504a630

Cascade failures from failure to find a type-parameter parent.

44e44dc

Note Kotlin class ID distinction is a TODO

53593a3

Rename classId

b60298d

igfoo approved these changes Dec 13, 2024

View reviewed changes

smowton merged commit 2aa5cf8 into github:ke2 Dec 13, 2024
6 checks passed

KE2: Upgrade to Kotlin 2.1.0; restore basic type parameter and type argument extraction #18215

KE2: Upgrade to Kotlin 2.1.0; restore basic type parameter and type argument extraction #18215

Uh oh!

Conversation

smowton commented Dec 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-manes commented Dec 5, 2024

Uh oh!

smowton commented Dec 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Dec 11, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants