Skip to content

Commit

Permalink
Creates ForbiddenSuppress rule
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
gfreivasc committed Jun 2, 2022
1 parent 4cc4051 commit e439cc7
Show file tree
Hide file tree
Showing 4 changed files with 295 additions and 0 deletions.
3 changes: 3 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Expand Up @@ -526,6 +526,9 @@ style:
ignorePackages:
- '*.internal'
- '*.internal.*'
ForbiddenSuppress:
active: false
rules: []
ForbiddenVoid:
active: true
ignoreOverridden: false
Expand Down
@@ -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.
*
* <noncompliant>
* package foo
*
* // When the rule "MaximumLineLength" is forbidden
* @@Suppress("MaximumLineLength", "UNCHECKED_CAST")
* class Bar
* </noncompliant>
*
* <compliant>
* package foo
*
* // When the rule "MaximumLineLength" is forbidden
* @@Suppress("UNCHECKED_CAST")
* class Bar
* </compliant>
*/
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<String> 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<KtValueArgument>()
?.filterForbiddenRules()
.orEmpty()
if (nonCompliantRules.isNotEmpty()) {
report(
CodeSmell(
issue,
Entity.from(annotationEntry),
message = "Cannot @Suppress ${nonCompliantRules.formatMessage()} " +
"due to the current configuration.",
)
)
}
}
}

private fun List<String>.formatMessage(): String = if (size > 1) {
"rules "
} else {
"rule "
} + joinToString(", ") { "\"$it\"" }

private fun List<KtValueArgument>.filterForbiddenRules(): List<String> = mapNotNull { argument ->
val text = argument.findDescendantOfType<KtLiteralStringTemplateEntry>()?.text
if (rules.contains(text)) text else null
}

private companion object {
private const val KOTLIN_SUPPRESS = "Suppress"
private const val JAVA_SUPPRESS = "SuppressWarnings"
}
}
Expand Up @@ -38,6 +38,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
ForbiddenImport(config),
ForbiddenMethodCall(config),
ForbiddenPublicDataClass(config),
ForbiddenSuppress(config),
FunctionOnlyReturningConstant(config),
SpacingBetweenPackageAndImports(config),
LoopWithTooManyJumpStatements(config),
Expand Down
@@ -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)
}
}
}

0 comments on commit e439cc7

Please sign in to comment.