diff --git a/build-logic/src/main/kotlin/mops.kotlin-jvm-conventions.gradle.kts b/build-logic/src/main/kotlin/mops.kotlin-jvm-conventions.gradle.kts index 2a6cc3d2..620f9504 100644 --- a/build-logic/src/main/kotlin/mops.kotlin-jvm-conventions.gradle.kts +++ b/build-logic/src/main/kotlin/mops.kotlin-jvm-conventions.gradle.kts @@ -21,5 +21,6 @@ tasks.named("check") { } dependencies { + detektPlugins(project(":detekt-rules")) testImplementation(kotlin("test")) } diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index ef69b823..eb353464 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -1,6 +1,6 @@ -# Only the complexity and exceptions rule sets are active. Every other rule set is turned off, so detekt reports method -# length, complexity, nesting, parameter count, class and file size, and exception handling, and nothing about style or -# naming. +# Only the complexity, exceptions, and mops rule sets are active. Every other rule set is turned off, so detekt reports +# method length, complexity, nesting, parameter count, class and file size, exception handling, and the project's own +# rules, and nothing about style or naming. # See "Code structure checks" in docs/development.md for how the per-module baselines work. complexity: @@ -40,3 +40,9 @@ potential-bugs: active: false style: active: false + +# The project's own rules, from the detekt-rules module. +mops: + active: true + UnexplainedInnerClass: + active: true diff --git a/daemon/src/main/kotlin/com/specificlanguages/mops/daemon/JetBrainsMpsAccess.kt b/daemon/src/main/kotlin/com/specificlanguages/mops/daemon/JetBrainsMpsAccess.kt index c5cbe098..3fc28fc8 100644 --- a/daemon/src/main/kotlin/com/specificlanguages/mops/daemon/JetBrainsMpsAccess.kt +++ b/daemon/src/main/kotlin/com/specificlanguages/mops/daemon/JetBrainsMpsAccess.kt @@ -65,6 +65,8 @@ class JetBrainsMpsAccess( return captureRequestErrors { JetBrainsMpsExtra().block() }.getOrThrow() } + // Inner: the read operations use the project, the exporters, the resolvers, and the private helpers of this + // JetBrainsMpsAccess. private open inner class JetBrainsMpsRead : MpsRead { override fun list(target: List?, depth: Int, limit: Int, summary: Boolean, role: String?): MpsListEntryJson = when (target) { @@ -447,6 +449,8 @@ class JetBrainsMpsAccess( ?: throw MpsRequestException(code = MpsErrorCode.NODE_NOT_FOUND, message = "node not found") } + // Inner: extends JetBrainsMpsRead, which is inner, and uses the project and the edit batch executor of this + // JetBrainsMpsAccess. private inner class JetBrainsMpsWrite( private val writeScope: WriteTransaction.WriteScope, ) : JetBrainsMpsRead(), MpsWrite { @@ -461,6 +465,8 @@ class JetBrainsMpsAccess( override fun createModel(request: CreateModelRequest): ModelCreationResponse = ModelCreator(project).create(request) } + // Inner: the extra operations use the project, the workspace, the platform, and the private helpers of this + // JetBrainsMpsAccess, and pass this JetBrainsMpsAccess on to Code Mode and test runs. private inner class JetBrainsMpsExtra : MpsExtra { private val projectMake = ProjectMake(project) diff --git a/detekt-rules/build.gradle.kts b/detekt-rules/build.gradle.kts new file mode 100644 index 00000000..76bcac39 --- /dev/null +++ b/detekt-rules/build.gradle.kts @@ -0,0 +1,12 @@ +plugins { + id("mops.kotlin-jvm-conventions") +} + +dependencies { + compileOnly(libs.detekt.api) + + testImplementation(libs.detekt.api) + testImplementation(libs.detekt.test) + testImplementation(libs.junit.jupiter) + testRuntimeOnly(libs.junit.platform.launcher) +} diff --git a/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/MopsRuleSetProvider.kt b/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/MopsRuleSetProvider.kt new file mode 100644 index 00000000..ea2e46e3 --- /dev/null +++ b/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/MopsRuleSetProvider.kt @@ -0,0 +1,16 @@ +package com.specificlanguages.mops.detekt + +import dev.detekt.api.RuleName +import dev.detekt.api.RuleSet +import dev.detekt.api.RuleSetId +import dev.detekt.api.RuleSetProvider + +/** The project's own detekt rules, configured under `mops:` in `config/detekt/detekt.yml`. */ +class MopsRuleSetProvider : RuleSetProvider { + override val ruleSetId = RuleSetId("mops") + + override fun instance() = RuleSet( + ruleSetId, + mapOf(RuleName("UnexplainedInnerClass") to ::UnexplainedInnerClass), + ) +} diff --git a/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/UnexplainedInnerClass.kt b/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/UnexplainedInnerClass.kt new file mode 100644 index 00000000..cb5e4162 --- /dev/null +++ b/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/UnexplainedInnerClass.kt @@ -0,0 +1,40 @@ +package com.specificlanguages.mops.detekt + +import com.intellij.psi.PsiComment +import com.intellij.psi.PsiWhiteSpace +import dev.detekt.api.Config +import dev.detekt.api.Entity +import dev.detekt.api.Finding +import dev.detekt.api.Rule +import org.jetbrains.kotlin.psi.KtClass +import org.jetbrains.kotlin.psi.psiUtil.allChildren +import org.jetbrains.kotlin.psi.psiUtil.siblings + +/** + * Reports an `inner` class that has no comment directly above it. See the inner class bullet in + * `docs/agents/kotlin-coding-guidelines.md`. + */ +class UnexplainedInnerClass(config: Config) : + Rule(config, "An inner class has no comment that says why it is inner.") { + + override fun visitClass(klass: KtClass) { + super.visitClass(klass) + if (klass.isInner() && !hasLeadingComment(klass)) { + report( + Finding( + Entity.atName(klass), + "Inner class ${klass.name} has no comment that says why it is inner. Make it a nested or " + + "top-level class, or add the comment.", + ), + ) + } + } + + // The Kotlin parser attaches comments directly above a declaration to the declaration itself, so they are its + // first children. The previous siblings are checked too in case a comment is not attached. + private fun hasLeadingComment(klass: KtClass): Boolean = + klass.allChildren.takeWhile { it is PsiComment || it is PsiWhiteSpace }.any { it is PsiComment } || + klass.siblings(forward = false, withItself = false) + .takeWhile { it is PsiComment || it is PsiWhiteSpace } + .any { it is PsiComment } +} diff --git a/detekt-rules/src/main/resources/META-INF/services/dev.detekt.api.RuleSetProvider b/detekt-rules/src/main/resources/META-INF/services/dev.detekt.api.RuleSetProvider new file mode 100644 index 00000000..3b84a785 --- /dev/null +++ b/detekt-rules/src/main/resources/META-INF/services/dev.detekt.api.RuleSetProvider @@ -0,0 +1 @@ +com.specificlanguages.mops.detekt.MopsRuleSetProvider diff --git a/detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/UnexplainedInnerClassTest.kt b/detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/UnexplainedInnerClassTest.kt new file mode 100644 index 00000000..14b7d9dc --- /dev/null +++ b/detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/UnexplainedInnerClassTest.kt @@ -0,0 +1,73 @@ +package com.specificlanguages.mops.detekt + +import dev.detekt.test.TestConfig +import dev.detekt.test.lint +import kotlin.test.Test +import kotlin.test.assertEquals + +class UnexplainedInnerClassTest { + private fun lint(code: String): List = + UnexplainedInnerClass(TestConfig()).lint(code).map { "${it.entity.location.source.line}: ${it.message}" } + + @Test + fun `reports an inner class without a comment`() { + val findings = lint( + """ + class Outer { + var count = 0 + + @Suppress("unused") + private inner class Pass { + fun run() { + count = 1 + } + } + } + """.trimIndent(), + ) + assertEquals( + listOf( + "5: Inner class Pass has no comment that says why it is inner. Make it a nested or top-level class, " + + "or add the comment.", + ), + findings, + ) + } + + @Test + fun `does not report an inner class with a comment above it`() { + val findings = lint( + """ + class Outer { + val folders = mutableSetOf() + + // Inner: reads the folders of this Outer. + inner class LineComment { + fun has(folder: String) = folder in folders + } + + /** Inner: reads the folders of this Outer. */ + @Suppress("unused") + inner class KDoc { + fun has(folder: String) = folder in folders + } + } + """.trimIndent(), + ) + assertEquals(emptyList(), findings) + } + + @Test + fun `does not report a nested class that is not inner`() { + val findings = lint( + """ + class Outer { + class Pass(private val outer: Outer) { + fun run() = outer.toString() + } + } + """.trimIndent(), + ) + assertEquals(emptyList(), findings) + } +} diff --git a/docs/agents/kotlin-coding-guidelines.md b/docs/agents/kotlin-coding-guidelines.md index 5ddac21c..92f05d60 100644 --- a/docs/agents/kotlin-coding-guidelines.md +++ b/docs/agents/kotlin-coding-guidelines.md @@ -17,3 +17,13 @@ formatting and naming. `X.serializer()`. detekt's type resolution does not run the serialization compiler plugin, so it cannot see the generated `X.serializer()` and reports a compiler error. The plugin compiles `serializer()` to the same direct `X.Companion.serializer()` call. +- Keep mutable state owned by one class. A nested or inner class may read the state of its outer class but does not + change it. Return what changed and let the outer class apply it. +- Prefer a nested or top-level class to an inner class. An inner class can read and change the state of its outer class, + so that state can change from two places. When a class must be inner, put a comment above it that says why. The detekt + rule `UnexplainedInnerClass` reports an inner class without such a comment. +- Keep each decision in one place. When a reader asks "when does X happen?", one function should answer it. If the + answer needs two functions in different objects, move the rule into one of them. +- Make the role of a class clear from its name. A class is a value (immutable data), a service (does work, named with a + verb or `-er`), or a stateful policy (remembers history to decide). If a class needs a second role, give that role its + own class. diff --git a/docs/development.md b/docs/development.md index 62abaeda..3e3acea7 100644 --- a/docs/development.md +++ b/docs/development.md @@ -63,11 +63,12 @@ and publishes both files in a GitHub Release with notes generated from the merge `check` runs two kinds of structure checks on the Kotlin modules: -- **detekt** (`./gradlew detektMain`) runs the complexity and exceptions rule sets from `config/detekt/detekt.yml` on - the main source sets, with type resolution. The complexity rules check method length, cyclomatic and cognitive - complexity, nesting depth, parameter count, and class and file size. The exceptions rules check how exceptions are - thrown and caught. Name a caught exception that is deliberately ignored `_`. Test sources are not checked. Reports are - written to `/build/reports/detekt/`. detekt runs in a separate worker process (`detekt.use.worker.api` in +- **detekt** (`./gradlew detektMain`) runs the complexity, exceptions, and `mops` rule sets from + `config/detekt/detekt.yml` on the main source sets, with type resolution. The complexity rules check method length, + cyclomatic and cognitive complexity, nesting depth, parameter count, and class and file size. The exceptions rules + check how exceptions are thrown and caught. Name a caught exception that is deliberately ignored `_`. The `mops` rule + set holds the project's own rules from the `detekt-rules` module. Test sources are not checked. Reports are written to + `/build/reports/detekt/`. detekt runs in a separate worker process (`detekt.use.worker.api` in `gradle.properties`). - **Architecture tests** in `architecture-tests/` (`./gradlew :architecture-tests:test`) use [Konsist](https://docs.konsist.lemonappdev.com/) to check dependencies that Gradle cannot see. They check that CLI diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 4a2eeabc..d9e5b3fa 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -41,6 +41,8 @@ junit-platform-launcher = { module = "org.junit.platform:junit-platform-launcher json-schema-validator = { module = "com.networknt:json-schema-validator", version.ref = "json-schema-validator" } kotlin-gradle-plugin-marker = { module = "org.jetbrains.kotlin.jvm:org.jetbrains.kotlin.jvm.gradle.plugin", version.ref = "kotlin" } detekt-gradle-plugin-marker = { module = "dev.detekt:dev.detekt.gradle.plugin", version.ref = "detekt" } +detekt-api = { module = "dev.detekt:detekt-api", version.ref = "detekt" } +detekt-test = { module = "dev.detekt:detekt-test", version.ref = "detekt" } konsist = { module = "com.lemonappdev:konsist", version.ref = "konsist" } [plugins] diff --git a/settings.gradle.kts b/settings.gradle.kts index fb7dd6b6..4462df93 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -18,3 +18,4 @@ include("protocol") include("ant-test-adapters") include("architecture-tests") +include("detekt-rules")