Add option to add a reason to ForbiddenImport (#4909)

* Improve tests

* Improve issue message

* Add utility to help testing ValueWithReason configurations

* Add support for reasons in ForbiddenImport

* Improve non-compilant

* Update detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImportSpec.kt

Co-authored-by: marschwar <marschwar@users.noreply.github.com>

* data class

Co-authored-by: marschwar <marschwar@users.noreply.github.com>

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImport.kt

@@ -10,14 +10,14 @@ import io.gitlab.arturbosch.detekt.api.Severity
 import io.gitlab.arturbosch.detekt.api.config
 import io.gitlab.arturbosch.detekt.api.internal.Configuration
 import io.gitlab.arturbosch.detekt.api.simplePatternToRegex
+import io.gitlab.arturbosch.detekt.api.valuesWithReason
 import org.jetbrains.kotlin.psi.KtImportDirective
 
 /**
  * This rule allows to set a list of forbidden imports. This can be used to discourage the use of unstable, experimental
  * or deprecated APIs. Detekt will then report all imports that are forbidden.
  *
  * <noncompliant>
- * package foo
  * import kotlin.jvm.JvmField
  * import kotlin.SinceKotlin
  * </noncompliant>
@@ -33,8 +33,8 @@ class ForbiddenImport(config: Config = Config.empty) : Rule(config) {
     )
 
     @Configuration("imports which should not be used")
-    private val imports: List<Regex> by config(emptyList<String>()) {
-        it.distinct().map(String::simplePatternToRegex)
+    private val imports: List<Forbidden> by config(valuesWithReason()) { list ->
+        list.map { Forbidden(it.value.simplePatternToRegex(), it.reason) }
     }
 
     @Configuration("reports imports which match the specified regular expression. For example `net.*R`.")
@@ -44,18 +44,29 @@ class ForbiddenImport(config: Config = Config.empty) : Rule(config) {
         super.visitImportDirective(importDirective)
 
         val import = importDirective.importedFqName?.asString().orEmpty()
-        if (imports.any { it.matches(import) } || containsForbiddenPattern(import)) {
-            report(
-                CodeSmell(
-                    issue,
-                    Entity.from(importDirective),
-                    "The import " +
-                        "$import has been forbidden in the Detekt config."
-                )
-            )
+
+        val forbidden = imports.find { it.import.matches(import) }
+        val reason = if (forbidden != null) {
+            if (forbidden.reason != null) {
+                "The import `$import` has been forbidden: ${forbidden.reason}"
+            } else {
+                defaultReason(import)
+            }
+        } else {
+            if (containsForbiddenPattern(import)) defaultReason(import) else null
+        }
+
+        if (reason != null) {
+            report(CodeSmell(issue, Entity.from(importDirective), reason))
         }
     }
 
+    private fun defaultReason(forbiddenImport: String): String {
+        return "The import `$forbiddenImport` has been forbidden in the Detekt config."
+    }
+
     private fun containsForbiddenPattern(import: String): Boolean =
         forbiddenPatterns.pattern.isNotEmpty() && forbiddenPatterns.containsMatchIn(import)
 }
+
+private data class Forbidden(val import: Regex, val reason: String?)

detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImportSpec.kt

@@ -1,7 +1,9 @@
 package io.gitlab.arturbosch.detekt.rules.style
 
+import io.gitlab.arturbosch.detekt.api.ValueWithReason
 import io.gitlab.arturbosch.detekt.test.TestConfig
 import io.gitlab.arturbosch.detekt.test.lint
+import io.gitlab.arturbosch.detekt.test.toConfig
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.DisplayName
 import org.junit.jupiter.api.Test
@@ -29,38 +31,54 @@ class ForbiddenImportSpec {
 
     @Test
     fun `should report nothing when imports are blank`() {
-        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "  "))).lint(code)
+        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("  ")))).lint(code)
         assertThat(findings).isEmpty()
     }
 
     @Test
     fun `should report nothing when imports do not match`() {
-        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "org.*"))).lint(code)
+        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("org.*")))).lint(code)
         assertThat(findings).isEmpty()
     }
 
     @Test
     @DisplayName("should report kotlin.* when imports are kotlin.*")
     fun reportKotlinWildcardImports() {
-        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "kotlin.*"))).lint(code)
+        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("kotlin.*")))).lint(code)
+        assertThat(findings)
+            .extracting("message")
+            .containsExactlyInAnyOrder(
+                "The import `kotlin.jvm.JvmField` has been forbidden in the Detekt config.",
+                "The import `kotlin.SinceKotlin` has been forbidden in the Detekt config.",
+            )
+    }
+
+    @Test
+    @DisplayName("should report kotlin.* when imports are kotlin.* with reasons")
+    fun reportKotlinWildcardImports2() {
+        val config = TestConfig(mapOf(IMPORTS to listOf(ValueWithReason("kotlin.*", "I'm just joking!").toConfig())))
+        val findings = ForbiddenImport(config).lint(code)
         assertThat(findings).hasSize(2)
+        assertThat(findings[0].message)
+            .isEqualTo("The import `kotlin.jvm.JvmField` has been forbidden: I'm just joking!")
+        assertThat(findings[1].message)
+            .isEqualTo("The import `kotlin.SinceKotlin` has been forbidden: I'm just joking!")
     }
 
     @Test
     @DisplayName("should report kotlin.SinceKotlin when specified via fully qualified name")
     fun reportKotlinSinceKotlinWhenFqdnSpecified() {
-        val findings =
-            ForbiddenImport(TestConfig(mapOf(IMPORTS to "kotlin.SinceKotlin"))).lint(code)
-        assertThat(findings).hasSize(1)
+        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("kotlin.SinceKotlin")))).lint(code)
+        assertThat(findings)
+            .hasSize(1)
     }
 
     @Test
     @DisplayName("should report kotlin.SinceKotlin and kotlin.jvm.JvmField when specified via fully qualified names")
     fun reportMultipleConfiguredImportsCommaSeparated() {
         val findings =
-            ForbiddenImport(TestConfig(mapOf(IMPORTS to "kotlin.SinceKotlin,kotlin.jvm.JvmField"))).lint(
-                code
-            )
+            ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("kotlin.SinceKotlin", "kotlin.jvm.JvmField"))))
+                .lint(code)
         assertThat(findings).hasSize(2)
     }
 
@@ -81,22 +99,22 @@ class ForbiddenImportSpec {
     @Test
     @DisplayName("should report kotlin.SinceKotlin when specified via kotlin.Since*")
     fun reportsKotlinSinceKotlinWhenSpecifiedWithWildcard() {
-        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "kotlin.Since*"))).lint(code)
+        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("kotlin.Since*")))).lint(code)
         assertThat(findings).hasSize(1)
     }
 
     @Test
     @DisplayName("should report all of com.example.R.string, net.example.R.dimen, and net.example.R.dimension")
     fun preAndPostWildcard() {
-        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "*.R.*"))).lint(code)
+        val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("*.R.*")))).lint(code)
         assertThat(findings).hasSize(3)
     }
 
     @Test
     @DisplayName("should report net.example.R.dimen but not net.example.R.dimension")
     fun doNotReportSubstringOfFqdn() {
         val findings =
-            ForbiddenImport(TestConfig(mapOf(IMPORTS to "net.example.R.dimen"))).lint(code)
+            ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("net.example.R.dimen")))).lint(code)
         assertThat(findings).hasSize(1)
     }
 
@@ -112,5 +130,9 @@ class ForbiddenImportSpec {
         val findings =
             ForbiddenImport(TestConfig(mapOf(FORBIDDEN_PATTERNS to "net.*R|com.*expiremental"))).lint(code)
         assertThat(findings).hasSize(2)
+        assertThat(findings[0].message)
+            .isEqualTo("The import `net.example.R.dimen` has been forbidden in the Detekt config.")
+        assertThat(findings[1].message)
+            .isEqualTo("The import `net.example.R.dimension` has been forbidden in the Detekt config.")
     }
 }

detekt-test/api/detekt-test.api

@@ -55,6 +55,10 @@ public final class io/gitlab/arturbosch/detekt/test/TestConfig$Companion {
 	public final fun invoke ([Lkotlin/Pair;)Lio/gitlab/arturbosch/detekt/test/TestConfig;
 }
 
+public final class io/gitlab/arturbosch/detekt/test/TestConfigKt {
+	public static final fun toConfig (Lio/gitlab/arturbosch/detekt/api/ValueWithReason;)Ljava/util/Map;
+}
+
 public final class io/gitlab/arturbosch/detekt/test/ThresholdedCodeSmellAssert : org/assertj/core/api/AbstractAssert {
 	public fun <init> (Lio/gitlab/arturbosch/detekt/api/ThresholdedCodeSmell;)V
 	public final fun hasThreshold (I)V

detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/TestConfig.kt

@@ -1,6 +1,7 @@
 package io.gitlab.arturbosch.detekt.test
 
 import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.ValueWithReason
 import io.gitlab.arturbosch.detekt.core.config.tryParseBasedOnDefault
 import io.gitlab.arturbosch.detekt.core.config.valueOrDefaultInternal
 
@@ -53,3 +54,7 @@ open class TestConfig(
         operator fun invoke(vararg pairs: Pair<String, Any>) = TestConfig(mapOf(*pairs))
     }
 }
+
+fun ValueWithReason.toConfig(): Map<String, String?> {
+    return mapOf("value" to value, "reason" to reason)
+}