Allow users and rule authors to specify a reason per configured value (…

…#4611)

* implement first idea on values with reasons

* have ExplainedValues implement List by delegation

* Parse explained value defaults

* fix error message

* [WIP] support ExplainedValues in yaml

* print markdown value in DefaultValue directly

* quote values in yaml maps

* Update detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/ConfigProperty.kt

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

* Update detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/ConfigProperty.kt

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

* add test case for missing reason

* fail fast in case of invalid configuration

* rename explainedValues to valuesWithReason

* add api documentation

* resolve and suppress code smells

* suppress UNCHECKED_CAST

* merge RuleCollectorSpec

* reformat spec

* fix indentation of test code

Co-authored-by: Markus Schwarz <post@markus-schwarz.net>
Co-authored-by: Brais Gabín <braisgabin@gmail.com>

detekt-api/api/detekt-api.api

@@ -616,3 +616,31 @@ public abstract interface annotation class io/gitlab/arturbosch/detekt/api/Unsta
 	public abstract fun reason ()Ljava/lang/String;
 }
 
+public final class io/gitlab/arturbosch/detekt/api/ValueWithReason {
+	public fun <init> (Ljava/lang/String;Ljava/lang/String;)V
+	public synthetic fun <init> (Ljava/lang/String;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public final fun component1 ()Ljava/lang/String;
+	public final fun component2 ()Ljava/lang/String;
+	public final fun copy (Ljava/lang/String;Ljava/lang/String;)Lio/gitlab/arturbosch/detekt/api/ValueWithReason;
+	public static synthetic fun copy$default (Lio/gitlab/arturbosch/detekt/api/ValueWithReason;Ljava/lang/String;Ljava/lang/String;ILjava/lang/Object;)Lio/gitlab/arturbosch/detekt/api/ValueWithReason;
+	public fun equals (Ljava/lang/Object;)Z
+	public final fun getReason ()Ljava/lang/String;
+	public final fun getValue ()Ljava/lang/String;
+	public fun hashCode ()I
+	public fun toString ()Ljava/lang/String;
+}
+
+public final class io/gitlab/arturbosch/detekt/api/ValuesWithReason : java/lang/Iterable, kotlin/jvm/internal/markers/KMappedMarker {
+	public final fun copy (Ljava/util/List;)Lio/gitlab/arturbosch/detekt/api/ValuesWithReason;
+	public static synthetic fun copy$default (Lio/gitlab/arturbosch/detekt/api/ValuesWithReason;Ljava/util/List;ILjava/lang/Object;)Lio/gitlab/arturbosch/detekt/api/ValuesWithReason;
+	public fun equals (Ljava/lang/Object;)Z
+	public fun hashCode ()I
+	public fun iterator ()Ljava/util/Iterator;
+	public fun toString ()Ljava/lang/String;
+}
+
+public final class io/gitlab/arturbosch/detekt/api/ValuesWithReasonKt {
+	public static final fun valuesWithReason (Ljava/util/List;)Lio/gitlab/arturbosch/detekt/api/ValuesWithReason;
+	public static final fun valuesWithReason ([Lkotlin/Pair;)Lio/gitlab/arturbosch/detekt/api/ValuesWithReason;
+}
+

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/ConfigProperty.kt

@@ -111,14 +111,8 @@ fun <T : Any, U : Any> configWithAndroidVariants(
 private fun <T : Any> getValueOrDefault(configAware: ConfigAware, propertyName: String, defaultValue: T): T {
     @Suppress("UNCHECKED_CAST")
     return when (defaultValue) {
-        is List<*> -> {
-            if (defaultValue.all { it is String }) {
-                val defaultValueAsListOfStrings = defaultValue as List<String>
-                configAware.valueOrDefaultCommaSeparated(propertyName, defaultValueAsListOfStrings) as T
-            } else {
-                error("Only lists of strings are supported. '$propertyName' is invalid. ")
-            }
-        }
+        is ValuesWithReason -> configAware.getValuesWithReasonOrDefault(propertyName, defaultValue) as T
+        is List<*> -> configAware.getListOrDefault(propertyName, defaultValue) as T
         is String,
         is Boolean,
         is Int -> configAware.valueOrDefault(propertyName, defaultValue)
@@ -129,6 +123,45 @@ private fun <T : Any> getValueOrDefault(configAware: ConfigAware, propertyName:
     }
 }
 
+private fun ConfigAware.getListOrDefault(propertyName: String, defaultValue: List<*>): List<String> {
+    return if (defaultValue.all { it is String }) {
+        @Suppress("UNCHECKED_CAST")
+        val defaultValueAsListOfStrings = defaultValue as List<String>
+        valueOrDefaultCommaSeparated(propertyName, defaultValueAsListOfStrings)
+    } else {
+        error("Only lists of strings are supported. '$propertyName' is invalid. ")
+    }
+}
+
+private fun ConfigAware.getValuesWithReasonOrDefault(
+    propertyName: String,
+    defaultValue: ValuesWithReason
+): ValuesWithReason {
+    val valuesAsList: List<*> = valueOrNull(propertyName) ?: return defaultValue
+    if (valuesAsList.all { it is String }) {
+        return ValuesWithReason(values = valuesAsList.map { ValueWithReason(it as String) })
+    }
+    if (valuesAsList.all { it is Map<*, *> }) {
+        return ValuesWithReason(
+            valuesAsList
+                .map { it as Map<*, *> }
+                .map { dict ->
+                    try {
+                        ValueWithReason(
+                            value = dict["value"] as String,
+                            reason = dict["reason"] as String?
+                        )
+                    } catch (e: ClassCastException) {
+                        throw Config.InvalidConfigurationError(e)
+                    } catch (@Suppress("TooGenericExceptionCaught") e: NullPointerException) {
+                        throw Config.InvalidConfigurationError(e)
+                    }
+                }
+        )
+    }
+    error("Only lists of strings or maps with keys 'value' and 'reason' are supported. '$propertyName' is invalid.")
+}
+
 private abstract class MemoizedConfigProperty<U : Any> : ReadOnlyProperty<ConfigAware, U> {
     private var value: U? = null
 

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/ValuesWithReason.kt

@@ -0,0 +1,42 @@
+package io.gitlab.arturbosch.detekt.api
+
+/**
+ * This factory method can be used by rule authors to specify one or many configuration values along with an
+ * explanation for each value. For example:
+ *
+ * ```kotlin
+ *  @Configuration("imports which should not be used")
+ *  private val imports: ValuesWithReason by config(
+ *      valuesWithReason("org.junit.Test" to "Do not use Junit4. Use org.junit.jupiter.api.Test instead.")
+ *  )
+ * ```
+ *
+ * Note that the [config] property delegate only supports the factory methods when defining [ValuesWithReason].
+ */
+fun valuesWithReason(vararg values: Pair<String, String?>): ValuesWithReason {
+    return valuesWithReason(values.map { ValueWithReason(it.first, it.second) })
+}
+
+/**
+ * This factory method can be used by rule authors to specify one or many configuration values along with an
+ * explanation for each value.
+ *
+ * Note that the [config] property delegate only supports the factory methods when defining [ValuesWithReason].
+ */
+fun valuesWithReason(values: List<ValueWithReason>): ValuesWithReason {
+    return ValuesWithReason(values)
+}
+
+/**
+ * [ValuesWithReason] is essentially the same as [List] of [ValueWithReason]. Due to type erasure we cannot use the
+ * list directly. Instances of this type should always created using the [valuesWithReason] factory method.
+ */
+data class ValuesWithReason internal constructor(private val values: List<ValueWithReason>) :
+    Iterable<ValueWithReason> by values
+
+/**
+ * A ValueWithReason represents a single configuration value that may have an explanation as to why it is used.
+ * @property value the actual value that is configured
+ * @property reason an optional explanation for the configured value
+ */
+data class ValueWithReason(val value: String, val reason: String? = null)

detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/ConfigPropertySpec.kt

@@ -3,6 +3,7 @@ package io.gitlab.arturbosch.detekt.api
 import io.gitlab.arturbosch.detekt.test.TestConfig
 import org.assertj.core.api.Assertions.assertThat
 import org.assertj.core.api.Assertions.assertThatThrownBy
+import org.assertj.core.api.Assertions.tuple
 import org.junit.jupiter.api.Nested
 import org.junit.jupiter.api.Test
 import java.util.concurrent.atomic.AtomicInteger
@@ -65,6 +66,104 @@ class ConfigPropertySpec {
                     assertThat(subject.present).isEqualTo(configValue)
                 }
             }
+
+            @Nested
+            inner class `ValuesWithReason property` {
+                private val defaultValue = valuesWithReason("aValue" to "aReason")
+
+                @Nested
+                inner class `value defined as list` {
+                    private val subject = object : TestConfigAware("present" to listOf("a", "b", "c")) {
+                        val present: ValuesWithReason by config(defaultValue)
+                        val notPresent: ValuesWithReason by config(defaultValue)
+                    }
+
+                    @Test
+                    fun `uses the value provided in config if present`() {
+                        assertThat(subject.present)
+                            .extracting(ValueWithReason::value, ValueWithReason::reason)
+                            .containsExactly(tuple("a", null), tuple("b", null), tuple("c", null))
+                    }
+
+                    @Test
+                    fun `uses the default value if not present`() {
+                        assertThat(subject.notPresent).isEqualTo(defaultValue)
+                    }
+                }
+
+                @Nested
+                inner class `value defined as list of maps` {
+                    private val subject = object : TestConfigAware(
+                        "present" to listOf(
+                            mapOf("value" to "a", "reason" to "reasonA"),
+                            mapOf("value" to "b", "reason" to null),
+                            mapOf("value" to "c"),
+                        )
+                    ) {
+                        val present: ValuesWithReason by config(defaultValue)
+                        val notPresent: ValuesWithReason by config(defaultValue)
+                    }
+
+                    @Test
+                    fun `uses the value provided in config if present`() {
+                        assertThat(subject.present)
+                            .extracting(ValueWithReason::value, ValueWithReason::reason)
+                            .containsExactly(
+                                tuple("a", "reasonA"),
+                                tuple("b", null),
+                                tuple("c", null)
+                            )
+                    }
+
+                    @Test
+                    fun `uses the default value if not present`() {
+                        assertThat(subject.notPresent).isEqualTo(defaultValue)
+                    }
+                }
+
+                @Nested
+                inner class `value defined as list of maps with invalid data` {
+
+                    @Test
+                    fun `value missing`() {
+                        assertThatThrownBy {
+                            object : TestConfigAware(
+                                "present" to listOf(
+                                    mapOf("reason" to "reason")
+                                )
+                            ) {
+                                val present: ValuesWithReason by config(defaultValue)
+                            }.present
+                        }.isInstanceOf(Config.InvalidConfigurationError::class.java)
+                    }
+
+                    @Test
+                    fun `value with an invalid type`() {
+                        assertThatThrownBy {
+                            object : TestConfigAware(
+                                "present" to listOf(
+                                    mapOf("value" to 42, "reason" to "reason")
+                                )
+                            ) {
+                                val present: ValuesWithReason by config(defaultValue)
+                            }.present
+                        }.isInstanceOf(Config.InvalidConfigurationError::class.java)
+                    }
+
+                    @Test
+                    fun `reason with an invalid type`() {
+                        assertThatThrownBy {
+                            object : TestConfigAware(
+                                "present" to listOf(
+                                    mapOf("value" to "a", "reason" to 42)
+                                )
+                            ) {
+                                val present: ValuesWithReason by config(defaultValue)
+                            }.present
+                        }.isInstanceOf(Config.InvalidConfigurationError::class.java)
+                    }
+                }
+            }
         }
 
         @Nested

detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/config/YamlConfigSpec.kt

@@ -126,6 +126,46 @@ class YamlConfigSpec {
             assertThat(config).isNotNull
         }
 
+        @Nested
+        inner class `Values with reason` {
+            private val config = YamlConfig.load(resourceAsPath("values-with-reason.yml"))
+
+            @Test
+            fun `can be parsed`() {
+                assertThat(config).isNotNull
+            }
+
+            @Test
+            fun `supports lists`() {
+                val actualAsList: List<*>? = config
+                    .subConfig("style")
+                    .subConfig("AsList")
+                    .valueOrNull("values")
+                assertThat(actualAsList).hasSize(3)
+            }
+
+            @Test
+            fun `supports dictionaries`() {
+                val actualAsMap: List<Map<*, *>>? = config
+                    .subConfig("style")
+                    .subConfig("AsListOfMaps")
+                    .valueOrNull("values")
+                assertThat(actualAsMap)
+                    .hasSize(3)
+            }
+
+            @Test
+            fun `supports empty dictionaries`() {
+                val actualAsMap: List<Map<*, *>>? = config
+                    .subConfig("style")
+                    .subConfig("EmptyListOfMaps")
+                    .valueOrNull("values")
+                assertThat(actualAsMap)
+                    .isNotNull
+                    .isEmpty()
+            }
+        }
+
         @Test
         fun `throws an exception on an non-existing file`() {
             val path = Paths.get("doesNotExist.yml")

detekt-core/src/test/resources/values-with-reason.yml

@@ -0,0 +1,15 @@
+style:
+  AsList:
+    values:
+      - a
+      - b
+      - c
+  AsListOfMaps:
+    values:
+      - value: a
+        reason: reason A
+      - value: b
+      - value: c
+        reason: reason C
+  EmptyListOfMaps:
+    values: []

detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/Configuration.kt

@@ -8,8 +8,4 @@ data class Configuration(
     val deprecated: String?
 ) {
     fun isDeprecated() = deprecated != null
-
-    fun isDefaultValueNonEmptyList() = defaultValue.isNonEmptyList()
-
-    fun getDefaultValueAsList(): List<String> = defaultValue.getAsList()
 }