Skip to content

Commit

Permalink
Fix missing jdeps by consistently calling collectTypeReferences (#1045
Browse files Browse the repository at this point in the history 
)

---------

Co-authored-by: Steve Cosenza <[email protected]>
  • Loading branch information
@scosenza
scosenza and Steve Cosenza authored Oct 24, 2023
1 parent 0229db6 commit cfe427b
Show file tree
Hide file tree
Showing 3 changed files with 213 additions and 36 deletions.
68 changes: 34 additions & 34 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 @@ -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
174 changes: 172 additions & 2 deletions src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt
View file
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
7 changes: 7 additions & 0 deletions src/test/kotlin/io/bazel/kotlin/builder/tasks/testFixtures/JavaBaseWithTypeParam.java
View file
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;
}
}

0 comments on commit cfe427b

Please sign in to comment.