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..46255810dfc0 --- /dev/null +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppress.kt @@ -0,0 +1,90 @@ +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.KtValueArgument +import org.jetbrains.kotlin.psi.KtValueArgumentList +import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType + +/** + * 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) { + if (rules.isEmpty()) return + val shortName = annotationEntry.shortName?.asString() + if (shortName == KOTLIN_SUPPRESS || shortName == JAVA_SUPPRESS) { + val nonCompliantRules = annotationEntry.children + .find { it is KtValueArgumentList } + ?.children + ?.filterIsInstance() + ?.filterForbiddenRules() + .orEmpty() + 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.findDescendantOfType()?.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..668b7c941473 --- /dev/null +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppressSpec.kt @@ -0,0 +1,201 @@ +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 + +internal class ForbiddenSuppressSpec { + + @Nested + inner class `checking for suppressions of rule ARule` { + private val subject = ForbiddenSuppress( + TestConfig("rules" to listOf("ARule")) + ) + + @Test + fun `supports java suppress annotations`() { + val code = """ + package config + + @SuppressWarnings("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." + ) + } + + @Test + fun `runs and does not report suppress without rules`() { + val code = """ + @file:Suppress() + package config + + @Suppress + fun foo() { } + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(0) + } + } + + @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) + } + } + + @Nested + inner class `checking active with no rules defined` { + private val subject = ForbiddenSuppress(TestConfig()) + + @Test + fun `will not report issues with no forbidden rules defined`() { + val code = """ + @file:Suppress("ARule") + package config + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(0) + } + } +}