JVM 17 & Kotlin 1.6.x Support

[Nitewriter]

The changes made to the final version of 1.20.0 have reverted fixes that addressed the invalid jvm target error experienced in this ticket. #4287

* What went wrong:
Execution failed for task ':match-api:detekt'.
> Invalid value passed to --jvm-target

I'm currently unable to upgrade to resolve the current XML External Entity (XXE) Injection vulnerability. Version 1.20.0-RC2 continues to have no issues with JVM 17 and Kotlin 1.6.x, but does not have a resolution for the vulnerability.

[cortinico]

The changes made to the final version of 1.20.0 have reverted fixes that addressed the invalid jvm target error experienced in this ticket. #4287

Could you link to the revert you're talking about?

[mkaulfers]

@cortinico this is the ticket he's referring to that should have fixed it, but it is no longer working. #4277

[cortinico]

this is the ticket he's referring to that should have fixed it, but it is no longer working. #4277

This hasn't been reverted though 🤔 I'm wondering if @Nitewriter was referring to a specific revert or not

[rocketraman]

Is there a workaround, or do we need to revert to 1.19 for now?

[cortinico]

How are you folks passing the --jvm-target? Via CLI or via Gradle? Which value are you specifying?
Could you provide a reproducer of any form to help us understand what's going on?

[rocketraman]

In my case I'm using the gradle plugin. I am not passing that argument explicitly at all.

[cortinico]

In my case I'm using the gradle plugin. I am not passing that argument explicitly at all.

I'm not able to reproduce. Tested with:

$ java -version
openjdk version "17.0.2" 2022-01-18 LTS
OpenJDK Runtime Environment Zulu17.32+13-CA (build 17.0.2+8-LTS)
OpenJDK 64-Bit Server VM Zulu17.32+13-CA (build 17.0.2+8-LTS, mixed mode, sharing)

Running ./gradlew detetkMain without specifying --jvm-target leads to a successful build.

[rocketraman]

Seems like the problem might be this declaration in my Gradle build:

tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>().configureEach {
  kotlinOptions {
    jvmTarget = JavaVersion.VERSION_11.toString()
  }
}

It also doesn't seem to matter which version is actually specified -- detekt bails in any case.

[cortinico]

jvmTarget = JavaVersion.VERSION_11.toString()

That should work. Could you try with just "11" or with JvmTarget.JVM_11?
Also @3flex do you have some context here as you introduced the JvmTargetConverter?

[rocketraman]

That should work. Could you try with just "11" or with JvmTarget.JVM_11?

The output of JavaVersion.VERSION_11.toString() is already "11" but I tried it anyway, and as expected, the same error results.

[rocketraman]

Thanks for the hint @cortinico on JvmTargetConverter... I added a breakpoint to DetektInvoker. Assuming I'm looking at the right place, I see that my build does not pass a --jvm-target option at all.

Looking at the code at https://github.com/detekt/detekt/blob/main/detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/ArgumentConverters.kt#L54-L57, it looks like this code assumes that --jvm-target is a required argument.

I now see also in the README at https://github.com/detekt/detekt#with-gradle it does actually specify that the jvm target needs to be set explicitly for Detekt -- this appears to be new in 1.20 and was not called out in the migration guide at all.

I'm wondering if the plugin can simply use the JVM target from the Kotlin compiler options rather than having to inject this value separately.

[cortinico]

Thanks for looking into this @rocketraman
Yes, it's a required argument but it has a default specified, if you don't specify any:

detekt/detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt

Lines 189 to 195 in 16bbc16

	@Parameter(
	names = ["--jvm-target"],
	converter = JvmTargetConverter::class,
	description = "EXPERIMENTAL: Target version of the generated JVM bytecode that was generated during " +
	"compilation and is now being used for type resolution"
	)
	var jvmTarget: JvmTarget = JvmTarget.DEFAULT

[rocketraman]

I assumed when I wrote my previous message that adding the explicit jvmTarget configuration for the Gradle plugin would fix the problem, but nope.

Debugging further, I think I see the problem... the CompilerSpec generated by the CLI invoker tooling is setting the jvmTarget value to "JVM_11", which is the toString value of the JvmTarget enum. It's this "JVM_" value that is causing the problem.

[rocketraman]

The failure seems to be at this line:

val filesToAnalyze = measure(Phase.Parsing) { parsingStrategy.invoke(settings) }

in the Lifecycle interface.

[rocketraman]

More specifically, EnvironmentFacade.environment calls createCompilerConfiguration, which has a parameter with the value compilerSpec.parseJvmTarget().

This fails because JvmTarget.fromString(jvmTarget) on the toString output of JvmTarget is invalid i.e.

JvmTarget.fromString(JvmTarget.JVM_11.toString()) == null

[rocketraman]

I submitted a PR #4800 to hopefully fix this. I haven't tested this TBH but it does compile.

[cortinico]

I'd like @3flex to take a look here and eventually revert #4694 if needed.

[3flex]

I've added some new tests in #4801. Strings are mapped from Gradle plugin and parsed correctly by the CLI.

I'm not sure what I'm missing here, so a full reproducer will be helpful, but I will continue investigating in the meantime.

[3flex]

What could be the problem is if JvmTarget.JVM_11.toString() is called from Gradle, and an older version of Kotlin is on Gradle's classpath, it will return "JVM_11", because Gradle embeds an earlier version of Kotlin which doesn't have the new toString() override on JvmTarget that detekt CLI relies on.

It should work if you just set jvmTarget = "11" in the build file, or jvmTarget = JavaVersion.JVM_11.toString(), and don't use JvmTarget.JVM_11.toString() in build files.

[rocketraman]

What could be the problem is if JvmTarget.JVM_11.toString() is called from Gradle, and an older version of Kotlin is on Gradle's classpath, it will return "JVM_11", because Gradle embeds an earlier version of Kotlin which doesn't have the new toString() override on JvmTarget that detekt CLI relies on.

I was on Gradle 7.3.3. I updated to 7.4.2 to check if that was the issue. Same problem with both versions. There should be no change in the version of Kotlin on the classpath -- I have no local customizations in this regard. Gradle uses whatever it uses, my build uses 1.6.10.

It should work if you just set jvmTarget = "11" in the build file, or jvmTarget = JavaVersion.JVM_11.toString(), and don't use JvmTarget.JVM_11.toString() in build files.

No this does not work. In fact, I've never had JvmTarget.JVM_11.toString() in any of my build files. The call to JvmTarget.JVM_11.toString() is happening later in Detekt inside Spec.kt.

I'm not sure why all of these conversions from JvmTarget to and from Strings are going on. If you look at PR #4800 I submitted earlier, eventually you need to pass a JvmTarget, so why not just remove the multiple back-and-forths between JvmTarget and Strings and just keep the parsed JvmTarget value all the way through?

[3flex]

Please provide a reproducer and we can hopefully narrow down exactly what's going on.

[3flex]

A build scan may also help.

[rocketraman]

If you saw an earlier email from me due to a comment on this issue, please ignore it -- I thought I had created a reproducer but it was a silly mistake on my part. So far I'm not having any luck with the reproducer.

I'll continue trying to reproduce for a bit, but I'm not sure how much longer I can spend on this. I'm happy to jump on a meeting to debug this together.

[rocketraman]

I finally reproduced it... here is the repro repository: https://github.com/rocketraman/test-detekt-4786.

Looks like this particular block in my gradle build is the source of the problem: https://github.com/rocketraman/test-detekt-4786/blob/master/build.gradle.kts#L13-L21.

I normally do this in my builds to ensure that my chosen version of Kotlin is used everywhere in my build.

[rocketraman]

It looks like that block is pulling in the 1.6.10 version of kotlin-compiler-embeddable whereas detekt is expecting the 1.6.20 version.

Build scan with invalid target, showing the kotlin-compiler-embeddable dependency:

https://scans.gradle.com/s/xhnopmleir2au/dependencies?dependencies=kotlin-compiler-embeddable&expandAll

and same with the working detekt:

https://scans.gradle.com/s/ctcsi2vahsa7o/dependencies?dependencies=kotlin-compiler-embeddable&expandAll

[3flex]

It looks like that block is pulling in the 1.6.10 version of kotlin-compiler-embeddable whereas detekt is expecting the 1.6.20 version.

That's exactly what's happening - detekt uses the "detekt" configuration to manage the Gradle plugin's dependencies, which would normally be pulling in detekt-cli 1.20.0 and kotlin-compiler-embeddable 1.6.20 (or 1.6.21, can't recall which we shipped with).

When you override all configurations you're also overriding the "detekt" configuration, overriding the expected kotlin-compiler-embeddable version.

You can do this instead: configurations.matching { it.name != "detekt" }.all {

I've tested on your repo and it fixes the issue.

[3flex]

@Nitewriter I'm conscious that you might have a different root cause - or is this enough info to resolve your issue as well?

[rocketraman]

When you override all configurations you're also overriding the "detekt" configuration, overriding the expected kotlin-compiler-embeddable version.

Is it correct for detekt to be tied so strongly to a particular kotlin version? And even if yes, then should detekt fail in such an esoteric way if that constraint is not met?

[3flex]

Is it correct for detekt to be tied so strongly to a particular kotlin version?

Yes. detekt utilises internal compiler code for its analysis. There's no public API as such so we don't have much choice here. Some updates to Kotlin are breaking for detekt so maintaining compatibility with multiple Kotlin versions isn't realistic.

should detekt fail in such an esoteric way if that constraint is not met?

I think that's debateable. In your case it's a build configuration issue, and we can't account for everything that someone might do in their own build files when they inadvertently override the behaviour of the detekt Gradle plugin. We can't lock the specific Kotlin compiler version in the Gradle plugin either, because users can choose different detekt-cli versions and apply to the "detekt" configuration. A warning might be feasible, PR welcome.

[rocketraman]

I think that's debateable. In your case it's a build configuration issue, and we can't account for everything that someone might do in their own build files when they inadvertently override the behaviour of the detekt Gradle plugin.

There is no need to account for "everything that someone might do". There should be a need to account for fundamental constraints of detekt being violated and warn or error on that accordingly.

Yes. detekt utilises internal compiler code for its analysis. There's no public API as such so we don't have much choice here. Some updates to Kotlin are breaking for detekt so maintaining compatibility with multiple Kotlin versions isn't realistic.

If this statement is true (it may be, but not really in this case -- the error condition in question was not anything fundamental with the compiler here, but rather the code relying on a specific version's behavior related to an ancillary function like JvmTarget.toString() which could be handled in a different way that would work across versions i.e. avoid converting from String -> JvmTarget -> String -> JvmTarget which is brittle by nature), then Detekt should do everything possible to ensure that this constraint is actually met, and if it is not (regardless of what esoteric condition caused the constraint failure), then it should warn with a message like:

"Detekt requires Kotlin compiler version 1.6.20 for analysis. Detekt is currently running with Kotlin compiler version x.x.x. Check the Detekt configuration."

instead of a message like:

"Invalid --jvm-target value"

A warning might be feasible, PR welcome.

I've spent way more time on this issue already than I can justify -- might as well spend a bit more :-) Maybe I'll get to this. Pointers/guidance welcome.

[rocketraman]

BTW, other libraries that depend on specific compiler versions, like Compose, have just such logic in them. Compose will fail in the presence of the wrong Kotlin version, unless a specific setting is used to override that check.

Furthermore, IIRC Compose uses the version of Kotlin in the project and requires that to match the version expected by Compose -- it doesn't try to bring in its own version and use that, regardless of the version selected by the user. It seems the approach of selecting a compiler version which may not match the user's selected version is likely to cause other problems.

[BraisGabin]

I'm using JDK11, detekt 1.20.0 and kotlin 1.6.10 and all works perfect so this is a bit more complex. I mean, detekt right now works on other versions of kotlin too.

[3flex]

See #4786 (comment)

We have the root cause and a fix for that issue, but I'm not sure if the original reporter has a different issue.

[itd-pal]

You can do this instead: configurations.matching { it.name != "detekt" }.all {

@3flex
I'm new to detekt so would it be possible to create and share a pull request on the repodruced repo where to add this change ?

Thanks

[3flex]

@itd-pal I think that would be better raised in a separate support request, thanks! Try https://github.com/detekt/detekt/discussions

[itd-pal]

I opened #4813 @3flex

[cortinico]

@itd-pal I think that would be better raised in a separate support request, thanks! Try detekt/detekt/discussions

Should we close this issue then?

[itd-pal]

FYI
For me it fixed the problem by updating either
detekt to 1.20.0-RC2 with kotlin 1.6.10
or updating kotlin to 1.6.20 and detekt 1.20.0