Kotlin-dsl-samples: Kotlin DSL progressive mode warning is confusing and ambiguous

[mark-vieira]

Current Behavior

With the latest Gradle 4.10 build users now receive this message when using the Kotlin DSL.

WARNING
The `kotlin-dsl` plugin relies on Kotlin compiler's progressive mode and experimental features to enable among other things SAM conversion for Kotlin functions.

Once built and published, artifacts produced by this project will continue to work on future Gradle versions.
However, you may have to fix the sources of this project after upgrading the Gradle wrapper of this build.

To silence this warning add the following to this project's build script:

    kotlinDslPluginOptions.progressive.set(ProgressiveModeState.ENABLED)

To disable Gradle Kotlin DSL progressive mode and give up SAM conversion for Kotlin functions add the following to this project's build script:

    kotlinDslPluginOptions.progressive.set(ProgressiveModeState.DISABLED)

See the `progressive` property documentation for more information.

This looks to be a direct result of work done in #971. I'll note that one of the goals listed in that issue is:

translate the related w: ATTENTION! message to be clearer about the implications for kotlin-dsl users

I would assert that this has not been achieved at all. There are a number of issues with the message above.

1. "WARNING The kotlin-dsl plugin relies on Kotlin compiler's progressive mode and experimental features to enable among other things SAM conversion for Kotlin functions."

Ok? What am I suppose to think of this as a user? So Gradle's Kotlin DSL relies on an experimental Kotlin feature, got it. What does that _mean_? Is it really even necessary to say this given that Kotlin DSL itself is pre-1.0 and at least implicitly expected to be experimental anyway?

1. "Once built and published, artifacts produced by this project will continue to work on future Gradle versions."

Also have no idea what this means. It's ambiguous what "this project" here is referring to. Does this mean the user's Gradle project or the Kotlin DSL project? If the former, which I assume is the case, is it explicitly necessary to say this? It _seems_ like the purpose of this message is to reassure folks "hey, your code is going to continue to work, don't worry" but it's lack of clarity has the opposite effect.

1. "However, you may have to fix the sources of this project after upgrading the Gradle wrapper of this build."

We then immediately contradict the previous statement and tell folks that they may need to update their project. Also what is "sources of this project"? Are we talking about user code or build scripts? Does this Kotlin compiler setting affect folks that are building Kotlin in their projects? If so that seems like something we should absolutely not be doing.

Also, we give absolutely no guidance as to what changes might be necessary. Is there some documentation we can point folks at? Just saying "this might break stuff you may need to fix" is simply not sufficient here.

1. "See the progressive property documentation for more information."

Providing a link to this documentation seems like an obvious thing to do here.

[big-guy]

+1

IIUC, what we're saying is "kotlin-dsl enables some experimental stuff that we promise we will keep working in a binary compatible way, but your source code may not compile between upgrades because of reasons out of our control."

If that's the case, I would get rid of the option entirely and it's just a documentation issue that Kotlin plugins uses kotlin-dsl require that flag internally. It's not clear to me why someone would want to cripple their use of kotlin-dsl (by disabling the flag) because their Kotlin code didn't work otherwise?

[bamboo]

@big-guy your understanding is correct and yes, moving the whole warning to the documentation sounds like a good idea to me.

It's not clear to me why someone would want to cripple their use of kotlin-dsl (by disabling the flag) because their Kotlin code didn't work otherwise?

Disabling the flag removes the -XXLanguage:+NewInference and -XXLanguage:+SamConversionForKotlinFunctions from the compiler options.

One might feel compelled to do it whenever these options cause code that previously compiled successfully to fail.

1.-XXLanguage:+NewInference introduces stricter type inference semantics so it might cause types previously inferred as nonnull to be inferred as nullable, for example.

1. -XXLanguage:+SamConversionForKotlinFunctions enables automatic conversions to Java Sam interfaces, even when they are used as Kotlin function parameters.

fun kotlinFunctionWithJavaSam(action: org.gradle.api.Action<Any>) = TODO()

fun useSamConversion() {
    kotlinFunctionWithJavaSam {
        // Won't compile without SamConversionForKotlinFunctions
   }
}

Which allows Gradle plugins written in Kotlin to expose an uniform org.gradle.api.Action<T> based API to Groovy DSL and Kotlin DSL scripts.

At the same time, it can cause previously unambiguous call sites to become ambiguous:

fun <T> applyTo(target: T, action: Action<T>) = action.execute(target)

fun <T> applyTo(target: T, action: (T) -> Unit) = action(target)

applyTo(42) {
  // Ambiguous: Action<T> or (T) -> Unit?
}

That pattern is common in Kotlin libraries meant to make Java libraries easier to consume from Kotlin like mockito-kotlin, for example.

[mark-vieira]

@bamboo What you described above are important details that need to be communicated as part of this.

Also, if I understand it, these effect only build script sources? Not user code? If so we need to reword this warning to make that more clear. Saying "project sources" makes me think these compiler arguments effect my project's production code.

[bamboo]

Also, if I understand it, these effect only build script sources? Not user code?

User code in a project with the kotlin-dsl plugin applied, most commonly, buildSrc but also plugin projects.

We really meant the warning for plugin projects not buildSrc.

[mark-vieira]

Right. I think it's going to be a common enough scenario folks are going to use this plugin in buildSrc. This distinction is simply not clear enough. Perhaps we can detect the buildSrc case and provide as lightly different message. At the very least we can say _which_ project is using this plugin so folks know _where_ to look for this issue.

I'm also leaning towards's @big-guy's suggesting of ditching this altogether. I think we can still make breaking changes here. Basically, if you use the newer version of the kotlin-dsl plugin it uses these new compiler args and you'll have to update. I think that's better than this obscure warning that forces folks to make a choice they don't quite understand.

[big-guy]

One might feel compelled to do it whenever these options cause code that previously compiled successfully to fail.

I think the approach I would take is that if you're using kotlin-dsl, you're in a "Gradle context", which needs these extra features are enabled. In that case, "previously working code" doesn't exist because these features are always enabled and you have no choice.

If there's existing code from a third party library that's ambiguous, that's just an incompatibility between what we need for the "Gradle context" and what the library provides. We would treat this like a Groovy library that uses features that are awkward from Kotlin/Java. If something is ambiguous, is there a particular way someone chooses the right one or are you just out of luck?

I may be wrong, but I think if we allow this to be enabled/disabled, it would mean that someone could inadvertently write a Gradle plugin in Kotlin that runs into this situation, they could disable the extra features and then produce a Gradle plugin that would require other people to disable these extra features just to work with it.

[JLLeitschuh]

Do these flags need to be enabled on the plugin producer's compiler or on the plugin consumers plugin to be useful?
If I'm writing a plugin in Kotlin and I don't have the kotlin-dsl plugin applied, am I going to be addling additional work for my consumers down the line?
I'm hesitant to use the kotlin-dsl in any of my plugins because, although the API is syntactically pretty stable, there have been quite a few binary breaking changes that, in the past, have caused versions of my plugins to not work with newer versions of Gradle.

[eskatos]

We should not special-case buildSrc. Applying the kotlin-dsl Gradle plugin configures a project to enable Gradle Kotlin DSL features in its "production code". This is for authoring build-logic, in regular source sets: buildSrc, included build-logic builds, Gradle plugin builds. It doesn't change anything wrt build scripts.

I'm with Sterling on the fact that we should have a single behavior to make things easier to understand and support. One less thing to worry about for build and plugin authors (@JLLeitschuh ;) ). Builds and plugins written against previous 0.x versions might break, but that's acceptable. Going forward breaking changes such as enabling "sam conversion for kotlin functions" should happen on major Gradle releases.

[eskatos]

1012 removed the ability to opt-out of experimental kotlin compiler settings, moving to a single behavior situation. and changed the warning to:

The `kotlin-dsl` plugin applied to project ':buildSrc' enables experimental Kotlin compiler features.
For more information see https://docs.gradle.org/4.10/userguide/kotlin_dsl.html#sec:kotlin-dsl_plugin

https://github.com/gradle/gradle/pull/6214 introduces corresponding versioned documentation in the user manual.

[mark-vieira]

The updated warning message is _much_ better.

[eskatos]

Documentation PR https://github.com/gradle/gradle/pull/6214 has been merged.
Closing

[wolfs]

@eskatos Now that I am seeing the warning in the gradle/gradle build: How can I make it go away? I don't want every user to see the warning once for every subproject in buildSrc. I just want to say that I am good with that and that the plugin shouldn't show me the warning going forward.

Another thing to check out is why the warning isn't shown in build scans: https://e.grdev.net/s/h3q7axo67mcya/console-log

[eskatos]

@wolfs good catch about the missing documentation, see https://github.com/gradle/gradle/pull/6347

[wolfs]

@eskatos: Cool, that fixes the warning.

There is still the issue that the warning doesn't show up in build scans. Note that this is different to #1031 since the warning is coming from the kotlin-dsl plugin and not from Kotlin compilation, right?

[eskatos]

@wolfs after a bit of scrutiny, this is the same as #1031. The kotlin-dsl plugin registers a listener that rewrites the warning emitted by the KotlinCompile task in order to provides meaningful information to kotlin-dsl plugin users. This explains the lack of bound build operation in the same way.