diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 73c008d754ff..6a2f11f74cb9 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -526,6 +526,9 @@ style: ignorePackages: - '*.internal' - '*.internal.*' + ForbiddenSuppress: + active: false + rules: [] ForbiddenVoid: active: true ignoreOverridden: false diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppress.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppress.kt new file mode 100644 index 000000000000..869cbc7e437b --- /dev/null +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppress.kt @@ -0,0 +1,91 @@ +package io.gitlab.arturbosch.detekt.rules.style + +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 io.gitlab.arturbosch.detekt.api.config +import io.gitlab.arturbosch.detekt.api.internal.Configuration +import org.jetbrains.kotlin.psi.KtAnnotationEntry +import org.jetbrains.kotlin.psi.KtLiteralStringTemplateEntry +import org.jetbrains.kotlin.psi.KtStringTemplateExpression +import org.jetbrains.kotlin.psi.KtValueArgument +import org.jetbrains.kotlin.psi.KtValueArgumentList + +/** + * This rule allows to set a list of rules whose suppression is forbidden. This can be used to discourage abuse of the + * `Suppress` and `SuppressWarnings` annotations. Detekt will report suppression of all forbidden rules. This rule is + * not capable of reporting suppression of itself, as that's a language feature with precedence. + * + * + * package foo + * + * // When the rule "MaximumLineLength" is forbidden + * @@Suppress("MaximumLineLength", "UNCHECKED_CAST") + * class Bar + * + * + * + * package foo + * + * // When the rule "MaximumLineLength" is forbidden + * @@suppress("UNCHECKED_CAST") + * class Bar + * + */ +class ForbiddenSuppress(config: Config = Config.empty) : Rule(config) { + + override val issue = Issue( + javaClass.simpleName, + Severity.Maintainability, + "Suppressing a rule which is forbidden in current configuration.", + Debt.TEN_MINS + ) + + @Configuration("Rules whose suppression is forbidden.") + private val rules: List by config(emptyList()) + + override fun visitAnnotationEntry(annotationEntry: KtAnnotationEntry) { + val shortName = annotationEntry.shortName?.asString() + if (shortName == KOTLIN_SUPPRESS || shortName == JAVA_SUPPRESS) { + val nonCompliantRules = annotationEntry.children + .find { it is KtValueArgumentList } + ?.children + ?.filterIsInstance() + ?.filterForbiddenRules() ?: emptyList() + if (nonCompliantRules.isNotEmpty()) { + report( + CodeSmell( + issue, + Entity.from(annotationEntry), + message = "Cannot @Suppress ${nonCompliantRules.formatMessage()} " + + "due to the current configuration.", + ) + ) + } + } + } + + private fun List.formatMessage(): String = if (size > 1) { + "rules " + } else { + "rule " + } + joinToString(", ") { "\"$it\"" } + + private fun List.filterForbiddenRules(): List = mapNotNull { argument -> + val text = argument.children + .find { it is KtStringTemplateExpression } + ?.children + ?.find { it is KtLiteralStringTemplateEntry } + ?.text + if (rules.contains(text)) text else null + } + + private companion object { + private const val KOTLIN_SUPPRESS = "Suppress" + private const val JAVA_SUPPRESS = "SuppressWarnings" + } +} diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt index 704733859041..7d6cbc250cc9 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt @@ -38,6 +38,7 @@ class StyleGuideProvider : DefaultRuleSetProvider { ForbiddenImport(config), ForbiddenMethodCall(config), ForbiddenPublicDataClass(config), + ForbiddenSuppress(config), FunctionOnlyReturningConstant(config), SpacingBetweenPackageAndImports(config), LoopWithTooManyJumpStatements(config), diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppressSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppressSpec.kt new file mode 100644 index 000000000000..be04e08dd458 --- /dev/null +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppressSpec.kt @@ -0,0 +1,176 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.test.TestConfig +import io.gitlab.arturbosch.detekt.test.assertThat +import io.gitlab.arturbosch.detekt.test.compileAndLint +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource + +internal class ForbiddenSuppressSpec { + + @Nested + inner class `checking for suppressions of rule ARule` { + private val subject = ForbiddenSuppress( + TestConfig("rules" to listOf("ARule")) + ) + + @ParameterizedTest + @ValueSource(strings = ["Suppress", "SuppressWarnings"]) + fun `supports both kotlin and java suppress annotations`(annotation: String) { + val code = """ + package config + + @file:$annotation("ARule") + class Foo + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(1) + assertThat(findings).hasSourceLocation(3, 1) + assertThat(findings.first()).hasMessage( + "Cannot @Suppress rule \"ARule\" due to the current configuration." + ) + } + + @Test + fun `reports file-level suppression of forbidden rule`() { + val code = """ + @file:Suppress("ARule") + package config + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(1) + assertThat(findings).hasSourceLocation(1, 1) + assertThat(findings.first()).hasMessage( + "Cannot @Suppress rule \"ARule\" due to the current configuration." + ) + } + + @Test + fun `reports top-level suppression of forbidden rule`() { + val code = """ + package config + + @Suppress("ARule") + fun foo() { } + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(1) + assertThat(findings).hasSourceLocation(3, 1) + assertThat(findings.first()).hasMessage( + "Cannot @Suppress rule \"ARule\" due to the current configuration." + ) + } + + @Test + fun `reports line-level suppression of forbidden rule`() { + val code = """ + package config + + fun foo() { + @Suppress("ARule") + println("bar") + } + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(1) + assertThat(findings).hasSourceLocation(4, 5) + assertThat(findings.first()).hasMessage( + "Cannot @Suppress rule \"ARule\" due to the current configuration." + ) + } + + @Test + fun `doesn't report non-forbidden rule`() { + val code = """ + package config + + @Suppress("UNCHECKED_CAST") + fun foo() { } + """ + val findings = subject.compileAndLint(code) + assertThat(findings).isEmpty() + } + + @Test + fun `does not include non-forbidden rule in report`() { + val code = """ + package config + + @Suppress("UNCHECKED_CAST", "ARule") + fun foo() { } + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(1) + assertThat(findings.first()).hasMessage( + "Cannot @Suppress rule \"ARule\" due to the current configuration." + ) + } + } + + @Nested + inner class `checking multiple forbidden rules` { + private val subject = ForbiddenSuppress( + TestConfig("rules" to listOf("ARule", "BRule")) + ) + + @Test + fun `reports suppression of both forbidden rules`() { + val code = """ + @file:Suppress("ARule", "BRule") + package config + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(1) + assertThat(findings).hasSourceLocation(1, 1) + assertThat(findings.first()).hasMessage( + "Cannot @Suppress rules \"ARule\", \"BRule\" " + + "due to the current configuration." + ) + } + + @Test + fun `reports method-level suppression of one of two forbidden rules`() { + val code = """ + package config + + @Suppress("BRule") + fun foo() { } + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(1) + assertThat(findings).hasSourceLocation(3, 1) + assertThat(findings.first()).hasMessage( + "Cannot @Suppress rule \"BRule\" due to the current configuration." + ) + } + } + + @Nested + inner class `checking suppression of self` { + private val subject = ForbiddenSuppress( + TestConfig("rules" to listOf("ForbiddenSuppress", "ARule")) + ) + + @Test + fun `does not catch self-suppression`() { + val code = """ + @file:Suppress("ForbiddenSuppress") + package config + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(0) + } + + @Test + fun `does not catch suppression of any forbidden rule when one of them`() { + val code = """ + @file:Suppress("ForbiddenSuppress", "ARule") + package config + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(0) + } + } +}