Add custom ktlint rules to prevent Java interop issues (#1414)

(cherry picked from commit 4851cac)

.editorconfig

@@ -0,0 +1,3 @@
+# Ignore custom ktlint rules for tests
+[**/test/**.kt]
+disabled_rules = custom-ktlint-rules:top-level-internal,custom-ktlint-rules:top-level-public

.github/workflows/build.yml

@@ -40,6 +40,11 @@ jobs:
           GRADLE_OPTS: -Dorg.gradle.jvmargs="-XX:MaxMetaspaceSize=1g"
         run: ./gradlew build jacocoTestReport
 
+      # Custom ktlint rules are only run when the `custom-ktlint-rules` property is set.
+      # Once these rules are run by default, this GH Action step can be removed.
+      - name: Run custom ktlint rules
+        run: ./gradlew ktlintCheck -Pcustom-ktlint-rules
+
       # Upload coverage for CLI, LANG, PTS, TEST_SCRIPT, and EXAMPLES
       - name: Upload CLI coverage
         uses: codecov/codecov-action@v3

buildSrc/build.gradle.kts

@@ -27,7 +27,7 @@ object Versions {
     const val detekt = "1.20.0-RC1"
     const val dokka = "1.6.10"
     const val kotlin = "1.6.20"
-    const val ktlint = "10.2.1"
+    const val ktlintGradle = "10.2.1"
     const val pig = "0.6.1"
 }
 
@@ -36,15 +36,15 @@ object Plugins {
     const val detekt = "io.gitlab.arturbosch.detekt:detekt-gradle-plugin:${Versions.detekt}"
     const val dokka = "org.jetbrains.dokka:dokka-gradle-plugin:${Versions.dokka}"
     const val kotlinGradle = "org.jetbrains.kotlin:kotlin-gradle-plugin:${Versions.kotlin}"
-    const val ktlint = "org.jlleitschuh.gradle:ktlint-gradle:${Versions.ktlint}"
+    const val ktlintGradle = "org.jlleitschuh.gradle:ktlint-gradle:${Versions.ktlintGradle}"
     const val pig = "org.partiql:pig-gradle-plugin:${Versions.pig}"
 }
 
 dependencies {
     implementation(Plugins.detekt)
     implementation(Plugins.dokka)
     implementation(Plugins.kotlinGradle)
-    implementation(Plugins.ktlint)
+    implementation(Plugins.ktlintGradle)
     implementation(Plugins.pig)
     implementation(Plugins.binaryCompatibilityValidator)
 }

buildSrc/src/main/kotlin/partiql.conventions.gradle.kts

@@ -40,6 +40,12 @@ dependencies {
     testImplementation(Deps.kotlinTest)
     testImplementation(Deps.kotlinTestJunit)
     testImplementation(Deps.junitParams)
+    // Custom ktlint rules are added by adding to the `dependencies` block: https://github.com/JLLeitschuh/ktlint-gradle/tree/v10.2.0?tab=readme-ov-file#configuration
+    // Currently, we only run the rules when the `custom-ktlint-rules` property is set.
+    // Once we enable the custom rules to run by default, this conditional can be removed.
+    if (hasProperty("custom-ktlint-rules")) {
+        ktlintRuleset(project(":custom-ktlint-rules"))
+    }
 }
 
 java {
@@ -73,6 +79,13 @@ tasks.compileTestKotlin {
 }
 
 configure<KtlintExtension> {
+    version.set(Versions.ktlint)
+    // Currently set `ktlintCheck` to not fail on the custom rules.
+    // Once we enable the custom rules to run by default, this conditional can be removed.
+    if (hasProperty("custom-ktlint-rules")) {
+        ignoreFailures.set(true)
+    }
+    outputToConsole.set(true)
     filter {
         exclude { it.file.path.contains(generatedSrc) }
     }

buildSrc/src/main/kotlin/partiql.versions.kt

@@ -45,10 +45,10 @@ object Versions {
     const val kotlinxCollections = "0.3.5"
     const val picoCli = "4.7.0"
     const val kasechange = "1.3.0"
-    const val ktlint = "11.6.0"
     const val pig = "0.6.2"
     const val kotlinxCoroutines = "1.6.0"
     const val kotlinxCoroutinesJdk8 = "1.6.0"
+    const val ktlint = "0.42.1" // we're on an old version of ktlint. TODO upgrade https://github.com/partiql/partiql-lang-kotlin/issues/1418
 
     // Testing
     const val assertj = "3.11.0"
@@ -92,6 +92,7 @@ object Deps {
     const val pigRuntime = "org.partiql:partiql-ir-generator-runtime:${Versions.pig}"
     const val kotlinxCoroutines = "org.jetbrains.kotlinx:kotlinx-coroutines-core:${Versions.kotlinxCoroutines}"
     const val kotlinxCoroutinesJdk8 = "org.jetbrains.kotlinx:kotlinx-coroutines-jdk8:${Versions.kotlinxCoroutinesJdk8}"
+    const val ktlint = "com.pinterest.ktlint:ktlint-core:${Versions.ktlint}"
 
     // Testing
     const val assertj = "org.assertj:assertj-core:${Versions.assertj}"
@@ -106,6 +107,7 @@ object Deps {
     const val mockito = "org.mockito:mockito-junit-jupiter:${Versions.mockito}"
     const val mockk = "io.mockk:mockk:${Versions.mockk}"
     const val kotlinxCoroutinesTest = "org.jetbrains.kotlinx:kotlinx-coroutines-test:${Versions.kotlinxCoroutinesTest}"
+    const val ktlintTest = "com.pinterest.ktlint:ktlint-test:${Versions.ktlint}"
 
     // JMH Benchmarking
     const val jmhCore = "org.openjdk.jmh:jmh-core:${Versions.jmhCore}"
@@ -125,7 +127,6 @@ object Plugins {
     const val detekt = "io.gitlab.arturbosch.detekt"
     const val dokka = "org.jetbrains.dokka"
     const val jmh = "me.champeau.gradle.jmh"
-    const val ktlint = "org.jlleitschuh.gradle.ktlint"
     const val library = "org.gradle.java-library"
     const val testFixtures = "org.gradle.java-test-fixtures"
 }

custom-ktlint-rules/build.gradle.kts

@@ -0,0 +1,16 @@
+plugins {
+    id(Plugins.conventions)
+    `java-library`
+}
+
+dependencies {
+    implementation(Deps.ktlint)
+
+    testImplementation(Deps.assertj)
+    testImplementation(Deps.ktlintTest)
+    testImplementation(Deps.junitParams)
+}
+
+repositories {
+    mavenCentral()
+}

custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/CustomRuleSetProvider.kt

@@ -0,0 +1,10 @@
+package org.partiql.ktlint
+
+import com.pinterest.ktlint.core.RuleSet
+import com.pinterest.ktlint.core.RuleSetProvider
+import org.partiql.ktlint.rule.TopLevelInternalRule
+import org.partiql.ktlint.rule.TopLevelPublicRule
+
+class CustomRuleSetProvider : RuleSetProvider {
+    override fun get(): RuleSet = RuleSet("custom-ktlint-rules", TopLevelInternalRule(), TopLevelPublicRule())
+}

custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/rule/TopLevelInternalRule.kt

@@ -0,0 +1,38 @@
+package org.partiql.ktlint.rule
+
+import com.pinterest.ktlint.core.Rule
+import com.pinterest.ktlint.core.ast.ElementType
+import com.pinterest.ktlint.core.ast.children
+import org.jetbrains.kotlin.com.intellij.lang.ASTNode
+
+public class TopLevelInternalRule : Rule("top-level-internal") {
+
+    override fun visit(
+        node: ASTNode,
+        autoCorrect: Boolean,
+        emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
+    ) {
+        if (node.elementType != ElementType.IDENTIFIER) {
+            return
+        }
+
+        // Focus on just functions and values
+        val parent = node.treeParent
+        if (parent.elementType != ElementType.FUN && parent.elementType != ElementType.PROPERTY) {
+            return
+        }
+
+        // Check grandparent of node is FILE; if so, is top-level declaration
+        if (parent.treeParent.elementType != ElementType.FILE) {
+            return
+        }
+        val modifiers = parent.findChildByType(ElementType.MODIFIER_LIST)?.children()
+        if (modifiers != null && modifiers.any { it.elementType == ElementType.INTERNAL_KEYWORD }) {
+            emit(
+                node.startOffset,
+                "Top-level internal declaration found: ${node.text}",
+                false
+            )
+        }
+    }
+}

custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/rule/TopLevelPublicRule.kt

@@ -0,0 +1,63 @@
+package org.partiql.ktlint.rule
+
+import com.pinterest.ktlint.core.Rule
+import com.pinterest.ktlint.core.ast.ElementType
+import com.pinterest.ktlint.core.ast.children
+import org.jetbrains.kotlin.com.intellij.lang.ASTNode
+
+public class TopLevelPublicRule : Rule("top-level-public") {
+    override fun visit(
+        node: ASTNode,
+        autoCorrect: Boolean,
+        emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
+    ) {
+        if (node.elementType != ElementType.IDENTIFIER) {
+            return
+        }
+
+        // Focus on just functions and values
+        val parent = node.treeParent
+        if (parent.elementType != ElementType.FUN && parent.elementType != ElementType.PROPERTY) {
+            return
+        }
+
+        // Check grandparent of node is FILE; if so, is top-level declaration
+        val grandParent = parent.treeParent
+        if (grandParent.elementType != ElementType.FILE) {
+            return
+        }
+
+        val modifiers = parent.findChildByType(ElementType.MODIFIER_LIST)
+        if (modifiers != null && modifiers.isNotPublic()) {
+            return
+        }
+
+        val annotationEntry = grandParent.findChildByType(ElementType.FILE_ANNOTATION_LIST)?.findChildByType(ElementType.ANNOTATION_ENTRY)
+        if (annotationEntry == null || !annotationEntry.containsFileJvmName()) {
+            emit(
+                node.startOffset,
+                "Top-level public declaration found without `@file:JvmName` annotation: ${node.text}",
+                false
+            )
+        }
+    }
+
+    // returns true iff modifiers is not one of `PRIVATE_KEYWORD`, `INTERNAL_KEYWORD` or `PROTECTED_KEYWORD`
+    private fun ASTNode.isNotPublic(): Boolean {
+        val modifiers = this.children().map { it.elementType }
+        return modifiers.any { it == ElementType.PRIVATE_KEYWORD || it == ElementType.INTERNAL_KEYWORD || it == ElementType.PROTECTED_KEYWORD }
+    }
+
+    // returns true iff node is `@file:JvmName(<some name>)`
+    private fun ASTNode.containsFileJvmName(): Boolean {
+        val annotationTarget = this.findChildByType(ElementType.ANNOTATION_TARGET)
+        if (annotationTarget == null || annotationTarget.text.lowercase() != "file") {
+            return false
+        }
+        val constructorCallee = this.findChildByType(ElementType.CONSTRUCTOR_CALLEE)
+        if (constructorCallee == null || constructorCallee.text.lowercase() != "jvmname") {
+            return false
+        }
+        return true
+    }
+}

custom-ktlint-rules/src/main/resources/META-INF/services/com.pinterest.ktlint.core.RuleSetProvider

@@ -0,0 +1 @@
+org.partiql.ktlint.CustomRuleSetProvider

custom-ktlint-rules/src/test/kotlin/org/partiql/ktlint/rule/TopLevelInternalRuleTest.kt

@@ -0,0 +1,48 @@
+package org.partiql.ktlint.rule
+
+import com.pinterest.ktlint.core.LintError
+import com.pinterest.ktlint.test.lint
+import org.assertj.core.api.Assertions
+import org.junit.jupiter.api.Test
+
+class TopLevelInternalRuleTest {
+    @Test
+    fun `top-level internal`() {
+        val code =
+            """
+            internal fun internalTopLevelFun() {}   // ktlint error
+            
+            internal val internalTopLevelVal = 123  // ktlint error
+            
+            internal var internalTopLevelVar = 456    // ktlint error
+            
+            // No errors for below (for this rule)
+            public fun publicTopLevelFun() {}
+            
+            public val publicTopLevelVal = 123
+            
+            public var publicTopLevelVar = 456
+            
+            fun publicTopLevelFun2() {}
+            
+            val publicTopLevelVal = 123
+            
+            var publicTopLevelVar = 456
+            
+            public class PublicClass {
+                internal fun internalFun() {}
+            
+                internal val internalVal = 123
+                
+                public fun publicFun() {}
+                
+                public val publicVal = 123
+            }
+            """.trimIndent()
+        Assertions.assertThat(TopLevelInternalRule().lint(code)).containsExactly(
+            LintError(1, 14, "top-level-internal", "Top-level internal declaration found: internalTopLevelFun"),
+            LintError(3, 14, "top-level-internal", "Top-level internal declaration found: internalTopLevelVal"),
+            LintError(5, 14, "top-level-internal", "Top-level internal declaration found: internalTopLevelVar")
+        )
+    }
+}