Rule Configuration using annotations (#3637)

* Create initial idea for @Configuration annotation
* Use config parameter constants again
* Generate documentation from @Configuration
* Introduce config delegate and extract default value from it
* Exclude rules configured with annotation from checks
* Remove support for factory methods in RuleSetProviderCollector
* Add support for lists that are empty by default
* Update documentation for rule contribution
* Restrict config delegate to supported types
* Update .github/CONTRIBUTING.md

Co-authored-by: Brais Gabín <braisgabin@gmail.com>
Co-authored-by: Markus Schwarz <post@markus-schwarz.net>
Co-authored-by: Chao Zhang <zhangchao6865@gmail.com>
This commit is contained in:
marschwar
2021-04-09 01:34:22 +02:00
committed by GitHub
parent 7ae2a96d81
commit cbee5e7e2e
13 changed files with 838 additions and 255 deletions

View File

@@ -14,48 +14,12 @@
- ... do not forget to add the new rule to a `RuleSetProvider` (e.g. StyleGuideProvider)
- ... do not forget to write a description for the issue of the new rule.
- Add the correct KDoc to the Rule class. This KDoc is used to generate wiki pages and the `default-detekt-config.yml`
automatically. The format of the KDoc should be as follows:
```kotlin
/**
* This is a nice description for the rule, explaining what it checks, why it exists and how violations can be
* solved.
*
* <noncompliant>
* // add the non-compliant code example here
* </noncompliant>
*
* <compliant>
* // add the compliant code example here
* </compliant>
*
* @configuration name - Description for the configuration option (default: `whatever should be the default`)
*/
class SomeRule : Rule {
}
```
The description should be as detailed as possible as it will act as the documentation of the rule. Add links to
references that explain the rationale for the rule if possible.
The `<noncompliant>` and `<compliant>` code examples should be added right after the description of the rule.
The `@configuration` tag should follow the correct pattern. The name of the configuration option *has* to match the
actual name used in the code, otherwise an invalid `default-detekt-config.yml` will be generated and the rule won't
function correctly by default.
The default value will be taken as is for the configuration option and pasted into the `default-detekt-config.yml`.
A `@configuration` tag as described above will translate to a rule entry in the `default-detekt-config.yml`:
```yml
SomeRule:
active: false
name: whatever should be the default
```
- ... add the [correct KDoc](#contents-and-structure-of-a-rules-kdoc) and [annotations](#rule-annotations) to your `Rule` class. This is used to generate documentation pages and the `default-detekt-config.yml` automatically.
- ... do not forget to test the new rule and/or add tests for any changes made to a rule.
Run detekt on itself and other kotlin projects with the `--run-rule RuleSet:RuleId` option to test your rule in isolation.
Make use of the `scripts/get_analysis_projects.groovy` script to automatically establish a set of analysis projects.
- ... run `./gradlew generateDocumentation` to add your rule and its config options to the `default-detekt-config.yml`.
- ... do not forget to run `./gradlew build`. This will execute tests locally.
- ... do not forget to test the new rule and/or add tests for any changes made to a rule. Run detekt on itself and other
kotlin projects with the `--run-rule RuleSet:RuleId` option to test your rule in isolation. Make use of
the `scripts/get_analysis_projects.groovy` script to automatically establish a set of analysis projects.
- To print the AST of sources you can pass the `--print-ast` flag to the CLI which will print each
Kotlin files AST. This can be helpful when implementing and debugging rules.
- To view the AST (PSI) of your source code you can use the [PSI Viewer plugin](https://plugins.jetbrains.com/plugin/227-psiviewer) for IntelliJ.
@@ -63,7 +27,58 @@ Kotlin files AST. This can be helpful when implementing and debugging rules.
After some time and testing there is a chance this rule will become active on default.
Rules annotated with `@ActiveByDefault` will be marked as active in the`default-detekt-config.yml`.
#### Contents and structure of a rule's KDoc
```kotlin
/**
* This is a nice description for the rule explaining what it checks, why it
* exists and how violations can be solved.
*
* <noncompliant>
* // add the non-compliant code example here
* </noncompliant>
*
* <compliant>
* // add the compliant code example here
* </compliant>
*/
class SomeRule(config: Config = Config.empty) : Rule(config) {
}
```
The description should be as detailed as possible as it will act as the documentation of the rule. Add links to
references that explain the rationale for the rule if possible.
The `<noncompliant>` and `<compliant>` code examples should be added right after the description of the rule.
#### Rule annotations
```kotlin
@ActiveByDefault(since = "1.0.0")
@RequiresTypeResolution
class SomeRule(config: Config = Config.empty) : Rule(config) {
@Configuration("This is the description for the configuration parameter below.")
private val name: String by config(default = "whatever should be the default")
}
```
Use the `@Configuration` annotation in combination with the `config` delegate to create a configurable property for your rule. The name of the property will become the key and the provided default will be the value in the `default-detekt-config.yml`. All information are also used to generate the rule documentation in the wiki.
Note that a property that is marked with `@Configuration` must use the config delegate (and vice versa).
Rules annotated with `@ActiveByDefault` will be marked as active in the `default-detekt-config.yml`. Generally this will not be the case for new rules.
A rule that requires type resolution must be marked with `@RequiresTypeResolution`. See [the type resolution wiki page](../docs/pages/gettingstarted/type-resolution.md) for more detail on this topic.
The rule defined above will translate to a rule entry in the `default-detekt-config.yml`:
```yml
SomeRule:
active: true
name: 'whatever should be the default'
```
### When updating the website ...

View File

@@ -0,0 +1,26 @@
package io.gitlab.arturbosch.detekt.api.internal
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> =
SimpleConfigProperty(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 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)
}
}

View File

@@ -0,0 +1,7 @@
package io.gitlab.arturbosch.detekt.api.internal
@Target(AnnotationTarget.PROPERTY)
@Retention(AnnotationRetention.RUNTIME)
annotation class Configuration(
val description: String,
)

View File

@@ -87,7 +87,7 @@ complexity:
ignoreSingleWhenExpression: false
ignoreSimpleWhenEntries: false
ignoreNestingFunctions: false
nestingFunctions: [run, let, apply, with, also, use, forEach, isNotNull, ifNull]
nestingFunctions: ['run', 'let', 'apply', 'with', 'also', 'use', 'forEach', 'isNotNull', 'ifNull']
LabeledExpression:
active: false
ignoredLabels: []

View File

@@ -2,6 +2,7 @@ package io.gitlab.arturbosch.detekt.core
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.internal.Configuration
import io.gitlab.arturbosch.detekt.core.config.DefaultConfig
import io.gitlab.arturbosch.detekt.core.config.YamlConfig
import org.assertj.core.api.Assertions.assertThat
@@ -45,6 +46,8 @@ class ConfigAssert(
}
private fun checkOptions(ymlOptions: HashMap<String, *>, ruleClass: Class<out Rule>) {
if (ruleClass.isConfiguredWithAnnotations()) return
val configFields = ruleClass.declaredFields.filter { isPublicStaticFinal(it) && it.name != "Companion" }
var filter = ymlOptions.filterKeys { it !in allowedOptions }
if (filter.containsKey(THRESHOLD)) {
@@ -59,6 +62,9 @@ class ConfigAssert(
}
}
private fun Class<out Rule>.isConfiguredWithAnnotations(): Boolean =
declaredMethods.any { it.isAnnotationPresent(Configuration::class.java) }
private fun getYmlRuleConfig() = config.subConfig(name) as? YamlConfig
?: error("yaml config expected but got ${config.javaClass}")

View File

@@ -1,16 +1,16 @@
package io.gitlab.arturbosch.detekt.generator.collection
import org.jetbrains.kotlin.psi.KtAnnotated
import org.jetbrains.kotlin.psi.KtAnnotationEntry
import org.jetbrains.kotlin.psi.KtClassOrObject
import kotlin.reflect.KClass
fun KtClassOrObject.isAnnotatedWith(annotation: KClass<out Annotation>): Boolean =
fun KtAnnotated.isAnnotatedWith(annotation: KClass<out Annotation>): Boolean =
annotationEntries.any { it.isOfType(annotation) }
fun KtClassOrObject.firstAnnotationParameter(annotation: KClass<out Annotation>): String =
fun KtAnnotated.firstAnnotationParameter(annotation: KClass<out Annotation>): String =
checkNotNull(firstAnnotationParameterOrNull(annotation))
fun KtClassOrObject.firstAnnotationParameterOrNull(annotation: KClass<out Annotation>): String? =
fun KtAnnotated.firstAnnotationParameterOrNull(annotation: KClass<out Annotation>): String? =
annotationEntries
.firstOrNull { it.isOfType(annotation) }
?.firstParameterOrNull()
@@ -25,6 +25,9 @@ private fun KtAnnotationEntry.firstParameterOrNull() =
?.text
?.withoutQuotes()
private fun String.withoutQuotes() = removePrefix(QUOTES).removeSuffix(QUOTES)
internal fun String.withoutQuotes() = removePrefix(QUOTES)
.removeSuffix(QUOTES)
.replace(STRING_CONCAT_REGEX, "")
private const val QUOTES = "\""
private val STRING_CONCAT_REGEX = """["]\s*\+[\n\s]*["]""".toRegex()

View File

@@ -0,0 +1,166 @@
package io.gitlab.arturbosch.detekt.generator.collection
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.KtObjectDeclaration
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtPropertyDelegate
import org.jetbrains.kotlin.psi.KtStringTemplateExpression
import org.jetbrains.kotlin.psi.psiUtil.anyDescendantOfType
import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType
import org.jetbrains.kotlin.psi.psiUtil.referenceExpression
import io.gitlab.arturbosch.detekt.api.internal.Configuration as ConfigAnnotation
class ConfigurationCollector {
private val constantsByName = mutableMapOf<String, String>()
private val properties = mutableListOf<KtProperty>()
fun getConfiguration(): List<Configuration> {
return properties.mapNotNull { it.parseConfigurationAnnotation() }
}
fun addProperty(prop: KtProperty) {
properties.add(prop)
}
fun addCompanion(aRuleCompanion: KtObjectDeclaration) {
constantsByName.putAll(
aRuleCompanion
.collectDescendantsOfType<KtProperty>()
.mapNotNull(::resolveConstantOrNull)
)
}
private fun resolveConstantOrNull(prop: KtProperty): Pair<String, String>? {
if (prop.isVar) return null
val propertyName = checkNotNull(prop.name)
val constantOrNull = prop.getConstantValueAsStringOrNull()
return constantOrNull?.let { propertyName to it }
}
private fun KtProperty.getConstantValueAsStringOrNull(): String? {
if (hasListDeclaration()) {
return getListDeclarationAsConfigString()
}
return findDescendantOfType<KtConstantExpression>()?.text
?: findDescendantOfType<KtStringTemplateExpression>()?.text?.withoutQuotes()
}
private fun KtProperty.getListDeclarationAsConfigString(): String {
return getListDeclaration()
.valueArguments
.map { "'${it.text.withoutQuotes()}'" }.toString()
}
private fun KtProperty.parseConfigurationAnnotation(): Configuration? {
if (isAnnotatedWith(ConfigAnnotation::class)) return toConfiguration()
if (isInitializedWithConfigDelegate()) {
invalidDocumentation {
"'$name' is using the config delegate but is not annotated with @Configuration"
}
}
return null
}
private fun KtProperty.toConfiguration(): Configuration {
if (!hasSupportedType()) {
invalidDocumentation {
"Type of '$name' is not supported. " +
"For properties annotated with @Configuration use one of$SUPPORTED_TYPES."
}
}
if (!isInitializedWithConfigDelegate()) {
invalidDocumentation { "'$name' is not using the '$DELEGATE_NAME' delegate" }
}
val propertyName: String = checkNotNull(name)
val deprecationMessage = firstAnnotationParameterOrNull(Deprecated::class)
val description: String = firstAnnotationParameter(ConfigAnnotation::class)
val defaultValueAsString = delegate?.getDefaultValueAsString()
?: invalidDocumentation { "'$propertyName' is not a delegated property" }
return Configuration(
name = propertyName,
description = description,
defaultValue = defaultValueAsString,
deprecated = deprecationMessage
)
}
private fun KtPropertyDelegate.getDefaultValueAsString(): String {
val delegateArgument = checkNotNull(
(expression as KtCallExpression).valueArguments[0].getArgumentExpression()
)
val listDeclarationForDefault = delegateArgument.getListDeclarationOrNull()
if (listDeclarationForDefault != null) {
return listDeclarationForDefault.valueArguments.map {
val value = constantsByName[it.text] ?: it.text
"'${value.withoutQuotes()}'"
}.toString()
}
val defaultValueOrConstantName = checkNotNull(
delegateArgument.text?.withoutQuotes()
)
val defaultValue = constantsByName[defaultValueOrConstantName] ?: defaultValueOrConstantName
return property.formatDefaultValueAccordingToType(defaultValue)
}
companion object {
private const val DELEGATE_NAME = "config"
private const val LIST_OF = "listOf"
private const val EMPTY_LIST = "emptyList"
private val LIST_CREATORS = setOf(LIST_OF, EMPTY_LIST)
private const val TYPE_STRING = "String"
private const val TYPE_BOOLEAN = "Boolean"
private const val TYPE_INT = "Int"
private const val TYPE_LONG = "Long"
private const val TYPE_STRING_LIST = "List<String>"
private val SUPPORTED_TYPES = listOf(TYPE_STRING, TYPE_BOOLEAN, TYPE_INT, TYPE_LONG, TYPE_STRING_LIST)
private val KtPropertyDelegate.property: KtProperty
get() = parent as KtProperty
private val KtProperty.declaredTypeOrNull: String?
get() = typeReference?.text
private fun KtElement.getListDeclaration(): KtCallExpression =
checkNotNull(getListDeclarationOrNull())
private fun KtElement.getListDeclarationOrNull(): KtCallExpression? =
findDescendantOfType { it.isListDeclaration() }
private fun KtProperty.isInitializedWithConfigDelegate(): Boolean =
delegate?.expression?.referenceExpression()?.text == DELEGATE_NAME
private fun KtProperty.hasSupportedType(): Boolean =
declaredTypeOrNull in SUPPORTED_TYPES
private fun KtProperty.formatDefaultValueAccordingToType(value: String): String {
val defaultValue = value.withoutQuotes()
return when (declaredTypeOrNull) {
TYPE_STRING -> "'$defaultValue'"
TYPE_BOOLEAN, TYPE_INT, TYPE_LONG, TYPE_STRING_LIST -> defaultValue
else -> error("Unable to format unexpected type '$declaredTypeOrNull'")
}
}
private fun KtProperty.hasListDeclaration(): Boolean =
anyDescendantOfType<KtCallExpression> { it.isListDeclaration() }
private fun KtCallExpression.isListDeclaration() =
referenceExpression()?.text in LIST_CREATORS
private fun KtElement.invalidDocumentation(message: () -> String): Nothing {
throw InvalidDocumentationException("[${containingFile.name}] ${message.invoke()}")
}
}
}

View File

@@ -0,0 +1,81 @@
package io.gitlab.arturbosch.detekt.generator.collection
import io.gitlab.arturbosch.detekt.generator.collection.exception.InvalidCodeExampleDocumentationException
import org.jetbrains.kotlin.psi.KtClassOrObject
class DocumentationCollector {
private var name: String = ""
var description: String = ""
private set
var compliant: String = ""
private set
var nonCompliant: String = ""
private set
fun setClass(classOrObject: KtClassOrObject) {
name = classOrObject.name?.trim() ?: ""
classOrObject.kDocSection()
?.getContent()
?.trim()
?.replace("@@", "@")
?.let(::extractRuleDocumentation)
}
private fun extractRuleDocumentation(comment: String) {
val nonCompliantIndex = comment.indexOf(TAG_NONCOMPLIANT)
val compliantIndex = comment.indexOf(TAG_COMPLIANT)
when {
nonCompliantIndex != -1 -> {
extractNonCompliantDocumentation(comment, nonCompliantIndex)
extractCompliantDocumentation(comment, compliantIndex)
}
compliantIndex != -1 -> throw InvalidCodeExampleDocumentationException(
"Rule $name contains a compliant without a noncompliant code definition"
)
else -> description = comment
}
}
private fun extractNonCompliantDocumentation(comment: String, nonCompliantIndex: Int) {
val nonCompliantEndIndex = comment.indexOf(ENDTAG_NONCOMPLIANT)
if (nonCompliantEndIndex == -1) {
throw InvalidCodeExampleDocumentationException(
"Rule $name contains an incorrect noncompliant code definition"
)
}
description = comment.substring(0, nonCompliantIndex).trim()
nonCompliant = comment.substring(nonCompliantIndex + TAG_NONCOMPLIANT.length, nonCompliantEndIndex)
.trimStartingLineBreaks()
.trimEnd()
}
private fun extractCompliantDocumentation(comment: String, compliantIndex: Int) {
val compliantEndIndex = comment.indexOf(ENDTAG_COMPLIANT)
if (compliantIndex != -1) {
if (compliantEndIndex == -1) {
throw InvalidCodeExampleDocumentationException(
"Rule $name contains an incorrect compliant code definition"
)
}
compliant = comment.substring(compliantIndex + TAG_COMPLIANT.length, compliantEndIndex)
.trimStartingLineBreaks()
.trimEnd()
}
}
private fun String.trimStartingLineBreaks(): String {
var i = 0
while (i < this.length && (this[i] == '\n' || this[i] == '\r')) {
i++
}
return this.substring(i)
}
companion object {
private const val TAG_NONCOMPLIANT = "<noncompliant>"
private const val ENDTAG_NONCOMPLIANT = "</noncompliant>"
private const val TAG_COMPLIANT = "<compliant>"
private const val ENDTAG_COMPLIANT = "</compliant>"
}
}

View File

@@ -100,8 +100,8 @@ class RuleSetProviderVisitor : DetektVisitor() {
val ruleArgumentNames = (ruleListExpression as? KtCallExpression)
?.valueArguments
?.map { it.getArgumentExpression() }
?.mapNotNull { it?.referenceExpression()?.text }
?.mapNotNull { it.getArgumentExpression() }
?.mapNotNull { it.referenceExpression()?.text }
?: emptyList()
ruleNames.addAll(ruleArgumentNames)

View File

@@ -7,13 +7,13 @@ import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import io.gitlab.arturbosch.detekt.formatting.FormattingRule
import io.gitlab.arturbosch.detekt.generator.collection.exception.InvalidAliasesDeclaration
import io.gitlab.arturbosch.detekt.generator.collection.exception.InvalidCodeExampleDocumentationException
import io.gitlab.arturbosch.detekt.generator.collection.exception.InvalidDocumentationException
import io.gitlab.arturbosch.detekt.generator.collection.exception.InvalidIssueDeclaration
import io.gitlab.arturbosch.detekt.rules.empty.EmptyRule
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtClassOrObject
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtSuperTypeList
import org.jetbrains.kotlin.psi.KtValueArgument
import org.jetbrains.kotlin.psi.psiUtil.containingClass
@@ -24,10 +24,8 @@ internal class RuleVisitor : DetektVisitor() {
val containsRule
get() = classesMap.any { it.value }
private var description = ""
private var nonCompliant = ""
private var compliant = ""
private var name = ""
private val documentationCollector = DocumentationCollector()
private var defaultActivationStatus: DefaultActivationStatus = Inactive
private var autoCorrect = false
private var requiresTypeResolution = false
@@ -35,25 +33,33 @@ internal class RuleVisitor : DetektVisitor() {
private var debt = ""
private var aliases: String? = null
private var parent = ""
private val configuration = mutableListOf<Configuration>()
private var configurationByKdoc = emptyList<Configuration>()
private val configurationCollector = ConfigurationCollector()
private val classesMap = mutableMapOf<String, Boolean>()
fun getRule(): Rule {
if (description.isEmpty()) {
if (documentationCollector.description.isEmpty()) {
throw InvalidDocumentationException("Rule $name is missing a description in its KDoc.")
}
val configurationByAnnotation = configurationCollector.getConfiguration()
if (configurationByAnnotation.isNotEmpty() && configurationByKdoc.isNotEmpty()) {
throw InvalidDocumentationException(
"Rule $name is using both annotations and kdoc to define configuration parameter."
)
}
return Rule(
name = name,
description = description,
nonCompliantCodeExample = nonCompliant,
compliantCodeExample = compliant,
description = documentationCollector.description,
nonCompliantCodeExample = documentationCollector.nonCompliant,
compliantCodeExample = documentationCollector.compliant,
defaultActivationStatus = defaultActivationStatus,
severity = severity,
debt = debt,
aliases = aliases,
parent = parent,
configuration = configuration,
configuration = configurationByAnnotation + configurationByKdoc,
autoCorrect = autoCorrect,
requiresTypeResolution = requiresTypeResolution
)
@@ -97,52 +103,19 @@ internal class RuleVisitor : DetektVisitor() {
autoCorrect = classOrObject.hasKDocTag(TAG_AUTO_CORRECT)
requiresTypeResolution = classOrObject.isAnnotatedWith(RequiresTypeResolution::class)
configurationByKdoc = classOrObject.parseConfigurationTags()
val comment = classOrObject.kDocSection()?.getContent()?.trim()?.replace("@@", "@") ?: return
extractRuleDocumentation(comment)
configuration.addAll(classOrObject.parseConfigurationTags())
documentationCollector.setClass(classOrObject)
}
private fun extractRuleDocumentation(comment: String) {
val nonCompliantIndex = comment.indexOf(TAG_NONCOMPLIANT)
val compliantIndex = comment.indexOf(TAG_COMPLIANT)
when {
nonCompliantIndex != -1 -> {
extractNonCompliantDocumentation(comment, nonCompliantIndex)
extractCompliantDocumentation(comment, compliantIndex)
}
compliantIndex != -1 -> throw InvalidCodeExampleDocumentationException(
"Rule $name contains a compliant without a noncompliant code definition"
)
else -> description = comment
}
override fun visitProperty(property: KtProperty) {
super.visitProperty(property)
configurationCollector.addProperty(property)
}
private fun extractNonCompliantDocumentation(comment: String, nonCompliantIndex: Int) {
val nonCompliantEndIndex = comment.indexOf(ENDTAG_NONCOMPLIANT)
if (nonCompliantEndIndex == -1) {
throw InvalidCodeExampleDocumentationException(
"Rule $name contains an incorrect noncompliant code definition"
)
}
description = comment.substring(0, nonCompliantIndex).trim()
nonCompliant = comment.substring(nonCompliantIndex + TAG_NONCOMPLIANT.length, nonCompliantEndIndex)
.trimStartingLineBreaks()
.trimEnd()
}
private fun extractCompliantDocumentation(comment: String, compliantIndex: Int) {
val compliantEndIndex = comment.indexOf(ENDTAG_COMPLIANT)
if (compliantIndex != -1) {
if (compliantEndIndex == -1) {
throw InvalidCodeExampleDocumentationException(
"Rule $name contains an incorrect compliant code definition"
)
}
compliant = comment.substring(compliantIndex + TAG_COMPLIANT.length, compliantEndIndex)
.trimStartingLineBreaks()
.trimEnd()
}
override fun visitClass(klass: KtClass) {
super.visitClass(klass)
klass.companionObjects.forEach(configurationCollector::addCompanion)
}
private fun extractAliases(klass: KtClass) {
@@ -197,20 +170,8 @@ internal class RuleVisitor : DetektVisitor() {
)
private const val TAG_AUTO_CORRECT = "autoCorrect"
private const val TAG_NONCOMPLIANT = "<noncompliant>"
private const val ENDTAG_NONCOMPLIANT = "</noncompliant>"
private const val TAG_COMPLIANT = "<compliant>"
private const val ENDTAG_COMPLIANT = "</compliant>"
private const val ISSUE_ARGUMENT_SIZE = 4
private const val DEBT_ARGUMENT_INDEX = 3
}
}
private fun String.trimStartingLineBreaks(): String {
var i = 0
while (i < this.length && (this[i] == '\n' || this[i] == '\r')) {
i++
}
return this.substring(i)
}

View File

@@ -151,114 +151,422 @@ class RuleCollectorSpec : Spek({
assertThat(items[0].aliases).isEqualTo("RULE, RULE2")
}
it("contains no configuration options by default") {
val code = """
/**
* description
*/
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].configuration).isEmpty()
assertThat(items[0].requiresTypeResolution).isFalse()
describe("collects configuration options") {
describe("using annotation") {
it("contains no configuration options by default") {
val code = """
/**
* description
*/
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].configuration).isEmpty()
}
it("contains one configuration option with correct formatting") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: String by config("[A-Z$]")
}
"""
val items = subject.run(code)
assertThat(items[0].configuration).hasSize(1)
val expectedConfiguration = Configuration(
name = "config",
description = "description",
defaultValue = "'[A-Z$]'",
deprecated = null
)
assertThat(items[0].configuration[0]).isEqualTo(expectedConfiguration)
}
it("contains one configuration option of type Int") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: Int by config(99)
}
"""
val items = subject.run(code)
assertThat(items[0].configuration).hasSize(1)
assertThat(items[0].configuration[0].defaultValue).isEqualTo("99")
}
it("extracts default value when defined with named parameter") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: Int by config(defaultValue = 99)
}
"""
val items = subject.run(code)
assertThat(items[0].configuration[0].defaultValue).isEqualTo("99")
}
it("extracts default value for list of strings") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: List<String> by config(
listOf(
"a",
"b"
)
)
}
"""
val items = subject.run(code)
assertThat(items[0].configuration[0].defaultValue).isEqualTo("['a', 'b']")
}
it("contains multiple configuration options") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: String by config("")
@Configuration("description")
private val config2: String by config("")
}
"""
val items = subject.run(code)
assertThat(items[0].configuration).hasSize(2)
}
it("has description that is concatenated") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration(
"This is a " +
"multi line " +
"description")
private val config: String by config("a")
}
"""
val items = subject.run(code)
assertThat(items[0].configuration[0].description).isEqualTo("This is a multi line description")
assertThat(items[0].configuration[0].defaultValue).isEqualTo("'a'")
}
it("extracts default value when it is an Int constant") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: Int by config(DEFAULT_CONFIG_VALUE)
companion object {
private const val DEFAULT_CONFIG_VALUE = 99
}
}
"""
val items = subject.run(code)
assertThat(items[0].configuration[0].defaultValue).isEqualTo("99")
}
it("extracts default value when it is an Int constant as named parameter") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: Int by config(defaultValue = DEFAULT_CONFIG_VALUE)
companion object {
private const val DEFAULT_CONFIG_VALUE = 99
}
}
"""
val items = subject.run(code)
assertThat(items[0].configuration[0].defaultValue).isEqualTo("99")
}
it("extracts default value when it is a String constant") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: String by config(DEFAULT_CONFIG_VALUE)
companion object {
private const val DEFAULT_CONFIG_VALUE = "a"
}
}
"""
val items = subject.run(code)
assertThat(items[0].configuration[0].defaultValue).isEqualTo("'a'")
}
it("extracts default value for list of strings from constant") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config1: List<String> by config(DEFAULT_CONFIG_VALUE)
@Configuration("description")
private val config2: List<String> by config(listOf(DEFAULT_CONFIG_VALUE_A, "b"))
companion object {
private val DEFAULT_CONFIG_VALUE = listOf("a", "b")
private val DEFAULT_CONFIG_VALUE_A = "a"
}
}
"""
val items = subject.run(code)
val expected = "['a', 'b']"
assertThat(items[0].configuration[0].defaultValue).isEqualTo(expected)
assertThat(items[0].configuration[1].defaultValue).isEqualTo(expected)
}
it("extracts emptyList default value") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config1: List<String> by config(listOf())
@Configuration("description")
private val config2: List<String> by config(emptyList())
companion object {
private val DEFAULT_CONFIG_VALUE_A = "a"
}
}
"""
val items = subject.run(code)
assertThat(items[0].configuration[0].defaultValue).isEqualTo("[]")
assertThat(items[0].configuration[1].defaultValue).isEqualTo("[]")
}
it("is marked as deprecated as well") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Deprecated("use config1 instead")
@Configuration("description")
private val config: String by config("")
}
"""
val items = subject.run(code)
assertThat(items[0].configuration[0].deprecated).isEqualTo("use config1 instead")
}
it("fails if annotation and kdoc are used both to define configuration") {
val code = """
/**
* description
* @configuration config1 - description (default: `''`)
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: String by config("")
}
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("fails if not used in combination with delegate") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: String = "foo"
}
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("fails if not used in combination with config delegate") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: String by lazy { "foo" }
}
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("fails if config delegate is used without annotation") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
private val config: String by config("")
}
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("fails if config delegate is used with unsupported type") {
val code = """
/**
* description
*/
class SomeRandomClass() : Rule {
@Configuration("description")
private val config: List<Int> by config(listOf(1, 2))
}
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
}
describe("as part of kdoc") {
it("contains no configuration options by default") {
val code = """
/**
* description
*/
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].configuration).isEmpty()
}
it("contains one configuration option with correct formatting") {
val code = """
/**
* description
* @configuration config - description (default: `'[A-Z$]'`)
*/
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].configuration).hasSize(1)
assertThat(items[0].configuration[0].name).isEqualTo("config")
assertThat(items[0].configuration[0].description).isEqualTo("description")
assertThat(items[0].configuration[0].defaultValue).isEqualTo("'[A-Z$]'")
}
it("contains multiple configuration options") {
val code = """
/**
* description
* @configuration config - description (default: `''`)
* @configuration config2 - description2 (default: `''`)
*/
class SomeRandomClass: Rule
"""
val items = subject.run(code)
assertThat(items[0].configuration).hasSize(2)
}
it("config option doesn't have a default value") {
val code = """
/**
* description
* @configuration config - description
*/
class SomeRandomClass : Rule
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("has a blank default value") {
val code = """
/**
* description
* @configuration config - description (default: ``)
*/
class SomeRandomClass : Rule
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("has an incorrectly delimited default value") {
val code = """
/**
* description
* @configuration config - description (default: true)
*/
class SomeRandomClass : Rule
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("contains a misconfigured configuration option") {
val code = """
/**
* description
* @configuration something: description
*/
class SomeRandomClass : Rule
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
}
}
it("contains one configuration option with correct formatting") {
val code = """
/**
* description
* @configuration config - description (default: `'[A-Z$]'`)
*/
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].configuration).hasSize(1)
assertThat(items[0].configuration[0].name).isEqualTo("config")
assertThat(items[0].configuration[0].description).isEqualTo("description")
assertThat(items[0].configuration[0].defaultValue).isEqualTo("'[A-Z$]'")
}
describe("collects type resolution information") {
it("has no type resolution by default") {
val code = """
/**
* description
*/
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].requiresTypeResolution).isFalse()
}
it("contains multiple configuration options") {
val code = """
/**
* description
* @configuration config - description (default: `''`)
* @configuration config2 - description2 (default: `''`)
*/
class SomeRandomClass: Rule
"""
val items = subject.run(code)
assertThat(items[0].configuration).hasSize(2)
}
it("collects the flag that it requires type resolution") {
val code = """
/**
* description
*/
@RequiresTypeResolution
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].requiresTypeResolution).isTrue()
}
it("collects the flag that it requires type resolution") {
val code = """
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
/**
* description
*/
@RequiresTypeResolution
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].requiresTypeResolution).isTrue()
}
it("collects the flag that it requires type resolution from fully qualified annotation") {
val code = """
/**
* description
*/
@io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].requiresTypeResolution).isTrue()
}
it("config option doesn't have a default value") {
val code = """
/**
* description
* @configuration config - description
*/
class SomeRandomClass : Rule
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("has a blank default value") {
val code = """
/**
* description
* @configuration config - description (default: ``)
*/
class SomeRandomClass : Rule
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("has an incorrectly delimited default value") {
val code = """
/**
* description
* @configuration config - description (default: true)
*/
class SomeRandomClass : Rule
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
}
it("contains a misconfigured configuration option") {
val code = """
/**
* description
* @configuration something: description
*/
class SomeRandomClass : Rule
"""
assertThatExceptionOfType(InvalidDocumentationException::class.java).isThrownBy { subject.run(code) }
it("collects the flag that it requires type resolution from fully qualified annotation") {
val code = """
/**
* description
*/
@io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
class SomeRandomClass : Rule
"""
val items = subject.run(code)
assertThat(items[0].requiresTypeResolution).isTrue()
}
}
it("contains compliant and noncompliant code examples") {

View File

@@ -1,17 +1,17 @@
package io.gitlab.arturbosch.detekt.rules.complexity
import io.github.detekt.metrics.CyclomaticComplexity
import io.github.detekt.metrics.CyclomaticComplexity.Companion.DEFAULT_NESTING_FUNCTIONS
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.Metric
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.ThresholdRule
import io.gitlab.arturbosch.detekt.api.ThresholdedCodeSmell
import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
import io.gitlab.arturbosch.detekt.api.internal.valueOrDefaultCommaSeparated
import io.gitlab.arturbosch.detekt.api.internal.Configuration
import io.gitlab.arturbosch.detekt.api.internal.config
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtNamedFunction
@@ -36,21 +36,24 @@ import org.jetbrains.kotlin.psi.KtWhenExpression
* - __Exceptions__ - `catch`, `use`
* - __Scope Functions__ - `let`, `run`, `with`, `apply`, and `also` ->
* [Reference](https://kotlinlang.org/docs/reference/scope-functions.html)
*
* @configuration threshold - McCabe's Cyclomatic Complexity (MCC) number for a method (default: `15`)
* @configuration ignoreSingleWhenExpression - Ignores a complex method if it only contains a single when expression.
* (default: `false`)
* @configuration ignoreSimpleWhenEntries - Whether to ignore simple (braceless) when entries. (default: `false`)
* @configuration ignoreNestingFunctions - Whether to ignore functions which are often used instead of an `if` or
* `for` statement (default: `false`)
* @configuration nestingFunctions - Comma separated list of function names which add complexity
* (default: `[run, let, apply, with, also, use, forEach, isNotNull, ifNull]`)
*/
@ActiveByDefault(since = "1.0.0")
class ComplexMethod(
config: Config = Config.empty,
threshold: Int = DEFAULT_THRESHOLD_METHOD_COMPLEXITY
) : ThresholdRule(config, threshold) {
class ComplexMethod(config: Config = Config.empty) : Rule(config) {
@Configuration("McCabe's Cyclomatic Complexity (MCC) number for a method.")
private val threshold: Int by config(DEFAULT_THRESHOLD_METHOD_COMPLEXITY)
@Configuration("Ignores a complex method if it only contains a single when expression.")
private val ignoreSingleWhenExpression: Boolean by config(false)
@Configuration("Whether to ignore simple (braceless) when entries.")
private val ignoreSimpleWhenEntries: Boolean by config(false)
@Configuration("Whether to ignore functions which are often used instead of an `if` or `for` statement.")
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)
override val issue = Issue(
"ComplexMethod",
@@ -59,11 +62,7 @@ class ComplexMethod(
Debt.TWENTY_MINS
)
private val ignoreSingleWhenExpression = valueOrDefault(IGNORE_SINGLE_WHEN_EXPRESSION, false)
private val ignoreSimpleWhenEntries = valueOrDefault(IGNORE_SIMPLE_WHEN_ENTRIES, false)
private val ignoreNestingFunctions = valueOrDefault(IGNORE_NESTING_FUNCTIONS, false)
private val nestingFunctions = valueOrDefaultCommaSeparated(NESTING_FUNCTIONS, DEFAULT_NESTING_FUNCTIONS.toList())
.toSet()
private val nestingFunctionsAsSet: Set<String> = nestingFunctions.toSet()
override fun visitNamedFunction(function: KtNamedFunction) {
if (ignoreSingleWhenExpression && hasSingleWhenExpression(function.bodyExpression)) {
@@ -73,7 +72,7 @@ class ComplexMethod(
val complexity = CyclomaticComplexity.calculate(function) {
this.ignoreSimpleWhenEntries = this@ComplexMethod.ignoreSimpleWhenEntries
this.ignoreNestingFunctions = this@ComplexMethod.ignoreNestingFunctions
this.nestingFunctions = this@ComplexMethod.nestingFunctions
this.nestingFunctions = this@ComplexMethod.nestingFunctionsAsSet
}
if (complexity >= threshold) {
@@ -104,9 +103,16 @@ class ComplexMethod(
companion object {
const val DEFAULT_THRESHOLD_METHOD_COMPLEXITY = 15
const val IGNORE_SINGLE_WHEN_EXPRESSION = "ignoreSingleWhenExpression"
const val IGNORE_SIMPLE_WHEN_ENTRIES = "ignoreSimpleWhenEntries"
const val IGNORE_NESTING_FUNCTIONS = "ignoreNestingFunctions"
const val NESTING_FUNCTIONS = "nestingFunctions"
val DEFAULT_NESTING_FUNCTIONS = listOf(
"run",
"let",
"apply",
"with",
"also",
"use",
"forEach",
"isNotNull",
"ifNull"
)
}
}

View File

@@ -10,6 +10,8 @@ import io.gitlab.arturbosch.detekt.test.lint
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe
private val defaultConfigMap: Map<String, Any> = mapOf("threshold" to "1")
class ComplexMethodSpec : Spek({
val defaultComplexity = 1
@@ -19,7 +21,7 @@ class ComplexMethodSpec : Spek({
context("different complex constructs") {
it("counts different loops") {
val findings = ComplexMethod(threshold = 1).compileAndLint(
val findings = ComplexMethod(TestConfig(defaultConfigMap)).compileAndLint(
"""
fun test() {
for (i in 1..10) {}
@@ -34,7 +36,7 @@ class ComplexMethodSpec : Spek({
}
it("counts catch blocks") {
val findings = ComplexMethod(threshold = 1).compileAndLint(
val findings = ComplexMethod(TestConfig(defaultConfigMap)).compileAndLint(
"""
fun test() {
try {} catch(e: IllegalArgumentException) {} catch(e: Exception) {} finally {}
@@ -46,7 +48,7 @@ class ComplexMethodSpec : Spek({
}
it("counts nested conditional statements") {
val findings = ComplexMethod(threshold = 1).compileAndLint(
val findings = ComplexMethod(TestConfig(defaultConfigMap)).compileAndLint(
"""
fun test() {
try {
@@ -79,27 +81,27 @@ class ComplexMethodSpec : Spek({
"""
it("counts three with nesting function 'forEach'") {
val config = TestConfig(mapOf(ComplexMethod.IGNORE_NESTING_FUNCTIONS to "false"))
val config = TestConfig(defaultConfigMap.plus("ignoreNestingFunctions" to "false"))
assertExpectedComplexityValue(code, config, expectedValue = 3)
}
it("can ignore nesting functions like 'forEach'") {
val config = TestConfig(mapOf(ComplexMethod.IGNORE_NESTING_FUNCTIONS to "true"))
val config = TestConfig(defaultConfigMap.plus("ignoreNestingFunctions" to "true"))
assertExpectedComplexityValue(code, config, expectedValue = 2)
}
it("skips all if if the nested functions is empty") {
val config = TestConfig(mapOf(ComplexMethod.NESTING_FUNCTIONS to ""))
val config = TestConfig(defaultConfigMap.plus("nestingFunctions" to ""))
assertExpectedComplexityValue(code, config, expectedValue = 2)
}
it("skips 'forEach' as it is not specified") {
val config = TestConfig(mapOf(ComplexMethod.NESTING_FUNCTIONS to "let,apply,also"))
val config = TestConfig(defaultConfigMap.plus("nestingFunctions" to "let,apply,also"))
assertExpectedComplexityValue(code, config, expectedValue = 2)
}
it("skips 'forEach' as it is not specified list") {
val config = TestConfig(mapOf(ComplexMethod.NESTING_FUNCTIONS to listOf("let", "apply", "also")))
val config = TestConfig(defaultConfigMap.plus("nestingFunctions" to listOf("let", "apply", "also")))
assertExpectedComplexityValue(code, config, expectedValue = 2)
}
}
@@ -111,16 +113,18 @@ class ComplexMethodSpec : Spek({
it("does not report complex methods with a single when expression") {
val config = TestConfig(
mapOf(
ComplexMethod.IGNORE_SINGLE_WHEN_EXPRESSION to "true"
"threshold" to "4",
"ignoreSingleWhenExpression" to "true"
)
)
val subject = ComplexMethod(config, threshold = 4)
val subject = ComplexMethod(config)
assertThat(subject.lint(path)).hasSourceLocations(SourceLocation(43, 5))
}
it("reports all complex methods") {
val subject = ComplexMethod(threshold = 4)
val config = TestConfig(mapOf("threshold" to "4"))
val subject = ComplexMethod(config)
assertThat(subject.lint(path)).hasSourceLocations(
SourceLocation(6, 5),
@@ -132,7 +136,7 @@ class ComplexMethodSpec : Spek({
}
it("does not trip for a reasonable amount of simple when entries when ignoreSimpleWhenEntries is true") {
val config = TestConfig(mapOf(ComplexMethod.IGNORE_SIMPLE_WHEN_ENTRIES to "true"))
val config = TestConfig(mapOf("ignoreSimpleWhenEntries" to "true"))
val subject = ComplexMethod(config)
val code = """
fun f() {
@@ -216,7 +220,7 @@ class ComplexMethodSpec : Spek({
})
private fun assertExpectedComplexityValue(code: String, config: TestConfig, expectedValue: Int) {
val findings = ComplexMethod(config, threshold = 1).lint(code)
val findings = ComplexMethod(config).lint(code)
assertThat(findings).hasSourceLocations(SourceLocation(1, 5))