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

use shaded libs to improve detekt stability #496

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

litols
Copy link
Contributor

@litols litols commented Oct 27, 2023

Motivation:

  • detekt relies on kotlin-compiler as a dependency, requiring strict adherence to Kotlin's version.
  • The detekt plugin leverages the Kotlin compiler included in IntelliJ IDEA's Kotlin plugin, making it challenging to specify the Kotlin plugin's version precisely on the plugin side.
  • Consequently, version mismatches are prone to occur, leading to the instability of the detekt-intellij-plugin.

Modification:

  • To address this issue and ensure the stability of detekt, we propose creating a shaded JAR with relocated kotlin compiler PSI dependencies.
  • This allows us to fix the Kotlin PSI version used by detekt, minimizing potential issues stemming from Kotlin version discrepancies in the detekt-intellij-plugin.

@litols litols marked this pull request as draft October 27, 2023 09:59
@litols litols marked this pull request as ready for review October 27, 2023 10:05
@litols litols marked this pull request as draft October 27, 2023 14:40
@litols litols marked this pull request as ready for review November 1, 2023 03:00
tasks.withType<PrepareSandboxTask> {
pluginJar = project.tasks.withType<ShadowJar>().getByName("shadowJar").archiveFile
// disable to collect libraries located in runtime classpath (replace with empty file collection)
runtimeClasspathFiles = project.objects.fileCollection()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/JetBrains/gradle-intellij-plugin/blob/a10fd139467dd34c8e292296b563edfed14fea9d/src/main/kotlin/org/jetbrains/intellij/IntelliJPlugin.kt#L611
runtimeClasspathFiles uses to include depended libraries and plugins.
From this changes applied, we use shadow jar, which included libraries, so we can replace runtimeClasspathFiles empty.

@arturbosch
Copy link
Member

Hi,
which problems did you had with mixing the detekt and kotlin plugins?
Detekt uses the embeddable compiler and Intellij the unshaded compiler, which conflicts occur?
For example gradle runIde runs the detekt plugin besides the Kotlin 1.6 plugin and it works fine on my side.

You think this change also fix showing Kotlin compiler errors as detekt plugin errors #271 ?

Unfortunately shading the compiler lib does not help with using the normal compiler classes: #271
2023-11-05 16:45:29,195 [ 25461] SEVERE - #c.i.o.u.ObjectTree - class org.jetbrains.kotlin.psi.KtNamedFunction cannot be cast to class detekt.shadow.org.jetbrains.kotlin.psi.KtModifierListOwner

build.gradle.kts Outdated
}
}

tasks.withType<PrepareSandboxTask> {
Copy link
Member

Choose a reason for hiding this comment

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

I think this line triggers the shadowJar task eagerly.

tasks.withType<PrepareSandboxTask>.configure {

build.gradle.kts Outdated
@@ -54,6 +57,20 @@ tasks.publishPlugin {
channels.set(listOf(project.version.toString().split('-').getOrElse(1) { "default" }.split('.').first()))
}

tasks {
withType<ShadowJar> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
withType<ShadowJar> {
tasks.withType<ShadowJar>.configure {

@litols
Copy link
Contributor Author

litols commented Nov 9, 2023

sorry for too late response.

which problems did you had with mixing the detekt and kotlin plugins?

#271. (but after upgrade the plugin to 2.2.0, running in IntelliJ IDEA 2023.2.4 + Kotlin plugin 232-1.9.20-release-507-IJ10072.2, these error is not occurring...)

Detekt uses the embeddable compiler and Intellij the unshaded compiler, which conflicts occur?
You think this change also fix showing Kotlin compiler errors as detekt plugin errors #271 ?

I think kotlin plugin picking up the kotlin compiler from detekt plugin wrongly.
Reasons:

  • many issues reports detekt plugin related, however it is not have a detekt related classes in stacktrace.
  • newly kotlin plugin + latest detekt plugin is works fine

Thus,I think we can mitigate #271 with relocating the classes which uses in kotlin plugin.

JFYI, kotlin-intellij-plugin is relocating kotlin PSI files,
https://github.com/nbadal/ktlint-intellij-plugin/tree/main/lib
but the relocating is applied partially, it seems we need picking up correct classes.
maybe KtNamedFunction need to excluding from relocating in detekt-intellij-plugin.

Anyway, I've applied your review comments to this pull request. thank you for your review!

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.

2 participants