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

WIP: Fix missing jdeps by consistently using collectTypeReferences #1044

Conversation

scosenza
Copy link
Contributor

@scosenza scosenza commented Oct 6, 2023

Problem

Kotlin jdeps are missing in some cases which is preventing Airbnb from enabling certain compile-time pruning optimizations in our fork of the IntelliJ Bazel Plugin. For example, in the below repro case, Airbnb's IntelliJ Bazel Plugin compile time pruning drops the jar containing Base which results in the following compilation error:

 Kotlin: Supertypes of the following classes cannot be resolved. Please make sure you have the required dependencies in the classpath:
        class something.ReferencesClassWithSuperClass, unresolved supertypes: something.Base

Without the change in this PR, ReferencesClassWithSuperClass has a missing jdep on superclass Base:

open class Base

class Derived : Base() {
  fun hi(): String {
    return "Hello"
  }
}

class ReferencesClassWithSuperClass {
    fun stuff(): String {
        return Derived().hi()
    }
}

Solution

Use collectTypeReferences instead of directly calling addImplicitDep or addExplicitDep as collectTypeReferences additionally always adds superclasses as implicit deps.

Note that the initial fix originally caused two existing test cases to fail:

1) function call return type[enableK2Compiler=false](io.bazel.kotlin.builder.tasks.jvm.KotlinBuilderJvmJdepsTest)
io.bazel.kotlin.builder.toolchain.CompilationStatusException: compile phase failed:
error: supertypes of the following classes cannot be resolved. Please make sure you have the required dependencies in the classpath:
    class something.SomeType, unresolved supertypes: something.SomeSuperType
Adding -Xextended-compiler-checks argument might provide additional information.

        at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.executeCompilerTask(CompilationTaskContext.kt:135)

This existing test failure seems related to both:

To fix this test, I added c.addTransitiveDependencies(depWithReturnTypesSuperType) as SomeSuperType is required to compile ReferencesClassWithSuperClass.

@google-cla
Copy link

google-cla bot commented Oct 6, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@scosenza scosenza changed the title WIP: Repro case for missing jdep on a lazy superclass WIP: Repro case for missing jdep on a superclass Oct 6, 2023
@scosenza scosenza changed the title WIP: Repro case for missing jdep on a superclass Fix missing jdep on superclass by using collectTypeReferences instead of addImplicitDep Oct 6, 2023
@scosenza scosenza changed the title Fix missing jdep on superclass by using collectTypeReferences instead of addImplicitDep WIP: Fix missing jdep on superclass by using collectTypeReferences instead of addImplicitDep Oct 6, 2023
@scosenza scosenza changed the title WIP: Fix missing jdep on superclass by using collectTypeReferences instead of addImplicitDep WIP: Fix missing jdeps by using collectTypeReferences instead of addImplicitDep Oct 7, 2023
@@ -444,6 +444,62 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) {
assertIncomplete(jdeps).isEmpty()
}

@Test
fun `kotlin indirect property reference on object 2`() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Rename test case and add additional tests

@scosenza scosenza changed the title WIP: Fix missing jdeps by using collectTypeReferences instead of addImplicitDep WIP: Fix missing jdeps by consistently using collectTypeReferences Oct 8, 2023
@scosenza scosenza closed this Oct 8, 2023
@scosenza scosenza deleted the missing_jdeps_on_lazy_class_super_classes branch October 8, 2023 01:50
@scosenza
Copy link
Contributor Author

scosenza commented Oct 9, 2023

Replaced this PR with #1045

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant