From 0bd1768caff12c7b954e12981b7de34f95f8ab9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergej=20Ko=C5=A1=C4=8Dejev?= Date: Wed, 11 Nov 2026 21:05:57 +0100 Subject: [PATCH 1/2] build: add detekt rule against inner classes changing outer state --- .../mops.kotlin-jvm-conventions.gradle.kts | 4 + config/detekt/detekt.yml | 12 +- detekt-rules/build.gradle.kts | 12 ++ .../detekt/InnerClassChangesOuterState.kt | 106 ++++++++++++++++++ .../mops/detekt/MopsRuleSetProvider.kt | 16 +++ .../services/dev.detekt.api.RuleSetProvider | 1 + .../detekt/InnerClassChangesOuterStateTest.kt | 105 +++++++++++++++++ docs/agents/kotlin-coding-guidelines.md | 8 ++ docs/development.md | 13 ++- gradle/libs.versions.toml | 2 + settings.gradle.kts | 1 + 11 files changed, 271 insertions(+), 9 deletions(-) create mode 100644 detekt-rules/build.gradle.kts create mode 100644 detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterState.kt create mode 100644 detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/MopsRuleSetProvider.kt create mode 100644 detekt-rules/src/main/resources/META-INF/services/dev.detekt.api.RuleSetProvider create mode 100644 detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterStateTest.kt 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..861626b9 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 @@ -23,3 +23,7 @@ tasks.named("check") { dependencies { testImplementation(kotlin("test")) } + +dependencies { + detektPlugins(project(":detekt-rules")) +} diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index ef69b823..80301a75 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 + InnerClassChangesOuterState: + active: true 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/InnerClassChangesOuterState.kt b/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterState.kt new file mode 100644 index 00000000..3624c57f --- /dev/null +++ b/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterState.kt @@ -0,0 +1,106 @@ +package com.specificlanguages.mops.detekt + +import dev.detekt.api.Config +import dev.detekt.api.Entity +import dev.detekt.api.Finding +import dev.detekt.api.RequiresAnalysisApi +import dev.detekt.api.Rule +import org.jetbrains.kotlin.analysis.api.KaExperimentalApi +import org.jetbrains.kotlin.analysis.api.analyze +import org.jetbrains.kotlin.analysis.api.symbols.KaPropertySymbol +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtArrayAccessExpression +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtClass +import org.jetbrains.kotlin.psi.KtClassOrObject +import org.jetbrains.kotlin.psi.KtDotQualifiedExpression +import org.jetbrains.kotlin.psi.KtElement +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.KtParenthesizedExpression +import org.jetbrains.kotlin.psi.KtThisExpression +import org.jetbrains.kotlin.psi.KtUnaryExpression +import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType +import org.jetbrains.kotlin.psi.psiUtil.parents + +/** + * Reports code in an `inner` class that changes a property of an enclosing class: an assignment (`=`, `+=`, ...), an + * increment or decrement, an indexed assignment (`map[key] = value`), or a call to a mutating collection function such + * as `add` or `put`. + * + * The enclosing class should own its state. The inner class can return what changed and let the owner apply it. + */ +class InnerClassChangesOuterState(config: Config) : + Rule(config, "An inner class changes a property of its enclosing class."), + RequiresAnalysisApi { + + override fun visitBinaryExpression(expression: KtBinaryExpression) { + super.visitBinaryExpression(expression) + if (expression.operationToken in KtTokens.ALL_ASSIGNMENTS) { + check(expression, expression.left) + } + } + + override fun visitUnaryExpression(expression: KtUnaryExpression) { + super.visitUnaryExpression(expression) + if (expression.operationToken == KtTokens.PLUSPLUS || expression.operationToken == KtTokens.MINUSMINUS) { + check(expression, expression.baseExpression) + } + } + + override fun visitCallExpression(expression: KtCallExpression) { + super.visitCallExpression(expression) + val name = (expression.calleeExpression as? KtNameReferenceExpression)?.getReferencedName() + val qualified = expression.parent as? KtDotQualifiedExpression + if (name in MUTATING_FUNCTIONS && qualified != null && qualified.selectorExpression == expression) { + check(expression, qualified.receiverExpression) + } + } + + private fun check(change: KtElement, target: KtExpression?) { + val reference = propertyReference(target) ?: return + val innerClass = change.parents.filterIsInstance().firstOrNull { it.isInner() } ?: return + val owner = declaringClass(reference) ?: return + if (owner in innerClass.parents) { + report( + Finding( + Entity.from(change), + "Inner class ${innerClass.name} changes ${reference.getReferencedName()} of ${owner.name}. " + + "Return the change and let ${owner.name} apply it.", + ), + ) + } + } + + @OptIn(KaExperimentalApi::class) + private fun declaringClass(reference: KtNameReferenceExpression): KtClassOrObject? { + val declaration = analyze(reference) { + (reference.resolveSymbol() as? KaPropertySymbol)?.psi + } ?: return null + return declaration.getStrictParentOfType() + } + + /** The property reference that [target] changes: `x`, `this.x`, `this@Outer.x`, or `x[key]`. */ + private fun propertyReference(target: KtExpression?): KtNameReferenceExpression? = when (target) { + is KtNameReferenceExpression -> target + is KtParenthesizedExpression -> propertyReference(target.expression) + is KtArrayAccessExpression -> propertyReference(target.arrayExpression) + is KtDotQualifiedExpression -> + if (target.receiverExpression is KtThisExpression) { + target.selectorExpression as? KtNameReferenceExpression + } else { + null + } + else -> null + } + + private companion object { + val MUTATING_FUNCTIONS = setOf( + "add", "addAll", "addFirst", "addLast", + "remove", "removeAll", "removeAt", "removeIf", "removeFirst", "removeLast", "retainAll", + "put", "putAll", "putIfAbsent", "getOrPut", "compute", "computeIfAbsent", "computeIfPresent", "merge", + "set", "clear", "sort", "sortBy", "sortWith", "replaceAll", + ) + } +} 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..4d265681 --- /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("InnerClassChangesOuterState") to ::InnerClassChangesOuterState), + ) +} 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/InnerClassChangesOuterStateTest.kt b/detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterStateTest.kt new file mode 100644 index 00000000..5657edbb --- /dev/null +++ b/detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterStateTest.kt @@ -0,0 +1,105 @@ +package com.specificlanguages.mops.detekt + +import dev.detekt.test.TestConfig +import dev.detekt.test.lintWithContext +import dev.detekt.test.utils.createEnvironment +import kotlin.test.Test +import kotlin.test.assertEquals + +class InnerClassChangesOuterStateTest { + private val env = createEnvironment() + + private fun lint(code: String): List = + InnerClassChangesOuterState(TestConfig()).lintWithContext(env, code).map { it.message } + + @Test + fun `reports an assignment to an outer var`() { + val messages = lint( + """ + class Outer { + var count = 0 + + inner class Pass { + fun run() { + count = 1 + this@Outer.count = 2 + count++ + } + } + } + """.trimIndent(), + ) + assertEquals(3, messages.size, messages.toString()) + assertEquals( + "Inner class Pass changes count of Outer. Return the change and let Outer apply it.", + messages.first(), + ) + } + + @Test + fun `reports changes to an outer mutable collection`() { + val messages = lint( + """ + class Outer { + val folders = mutableSetOf() + val sizes = mutableMapOf() + + inner class Pass { + fun run(folder: String) { + folders += folder + folders.add(folder) + sizes[folder] = 1 + sizes.put(folder, 2) + } + } + } + """.trimIndent(), + ) + assertEquals(4, messages.size, messages.toString()) + } + + @Test + fun `does not report an inner class that only reads outer state`() { + val messages = lint( + """ + class Outer { + val folders = mutableSetOf() + var count = 0 + + inner class Pass { + private val seen = mutableSetOf() + private var total = 0 + + fun run(folder: String): Boolean { + seen.add(folder) + total += count + return folder in folders + } + } + } + """.trimIndent(), + ) + assertEquals(emptyList(), messages) + } + + @Test + fun `does not report a nested class that is not inner`() { + val messages = lint( + """ + class Outer { + val folders = mutableSetOf() + + class Pass(private val outer: Outer) { + var count = 0 + + fun run(folder: String) { + count = 1 + outer.folders.add(folder) + } + } + } + """.trimIndent(), + ) + assertEquals(emptyList(), messages) + } +} diff --git a/docs/agents/kotlin-coding-guidelines.md b/docs/agents/kotlin-coding-guidelines.md index 5ddac21c..f06e483f 100644 --- a/docs/agents/kotlin-coding-guidelines.md +++ b/docs/agents/kotlin-coding-guidelines.md @@ -17,3 +17,11 @@ 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. An 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. The detekt rule `InnerClassChangesOuterState` enforces this for + `inner` classes. +- 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..019a039c 100644 --- a/docs/development.md +++ b/docs/development.md @@ -63,12 +63,13 @@ 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 - `gradle.properties`). +- **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, such as `InnerClassChangesOuterState`. 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 command packages (`cli.find`, `cli.get`, ...) do not depend on each other. Gradle module dependencies already decide 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") From 72b61d289d301019c4c067ca7b05bbbb1237a071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergej=20Ko=C5=A1=C4=8Dejev?= Date: Wed, 11 Nov 2026 22:03:16 +0100 Subject: [PATCH 2/2] build: replace the inner class state rule with a rule against unexplained inner classes --- .../mops.kotlin-jvm-conventions.gradle.kts | 5 +- config/detekt/detekt.yml | 2 +- .../mops/daemon/JetBrainsMpsAccess.kt | 6 + .../detekt/InnerClassChangesOuterState.kt | 106 ------------------ .../mops/detekt/MopsRuleSetProvider.kt | 2 +- .../mops/detekt/UnexplainedInnerClass.kt | 40 +++++++ .../detekt/InnerClassChangesOuterStateTest.kt | 105 ----------------- .../mops/detekt/UnexplainedInnerClassTest.kt | 73 ++++++++++++ docs/agents/kotlin-coding-guidelines.md | 8 +- docs/development.md | 6 +- 10 files changed, 130 insertions(+), 223 deletions(-) delete mode 100644 detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterState.kt create mode 100644 detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/UnexplainedInnerClass.kt delete mode 100644 detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterStateTest.kt create mode 100644 detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/UnexplainedInnerClassTest.kt 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 861626b9..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 @@ -20,10 +20,7 @@ tasks.named("check") { dependsOn("detektMain") } -dependencies { - testImplementation(kotlin("test")) -} - dependencies { detektPlugins(project(":detekt-rules")) + testImplementation(kotlin("test")) } diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 80301a75..eb353464 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -44,5 +44,5 @@ style: # The project's own rules, from the detekt-rules module. mops: active: true - InnerClassChangesOuterState: + 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/src/main/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterState.kt b/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterState.kt deleted file mode 100644 index 3624c57f..00000000 --- a/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterState.kt +++ /dev/null @@ -1,106 +0,0 @@ -package com.specificlanguages.mops.detekt - -import dev.detekt.api.Config -import dev.detekt.api.Entity -import dev.detekt.api.Finding -import dev.detekt.api.RequiresAnalysisApi -import dev.detekt.api.Rule -import org.jetbrains.kotlin.analysis.api.KaExperimentalApi -import org.jetbrains.kotlin.analysis.api.analyze -import org.jetbrains.kotlin.analysis.api.symbols.KaPropertySymbol -import org.jetbrains.kotlin.lexer.KtTokens -import org.jetbrains.kotlin.psi.KtArrayAccessExpression -import org.jetbrains.kotlin.psi.KtBinaryExpression -import org.jetbrains.kotlin.psi.KtCallExpression -import org.jetbrains.kotlin.psi.KtClass -import org.jetbrains.kotlin.psi.KtClassOrObject -import org.jetbrains.kotlin.psi.KtDotQualifiedExpression -import org.jetbrains.kotlin.psi.KtElement -import org.jetbrains.kotlin.psi.KtExpression -import org.jetbrains.kotlin.psi.KtNameReferenceExpression -import org.jetbrains.kotlin.psi.KtParenthesizedExpression -import org.jetbrains.kotlin.psi.KtThisExpression -import org.jetbrains.kotlin.psi.KtUnaryExpression -import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType -import org.jetbrains.kotlin.psi.psiUtil.parents - -/** - * Reports code in an `inner` class that changes a property of an enclosing class: an assignment (`=`, `+=`, ...), an - * increment or decrement, an indexed assignment (`map[key] = value`), or a call to a mutating collection function such - * as `add` or `put`. - * - * The enclosing class should own its state. The inner class can return what changed and let the owner apply it. - */ -class InnerClassChangesOuterState(config: Config) : - Rule(config, "An inner class changes a property of its enclosing class."), - RequiresAnalysisApi { - - override fun visitBinaryExpression(expression: KtBinaryExpression) { - super.visitBinaryExpression(expression) - if (expression.operationToken in KtTokens.ALL_ASSIGNMENTS) { - check(expression, expression.left) - } - } - - override fun visitUnaryExpression(expression: KtUnaryExpression) { - super.visitUnaryExpression(expression) - if (expression.operationToken == KtTokens.PLUSPLUS || expression.operationToken == KtTokens.MINUSMINUS) { - check(expression, expression.baseExpression) - } - } - - override fun visitCallExpression(expression: KtCallExpression) { - super.visitCallExpression(expression) - val name = (expression.calleeExpression as? KtNameReferenceExpression)?.getReferencedName() - val qualified = expression.parent as? KtDotQualifiedExpression - if (name in MUTATING_FUNCTIONS && qualified != null && qualified.selectorExpression == expression) { - check(expression, qualified.receiverExpression) - } - } - - private fun check(change: KtElement, target: KtExpression?) { - val reference = propertyReference(target) ?: return - val innerClass = change.parents.filterIsInstance().firstOrNull { it.isInner() } ?: return - val owner = declaringClass(reference) ?: return - if (owner in innerClass.parents) { - report( - Finding( - Entity.from(change), - "Inner class ${innerClass.name} changes ${reference.getReferencedName()} of ${owner.name}. " + - "Return the change and let ${owner.name} apply it.", - ), - ) - } - } - - @OptIn(KaExperimentalApi::class) - private fun declaringClass(reference: KtNameReferenceExpression): KtClassOrObject? { - val declaration = analyze(reference) { - (reference.resolveSymbol() as? KaPropertySymbol)?.psi - } ?: return null - return declaration.getStrictParentOfType() - } - - /** The property reference that [target] changes: `x`, `this.x`, `this@Outer.x`, or `x[key]`. */ - private fun propertyReference(target: KtExpression?): KtNameReferenceExpression? = when (target) { - is KtNameReferenceExpression -> target - is KtParenthesizedExpression -> propertyReference(target.expression) - is KtArrayAccessExpression -> propertyReference(target.arrayExpression) - is KtDotQualifiedExpression -> - if (target.receiverExpression is KtThisExpression) { - target.selectorExpression as? KtNameReferenceExpression - } else { - null - } - else -> null - } - - private companion object { - val MUTATING_FUNCTIONS = setOf( - "add", "addAll", "addFirst", "addLast", - "remove", "removeAll", "removeAt", "removeIf", "removeFirst", "removeLast", "retainAll", - "put", "putAll", "putIfAbsent", "getOrPut", "compute", "computeIfAbsent", "computeIfPresent", "merge", - "set", "clear", "sort", "sortBy", "sortWith", "replaceAll", - ) - } -} 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 index 4d265681..ea2e46e3 100644 --- a/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/MopsRuleSetProvider.kt +++ b/detekt-rules/src/main/kotlin/com/specificlanguages/mops/detekt/MopsRuleSetProvider.kt @@ -11,6 +11,6 @@ class MopsRuleSetProvider : RuleSetProvider { override fun instance() = RuleSet( ruleSetId, - mapOf(RuleName("InnerClassChangesOuterState") to ::InnerClassChangesOuterState), + 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/test/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterStateTest.kt b/detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterStateTest.kt deleted file mode 100644 index 5657edbb..00000000 --- a/detekt-rules/src/test/kotlin/com/specificlanguages/mops/detekt/InnerClassChangesOuterStateTest.kt +++ /dev/null @@ -1,105 +0,0 @@ -package com.specificlanguages.mops.detekt - -import dev.detekt.test.TestConfig -import dev.detekt.test.lintWithContext -import dev.detekt.test.utils.createEnvironment -import kotlin.test.Test -import kotlin.test.assertEquals - -class InnerClassChangesOuterStateTest { - private val env = createEnvironment() - - private fun lint(code: String): List = - InnerClassChangesOuterState(TestConfig()).lintWithContext(env, code).map { it.message } - - @Test - fun `reports an assignment to an outer var`() { - val messages = lint( - """ - class Outer { - var count = 0 - - inner class Pass { - fun run() { - count = 1 - this@Outer.count = 2 - count++ - } - } - } - """.trimIndent(), - ) - assertEquals(3, messages.size, messages.toString()) - assertEquals( - "Inner class Pass changes count of Outer. Return the change and let Outer apply it.", - messages.first(), - ) - } - - @Test - fun `reports changes to an outer mutable collection`() { - val messages = lint( - """ - class Outer { - val folders = mutableSetOf() - val sizes = mutableMapOf() - - inner class Pass { - fun run(folder: String) { - folders += folder - folders.add(folder) - sizes[folder] = 1 - sizes.put(folder, 2) - } - } - } - """.trimIndent(), - ) - assertEquals(4, messages.size, messages.toString()) - } - - @Test - fun `does not report an inner class that only reads outer state`() { - val messages = lint( - """ - class Outer { - val folders = mutableSetOf() - var count = 0 - - inner class Pass { - private val seen = mutableSetOf() - private var total = 0 - - fun run(folder: String): Boolean { - seen.add(folder) - total += count - return folder in folders - } - } - } - """.trimIndent(), - ) - assertEquals(emptyList(), messages) - } - - @Test - fun `does not report a nested class that is not inner`() { - val messages = lint( - """ - class Outer { - val folders = mutableSetOf() - - class Pass(private val outer: Outer) { - var count = 0 - - fun run(folder: String) { - count = 1 - outer.folders.add(folder) - } - } - } - """.trimIndent(), - ) - assertEquals(emptyList(), messages) - } -} 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 f06e483f..92f05d60 100644 --- a/docs/agents/kotlin-coding-guidelines.md +++ b/docs/agents/kotlin-coding-guidelines.md @@ -17,9 +17,11 @@ 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. An 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. The detekt rule `InnerClassChangesOuterState` enforces this for - `inner` classes. +- 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 diff --git a/docs/development.md b/docs/development.md index 019a039c..3e3acea7 100644 --- a/docs/development.md +++ b/docs/development.md @@ -67,9 +67,9 @@ and publishes both files in a GitHub Release with notes generated from the merge `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, such as `InnerClassChangesOuterState`. 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`). + 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 command packages (`cli.find`, `cli.get`, ...) do not depend on each other. Gradle module dependencies already decide