build: add detekt rule against unexplained inner classes

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"))
 }

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

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<String>?, 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)
 

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)
+}

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),
+    )
+}

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 }
+}

detekt-rules/src/main/resources/META-INF/services/dev.detekt.api.RuleSetProvider

@@ -0,0 +1 @@
+com.specificlanguages.mops.detekt.MopsRuleSetProvider

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<String> =
+        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<String>()
+
+                // 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)
+    }
+}

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<X>()` 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.

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 `<module>/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
+  `<module>/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

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]

settings.gradle.kts

@@ -18,3 +18,4 @@ include("protocol")
 include("ant-test-adapters")
 
 include("architecture-tests")
+include("detekt-rules")