Explore converting org.gradle.api.Script to Kotin

[JLLeitschuh]

Signed-off-by: Jonathan Leitschuh Jonathan.Leitschuh@plexxi.com

Context

I was exploring why the KotlinBuildScript, the KotlinSettingsScript, and the KotlinInitScript don't have a common interface between them even though they all have exactly the same methods on them.
During that exploration I bumped into this comment here on org.gradle.kotlin.dsl.provider.ScriptApi

/**
 * Base contract for all Gradle Kotlin DSL scripts.
 *
 * This is the Kotlin flavored equivalent of [org.gradle.api.Script].
 *
 * It is not implemented directly by script templates to overcome ambiguous conflicts and Kotlin language
 * limitations. See each script template for actual implementations.
 *
 * `ScriptApiTest` validates that each script template provide compatible methods/properties.
 *
 * Members here must not use default parameters values.
 * Documentation should go to the actual implementations so that it shows up in IDEs properly.
 */

Link to real comment:
https://github.com/gradle/kotlin-dsl/blob/0eccacca4c9f87be19dd88045c2007ae3ee58f2f/subprojects/provider/src/main/kotlin/org/gradle/kotlin/dsl/provider/ScriptApi.kt#L36-L48

The primary reason that this is the case is because of the methods: getLogger, getLogging and getResources. If you override them in Kotlin you force API consumers to call getLogger instead of being able to just do logger.

The "simple" fix for this would be to convert some of the Gradle based interfaces that have these getters to Kotlin.

I decided to start with Script because if it were implemented in Kotlin then KotlinBuildScript, KotlinSettingsScript, and KotlinInitScript would all have a common interface.

This would allow for supporting a fourth kind of script (let's call it KotlinCommonScript and lets propose the extension *.common.gradle.kts) that can be applied to any of the aforementioned script types.

Current Pain

If you want to have one Gradle Kotlin script that can be called both by a *.settings.gradle.kts or .gradle.kts file you need this kind of nastiness to support both:

/**
 * Makes it so that this script can be called with `this` as both a
 * [KotlinBuildScript] & [KotlinSettingsScript] by providing an adapter
 * to the two things that `this` could be.
 *
 *  - [Issue #821](https://github.com/gradle/kotlin-dsl/issues/821)
 *  - [Issue #659](https://github.com/gradle/kotlin-dsl/issues/659)
 */
fun <R> callBasedOnContext(
    ifBuildScript: KotlinBuildScript.() -> R,
    ifSettingsScript: KotlinSettingsScript.() -> R
): R {
    /*
     * A bit of a hack to get around a compiler error when trying to do
     * `is KotlinBuildScript` and `is KotlinSettingsScript`.
     */
    val kotlinProjectClass: KClass<*> = KotlinBuildScript::class
    val kotlinSettingsClass: KClass<*> = KotlinSettingsScript::class

    /*
     * The following is what I'd like to try to be doing, but it would not compile:
     * ```
     * when(this) {
     *     is KotlinBuildScript -> ...
     *     is KotlinSettingsScript -> ..
     *     else -> ...
     * }
     * ```
     * This is because, according to the Kotlin compiler, when applied to a project script,
     * `this` will only ever be a `KotlinBuildScript`, and when applied to a settings script,
     * it will only ever be `KotlinSettingsScript`.
     * The compiler doesn't know that it will be compiled a second time with `this` as a different
     * type. Thus, we are left with this hacky solution.
     */
    return when {
        kotlinProjectClass.isInstance(this) -> (this as KotlinBuildScript).ifBuildScript()
        kotlinSettingsClass.isInstance(this) -> (this as KotlinSettingsScript).ifSettingsScript()
        else -> throw AssertionError("$this is not being applied to a supported type.")
    }
}

val extra: ExtraPropertiesExtension by lazy {
    callBasedOnContext(
        ifBuildScript = { extra },
        ifSettingsScript = { (settings as ExtensionAware).extra }
    )
}

fun hasPropertyHelper(propertyName: String): Boolean {
    return callBasedOnContext(
        ifBuildScript = { hasProperty(propertyName) },
        ifSettingsScript = { (settings as ExtensionAware).extra.properties.containsKey(propertyName) }
    )
}

By converting some of the common interfaces in the Gradle API to Kotlin these sort of issues with collisions between java interface getters and kotlin interface properties can be totally alleviated.

Known Issues

The only problem I can think of off the top of my head is that this will totally break the documentation generation for any files converted to Kotlin.

Contributor Checklist

• Review Contribution Guidelines
• Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
• Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
• Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
• Provide unit tests (under <subproject>/src/test) to verify logic
• Update User Guide, DSL Reference, and Javadoc for public-facing changes
• Ensure that tests pass locally: ./gradlew <changed-subproject>:check (no joke, it just worked!)

Gradle Core Team Checklist

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation
• Recognize contributor in release notes

[oehme]

Big -1 for making Gradles core depend on Kotlin. Doing so with Groovy already caused enough issues that we're still slowly fixing. The core should be pure Java and the DSL needs to adapt.

I also don't see why org.gradle.api.Script would need to change. This is effectively part of the Groovy DSL and shouldn't be used by the Kotlin DSL.

[JLLeitschuh]

The problem arises when you try to create common interfaces when one getter is defined in java and one is in Kotlin.

For example:

interface A {
    Logger getLogger();
}

interface B {
    val logger: Logger
}

class Impl: A, B {
    /*
     * The compiler won't let you do this because, even though the JVM signatures are exactly 
     * the same, one is a property and one is a method.
     */
}

The only way to make this work is to change the interface B to this:

interface B {
    fun getLogger() : Logger
}
class Impl: A, B {
    override fun getLogger() : Logger = Logger.create(Impl::class)
 }
fun usageExample(impl: Impl) {
    // This really isn't the API we want to expose
    impl.getLogger().log("Hi")
}

These issues are captured here:
https://youtrack.jetbrains.com/issue/KT-6653
gradle/kotlin-dsl-samples#1026

This is why the org.gradle.kotlin.dsl.provider.ScriptApi exists but isn't implemented by anything.
I'm trying to make it so that kotlin scripts can all share a common API without compromising on API quality.

To your concern about adding a dependnecy upon Kotlin, we may be able to add a dependency upon the Kotlin compiler without adding a dependency on the Kotlin standard lib.

[oehme]

The Kotlin DSL shouldn't be implementing org.gradle.api.Script. That's part of the Groovy DSL. It shouldn't be in coreApi either, that's just a historical mistake.

If we need the Kotlin compiler in coreApi, we're doing it wrong.

[JLLeitschuh]

This was really more of a POC to see if I could integrate the Kotlin compiler into the Gradle build ecosystem in order to see if it just worked.

I think the interface that actually needs to be converted to Kotlin to help alleviate the above mentioned problem is actually org.gradle.api.Project & org.gradle.api.initialization.Settings and org.gradle.api.invocation.Gradle. Doing so will prevent the overload collisions mentioned above.

Maybe I should convert this into an issue so that we can further discuss this outside of the context of this PR.

[oehme]

Yes please open an issue. I don't really understand what the problem with the missing common interface is. User code never abstracts over scripts.