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

Fix missing jdeps by consistently calling collectTypeReferences #1045

Merged
merged 5 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
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
68 changes: 34 additions & 34 deletions src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ class JdepsGenExtension(
)?.let { explicitClassesCanonicalPaths.add(it) }
}
is FunctionDescriptor -> {
resultingDescriptor.returnType?.let { addImplicitDep(it) }
resultingDescriptor.returnType?.let {
collectTypeReferences(it, isExplicit = false, collectTypeArguments = false)
}
resultingDescriptor.valueParameters.forEach { valueParameter ->
collectTypeReferences(valueParameter.type, isExplicit = false)
}
Expand Down Expand Up @@ -173,7 +175,7 @@ class JdepsGenExtension(
explicitClassesCanonicalPaths.add(virtualFileClass.file.path)
}
}
addImplicitDep(resultingDescriptor.type)
collectTypeReferences(resultingDescriptor.type, isExplicit = false)
}
else -> return
}
Expand Down Expand Up @@ -217,14 +219,6 @@ 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
Expand All @@ -234,35 +228,41 @@ class JdepsGenExtension(
private fun collectTypeReferences(
kotlinType: KotlinType,
isExplicit: Boolean = true,
collectTypeArguments: Boolean = true,
visitedKotlinTypes: MutableSet<Pair<KotlinType, Boolean>> = mutableSetOf(),
) {
if (isExplicit) {
addExplicitDep(kotlinType)
} else {
addImplicitDep(kotlinType)
}

kotlinType.supertypes().forEach {
addImplicitDep(it)
}

collectTypeArguments(kotlinType, isExplicit)
}
val kotlintTypeAndIsExplicit = Pair(kotlinType, isExplicit)
if (!visitedKotlinTypes.contains(kotlintTypeAndIsExplicit)) {
visitedKotlinTypes.add(kotlintTypeAndIsExplicit)

private fun collectTypeArguments(
kotlinType: KotlinType,
isExplicit: Boolean,
visitedKotlinTypes: MutableSet<KotlinType> = mutableSetOf(),
) {
visitedKotlinTypes.add(kotlinType)
kotlinType.arguments.map { it.type }.forEach { typeArgument ->
if (isExplicit) {
addExplicitDep(typeArgument)
getClassCanonicalPath(kotlinType.constructor)?.let {
explicitClassesCanonicalPaths.add(it)
}
} else {
addImplicitDep(typeArgument)
getClassCanonicalPath(kotlinType.constructor)?.let {
implicitClassesCanonicalPaths.add(it)
}
}

kotlinType.supertypes().forEach { supertype ->
collectTypeReferences(
supertype,
isExplicit = false,
collectTypeArguments = collectTypeArguments,
visitedKotlinTypes,
)
}
typeArgument.supertypes().forEach { addImplicitDep(it) }
if (!visitedKotlinTypes.contains(typeArgument)) {
collectTypeArguments(typeArgument, isExplicit, visitedKotlinTypes)

if (collectTypeArguments) {
kotlinType.arguments.map { it.type }.forEach { typeArgument ->
collectTypeReferences(
typeArgument,
isExplicit = isExplicit,
collectTypeArguments = true,
visitedKotlinTypes = visitedKotlinTypes,
)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import com.google.common.truth.Truth.assertThat
import com.google.devtools.build.lib.view.proto.Deps
import io.bazel.kotlin.builder.Deps.*
import io.bazel.kotlin.builder.KotlinJvmTestBuilder
import io.bazel.kotlin.builder.utils.BazelRunFiles
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.Parameterized
Expand Down Expand Up @@ -391,7 +390,7 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) {
}

@Test
fun `kotlin indirect property reference on object`() {
fun `kotlin indirect property reference on object calling helloWorld function`() {
val transitivePropertyTarget = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"Bar.kt",
Expand Down Expand Up @@ -444,6 +443,62 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) {
assertIncomplete(jdeps).isEmpty()
}

@Test
fun `kotlin indirect property reference on object without calling helloWorld function`() {
val transitivePropertyTarget = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"Bar.kt",
"""
package something

class Bar {
fun helloWorld() {}
}
"""
)
}

val dependentTarget = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"Foo.kt",
"""
package something

class Foo {
val bar = Bar()
}
"""
)
c.addDirectDependencies(transitivePropertyTarget)
}

val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"HasPropertyDependency.kt",
"""
package something

class HasPropertyDependency {
fun something(): Any {
val foo = Foo()
return foo.bar
}
}
"""
)
c.addDirectDependencies(dependentTarget)
c.addTransitiveDependencies(transitivePropertyTarget)
}
val jdeps = depsProto(dependingTarget)

assertThat(jdeps.ruleLabel).isEqualTo(dependingTarget.label())
assertThat(jdeps.dependencyCount).isEqualTo(2)
assertExplicit(jdeps).contains(dependentTarget.singleCompileJar())
assertImplicit(jdeps).contains(transitivePropertyTarget.singleCompileJar())
assertUnused(jdeps).isEmpty()
assertIncomplete(jdeps).isEmpty()
}

@Test
fun `kotlin extension property reference`() {
val dependentTarget = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
Expand Down Expand Up @@ -987,6 +1042,120 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) {
assertIncomplete(jdeps).isEmpty()
}

@Test
fun `function call on a class should collect the indirect super class of that class as an implicit dependency`() {
val implicitSuperClassDep = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"Base.kt",
"""
package something

open class Base
"""
)
}

val explicitSuperClassDep = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"Derived.kt",
"""
package something

class Derived : Base() {
fun hi(): String {
return "Hello"
}
}
"""
)
c.addDirectDependencies(implicitSuperClassDep)
}

val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"ReferencesClassWithSuperClass.kt",
"""
package something

class ReferencesClassWithSuperClass {
fun stuff(): String {
return Derived().hi()
}
}
"""
)
c.addDirectDependencies(explicitSuperClassDep)
c.addTransitiveDependencies(implicitSuperClassDep)
}
val jdeps = depsProto(dependingTarget)

assertThat(jdeps.ruleLabel).isEqualTo(dependingTarget.label())

assertExplicit(jdeps).containsExactly(explicitSuperClassDep.singleCompileJar())
assertImplicit(jdeps).containsExactly(implicitSuperClassDep.singleCompileJar())
assertUnused(jdeps).isEmpty()
assertIncomplete(jdeps).isEmpty()
}

@Test
fun `creating a kotlin class should collect the indirect java super class, with a kotlin type param class, of that class as an implicit dependency`() {
val implicitSuperClassGenericTypeParamDep = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"BaseGenericType.kt",
"""
package something

class BaseGenericType
"""
)
}

val explicitClassWithTypeParamJavaSuperclassDep = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"Derived.kt",
"""
package something
import something.JavaBaseWithTypeParam

class Derived : JavaBaseWithTypeParam<BaseGenericType>() {
fun hi(): String {
return "Hello"
}
}
"""
)
c.addDirectDependencies(TEST_FIXTURES_DEP)
c.addDirectDependencies(implicitSuperClassGenericTypeParamDep)
}

val dependingTarget = runJdepsCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
c.addSource(
"ReferencesClassWithSuperClass.kt",
"""
package something

class ReferencesClassWithSuperClass {
fun stuff(): String {
val derived = Derived()
return derived.toString()
}
}
"""
)
c.addDirectDependencies(explicitClassWithTypeParamJavaSuperclassDep)
c.addTransitiveDependencies(TEST_FIXTURES_DEP)
c.addTransitiveDependencies(implicitSuperClassGenericTypeParamDep)
}
val jdeps = depsProto(dependingTarget)

assertThat(jdeps.ruleLabel).isEqualTo(dependingTarget.label())
assertExplicit(jdeps).containsExactly(explicitClassWithTypeParamJavaSuperclassDep.singleCompileJar())
assertImplicit(jdeps).contains(TEST_FIXTURES_DEP.singleCompileJar())
assertImplicit(jdeps).contains(implicitSuperClassGenericTypeParamDep.singleCompileJar())
assertUnused(jdeps).isEmpty()
assertIncomplete(jdeps).isEmpty()
}

@Test
fun `class declaration all super class references should be an implicit dependency`() {
val implicitSuperClassDep = runCompileTask { c: KotlinJvmTestBuilder.TaskBuilder ->
Expand Down Expand Up @@ -1207,6 +1376,7 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) {
)
c.addDirectDependencies(depWithFunction)
c.addTransitiveDependencies(depWithReturnType)
c.addTransitiveDependencies(depWithReturnTypesSuperType)
c.setLabel("dependingTarget")
}
val jdeps = depsProto(dependingTarget)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package something;

public class JavaBaseWithTypeParam<T> {
public T passthru(T t) {
return t;
}
}