Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move suppressions to core #7101

Merged
merged 3 commits into from May 8, 2024
Merged

Move suppressions to core #7101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe6757b
Move suppression to Analyzer
BraisGabin Mar 27, 2024
8804306
Move Suppressions to core
BraisGabin Mar 27, 2024
aee13be
improve kdoc
BraisGabin May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 2 additions & 11 deletions detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Rule.kt
View file
Open in desktop
@@ -1,7 +1,6 @@
package io.gitlab.arturbosch.detekt.api

import dev.drewhamilton.poko.Poko
import io.gitlab.arturbosch.detekt.api.internal.isSuppressedBy
import io.gitlab.arturbosch.detekt.api.internal.validateIdentifier
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.resolve.BindingContext
Expand Down Expand Up @@ -45,8 +44,6 @@ open class Rule(
*/
open val defaultRuleIdAliases: Set<String> = emptySet()

private val ruleSetId: RuleSet.Id? get() = config.parent?.parentPath?.let(RuleSet::Id)

val autoCorrect: Boolean
get() = config.valueOrDefault(Config.AUTO_CORRECT_KEY, false) &&
(config.parent?.valueOrDefault(Config.AUTO_CORRECT_KEY, true) != false)
Expand Down Expand Up @@ -100,16 +97,10 @@ open class Rule(
}

/**
* Reports a single code smell finding.
*
* Before adding a finding, it is checked if it is not suppressed
* by @Suppress or @SuppressWarnings annotations.
* Adds a code smell to the findings,
*/
fun report(finding: Finding) {
val ktElement = finding.entity.ktElement
if (ktElement == null || !ktElement.isSuppressedBy(ruleId, aliases, ruleSetId)) {
findings.add(finding)
}
findings.add(finding)
}

@Poko
Expand Down
5 changes: 4 additions & 1 deletion detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt
View file
Open in desktop
Expand Up @@ -14,11 +14,11 @@ import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.RuleSet
import io.gitlab.arturbosch.detekt.api.RuleSetProvider
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.internal.isSuppressedBy
import io.gitlab.arturbosch.detekt.api.internal.whichDetekt
import io.gitlab.arturbosch.detekt.api.internal.whichJava
import io.gitlab.arturbosch.detekt.api.internal.whichOS
import io.gitlab.arturbosch.detekt.core.suppressors.buildSuppressors
import io.gitlab.arturbosch.detekt.core.suppressors.isSuppressedBy
import io.gitlab.arturbosch.detekt.core.util.isActiveOrDefault
import io.gitlab.arturbosch.detekt.core.util.shouldAnalyzeFile
import org.jetbrains.kotlin.config.languageVersionSettings
Expand Down Expand Up @@ -112,6 +112,9 @@ internal class Analyzer(

return (correctableRules + otherRules).flatMap { (ruleInfo, rule) ->
rule.visitFile(file, bindingContext, compilerResources)
.filterNot {
it.entity.ktElement?.isSuppressedBy(ruleInfo.id, rule.aliases, ruleInfo.ruleSetId) == true
}
.filterSuppressedFindings(rule, bindingContext)
.map { it.toIssue(ruleInfo, rule.computeSeverity()) }
}
Expand Down
2 changes: 1 addition & 1 deletion ...bosch/detekt/api/internal/Suppressions.kt → ...h/detekt/core/suppressors/Suppressions.kt
View file
Open in desktop
@@ -1,4 +1,4 @@
package io.gitlab.arturbosch.detekt.api.internal
package io.gitlab.arturbosch.detekt.core.suppressors

import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.RuleSet
Expand Down
23 changes: 23 additions & 0 deletions detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/AnalyzerSpec.kt
View file
Open in desktop
Expand Up @@ -384,6 +384,29 @@ class AnalyzerSpec(val env: KotlinCoreEnvironment) {
}
assertThat(findings).isEmpty()
}

@ParameterizedTest
@ValueSource(strings = ["MaxLineLength", "detekt.MaxLineLength", "MLL", "custom", "all"])
fun `when the ktElement is suppressed the issue is not raised`(suppress: String) {
BraisGabin marked this conversation as resolved.
Show resolved Hide resolved
val config = yamlConfigFromContent(
"""
custom:
MaxLineLength:
active: true
maxLineLength: 10
aliases: ["MLL"]
""".trimIndent()
)
val code = """
@Suppress("$suppress")
fun foo() = Unit
""".trimIndent()
val findings = createProcessingSettings(config = config).use { settings ->
Analyzer(settings, listOf(CustomRuleSetProvider()), emptyList())
.run(listOf(compileContentForTest(code)))
}
assertThat(findings).isEmpty()
}
}
}

Expand Down
5 changes: 2 additions & 3 deletions ...arturbosch/detekt/core/SuppressionSpec.kt → ...tekt/core/suppressors/SuppressionsSpec.kt
View file
Open in desktop
@@ -1,9 +1,8 @@
package io.gitlab.arturbosch.detekt.core
package io.gitlab.arturbosch.detekt.core.suppressors

import io.github.detekt.test.utils.compileContentForTest
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.RuleSet
import io.gitlab.arturbosch.detekt.api.internal.isSuppressedBy
import org.assertj.core.api.Assertions.assertThat
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtElement
Expand All @@ -16,7 +15,7 @@ import org.junit.jupiter.api.Test
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.ValueSource

class SuppressionSpec {
class SuppressionsSpec {

private fun KtElement.isSuppressedBy(): Boolean {
return isSuppressedBy(Rule.Id("RuleId"), setOf("alias1", "alias2"), RuleSet.Id("RuleSetId"))
Expand Down
130 changes: 0 additions & 130 deletions detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/SuppressingSpec.kt
View file
Open in desktop

This file was deleted.

16 changes: 12 additions & 4 deletions detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/RuleExtensions.kt
View file
Open in desktop
Expand Up @@ -6,6 +6,8 @@ import io.github.detekt.test.utils.compileForTest
import io.gitlab.arturbosch.detekt.api.CompilerResources
import io.gitlab.arturbosch.detekt.api.Finding
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.RuleSet
import io.gitlab.arturbosch.detekt.core.suppressors.isSuppressedBy
import org.intellij.lang.annotations.Language
import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
import org.jetbrains.kotlin.config.languageVersionSettings
Expand All @@ -25,12 +27,12 @@ fun Rule.compileAndLint(@Language("kotlin") content: String): List<Finding> {

fun Rule.lint(@Language("kotlin") content: String): List<Finding> {
val ktFile = compileContentForTest(content)
return visitFile(ktFile)
return visitFile(ktFile).filterSuppressed(this)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like to add this here too much but I think that for now it's the simpler solution.

Why do we need this? Right now we have some tests on the rules that check that when an annotation is set the finding is not raised. Those tests are testing two things. From one side they are testing that the suppression works, that's tested already somewhere else so it should not be tested. But they are also testing that the KtElement that they provide is correct or that the configured alias is correct. We could refactor those tests but the tests would be more difficult to follow (I tried). For that reason I decide to keep this here.

The good part is that, if we want, we could refactor those tests and remove this later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step-by.step is the way to go here. As you said, I don't think it's ideal approach, though sufficient for now.

}

fun Rule.lint(path: Path): List<Finding> {
val ktFile = compileForTest(path)
return visitFile(ktFile)
return visitFile(ktFile).filterSuppressed(this)
}

fun Rule.lintWithContext(
Expand All @@ -47,7 +49,7 @@ fun Rule.lintWithContext(

val dataFlowValueFactory = DataFlowValueFactoryImpl(languageVersionSettings)
val compilerResources = CompilerResources(languageVersionSettings, dataFlowValueFactory)
return visitFile(ktFile, bindingContext, compilerResources)
return visitFile(ktFile, bindingContext, compilerResources).filterSuppressed(this)
}

fun Rule.compileAndLintWithContext(
Expand All @@ -60,4 +62,10 @@ fun Rule.compileAndLintWithContext(
return lintWithContext(environment, content)
}

fun Rule.lint(ktFile: KtFile): List<Finding> = visitFile(ktFile)
fun Rule.lint(ktFile: KtFile): List<Finding> = visitFile(ktFile).filterSuppressed(this)

private fun List<Finding>.filterSuppressed(rule: Rule): List<Finding> {
return filterNot {
it.entity.ktElement?.isSuppressedBy(rule.ruleId, rule.aliases, RuleSet.Id("NoARuleSetId")) == true
}
}