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

Conversation

scosenza
Copy link
Contributor

@scosenza scosenza commented Oct 8, 2023

Problem

Kotlin jdeps are missing in some cases which is preventing Airbnb from enabling certain compile-time pruning optimizations in our optimized IntelliJ Bazel Plugin. For example, for the following repro case:

open class Base

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

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

Without this PR fix, Airbnb's compile time pruning (which relies on Kotlin jdeps) 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

Solution

Combine collectTypeReferences and collectTypeArguments and inline addExplicitDep and addImplicitDep, to prevent future missing jdep bugs where code paths forgot to traverse superclasses and type-params.

Note that the fix in this PR initially caused the following existing test case 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 8, 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.

@nkoroste
Copy link
Collaborator

Hey @scosenza should we promote this to a proper PR and try to merge it in if it fixes a legit jdeps issue?

@scosenza
Copy link
Contributor Author

scosenza commented Oct 21, 2023

Sorry for the delay @nkoroste! I'll take a look now into the Buildkite failures and then I'll remove the draft status.

@scosenza scosenza marked this pull request as ready for review October 21, 2023 05:30
@scosenza scosenza changed the title WIP: Fix missing jdeps by consistently calling collectTypeReferences Fix missing jdeps by consistently calling collectTypeReferences Oct 21, 2023
@restingbull restingbull merged commit cfe427b into bazelbuild:master Oct 24, 2023
2 checks passed
scosenza pushed a commit to scosenza/rules_kotlin that referenced this pull request Feb 26, 2024
…ncies (bazelbuild#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 bazelbuild#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.
Bencodes pushed a commit that referenced this pull request Mar 4, 2024
…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>
Bencodes pushed a commit to Bencodes/rules_kotlin that referenced this pull request Mar 13, 2024
…azelbuild#1118)

* Improve Kotlin jdeps logic to find more explicit and implicit dependencies (bazelbuild#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 bazelbuild#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>
oliviernotteghem pushed a commit to uber-common/rules_kotlin that referenced this pull request Aug 2, 2024
…azelbuild#1118)

* Improve Kotlin jdeps logic to find more explicit and implicit dependencies (bazelbuild#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 bazelbuild#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>
oliviernotteghem pushed a commit to uber-common/rules_kotlin that referenced this pull request Aug 2, 2024
…azelbuild#1118)

* Improve Kotlin jdeps logic to find more explicit and implicit dependencies (bazelbuild#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 bazelbuild#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>
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.

3 participants