From 5baf1f695542e1f4d1477f6777ee0bb65f98eb76 Mon Sep 17 00:00:00 2001 From: Gabriel Freitas Vasconcelos Date: Fri, 3 Jun 2022 13:19:23 -0300 Subject: [PATCH] Creates ForbiddenSuppress rule (#4899) This rule is based on discussion #4890. Checks for suppression of one or more rules that have been decided to never be suppressed within a code base. For example, if we've set MaximumLineLength to a value, every file must be compliant, and if there is a special case where compliance is discouraged, we can simply exclude this file within YML configuration. This rule cannot prevent itself from being suppressed, but should serve to discourage the over use of `@Suppress` annotations. --- .../main/resources/default-detekt-config.yml | 3 + .../detekt/rules/style/ForbiddenSuppress.kt | 90 ++++++++ .../detekt/rules/style/StyleGuideProvider.kt | 1 + .../rules/style/ForbiddenSuppressSpec.kt | 201 ++++++++++++++++++ 4 files changed, 295 insertions(+) create mode 100644 detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppress.kt create mode 100644 detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenSuppressSpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 9043fd73873..a5647513d66 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -535,6 +535,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 00000000000..52c822816c7 --- /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 the 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 70473385904..7d6cbc250cc 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 00000000000..668b7c94147 --- /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) + } + } +}