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.

Sign up for GitHub

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

Jump to bottom

Improve jdeps logic to find more explicit and implicit dependencies #1118

Merged
merged 11 commits into from
Mar 4, 2024
Merged

Improve jdeps logic to find more explicit and implicit dependencies #1118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
72b1641
Improve Kotlin jdeps logic to find more explicit and implicit depende…
Feb 20, 2024
59c3840
formatting
Feb 26, 2024
c58339e
formatting
Feb 26, 2024
816086e
revert
Feb 26, 2024
ab93f87
Remove println used for testing
scosenza Feb 27, 2024
f91e46d
merge master
Mar 2, 2024
425a78f
use new assertion style in jdeps test
Mar 2, 2024
1f5853b
Merge branch 'fix_missing_jdep_found_deps' of https://github.com/scos…
Mar 2, 2024
80ed7fb
add newline to trigger another build
Mar 2, 2024
f8605f8
Remove class in "js" package, that shouldn't be used and also is no l…
Mar 4, 2024
46ca4d9
Remove class in "js" package, that shouldn't be used and also is no l…
Mar 4, 2024
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
2 changes: 1 addition & 1 deletion .bazelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ common --enable_bzlmod=true
build --strategy=KotlinCompile=worker
build --test_output=all
build --verbose_failures

common --jvmopt=-Djava.security.manager=allow
174 changes: 122 additions & 52 deletions src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,43 @@ import org.jetbrains.kotlin.container.useInstance
import org.jetbrains.kotlin.descriptors.ClassDescriptor
import org.jetbrains.kotlin.descriptors.DeclarationDescriptor
import org.jetbrains.kotlin.descriptors.DeclarationDescriptorWithSource
import org.jetbrains.kotlin.descriptors.ValueDescriptor
import org.jetbrains.kotlin.descriptors.CallableDescriptor
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.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.js.translate.callTranslator.getReturnType
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the right function to be using? It looks like it's being loaded from the javascript package.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I briefly looked around and didn't see anything that stands out. If anything you can just inline the function itself, which is just calling into another function with resolvedCall as an argument.

Copy link
Contributor Author

@scosenza scosenza Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for flagging this.

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 +84,21 @@ 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 @@ -94,11 +115,16 @@ class JdepsGenExtension(
}
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 +156,87 @@ 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
collectTypeReferences(resolvedCall.getReturnType(), isExplicit = isExplicitReturnType, collectTypeArguments = false)
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 @@ -193,7 +252,7 @@ class JdepsGenExtension(
}
}
is FunctionDescriptor -> {
descriptor.returnType?.let { collectTypeReferences(it) }
descriptor.returnType?.let { collectTypeReferences(it, collectTypeArguments = false ) }
descriptor.valueParameters.forEach { valueParameter ->
collectTypeReferences(valueParameter.type)
}
Expand Down Expand Up @@ -245,6 +304,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
Open in desktop
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"]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might look less copy-paste grouping these different test figures inside directories within the testFixtures directory instead of just duplicating and adding a numerical index. Ex: testFixtures/a-sources/ testFixtures/b-sources/

)

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