Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for fallback property #3675

Merged
merged 9 commits into from
May 7, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,30 @@ import io.gitlab.arturbosch.detekt.api.ConfigAware
import kotlin.properties.ReadOnlyProperty
import kotlin.reflect.KProperty

fun config(defaultValue: String): ReadOnlyProperty<ConfigAware, String> = simpleConfig(defaultValue)
fun config(defaultValue: Int): ReadOnlyProperty<ConfigAware, Int> = simpleConfig(defaultValue)
fun config(defaultValue: Long): ReadOnlyProperty<ConfigAware, Long> = simpleConfig(defaultValue)
fun config(defaultValue: Boolean): ReadOnlyProperty<ConfigAware, Boolean> = simpleConfig(defaultValue)
fun config(defaultValue: List<String>): ReadOnlyProperty<ConfigAware, List<String>> = ListConfigProperty(defaultValue)

private fun <T : Any> simpleConfig(defaultValue: T): ReadOnlyProperty<ConfigAware, T> =
fun <T : Any> config(defaultValue: T): ReadOnlyProperty<ConfigAware, T> =
SimpleConfigProperty(defaultValue)

fun <T : Any> configWithFallback(fallbackProperty: String, defaultValue: T): ReadOnlyProperty<ConfigAware, T> =
FallbackConfigProperty(fallbackProperty, defaultValue)
marschwar marked this conversation as resolved.
Show resolved Hide resolved

fun configList(defaultValue: List<String>): ReadOnlyProperty<ConfigAware, List<String>> =
ListConfigProperty(defaultValue)

private class SimpleConfigProperty<T : Any>(private val defaultValue: T) : ReadOnlyProperty<ConfigAware, T> {
override fun getValue(thisRef: ConfigAware, property: KProperty<*>): T {
return thisRef.valueOrDefault(property.name, defaultValue)
}
}

private class FallbackConfigProperty<T : Any>(
private val fallbackProperty: String,
private val defaultValue: T
) : ReadOnlyProperty<ConfigAware, T> {
override fun getValue(thisRef: ConfigAware, property: KProperty<*>): T {
return thisRef.valueOrDefault(property.name, thisRef.valueOrDefault(fallbackProperty, defaultValue))
}
}

private class ListConfigProperty(private val defaultValue: List<String>) : ReadOnlyProperty<ConfigAware, List<String>> {
override fun getValue(thisRef: ConfigAware, property: KProperty<*>): List<String> {
return thisRef.valueOrDefaultCommaSeparated(property.name, defaultValue)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package io.gitlab.arturbosch.detekt.api.internal

import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.ConfigAware
import io.gitlab.arturbosch.detekt.api.RuleId
import io.gitlab.arturbosch.detekt.test.TestConfig
import org.assertj.core.api.Assertions.assertThat
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe

class ConfigPropertySpec : Spek({

describe("Config property delegate") {
context("string property") {
val configValue = "value"
val defaultValue = "default"
val subject by memoized {
object : TestConfigAware("present" to configValue) {
val present: String by config(defaultValue)
val notPresent: String by config(defaultValue)
}
}
it("uses the value provided in config if present") {
assertThat(subject.present).isEqualTo(configValue)
}
it("uses the default value if not present") {
assertThat(subject.notPresent).isEqualTo(defaultValue)
}
}
context("Int property") {
val configValue = 99
val defaultValue = -1
val subject by memoized {
object : TestConfigAware("present" to configValue) {
val present: Int by config(defaultValue)
val notPresent: Int by config(defaultValue)
}
}
it("uses the value provided in config if present") {
assertThat(subject.present).isEqualTo(configValue)
}
it("uses the default value if not present") {
assertThat(subject.notPresent).isEqualTo(defaultValue)
}
}
context("Int property defined as string") {
val configValue = 99
val subject by memoized {
object : TestConfigAware("present" to "$configValue") {
val present: Int by config(-1)
}
}
it("uses the value provided in config if present") {
assertThat(subject.present).isEqualTo(configValue)
}
}
context("String list property") {
val defaultValue by memoized { listOf("x") }
val subject by memoized {
object : TestConfigAware("present" to "a,b,c") {
val present: List<String> by configList(defaultValue)
val notPresent: List<String> by configList(defaultValue)
}
}
it("uses the value provided in config if present") {
assertThat(subject.present).isEqualTo(listOf("a", "b", "c"))
}
it("uses the default value if not present") {
assertThat(subject.notPresent).isEqualTo(defaultValue)
}
}
context("Int property with fallback") {
val configValue = 99
val defaultValue = 0
val fallbackValue = -1
val subject by memoized {
object : TestConfigAware("present" to "$configValue", "fallback" to fallbackValue) {
val present: Int by configWithFallback("fallback", defaultValue)
val notPresentWithFallback: Int by configWithFallback("fallback", defaultValue)
val notPresentFallbackMissing: Int by configWithFallback("missing", defaultValue)
}
}
it("uses the value provided in config if present") {
assertThat(subject.present).isEqualTo(configValue)
}
it("uses the value from fallback property if value is missing and fallback exists") {
assertThat(subject.notPresentWithFallback).isEqualTo(fallbackValue)
}
it("uses the default value if not present") {
assertThat(subject.notPresentFallbackMissing).isEqualTo(defaultValue)
}
}
}
})

private open class TestConfigAware(private vararg val data: Pair<String, Any>) : ConfigAware {
override val ruleId: RuleId
get() = "test"
override val ruleSetConfig: Config
get() = TestConfig(data.toMap())
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
package io.gitlab.arturbosch.detekt.generator.collection

import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ConfigWithFallbackSupport.FALLBACK_DELEGATE_NAME
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ConfigWithFallbackSupport.getFallbackPropertyName
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ConfigWithFallbackSupport.isFallbackConfigDelegate
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ConfigWithFallbackSupport.isUsingInvalidFallbackReference
import io.gitlab.arturbosch.detekt.generator.collection.exception.InvalidDocumentationException
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtConstantExpression
import org.jetbrains.kotlin.psi.KtElement
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtObjectDeclaration
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtPropertyDelegate
Expand Down Expand Up @@ -77,7 +82,13 @@ class ConfigurationCollector {
}
}
if (!isInitializedWithConfigDelegate()) {
invalidDocumentation { "'$name' is not using the '$DELEGATE_NAME' delegate" }
invalidDocumentation { "'$name' is not using one of the config property delegates ($DELEGATE_NAMES)" }
}
if (isFallbackConfigDelegate()) {
val fallbackPropertyName = getFallbackPropertyName()
if (isUsingInvalidFallbackReference(properties, fallbackPropertyName)) {
invalidDocumentation { "The fallback property '$fallbackPropertyName' is missing for property '$name'" }
}
}

val propertyName: String = checkNotNull(name)
Expand All @@ -95,10 +106,8 @@ class ConfigurationCollector {
}

private fun KtPropertyDelegate.getDefaultValueAsString(): String {
val delegateArgument = checkNotNull(
(expression as KtCallExpression).valueArguments[0].getArgumentExpression()
)
val listDeclarationForDefault = delegateArgument.getListDeclarationOrNull()
val defaultValueExpression = getDefaultValueExpression()
val listDeclarationForDefault = defaultValueExpression.getListDeclarationOrNull()
if (listDeclarationForDefault != null) {
return listDeclarationForDefault.valueArguments.map {
val value = constantsByName[it.text] ?: it.text
Expand All @@ -107,14 +116,49 @@ class ConfigurationCollector {
}

val defaultValueOrConstantName = checkNotNull(
delegateArgument.text?.withoutQuotes()
defaultValueExpression.text?.withoutQuotes()
)
val defaultValue = constantsByName[defaultValueOrConstantName] ?: defaultValueOrConstantName
return property.formatDefaultValueAccordingToType(defaultValue)
}

private fun KtPropertyDelegate.getDefaultValueExpression(): KtExpression {
val callExpression = expression as KtCallExpression
val arguments = callExpression.valueArguments
if (arguments.size == 1) {
return checkNotNull(arguments[0].getArgumentExpression())
}
val defaultArgument = arguments
.find { it.getArgumentName()?.text == DEFAULT_VALUE_ARGUMENT_NAME }
?: arguments.last()
return checkNotNull(defaultArgument.getArgumentExpression())
}

private object ConfigWithFallbackSupport {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today I learned: We can define a non-companion object inside a class.

const val FALLBACK_DELEGATE_NAME = "configWithFallback"
private const val FALLBACK_ARGUMENT_NAME = "fallbackProperty"

fun KtProperty.isFallbackConfigDelegate(): Boolean =
delegate?.expression?.referenceExpression()?.text == FALLBACK_DELEGATE_NAME

fun KtProperty.getFallbackPropertyName(): String {
val callExpression = delegate?.expression as KtCallExpression
val arguments = callExpression.valueArguments
val fallbackArgument = arguments
.find { it.getArgumentName()?.text == FALLBACK_ARGUMENT_NAME }
?: arguments.first()
return checkNotNull(fallbackArgument.getArgumentExpression()?.text?.withoutQuotes())
}

fun isUsingInvalidFallbackReference(properties: List<KtProperty>, fallbackPropertyName: String) =
properties.filter { it.isInitializedWithConfigDelegate() }.none { it.name == fallbackPropertyName }
}

companion object {
private const val DELEGATE_NAME = "config"
private const val SIMPLE_DELEGATE_NAME = "config"
private const val LIST_DELEGATE_NAME = "configList"
private val DELEGATE_NAMES = listOf(SIMPLE_DELEGATE_NAME, LIST_DELEGATE_NAME, FALLBACK_DELEGATE_NAME)
private const val DEFAULT_VALUE_ARGUMENT_NAME = "defaultValue"
private const val LIST_OF = "listOf"
private const val EMPTY_LIST = "emptyList"
private val LIST_CREATORS = setOf(LIST_OF, EMPTY_LIST)
Expand All @@ -139,7 +183,7 @@ class ConfigurationCollector {
findDescendantOfType { it.isListDeclaration() }

private fun KtProperty.isInitializedWithConfigDelegate(): Boolean =
delegate?.expression?.referenceExpression()?.text == DELEGATE_NAME
delegate?.expression?.referenceExpression()?.text in DELEGATE_NAMES

private fun KtProperty.hasSupportedType(): Boolean =
declaredTypeOrNull in SUPPORTED_TYPES
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ class RuleCollectorSpec : Spek({
assertThat(items[0].aliases).isEqualTo("RULE, RULE2")
}

describe("collects configuration options") {
describe("using annotation") {
context("collects configuration options") {
context("using annotation") {
it("contains no configuration options by default") {
val code = """
/**
Expand Down Expand Up @@ -221,7 +221,7 @@ class RuleCollectorSpec : Spek({
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: List<String> by config(
private val config: List<String> by configList(
listOf(
"a",
"b"
Expand Down Expand Up @@ -327,10 +327,10 @@ class RuleCollectorSpec : Spek({
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config1: List<String> by config(DEFAULT_CONFIG_VALUE)
private val config1: List<String> by configList(DEFAULT_CONFIG_VALUE)

@Configuration("description")
private val config2: List<String> by config(listOf(DEFAULT_CONFIG_VALUE_A, "b"))
private val config2: List<String> by configList(listOf(DEFAULT_CONFIG_VALUE_A, "b"))

companion object {
private val DEFAULT_CONFIG_VALUE = listOf("a", "b")
Expand All @@ -351,10 +351,10 @@ class RuleCollectorSpec : Spek({
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config1: List<String> by config(listOf())
private val config1: List<String> by configList(listOf())

@Configuration("description")
private val config2: List<String> by config(emptyList())
private val config2: List<String> by configList(emptyList())

companion object {
private val DEFAULT_CONFIG_VALUE_A = "a"
Expand Down Expand Up @@ -445,9 +445,67 @@ class RuleCollectorSpec : Spek({
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
context("fallback property") {
it("extracts default value") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val prop: Int by config(1)
@Configuration("description")
private val config1: Int by configWithFallback("prop", 99)
@Configuration("description")
private val config2: Int by configWithFallback(fallbackProperty = "prop", defaultValue = 99)
@Configuration("description")
private val config3: Int by configWithFallback(defaultValue = 99, fallbackProperty = "prop")
}
"""
val items = subject.run(code)
val fallbackProperties = items[0].configuration.filter { it.name.startsWith("config") }
assertThat(fallbackProperties).hasSize(3)
assertThat(fallbackProperties.map { it.defaultValue }).containsOnly("99")
}

it("reports an error if the property to fallback on does not exist") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: Int by configWithFallback("prop", 99)
}
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy {
subject.run(
code
)
}
}

it("reports an error if the property to fallback on exists but is not a config property") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
private val prop: Int = 1
@Configuration("description")
private val config: Int by configWithFallback("prop", 99)
}
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy {
subject.run(
code
)
}
}
}
}

describe("as part of kdoc") {
context("as part of kdoc") {
it("contains no configuration options by default") {
val code = """
/**
Expand Down Expand Up @@ -532,7 +590,7 @@ class RuleCollectorSpec : Spek({
}
}

describe("collects type resolution information") {
context("collects type resolution information") {
it("has no type resolution by default") {
val code = """
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import io.gitlab.arturbosch.detekt.api.ThresholdedCodeSmell
import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
import io.gitlab.arturbosch.detekt.api.internal.Configuration
import io.gitlab.arturbosch.detekt.api.internal.config
import io.gitlab.arturbosch.detekt.api.internal.configList
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtNamedFunction
Expand Down Expand Up @@ -53,7 +54,7 @@ class ComplexMethod(config: Config = Config.empty) : Rule(config) {
private val ignoreNestingFunctions: Boolean by config(false)

@Configuration("Comma separated list of function names which add complexity.")
private val nestingFunctions: List<String> by config(DEFAULT_NESTING_FUNCTIONS)
private val nestingFunctions: List<String> by configList(DEFAULT_NESTING_FUNCTIONS)

override val issue = Issue(
"ComplexMethod",
Expand Down