Merge pull request #670 from modelix/feature/MODELIX-794-Adapt-the-detekt-configuration-according-to-the-discussions-from-the-coding-conventions-workshop

MODELIX-794: adapt the detekt configuration according to the discussions from the coding conventions workshop

Author: languitar

.detekt.yml

@@ -10,3 +10,23 @@ style:
     active: false
   ThrowsCount:
     active: false
+
+# Enforce some proper documentation standards (cf. MODELIX-794)
+comments:
+  OutdatedDocumentation:
+    active: true
+  UndocumentedPublicClass:
+    active: true
+  UndocumentedPublicFunction:
+    active: true
+  UndocumentedPublicProperty:
+    active: true
+
+# Try to enforce good handling of nullability
+potential-bugs:
+  CastNullableToNonNullableType:
+    active: true
+
+modelix:
+  AvoidNonNullAssertionOperator:
+    active: true

.github/workflows/build.yaml

@@ -32,7 +32,7 @@ jobs:
       - name: Build
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-        run: ./gradlew --build-cache build detekt :koverHtmlReport :koverXmlReport -PciBuild=true
+        run: ./gradlew --build-cache build detekt detektJsMain detektJsTest detektJvmMain detektJvmTest :koverHtmlReport :koverXmlReport -PciBuild=true
       - name: Publish test results
         uses: EnricoMi/publish-unit-test-result-action@v2
         # Also report in case the build failed
@@ -56,10 +56,18 @@ jobs:
           token: ${{ secrets.GITHUB_TOKEN }}
           title: JVM coverage report
           update-comment: true
+        # We need to combine the SARIF files because GitHub has a limit of 20 runs. Our number of modules + targets
+        # exceeds this limit. Therefore, we combine the individual runs in the SARIF files.
+      - uses: actions/setup-node@v4
+        with:
+          node-version: '20.x'
+      - name: Combine SARIF files
+        run: |
+          npx @microsoft/sarif-multitool merge --merge-runs --output-file merged.sarif  $(find . -iname '*.sarif*')
       - name: Upload SARIF file
         uses: github/codeql-action/upload-sarif@v3
         with:
-          sarif_file: 'build/reports/detekt/'
+          sarif_file: merged.sarif
           category: detekt
 
   test-model-api-gen-gradle:

build.gradle.kts

@@ -43,7 +43,7 @@ plugins {
     alias(libs.plugins.spotless) apply false
     alias(libs.plugins.dokka)
     alias(libs.plugins.node) apply false
-    alias(libs.plugins.detekt) apply false
+    alias(libs.plugins.detekt)
     alias(libs.plugins.kotlinx.kover)
 }
 
@@ -82,7 +82,6 @@ subprojects {
     val subproject = this
     apply(plugin = "maven-publish")
     apply(plugin = "org.jetbrains.dokka")
-    apply(plugin = "io.gitlab.arturbosch.detekt")
     apply(plugin = "org.jetbrains.kotlinx.kover")
 
     version = rootProject.version
@@ -94,22 +93,6 @@ subprojects {
         }
     }
 
-    tasks.withType<Detekt> {
-        parallel = true
-        // For now, we only use the results here as hints
-        ignoreFailures = true
-
-        buildUponDefaultConfig = true
-        config.setFrom(parentProject.projectDir.resolve(".detekt.yml"))
-
-        reports {
-            sarif.required.set(true)
-            // This is required for the GitHub upload action to easily find all sarif files in a single directory.
-            sarif.outputLocation.set(parentProject.layout.buildDirectory.file("reports/detekt/${project.name}.sarif"))
-            html.required.set(true)
-        }
-    }
-
     val kotlinApiVersion = org.jetbrains.kotlin.gradle.dsl.KotlinVersion.KOTLIN_1_6
     subproject.tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>().configureEach {
         if (!name.lowercase().contains("test")) {
@@ -168,6 +151,29 @@ subprojects {
             download.set(true)
         }
     }
+
+    // Configure detekt including our custom rule sets
+    if (project.name != "detekt-rules") {
+        apply(plugin = "io.gitlab.arturbosch.detekt")
+        dependencies {
+            detektPlugins(project(":detekt-rules"))
+        }
+        tasks.withType<Detekt> {
+            dependsOn(":detekt-rules:assemble")
+
+            parallel = true
+            // For now, we only use the results here as hints
+            ignoreFailures = true
+
+            buildUponDefaultConfig = true
+            config.setFrom(parentProject.projectDir.resolve(".detekt.yml"))
+
+            reports {
+                sarif.required.set(true)
+                html.required.set(true)
+            }
+        }
+    }
 }
 
 allprojects {
@@ -203,7 +209,8 @@ allprojects {
                     url = uri("https://maven.pkg.github.com/modelix/modelix")
                     credentials {
                         username = project.findProperty("gpr.user") as? String ?: System.getenv("GITHUB_ACTOR")
-                        password = project.findProperty("gpr.universalkey") as? String ?: System.getenv("GHP_UNIVERSAL_TOKEN")
+                        password =
+                            project.findProperty("gpr.universalkey") as? String ?: System.getenv("GHP_UNIVERSAL_TOKEN")
                     }
                 } else {
                     url = uri("https://maven.pkg.github.com/modelix/modelix.core")

detekt-rules/build.gradle.kts

@@ -0,0 +1,15 @@
+plugins {
+    kotlin("jvm")
+}
+
+dependencies {
+    compileOnly(libs.detekt.api)
+
+    testImplementation(libs.junit)
+    testImplementation(libs.detekt.test)
+    testImplementation(kotlin("test"))
+}
+
+tasks.test {
+    useJUnitPlatform()
+}

detekt-rules/src/main/kotlin/org/modelix/detekt/AvoidNonNullAssertionOperator.kt

@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2024.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.modelix.detekt
+
+import io.gitlab.arturbosch.detekt.api.CodeSmell
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.Debt
+import io.gitlab.arturbosch.detekt.api.Entity
+import io.gitlab.arturbosch.detekt.api.Issue
+import io.gitlab.arturbosch.detekt.api.Rule
+import io.gitlab.arturbosch.detekt.api.Severity
+import org.jetbrains.kotlin.psi.KtReferenceExpression
+
+class AvoidNonNullAssertionOperator(config: Config) : Rule(config) {
+    override val issue = Issue(
+        javaClass.simpleName,
+        Severity.Warning,
+        "Reports instances of !! which should be replace by checkNotNull or requireNotNull " +
+            "with a descriptive message of why we assume a value to be not null",
+        Debt.FIVE_MINS,
+    )
+
+    override fun visitReferenceExpression(expression: KtReferenceExpression) {
+        super.visitReferenceExpression(expression)
+
+        if (expression.node.text == "!!") {
+            report(
+                CodeSmell(
+                    issue,
+                    Entity.from(expression.originalElement),
+                    "Prefer checkNotNull or requireNotNull with an explanatory message over the !! operator",
+                ),
+            )
+        }
+    }
+}

detekt-rules/src/main/kotlin/org/modelix/detekt/RuleSetProvider.kt

@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2024.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.modelix.detekt
+
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.RuleSet
+import io.gitlab.arturbosch.detekt.api.RuleSetProvider
+
+class RuleSetProvider : RuleSetProvider {
+    override val ruleSetId: String = "modelix"
+
+    override fun instance(config: Config): RuleSet = RuleSet(
+        ruleSetId,
+        listOf(
+            AvoidNonNullAssertionOperator(config),
+        ),
+    )
+}

detekt-rules/src/main/resources/META-INF/services/io.gitlab.arturbosch.detekt.api.RuleSetProvider

@@ -0,0 +1 @@
+org.modelix.detekt.RuleSetProvider

detekt-rules/src/main/resources/config.yml

@@ -0,0 +1,3 @@
+modelix:
+  AvoidNonNullAssertionOperator:
+    active: true

detekt-rules/src/test/kotlin/org/modelix/detekt/AvoidNonNullAssertionOperatorTest.kt

@@ -0,0 +1,22 @@
+package org.modelix.detekt
+
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.test.compileAndLint
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Test
+
+class AvoidNonNullAssertionOperatorTest {
+    @Test
+    fun `find double bang operator`() {
+        val code = """
+            fun main() {
+                val x: String? = null
+                val y = x!!
+            }
+        """
+
+        val findings = AvoidNonNullAssertionOperator(Config.empty).compileAndLint(code)
+
+        assertEquals(1, findings.size)
+    }
+}

gradle/libs.versions.toml

@@ -16,7 +16,7 @@ spotless = { id = "com.diffplug.spotless", version = "6.25.0" }
 modelix-mps-buildtools = { id = "org.modelix.mps.build-tools", version.ref = "modelixBuildtools" }
 dokka = {id = "org.jetbrains.dokka", version.ref = "dokka"}
 node = {id = "com.github.node-gradle.node", version = "7.0.2"}
-detekt = { id = "io.gitlab.arturbosch.detekt", version = "1.23.6" }
+detekt = { id = "io.gitlab.arturbosch.detekt", version.ref = "detekt" }
 npm-publish = { id = "dev.petuska.npm.publish", version = "3.4.2" }
 test-logger = { id = "com.adarshr.test-logger", version = "4.0.0" }
 shadow = { id = "com.github.johnrengelman.shadow", version = "8.1.1" }
@@ -37,6 +37,7 @@ modelixBuildtools="1.4.1"
 openapi = "7.5.0"
 micrometer = "1.12.5"
 dokka = "1.9.20"
+detekt = "1.23.6"
 
 [libraries]
 
@@ -116,3 +117,6 @@ modelix-buildtools-gradle = { group = "org.modelix.mps", name = "build-tools-gra
 modelix-buildtools-lib = { group = "org.modelix.mps", name = "build-tools-lib", version.ref = "modelixBuildtools"}
 
 micrometer-registry-prometheus = { group = "io.micrometer", name = "micrometer-registry-prometheus", version.ref = "micrometer"}
+
+detekt-api = { group = "io.gitlab.arturbosch.detekt", name= "detekt-api", version.ref = "detekt" }
+detekt-test = { group = "io.gitlab.arturbosch.detekt", name= "detekt-test", version.ref = "detekt" }