From fc4158c122111b322fc1ed19293bc62cf0b25214 Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Thu, 22 Feb 2024 14:36:49 -0800 Subject: [PATCH 1/7] Improve jdeps tracking and add test cases --- .../kotlin/plugin/jdeps/JdepsGenExtension.kt | 6 ++ .../tasks/jvm/KotlinBuilderJvmJdepsTest.kt | 81 +++++++++++++++++++ .../builder/tasks/testFixtures/BaseClass.java | 4 + .../tasks/testFixtures/ExampleException.java | 5 ++ .../SomeClassWithInheritance.java | 9 +++ 5 files changed, 105 insertions(+) create mode 100644 src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/BaseClass.java create mode 100644 src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/ExampleException.java create mode 100644 src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/SomeClassWithInheritance.java diff --git a/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt b/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt index b6343d3ee..485f949c2 100644 --- a/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt +++ b/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt @@ -191,6 +191,9 @@ class JdepsGenExtension( descriptor.typeConstructor.supertypes.forEach { collectTypeReferences(it) } + descriptor.annotations.forEach { annotation -> + collectTypeReferences(annotation.type) + } } is FunctionDescriptor -> { descriptor.returnType?.let { collectTypeReferences(it) } @@ -216,6 +219,9 @@ class JdepsGenExtension( is LocalVariableDescriptor -> { collectTypeReferences(descriptor.type) } + else -> { + System.err.println("$declaration") + } } } diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt b/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt index c47ef7f80..60a32b636 100644 --- a/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt @@ -221,6 +221,87 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) { assertIncomplete(jdeps).isEmpty() } + @Test + fun `java annotation on class is an explict dep`() { + val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder -> + c.addSource( + "AnotherClass.kt", + """ + package something + + @JavaAnnotation + class AnotherClass { + } + """ + ) + c.addDirectDependencies(TEST_FIXTURES_DEP) + } + val jdeps = depsProto(dependingTarget) + + assertThat(jdeps.ruleLabel).isEqualTo(dependingTarget.label()) + + assertExplicit(jdeps).contains(TEST_FIXTURES_DEP.singleCompileJar()) + assertImplicit(jdeps).isEmpty() + assertUnused(jdeps).isEmpty() + assertIncomplete(jdeps).isEmpty() + } + + @Test + fun `java sealed classes`() { + val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder -> + c.addSource( + "AnotherClass.kt", + """ + package something + + import pkg.assertion.ExampleException + + sealed class SomeClass { + companion object { + fun mapExceptions(exception: Exception) = when (exception) { + is ExampleException -> exception + else -> exception + } + } + } + """ + ) + c.addDirectDependencies(TEST_FIXTURES_DEP) + } + val jdeps = depsProto(dependingTarget) + + assertThat(jdeps.ruleLabel).isEqualTo(dependingTarget.label()) + + assertExplicit(jdeps).contains(TEST_FIXTURES_DEP.singleCompileJar()) + assertImplicit(jdeps).isEmpty() + assertUnused(jdeps).isEmpty() + assertIncomplete(jdeps).isEmpty() + } + + @Test + fun `java classes with inheritance`() { + val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder -> + c.addSource( + "AnotherClass.kt", + """ + package something + + class SomeClass : SomeClassWithInheritance.BaseClassCallback { + } + """ + ) + c.addDirectDependencies(TEST_FIXTURES_DEP) + } + val jdeps = depsProto(dependingTarget) + + assertThat(jdeps.ruleLabel).isEqualTo(dependingTarget.label()) + + assertExplicit(jdeps).contains(TEST_FIXTURES_DEP.singleCompileJar()) + assertImplicit(jdeps).isEmpty() + assertUnused(jdeps).isEmpty() + assertIncomplete(jdeps).isEmpty() + } + @Test fun `java annotation on property is an explict dep`() { val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder -> diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/BaseClass.java b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/BaseClass.java new file mode 100644 index 000000000..601fcccfd --- /dev/null +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/BaseClass.java @@ -0,0 +1,4 @@ +package something; + +public abstract class BaseClass { +} diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/ExampleException.java b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/ExampleException.java new file mode 100644 index 000000000..c2c2377b5 --- /dev/null +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/ExampleException.java @@ -0,0 +1,5 @@ +package pkg.assertion; + +public class ExampleException extends RuntimeException { + +} \ No newline at end of file diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/SomeClassWithInheritance.java b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/SomeClassWithInheritance.java new file mode 100644 index 000000000..10a5d62f2 --- /dev/null +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/SomeClassWithInheritance.java @@ -0,0 +1,9 @@ +package something; + +public class SomeClassWithInheritance extends BaseClass { + + + interface BaseClassCallback { + + } +} From e7ca30baa64661c72b393ed029d244d63631908d Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Thu, 22 Feb 2024 14:37:27 -0800 Subject: [PATCH 2/7] EOL --- .../kotlin/builder/tasks/testFixtures/ExampleException.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/ExampleException.java b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/ExampleException.java index c2c2377b5..ba145afbc 100644 --- a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/ExampleException.java +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/ExampleException.java @@ -2,4 +2,4 @@ public class ExampleException extends RuntimeException { -} \ No newline at end of file +} From 41e3ce6a8bd2b819c59d898176f8677f19e2914f Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Thu, 22 Feb 2024 14:38:02 -0800 Subject: [PATCH 3/7] Revert --- .../kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt b/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt index 485f949c2..770e414fc 100644 --- a/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt +++ b/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt @@ -219,9 +219,6 @@ class JdepsGenExtension( is LocalVariableDescriptor -> { collectTypeReferences(descriptor.type) } - else -> { - System.err.println("$declaration") - } } } From 6af0035c72e8c24e706fa450b6c42b5b2a32df4e Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Thu, 22 Feb 2024 15:05:33 -0800 Subject: [PATCH 4/7] Upstream some of the patches from Olivier --- .../kotlin/plugin/jdeps/JdepsGenExtension.kt | 85 ++++++++++++------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt b/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt index 770e414fc..6a2aa7d21 100644 --- a/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt +++ b/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt @@ -20,12 +20,14 @@ import org.jetbrains.kotlin.descriptors.impl.LocalVariableDescriptor import org.jetbrains.kotlin.extensions.StorageComponentContainerContributor import org.jetbrains.kotlin.load.java.descriptors.JavaMethodDescriptor import org.jetbrains.kotlin.load.java.descriptors.JavaPropertyDescriptor +import org.jetbrains.kotlin.load.java.lazy.descriptors.LazyJavaClassDescriptor import org.jetbrains.kotlin.load.java.sources.JavaSourceElement import org.jetbrains.kotlin.load.java.structure.impl.classFiles.BinaryJavaClass import org.jetbrains.kotlin.load.java.structure.impl.classFiles.BinaryJavaField import org.jetbrains.kotlin.load.kotlin.KotlinJvmBinarySourceElement import org.jetbrains.kotlin.load.kotlin.VirtualFileKotlinClass import org.jetbrains.kotlin.load.kotlin.getContainingKotlinJvmBinaryClass +import org.jetbrains.kotlin.name.Name import org.jetbrains.kotlin.platform.TargetPlatform import org.jetbrains.kotlin.psi.KtDeclaration import org.jetbrains.kotlin.psi.KtFile @@ -38,6 +40,7 @@ import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall import org.jetbrains.kotlin.resolve.calls.util.FakeCallableDescriptorForObject import org.jetbrains.kotlin.resolve.checkers.DeclarationChecker import org.jetbrains.kotlin.resolve.checkers.DeclarationCheckerContext +import org.jetbrains.kotlin.resolve.constants.ConstantValue import org.jetbrains.kotlin.resolve.jvm.extensions.AnalysisHandlerExtension import org.jetbrains.kotlin.types.KotlinType import org.jetbrains.kotlin.types.TypeConstructor @@ -45,6 +48,8 @@ import org.jetbrains.kotlin.types.typeUtil.supertypes import java.io.BufferedOutputStream import java.io.File import java.nio.file.Paths +import java.util.* + /** * Kotlin compiler extension that tracks classes (and corresponding classpath jars) needed to @@ -105,6 +110,18 @@ class JdepsGenExtension( ) } } + + fun getResourceName(descriptor: DeclarationDescriptorWithSource): String? { + if (descriptor.containingDeclaration is LazyJavaClassDescriptor) { + val fqName: String? = (descriptor.containingDeclaration as LazyJavaClassDescriptor)?.jClass?.fqName?.asString() + if (fqName != null) { + if (fqName.indexOf(".R.") > 0 || fqName.indexOf("R.") == 0) { + return fqName + "." + descriptor.name.asString() + } + } + } + return null + } } private val explicitClassesCanonicalPaths = mutableSetOf() @@ -143,8 +160,9 @@ class JdepsGenExtension( )?.let { explicitClassesCanonicalPaths.add(it) } } is FunctionDescriptor -> { + resultingDescriptor.returnType?.let { addImplicitDep(it) } resultingDescriptor.returnType?.let { - collectTypeReferences(it, isExplicit = false, collectTypeArguments = false) + collectTypeReferences(it, isExplicit = false) } resultingDescriptor.valueParameters.forEach { valueParameter -> collectTypeReferences(valueParameter.type, isExplicit = false) @@ -162,6 +180,7 @@ class JdepsGenExtension( } is JavaPropertyDescriptor -> { getClassCanonicalPath(resultingDescriptor)?.let { explicitClassesCanonicalPaths.add(it) } + getResourceName(resultingDescriptor)?.let { explicitClassesCanonicalPaths.add(it) } } is PropertyDescriptor -> { when (resultingDescriptor.containingDeclaration) { @@ -175,7 +194,7 @@ class JdepsGenExtension( explicitClassesCanonicalPaths.add(virtualFileClass.file.path) } } - collectTypeReferences(resultingDescriptor.type, isExplicit = false) + addImplicitDep(resultingDescriptor.type) } else -> return } @@ -222,6 +241,14 @@ class JdepsGenExtension( } } + private fun addImplicitDep(it: KotlinType) { + getClassCanonicalPath(it.constructor)?.let { implicitClassesCanonicalPaths.add(it) } + } + + private fun addExplicitDep(it: KotlinType) { + getClassCanonicalPath(it.constructor)?.let { explicitClassesCanonicalPaths.add(it) } + } + /** * Records direct and indirect references for a given type. Direct references are explicitly * used in the code, e.g: a type declaration or a generic type declaration. Indirect references @@ -231,41 +258,35 @@ class JdepsGenExtension( private fun collectTypeReferences( kotlinType: KotlinType, isExplicit: Boolean = true, - collectTypeArguments: Boolean = true, - visitedKotlinTypes: MutableSet> = mutableSetOf(), ) { - val kotlintTypeAndIsExplicit = Pair(kotlinType, isExplicit) - if (!visitedKotlinTypes.contains(kotlintTypeAndIsExplicit)) { - visitedKotlinTypes.add(kotlintTypeAndIsExplicit) + if (isExplicit) { + addExplicitDep(kotlinType) + } else { + addImplicitDep(kotlinType) + } + + kotlinType.supertypes().forEach { + addImplicitDep(it) + } + collectTypeArguments(kotlinType, isExplicit) + } + + private fun collectTypeArguments( + kotlinType: KotlinType, + isExplicit: Boolean, + visitedKotlinTypes: MutableSet = mutableSetOf(), + ) { + visitedKotlinTypes.add(kotlinType) + kotlinType.arguments.map { it.type }.forEach { typeArgument -> if (isExplicit) { - getClassCanonicalPath(kotlinType.constructor)?.let { - explicitClassesCanonicalPaths.add(it) - } + addExplicitDep(typeArgument) } else { - getClassCanonicalPath(kotlinType.constructor)?.let { - implicitClassesCanonicalPaths.add(it) - } + addImplicitDep(typeArgument) } - - kotlinType.supertypes().forEach { supertype -> - collectTypeReferences( - supertype, - isExplicit = false, - collectTypeArguments = collectTypeArguments, - visitedKotlinTypes, - ) - } - - if (collectTypeArguments) { - kotlinType.arguments.map { it.type }.forEach { typeArgument -> - collectTypeReferences( - typeArgument, - isExplicit = isExplicit, - collectTypeArguments = true, - visitedKotlinTypes = visitedKotlinTypes, - ) - } + typeArgument.supertypes().forEach { addImplicitDep(it) } + if (!visitedKotlinTypes.contains(typeArgument)) { + collectTypeArguments(typeArgument, isExplicit, visitedKotlinTypes) } } } From ac8570f5a89857e054d257dc078afcc1b7041fdc Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Thu, 22 Feb 2024 16:08:07 -0800 Subject: [PATCH 5/7] Expand java annotation test cases --- .../tasks/jvm/KotlinBuilderJvmJdepsTest.kt | 67 ++++++++++++++++--- .../tasks/testFixtures/JavaAnnotation.java | 3 + 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt b/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt index 60a32b636..ef5373db0 100644 --- a/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt @@ -247,7 +247,62 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) { } @Test - fun `java sealed classes`() { + fun `java annotation values on class are an explict dep`() { + val dependentTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder -> + c.addSource( + "ClassToReference.kt", + """ + package something; + + @JavaAnnotation + class ClassToReference + """ + ) + c.addDirectDependencies(TEST_FIXTURES_DEP) + } + + val anotherDepTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder -> + c.addSource( + "AnotherClassToReference.kt", + """ + package something; + + @JavaAnnotation(modules = [ClassToReference::class]) + class AnotherClassToReference + """ + ) + c.addDirectDependencies(dependentTarget) + c.addDirectDependencies(TEST_FIXTURES_DEP) + } + + val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder -> + c.addSource( + "AnotherClass.kt", + """ + package something + + @JavaAnnotation(modules = [AnotherClassToReference::class]) + class AnotherClass { + } + """ + ) + c.addTransitiveDependencies(dependentTarget) + c.addDirectDependencies(anotherDepTarget) + c.addDirectDependencies(TEST_FIXTURES_DEP) + } + val jdeps = depsProto(dependingTarget) + + assertThat(jdeps.ruleLabel).isEqualTo(dependingTarget.label()) + + assertExplicit(jdeps).contains(TEST_FIXTURES_DEP.singleCompileJar()) + assertExplicit(jdeps).contains(anotherDepTarget.singleCompileJar()) + assertImplicit(jdeps).contains(dependentTarget.singleCompileJar()) + assertUnused(jdeps).isEmpty() + assertIncomplete(jdeps).isEmpty() + } + + @Test + fun `types referenced inside when statements are explicit deps`() { val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder -> c.addSource( "AnotherClass.kt", @@ -256,13 +311,9 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) { import pkg.assertion.ExampleException - sealed class SomeClass { - companion object { - fun mapExceptions(exception: Exception) = when (exception) { - is ExampleException -> exception - else -> exception - } - } + fun mapExceptions(exception: Exception) = when (exception) { + is ExampleException -> exception + else -> exception } """ ) diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java index 0b0875ec6..749170691 100644 --- a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java @@ -2,7 +2,10 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; +import java.lang.reflect.Array; @Retention(RetentionPolicy.RUNTIME) public @interface JavaAnnotation { + + Class[] modules() default {}; } \ No newline at end of file From 27eddde45975a289a45eab193127c5f95968c0b7 Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Thu, 22 Feb 2024 16:14:32 -0800 Subject: [PATCH 6/7] EOL --- .../bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java index 749170691..91b0cd369 100644 --- a/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaAnnotation.java @@ -8,4 +8,4 @@ public @interface JavaAnnotation { Class[] modules() default {}; -} \ No newline at end of file +} From 59078a13e51707cb72cd28cf77bc58f871ed105e Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Fri, 23 Feb 2024 16:04:37 -0800 Subject: [PATCH 7/7] Imports --- .../kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt b/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt index 6a2aa7d21..17aa86200 100644 --- a/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt +++ b/src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt @@ -27,7 +27,6 @@ import org.jetbrains.kotlin.load.java.structure.impl.classFiles.BinaryJavaField import org.jetbrains.kotlin.load.kotlin.KotlinJvmBinarySourceElement import org.jetbrains.kotlin.load.kotlin.VirtualFileKotlinClass import org.jetbrains.kotlin.load.kotlin.getContainingKotlinJvmBinaryClass -import org.jetbrains.kotlin.name.Name import org.jetbrains.kotlin.platform.TargetPlatform import org.jetbrains.kotlin.psi.KtDeclaration import org.jetbrains.kotlin.psi.KtFile @@ -40,7 +39,6 @@ import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall import org.jetbrains.kotlin.resolve.calls.util.FakeCallableDescriptorForObject import org.jetbrains.kotlin.resolve.checkers.DeclarationChecker import org.jetbrains.kotlin.resolve.checkers.DeclarationCheckerContext -import org.jetbrains.kotlin.resolve.constants.ConstantValue import org.jetbrains.kotlin.resolve.jvm.extensions.AnalysisHandlerExtension import org.jetbrains.kotlin.types.KotlinType import org.jetbrains.kotlin.types.TypeConstructor @@ -50,7 +48,6 @@ import java.io.File import java.nio.file.Paths import java.util.* - /** * Kotlin compiler extension that tracks classes (and corresponding classpath jars) needed to * compile current kotlin target. Tracked data should include all classes whose changes could