From 4851cac1343ed041f01a9c5d4c0445ccfd2143fb Mon Sep 17 00:00:00 2001 From: Alan Cai Date: Wed, 17 Apr 2024 17:37:04 -0700 Subject: [PATCH] Add custom ktlint rules to prevent Java interop issues (#1414) --- .editorconfig | 3 + .github/workflows/build.yml | 5 + buildSrc/build.gradle.kts | 6 +- .../kotlin/partiql.conventions.gradle.kts | 13 ++ buildSrc/src/main/kotlin/partiql.versions.kt | 5 +- custom-ktlint-rules/build.gradle.kts | 16 +++ .../partiql/ktlint/CustomRuleSetProvider.kt | 10 ++ .../ktlint/rule/TopLevelInternalRule.kt | 38 ++++++ .../partiql/ktlint/rule/TopLevelPublicRule.kt | 63 +++++++++ .../com.pinterest.ktlint.core.RuleSetProvider | 1 + .../ktlint/rule/TopLevelInternalRuleTest.kt | 48 +++++++ .../ktlint/rule/TopLevelPublicRuleTest.kt | 129 ++++++++++++++++++ settings.gradle.kts | 1 + 13 files changed, 333 insertions(+), 5 deletions(-) create mode 100644 .editorconfig create mode 100644 custom-ktlint-rules/build.gradle.kts create mode 100644 custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/CustomRuleSetProvider.kt create mode 100644 custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/rule/TopLevelInternalRule.kt create mode 100644 custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/rule/TopLevelPublicRule.kt create mode 100644 custom-ktlint-rules/src/main/resources/META-INF/services/com.pinterest.ktlint.core.RuleSetProvider create mode 100644 custom-ktlint-rules/src/test/kotlin/org/partiql/ktlint/rule/TopLevelInternalRuleTest.kt create mode 100644 custom-ktlint-rules/src/test/kotlin/org/partiql/ktlint/rule/TopLevelPublicRuleTest.kt diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000000..8d6e19dfaf --- /dev/null +++ b/.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 diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 64813934fe..f101767951 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -41,6 +41,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 diff --git a/buildSrc/build.gradle.kts b/buildSrc/build.gradle.kts index 48c6cbf7dd..a6b932f03d 100644 --- a/buildSrc/build.gradle.kts +++ b/buildSrc/build.gradle.kts @@ -26,7 +26,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" } @@ -34,7 +34,7 @@ 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}" } @@ -42,7 +42,7 @@ dependencies { implementation(Plugins.detekt) implementation(Plugins.dokka) implementation(Plugins.kotlinGradle) - implementation(Plugins.ktlint) + implementation(Plugins.ktlintGradle) implementation(Plugins.pig) } diff --git a/buildSrc/src/main/kotlin/partiql.conventions.gradle.kts b/buildSrc/src/main/kotlin/partiql.conventions.gradle.kts index 9d07abba0b..7ac2a00aef 100644 --- a/buildSrc/src/main/kotlin/partiql.conventions.gradle.kts +++ b/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 { @@ -72,6 +78,13 @@ tasks.compileTestKotlin { } configure { + 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) } } diff --git a/buildSrc/src/main/kotlin/partiql.versions.kt b/buildSrc/src/main/kotlin/partiql.versions.kt index e543827c41..1d67f78c27 100644 --- a/buildSrc/src/main/kotlin/partiql.versions.kt +++ b/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" } \ No newline at end of file diff --git a/custom-ktlint-rules/build.gradle.kts b/custom-ktlint-rules/build.gradle.kts new file mode 100644 index 0000000000..1f62e65c66 --- /dev/null +++ b/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() +} diff --git a/custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/CustomRuleSetProvider.kt b/custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/CustomRuleSetProvider.kt new file mode 100644 index 0000000000..f4472d3dde --- /dev/null +++ b/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()) +} diff --git a/custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/rule/TopLevelInternalRule.kt b/custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/rule/TopLevelInternalRule.kt new file mode 100644 index 0000000000..5d4424ac21 --- /dev/null +++ b/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 + ) + } + } +} diff --git a/custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/rule/TopLevelPublicRule.kt b/custom-ktlint-rules/src/main/kotlin/org/partiql/ktlint/rule/TopLevelPublicRule.kt new file mode 100644 index 0000000000..b1855d5f50 --- /dev/null +++ b/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()` + 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 + } +} diff --git a/custom-ktlint-rules/src/main/resources/META-INF/services/com.pinterest.ktlint.core.RuleSetProvider b/custom-ktlint-rules/src/main/resources/META-INF/services/com.pinterest.ktlint.core.RuleSetProvider new file mode 100644 index 0000000000..49828fa156 --- /dev/null +++ b/custom-ktlint-rules/src/main/resources/META-INF/services/com.pinterest.ktlint.core.RuleSetProvider @@ -0,0 +1 @@ +org.partiql.ktlint.CustomRuleSetProvider diff --git a/custom-ktlint-rules/src/test/kotlin/org/partiql/ktlint/rule/TopLevelInternalRuleTest.kt b/custom-ktlint-rules/src/test/kotlin/org/partiql/ktlint/rule/TopLevelInternalRuleTest.kt new file mode 100644 index 0000000000..3e411e336b --- /dev/null +++ b/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") + ) + } +} diff --git a/custom-ktlint-rules/src/test/kotlin/org/partiql/ktlint/rule/TopLevelPublicRuleTest.kt b/custom-ktlint-rules/src/test/kotlin/org/partiql/ktlint/rule/TopLevelPublicRuleTest.kt new file mode 100644 index 0000000000..ad05029a83 --- /dev/null +++ b/custom-ktlint-rules/src/test/kotlin/org/partiql/ktlint/rule/TopLevelPublicRuleTest.kt @@ -0,0 +1,129 @@ +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 TopLevelPublicRuleTest { + @Test + fun `top-level public`() { + val code = + """ + public fun publicTopLevelFun() {} // ktlint error + + public val publicTopLevelVal = 123 // ktlint error + + public var publicTopLevelVar = 456 // ktlint error + + fun publicTopLevelFun2() {} // ktlint error + + val publicTopLevelVal = 123 // ktlint error + + var publicTopLevelVar = 456 // ktlint error + + // No errors for below (for this rule) + internal fun internalTopLevelFun() {} + + internal val internalTopLevelVal = 123 + + internal var internalTopLevelVar = 456 + + public class PublicClass { + internal fun internalFun() {} + + internal val internalVal = 123 + + public fun publicFun() {} + + public val publicVal = 123 + } + """.trimIndent() + Assertions.assertThat(TopLevelPublicRule().lint(code)).containsExactly( + LintError(1, 12, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelFun"), + LintError(3, 12, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelVal"), + LintError(5, 12, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelVar"), + LintError(7, 5, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelFun2"), + LintError(9, 5, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelVal"), + LintError(11, 5, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelVar") + ) + } + + @Test + fun `top-level public with file name`() { + val code = + """ + @file:JvmName("SomeName") + public fun publicTopLevelFun() {} + + public val publicTopLevelVal = 123 + + public var publicTopLevelVar = 456 + + fun publicTopLevelFun2() {} + + val publicTopLevelVal = 123 + + var publicTopLevelVar = 456 + + // No errors for below (for this rule) + internal fun internalTopLevelFun() {} + + internal val internalTopLevelVal = 123 + + internal var publicTopLevelVar = 456 + + public class PublicClass { + internal fun internalFun() {} + + internal val internalVal = 123 + + public fun publicFun() {} + + public val publicVal = 123 + } + """.trimIndent() + Assertions.assertThat(TopLevelPublicRule().lint(code)).containsExactly() + } + + @Test + fun `different modifier levels`() { + val code = + """ + public fun publicTopLevelFun() {} // ktlint error + + fun publicTopLevelFun2() {} // ktlint error + + internal fun publicTopLevelFun() {} + + protected fun publicTopLevelFun() {} + + private fun publicTopLevelFun() {} + """.trimIndent() + Assertions.assertThat(TopLevelPublicRule().lint(code)).containsExactly( + LintError(1, 12, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelFun"), + LintError(3, 5, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelFun2"), + ) + } + + @Test + fun `different top level annotation`() { + val code = + """ + @file:OptIn(PartiQLValueExperimental::class) + public fun publicTopLevelFun() {} // ktlint error + + fun publicTopLevelFun2() {} // ktlint error + + internal fun publicTopLevelFun() {} + + protected fun publicTopLevelFun() {} + + private fun publicTopLevelFun() {} + """.trimIndent() + Assertions.assertThat(TopLevelPublicRule().lint(code)).containsExactly( + LintError(2, 12, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelFun"), + LintError(4, 5, "top-level-public", "Top-level public declaration found without `@file:JvmName` annotation: publicTopLevelFun2"), + ) + } +} diff --git a/settings.gradle.kts b/settings.gradle.kts index 0876b5b072..26e39c7c27 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -15,6 +15,7 @@ rootProject.name = "partiql" include( + ":custom-ktlint-rules", "partiql-ast", "partiql-cli", "partiql-coverage",