Warn about enabled rules that are not going to run (#5226)

* Avoid unnecesary usage of null * Simplify code * Improve createProcessingSettings * Add asserts about output * Warn about enabled rules that are not going to run
detekt · Sep 10, 2022 · e3cae70 · e3cae70
1 parent 19d5f82
commit e3cae70
Show file tree

Hide file tree

Showing 5 changed files with 94 additions and 30 deletions.
diff --git a/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt b/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt
@@ -52,6 +52,9 @@ internal class Analyzer(
             } else {
                 runSync(ktFiles, bindingContext, compilerResources)
             }
+        if (bindingContext == BindingContext.EMPTY) {
+            warnAboutEnabledRequiresTypeResolutionRules(settings::info)
+        }
 
         val findingsPerRuleSet = HashMap<RuleSetId, List<Finding>>()
         for (findings in findingsPerFile) {
@@ -140,6 +143,19 @@ internal class Analyzer(
 
         return result
     }
+
+    private fun warnAboutEnabledRequiresTypeResolutionRules(log: (String) -> Unit) {
+        providers.asSequence()
+            .map { it to config.subConfig(it.ruleSetId) }
+            .filter { (_, ruleSetConfig) -> ruleSetConfig.isActive() }
+            .map { (provider, ruleSetConfig) -> provider.instance(ruleSetConfig) to ruleSetConfig }
+            .flatMap { (ruleSet, _) -> ruleSet.rules.asSequence() }
+            .filter { rule -> (rule as? Rule)?.active == true }
+            .filter { rule -> rule::class.hasAnnotation<RequiresTypeResolution>() }
+            .forEach { rule ->
+                log("The rule '${rule.ruleId}' requires type resolution but it was run without it.")
+            }
+    }
 }
 
 private fun filterSuppressedFindings(rule: BaseRule, bindingContext: BindingContext): List<Finding> {

diff --git a/detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/AnalyzerSpec.kt b/detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/AnalyzerSpec.kt
@@ -1,5 +1,6 @@
 package io.gitlab.arturbosch.detekt.core
 
+import io.github.detekt.test.utils.StringPrintStream
 import io.github.detekt.test.utils.compileForTest
 import io.gitlab.arturbosch.detekt.api.CodeSmell
 import io.gitlab.arturbosch.detekt.api.Config
@@ -44,13 +45,12 @@ class AnalyzerSpec(val env: KotlinCoreEnvironment) {
             val settings = createProcessingSettings(
                 inputPath = testFile,
                 config = yamlConfig("configs/config-value-type-wrong.yml"),
-                spec = createNullLoggingSpec {
-                    execution {
-                        parallelParsing = true
-                        parallelAnalysis = true
-                    }
+            ) {
+                execution {
+                    parallelParsing = true
+                    parallelAnalysis = true
                 }
-            )
+            }
             val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider()), emptyList())
 
             assertThatThrownBy { settings.use { analyzer.run(listOf(compileForTest(testFile))) } }
@@ -65,70 +65,101 @@ class AnalyzerSpec(val env: KotlinCoreEnvironment) {
         @Test
         fun `no findings`() {
             val testFile = path.resolve("Test.kt")
-            val settings = createProcessingSettings(testFile, yamlConfig("configs/config-value-type-correct.yml"))
+            val output = StringPrintStream()
+            val settings = createProcessingSettings(
+                testFile,
+                yamlConfig("configs/config-value-type-correct.yml"),
+                outputChannel = output,
+            )
             val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider()), emptyList())
 
             assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }.values.flatten()).isEmpty()
+            assertThat(output.toString()).isEqualTo(
+                "The rule 'RequiresTypeResolutionMaxLineLength' requires type resolution but it was run without it.\n"
+            )
         }
 
         @Test
         fun `with findings`() {
             val testFile = path.resolve("Test.kt")
-            val settings = createProcessingSettings(testFile, yamlConfig("configs/config-value-type-correct.yml"))
+            val output = StringPrintStream()
+            val settings = createProcessingSettings(
+                testFile,
+                yamlConfig("configs/config-value-type-correct.yml"),
+                outputChannel = output,
+            )
             val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider(30)), emptyList())
 
             assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }.values.flatten()).hasSize(1)
+            assertThat(output.toString()).isEqualTo(
+                "The rule 'RequiresTypeResolutionMaxLineLength' requires type resolution but it was run without it.\n"
+            )
         }
 
         @Test
         fun `with findings and context binding`() {
             val testFile = path.resolve("Test.kt")
-            val settings = createProcessingSettings(testFile, yamlConfig("configs/config-value-type-correct.yml"))
+            val output = StringPrintStream()
+            val settings = createProcessingSettings(
+                testFile,
+                yamlConfig("configs/config-value-type-correct.yml"),
+                outputChannel = output,
+            )
             val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider(30)), emptyList())
             val ktFile = compileForTest(testFile)
             val bindingContext = env.getContextForPaths(listOf(ktFile))
 
             assertThat(settings.use { analyzer.run(listOf(ktFile), bindingContext) }.values.flatten()).hasSize(2)
+            assertThat(output.toString()).isEmpty()
         }
 
         @Test
         fun `with findings but ignored`() {
             val testFile = path.resolve("Test.kt")
+            val output = StringPrintStream()
             val settings = createProcessingSettings(
                 testFile,
-                yamlConfig("configs/config-value-type-correct-ignore-annotated.yml")
+                yamlConfig("configs/config-value-type-correct-ignore-annotated.yml"),
+                outputChannel = output,
             )
             val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider(18)), emptyList())
 
             assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }.values.flatten()).isEmpty()
+            assertThat(output.toString()).isEmpty()
         }
 
         @Test
         fun `with faulty rule`() {
             val testFile = path.resolve("Test.kt")
+            val output = StringPrintStream()
             val settings = createProcessingSettings(
                 testFile,
-                yamlConfig("configs/config-value-type-correct.yml")
+                yamlConfig("configs/config-value-type-correct.yml"),
+                outputChannel = output,
             )
             val analyzer = Analyzer(settings, listOf(FaultyRuleSetProvider()), emptyList())
 
             assertThatThrownBy { settings.use { analyzer.run(listOf(compileForTest(testFile))) } }
                 .hasCauseInstanceOf(IllegalStateException::class.java)
                 .hasMessageContaining("Location: ${FaultyRule::class.java.name}")
+            assertThat(output.toString()).isEmpty()
         }
 
         @Test
         fun `with faulty rule without stack trace`() {
             val testFile = path.resolve("Test.kt")
+            val output = StringPrintStream()
             val settings = createProcessingSettings(
                 testFile,
-                yamlConfig("configs/config-value-type-correct.yml")
+                yamlConfig("configs/config-value-type-correct.yml"),
+                outputChannel = output,
             )
             val analyzer = Analyzer(settings, listOf(FaultyRuleNoStackTraceSetProvider()), emptyList())
 
             assertThatThrownBy { settings.use { analyzer.run(listOf(compileForTest(testFile))) } }
                 .hasCauseInstanceOf(IllegalStateException::class.java)
                 .hasMessageContaining("Location: ${null}")
+            assertThat(output.toString()).isEmpty()
         }
     }
 }

diff --git a/detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/ProcessingSettingsFactory.kt b/detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/ProcessingSettingsFactory.kt
@@ -7,6 +7,7 @@ import io.github.detekt.tooling.api.spec.ReportsSpec
 import io.github.detekt.tooling.dsl.ProcessingSpecBuilder
 import io.gitlab.arturbosch.detekt.api.Config
 import io.gitlab.arturbosch.detekt.core.reporting.DETEKT_OUTPUT_REPORT_PATHS_KEY
+import java.io.PrintStream
 import java.nio.file.Path
 import java.util.concurrent.AbstractExecutorService
 import java.util.concurrent.TimeUnit
@@ -18,16 +19,36 @@ fun createProcessingSettings(
     inputPath: Path? = null,
     config: Config = Config.empty,
     reportPaths: Collection<ReportsSpec.Report> = emptyList(),
-    spec: ProcessingSpec = createNullLoggingSpec {
+    outputChannel: PrintStream = NullPrintStream(),
+    init: ProcessingSpecBuilder.() -> Unit = { /* no-op */ },
+): ProcessingSettings {
+    val spec = ProcessingSpec {
         project {
-            inputPaths = inputPath?.let(::listOf).orEmpty()
+            inputPaths = listOfNotNull(inputPath)
         }
+        logging {
+            this.outputChannel = outputChannel
+            errorChannel = NullPrintStream()
+        }
+        config {
+            // Use an empty config in tests to be compatible with old ProcessingSettings config default.
+            // This was in particular done due to all console reports being active with an empty config.
+            // These outputs are used to assert test conditions.
+            configPaths = listOf(resourceAsPath("configs/empty.yml"))
+        }
+        execution {
+            executorService = DirectExecutor() // run in the same thread
+        }
+        init.invoke(this)
+    }
+    return ProcessingSettings(spec, config).apply {
+        register(DETEKT_OUTPUT_REPORT_PATHS_KEY, reportPaths)
     }
-) = ProcessingSettings(spec, config).apply {
-    register(DETEKT_OUTPUT_REPORT_PATHS_KEY, reportPaths)
 }
 
-fun createNullLoggingSpec(init: (ProcessingSpecBuilder.() -> Unit)? = null): ProcessingSpec =
+fun createNullLoggingSpec(
+    init: ProcessingSpecBuilder.() -> Unit = { /* no-op */ },
+): ProcessingSpec =
     ProcessingSpec {
         logging {
             outputChannel = NullPrintStream()
@@ -42,7 +63,7 @@ fun createNullLoggingSpec(init: (ProcessingSpecBuilder.() -> Unit)? = null): Pro
         execution {
             executorService = DirectExecutor() // run in the same thread
         }
-        init?.invoke(this)
+        init.invoke(this)
     }
 
 class DirectExecutor : AbstractExecutorService() {

diff --git a/.../test/kotlin/io/gitlab/arturbosch/detekt/core/config/validation/CheckConfigurationSpec.kt b/.../test/kotlin/io/gitlab/arturbosch/detekt/core/config/validation/CheckConfigurationSpec.kt
@@ -31,12 +31,11 @@ class CheckConfigurationSpec {
         createProcessingSettings(
             testDir,
             config,
-            spec = createNullLoggingSpec {
-                config {
-                    shouldValidateBeforeAnalysis = false
-                }
+        ) {
+            config {
+                shouldValidateBeforeAnalysis = false
             }
-        ).use {
+        }.use {
             assertThatCode { checkConfiguration(it, spec.getDefaultConfiguration()) }
                 .doesNotThrowAnyException()
         }

diff --git a/...t-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/settings/EnvironmentFacadeSpec.kt b/...t-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/settings/EnvironmentFacadeSpec.kt
@@ -1,6 +1,5 @@
 package io.gitlab.arturbosch.detekt.core.settings
 
-import io.gitlab.arturbosch.detekt.core.createNullLoggingSpec
 import io.gitlab.arturbosch.detekt.core.createProcessingSettings
 import org.assertj.core.api.Assertions.assertThat
 import org.jetbrains.kotlin.konan.file.File
@@ -22,10 +21,8 @@ class EnvironmentFacadeSpec {
     }
 }
 
-private fun testSettings(classpath: String) = createProcessingSettings(
-    spec = createNullLoggingSpec {
-        compiler {
-            this.classpath = classpath
-        }
+private fun testSettings(classpath: String) = createProcessingSettings {
+    compiler {
+        this.classpath = classpath
     }
-)
+}