Skip to content

Commit

Permalink
Improve jdeps logic to find more explicit and implicit dependencies (#…
Browse files Browse the repository at this point in the history 
…1118)

* Improve Kotlin jdeps logic to find more explicit and implicit dependencies (#17)

# Problem
Kotlin jdeps do not collect many direct and indirect dependencies that are required for compilation. These missing jdep found dependencies were discovered as Airbnb tests a "jdeps compile-time pruning" optimization which provides significant IntelliJ indexing, UI, and compilation performance benefits in our fork of the [Bazel IntelliJ Plugin](https://github.com/bazelbuild/intellij).

# Solution
As a followup to #1045, write test cases to repro missing dependencies found in Airbnb's monorepo, and then fix the underlying issues. In addition to these fixes helping our "jdeps compile-time pruning" optimization, these fixes should also help when the normal Bazel IntelliJ Plugin is used, as it also consults jdeps when deciding what jars to import into IntelliJ.

# Test Plan:
* Wrote Kotlin and Java test cases to repro missing dependencies found in the Airbnb monorepo
* Updated Airbnb's minimal rules_kotlin fork to use the changes in this PR, which has been successfully running CI for over a week.

* formatting

* formatting

* revert

* Remove println used for testing

* use new assertion style in jdeps test

* add newline to trigger another build

* Remove class in "js" package, that shouldn't be used and also is no longer needed.

* Remove class in "js" package, that shouldn't be used and also is no longer needed.

---------

Co-authored-by: Steve Cosenza <steve.cosenza@airbnb.com>
  • Loading branch information
@scosenza
scosenza and Steve Cosenza authored Mar 4, 2024
1 parent 12fd63a commit e528366
Show file tree
Hide file tree
Showing 24 changed files with 539 additions and 54 deletions.
181 changes: 129 additions & 52 deletions src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,39 +8,45 @@ import org.jetbrains.kotlin.analyzer.AnalysisResult
import org.jetbrains.kotlin.config.CompilerConfiguration
import org.jetbrains.kotlin.container.StorageComponentContainer
import org.jetbrains.kotlin.container.useInstance
import org.jetbrains.kotlin.descriptors.CallableDescriptor
import org.jetbrains.kotlin.descriptors.ClassDescriptor
import org.jetbrains.kotlin.descriptors.DeclarationDescriptor
import org.jetbrains.kotlin.descriptors.DeclarationDescriptorWithSource
import org.jetbrains.kotlin.descriptors.FunctionDescriptor
import org.jetbrains.kotlin.descriptors.ModuleDescriptor
import org.jetbrains.kotlin.descriptors.ParameterDescriptor
import org.jetbrains.kotlin.descriptors.PropertyDescriptor
import org.jetbrains.kotlin.descriptors.SourceElement
import org.jetbrains.kotlin.descriptors.ValueDescriptor
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.descriptors.JavaClassConstructorDescriptor
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.JvmPackagePartSource
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.platform.TargetPlatform
import org.jetbrains.kotlin.psi.KtDeclaration
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.resolve.BindingTrace
import org.jetbrains.kotlin.resolve.FunctionImportedFromObject
import org.jetbrains.kotlin.resolve.PropertyImportedFromObject
import org.jetbrains.kotlin.resolve.ImportedFromObjectCallableDescriptor
import org.jetbrains.kotlin.resolve.calls.checkers.CallChecker
import org.jetbrains.kotlin.resolve.calls.checkers.CallCheckerContext
import org.jetbrains.kotlin.resolve.calls.model.ExpressionKotlinCallArgument
import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall
import org.jetbrains.kotlin.resolve.calls.util.FakeCallableDescriptorForObject
import org.jetbrains.kotlin.resolve.calls.tower.NewAbstractResolvedCall
import org.jetbrains.kotlin.resolve.calls.tower.PSIKotlinCallArgument
import org.jetbrains.kotlin.resolve.checkers.DeclarationChecker
import org.jetbrains.kotlin.resolve.checkers.DeclarationCheckerContext
import org.jetbrains.kotlin.resolve.descriptorUtil.getImportableDescriptor
import org.jetbrains.kotlin.resolve.jvm.extensions.AnalysisHandlerExtension
import org.jetbrains.kotlin.serialization.deserialization.descriptors.DeserializedTypeAliasDescriptor
import org.jetbrains.kotlin.synthetic.SyntheticJavaPropertyDescriptor
import org.jetbrains.kotlin.types.KotlinType
import org.jetbrains.kotlin.types.TypeConstructor
import org.jetbrains.kotlin.types.getAbbreviation
import org.jetbrains.kotlin.types.typeUtil.supertypes
import java.io.BufferedOutputStream
import java.io.File
Expand Down Expand Up @@ -77,7 +83,22 @@ class JdepsGenExtension(
* @return the path corresponding to the JAR where this class was loaded from, or null.
*/
fun getClassCanonicalPath(descriptor: DeclarationDescriptorWithSource): String? {
return when (val sourceElement: SourceElement = descriptor.source) {
// Get the descriptor's source element which may be a type alias
val sourceElement = if (descriptor.source != SourceElement.NO_SOURCE) {
descriptor.source
} else {
when (val declarationDescriptor = descriptor.getImportableDescriptor()) {
is DeserializedTypeAliasDescriptor -> {
declarationDescriptor.containerSource
}

else -> {
null
}
}
}

return when (sourceElement) {
is JavaSourceElement ->
if (sourceElement.javaElement is BinaryJavaClass) {
(sourceElement.javaElement as BinaryJavaClass).virtualFile.canonicalPath
Expand All @@ -92,13 +113,21 @@ class JdepsGenExtension(
// Ignore Java source local to this module.
null
}

is KotlinJvmBinarySourceElement ->
(sourceElement.binaryClass as VirtualFileKotlinClass).file.canonicalPath
else -> null

is JvmPackagePartSource -> { // This case is needed to collect type aliases
((sourceElement.knownJvmBinaryClass) as VirtualFileKotlinClass).file.canonicalPath
}

else -> {
null
}
}
}

fun getClassCanonicalPath(typeConstructor: TypeConstructor): String? {
private fun getClassCanonicalPath(typeConstructor: TypeConstructor): String? {
return (typeConstructor.declarationDescriptor as? DeclarationDescriptorWithSource)?.let {
getClassCanonicalPath(
it,
Expand Down Expand Up @@ -130,54 +159,88 @@ class JdepsGenExtension(
reportOn: PsiElement,
context: CallCheckerContext,
) {
when (val resultingDescriptor = resolvedCall.resultingDescriptor) {
is FunctionImportedFromObject -> {
collectTypeReferences(resultingDescriptor.containingObject.defaultType)
}
is PropertyImportedFromObject -> {
collectTypeReferences(resultingDescriptor.containingObject.defaultType)
// First collect type from the Resolved Call
collectExplicitTypes(resolvedCall)

resolvedCall.valueArguments.keys.forEach { valueArgument ->
collectTypeReferences(valueArgument.type, isExplicit = false)
}

// And then collect types from any ResultingDescriptor
val resultingDescriptor = resolvedCall.resultingDescriptor
collectExplicitTypes(resultingDescriptor)

val isExplicitReturnType = resultingDescriptor is JavaClassConstructorDescriptor
resultingDescriptor.returnType?.let {
collectTypeReferences(it, isExplicit = isExplicitReturnType, collectTypeArguments = false)
}

resultingDescriptor.valueParameters.forEach { valueParameter ->
collectTypeReferences(valueParameter.type, isExplicit = false)
}

// Finally, collect types that depend on the type of the ResultingDescriptor and note that
// these descriptors may be composed of multiple classes that we need to extract types from
if (resultingDescriptor is DeclarationDescriptor) {
val containingDeclaration = resultingDescriptor.containingDeclaration
if (containingDeclaration is ClassDescriptor) {
collectTypeReferences(containingDeclaration.defaultType)
}
is JavaMethodDescriptor -> {
getClassCanonicalPath(
(resultingDescriptor.containingDeclaration as ClassDescriptor).typeConstructor,
)?.let { explicitClassesCanonicalPaths.add(it) }

if (resultingDescriptor is PropertyDescriptor) {
(
resultingDescriptor.getter
?.correspondingProperty as? SyntheticJavaPropertyDescriptor
)
?.let { syntheticJavaPropertyDescriptor ->
collectTypeReferences(syntheticJavaPropertyDescriptor.type, isExplicit = false)

val functionDescriptor = syntheticJavaPropertyDescriptor.getMethod
functionDescriptor.dispatchReceiverParameter?.type?.let { dispatchReceiverType ->
collectTypeReferences(dispatchReceiverType, isExplicit = false)
}
}
}
is FunctionDescriptor -> {
resultingDescriptor.returnType?.let {
collectTypeReferences(it, isExplicit = false, collectTypeArguments = false)
}
resultingDescriptor.valueParameters.forEach { valueParameter ->
collectTypeReferences(valueParameter.type, isExplicit = false)
}

if (resultingDescriptor is ImportedFromObjectCallableDescriptor<*>) {
collectTypeReferences(resultingDescriptor.containingObject.defaultType, isExplicit = false)
}

if (resultingDescriptor is ValueDescriptor) {
collectTypeReferences(resultingDescriptor.type, isExplicit = false)
}
}

private fun collectExplicitTypes(resultingDescriptor: CallableDescriptor) {
val kotlinJvmBinaryClass = resultingDescriptor.getContainingKotlinJvmBinaryClass()
if (kotlinJvmBinaryClass is VirtualFileKotlinClass) {
explicitClassesCanonicalPaths.add(kotlinJvmBinaryClass.file.path)
}
}

private fun collectExplicitTypes(resolvedCall: ResolvedCall<*>) {
val kotlinCallArguments = (resolvedCall as? NewAbstractResolvedCall)
?.resolvedCallAtom?.atom?.argumentsInParenthesis

// note that callArgument can be both a PSIKotlinCallArgument and an ExpressionKotlinCallArgument
kotlinCallArguments?.forEach { callArgument ->
if (callArgument is PSIKotlinCallArgument) {
val dataFlowInfos = listOf(callArgument.dataFlowInfoBeforeThisArgument) +
callArgument.dataFlowInfoAfterThisArgument

dataFlowInfos.forEach { dataFlowInfo ->
dataFlowInfo.completeTypeInfo.values().flatten().forEach { kotlinType ->
collectTypeReferences(kotlinType, isExplicit = true)
}
}
val virtualFileClass =
resultingDescriptor.getContainingKotlinJvmBinaryClass() as? VirtualFileKotlinClass
?: return
explicitClassesCanonicalPaths.add(virtualFileClass.file.path)
}
is ParameterDescriptor -> {
getClassCanonicalPath(resultingDescriptor)?.let { explicitClassesCanonicalPaths.add(it) }
}
is FakeCallableDescriptorForObject -> {
collectTypeReferences(resultingDescriptor.type)
}
is JavaPropertyDescriptor -> {
getClassCanonicalPath(resultingDescriptor)?.let { explicitClassesCanonicalPaths.add(it) }
}
is PropertyDescriptor -> {
when (resultingDescriptor.containingDeclaration) {
is ClassDescriptor -> collectTypeReferences(
(resultingDescriptor.containingDeclaration as ClassDescriptor).defaultType,
)
else -> {
val virtualFileClass =
(resultingDescriptor).getContainingKotlinJvmBinaryClass() as? VirtualFileKotlinClass
?: return
explicitClassesCanonicalPaths.add(virtualFileClass.file.path)
}

if (callArgument is ExpressionKotlinCallArgument) {
callArgument.receiver.allOriginalTypes.forEach { kotlinType ->
collectTypeReferences(kotlinType, isExplicit = true)
}
collectTypeReferences(resultingDescriptor.type, isExplicit = false)
}
else -> return
}
}

Expand All @@ -192,8 +255,9 @@ class JdepsGenExtension(
collectTypeReferences(it)
}
}

is FunctionDescriptor -> {
descriptor.returnType?.let { collectTypeReferences(it) }
descriptor.returnType?.let { collectTypeReferences(it, collectTypeArguments = false) }
descriptor.valueParameters.forEach { valueParameter ->
collectTypeReferences(valueParameter.type)
}
Expand All @@ -204,6 +268,7 @@ class JdepsGenExtension(
collectTypeReferences(it)
}
}

is PropertyDescriptor -> {
collectTypeReferences(descriptor.type)
descriptor.annotations.forEach { annotation ->
Expand All @@ -213,6 +278,7 @@ class JdepsGenExtension(
collectTypeReferences(annotation.type)
}
}

is LocalVariableDescriptor -> {
collectTypeReferences(descriptor.type)
}
Expand Down Expand Up @@ -245,6 +311,17 @@ class JdepsGenExtension(
}
}

// Collect type aliases aka abbreviations
// See: https://github.com/Kotlin/KEEP/blob/master/proposals/type-aliases.md#type-alias-expansion
kotlinType.getAbbreviation()?.let { abbreviationType ->
collectTypeReferences(
abbreviationType,
isExplicit = isExplicit,
collectTypeArguments = collectTypeArguments,
visitedKotlinTypes = visitedKotlinTypes,
)
}

kotlinType.supertypes().forEach { supertype ->
collectTypeReferences(
supertype,
Expand Down
14 changes: 13 additions & 1 deletion src/test/kotlin/io/bazel/kotlin/builder/tasks/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ java_library(
name = "JdepsParserTestFixtures",
testonly = True,
srcs = glob(["testFixtures/*.java"]),
deps = [
":JdepsParserTestFixtures2",
],
)

java_library(
name = "JdepsParserTestFixtures2",
testonly = True,
srcs = glob(["testFixtures2/*.java"]),
)

kt_rules_test(
Expand All @@ -44,7 +53,10 @@ kt_rules_test(
name = "KotlinBuilderJvmJdepsTest",
size = "large",
srcs = ["jvm/KotlinBuilderJvmJdepsTest.kt"],
data = [":JdepsParserTestFixtures"],
data = [
":JdepsParserTestFixtures",
":JdepsParserTestFixtures2",
],
)

kt_rules_test(
Expand Down
Loading

0 comments on commit e528366

Please sign in to comment.