From cdac21c2991c0fd3290e030fdda6e32e56debc45 Mon Sep 17 00:00:00 2001 From: Gabriel Vasconcelos Date: Wed, 1 Jun 2022 20:28:42 -0300 Subject: [PATCH] Creates ForbiddenSuppress rule 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 | 91 +++++++++ .../detekt/rules/style/StyleGuideProvider.kt | 1 + .../rules/style/ForbiddenSuppressSpec.kt | 176 ++++++++++++++++++ 4 files changed, 271 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 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..fa068dab8781 --- /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) + } + } +}