Sort and run correctable rules first - #2341

detekt · Mar 2, 2020 · 5022784 · 5022784
1 parent 07a83d3
commit 5022784
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 24 deletions.
diff --git a/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/BaseRule.kt b/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/BaseRule.kt
@@ -27,6 +27,8 @@ abstract class BaseRule(
     open val ruleId: RuleId = javaClass.simpleName
     var bindingContext: BindingContext = BindingContext.EMPTY
 
+    open val supportsAutoCorrect: Boolean = false
+
     /**
      * Before starting visiting kotlin elements, a check is performed if this rule should be triggered.
      * Pre- and post-visit-hooks are executed before/after the visiting process.

diff --git a/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Detektor.kt b/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Detektor.kt
@@ -1,14 +1,15 @@
 package io.gitlab.arturbosch.detekt.core
 
+import io.gitlab.arturbosch.detekt.api.BaseRule
 import io.gitlab.arturbosch.detekt.api.Config
 import io.gitlab.arturbosch.detekt.api.FileProcessListener
 import io.gitlab.arturbosch.detekt.api.Finding
 import io.gitlab.arturbosch.detekt.api.RuleSetId
 import io.gitlab.arturbosch.detekt.api.RuleSetProvider
+import io.gitlab.arturbosch.detekt.core.rules.associateRuleIdToRuleSetId
 import io.gitlab.arturbosch.detekt.core.rules.createRuleSet
 import io.gitlab.arturbosch.detekt.core.rules.isActive
 import io.gitlab.arturbosch.detekt.core.rules.shouldAnalyzeFile
-import io.gitlab.arturbosch.detekt.core.rules.visitFile
 import org.jetbrains.kotlin.psi.KtFile
 import org.jetbrains.kotlin.resolve.BindingContext
 
@@ -44,7 +45,7 @@ class Detektor(
     ): FindingsResult =
         ktFiles.map { file ->
             processors.forEach { it.onProcess(file) }
-            val findings = runCatching { file.analyze(bindingContext) }
+            val findings = runCatching { analyze(file, bindingContext) }
                 .onFailure { settings.error(createErrorMessage(file, it), it) }
                 .getOrDefault(emptyMap())
             processors.forEach { it.onProcessComplete(file, findings) }
@@ -59,7 +60,7 @@ class Detektor(
         val tasks: TaskList<Map<RuleSetId, List<Finding>>?> = ktFiles.map { file ->
             service.task {
                 processors.forEach { it.onProcess(file) }
-                val findings = file.analyze(bindingContext)
+                val findings = analyze(file, bindingContext)
                 processors.forEach { it.onProcessComplete(file, findings) }
                 findings
             }.recover {
@@ -70,12 +71,36 @@ class Detektor(
         return awaitAll(tasks).filterNotNull()
     }
 
-    private fun KtFile.analyze(bindingContext: BindingContext): Map<RuleSetId, List<Finding>> =
-        providers.asSequence()
+    private fun analyze(file: KtFile, bindingContext: BindingContext): Map<RuleSetId, List<Finding>> {
+        val ruleSets = providers.asSequence()
             .filter { it.isActive(config) }
             .map { it.createRuleSet(config) }
-            .filter { it.shouldAnalyzeFile(this, config) }
-            .sortedBy { it.id }
-            .map { ruleSet -> ruleSet.id to ruleSet.visitFile(this, bindingContext) }
-            .toMergedMap()
+            .filter { it.shouldAnalyzeFile(file, config) }
+            .associate { it.id to it.rules }
+
+        val idMapping = associateRuleIdToRuleSetId(ruleSets)
+        val (correctableRules, otherRules) =
+            ruleSets.asSequence()
+                .flatMap { (_, value) -> value.asSequence() }
+                .partition { it.supportsAutoCorrect }
+
+        val result = HashMap<RuleSetId, MutableList<Finding>>()
+
+        fun executeRules(rules: List<BaseRule>) {
+            for (rule in rules) {
+                rule.visitFile(file, bindingContext)
+                for (finding in rule.findings) {
+                    val mappedRuleSet = idMapping[finding.id]
+                        ?: error("Mapping for '${finding.id}' expected.")
+                    result.putIfAbsent(mappedRuleSet, ArrayList())
+                    result[mappedRuleSet]?.add(finding)
+                }
+            }
+        }
+
+        executeRules(correctableRules)
+        executeRules(otherRules)
+
+        return result
+    }
 }
diff --git a/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Junk.kt b/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Junk.kt
@@ -8,7 +8,6 @@ import org.jetbrains.kotlin.psi.KtFile
 import java.io.PrintStream
 import java.nio.file.Files
 import java.nio.file.Path
-import java.util.HashMap
 
 fun Path.exists(): Boolean = Files.exists(this)
 fun Path.isFile(): Boolean = Files.isRegularFile(this)
@@ -27,16 +26,6 @@ fun Throwable.printStacktraceRecursively(logger: PrintStream) {
     cause?.printStacktraceRecursively(logger)
 }
 
-fun <K, V> Sequence<Pair<K, List<V>>>.toMergedMap(): Map<K, List<V>> {
-    val map = HashMap<K, MutableList<V>>()
-    for ((key, values) in this) {
-        map.merge(key, values.toMutableList()) { l1, l2 ->
-            l1.apply { addAll(l2) }
-        }
-    }
-    return map
-}
-
 typealias FindingsResult = List<Map<RuleSetId, List<Finding>>>
 
 fun createErrorMessage(file: KtFile, error: Throwable): String =

diff --git a/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/rules/RuleSets.kt b/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/rules/RuleSets.kt
@@ -1,14 +1,20 @@
 package io.gitlab.arturbosch.detekt.core.rules
 
+import io.gitlab.arturbosch.detekt.api.BaseRule
 import io.gitlab.arturbosch.detekt.api.Config
 import io.gitlab.arturbosch.detekt.api.Finding
+import io.gitlab.arturbosch.detekt.api.MultiRule
+import io.gitlab.arturbosch.detekt.api.Rule
+import io.gitlab.arturbosch.detekt.api.RuleId
 import io.gitlab.arturbosch.detekt.api.RuleSet
+import io.gitlab.arturbosch.detekt.api.RuleSetId
 import io.gitlab.arturbosch.detekt.api.RuleSetProvider
 import io.gitlab.arturbosch.detekt.api.internal.PathFilters
 import io.gitlab.arturbosch.detekt.api.internal.absolutePath
 import org.jetbrains.kotlin.psi.KtFile
 import org.jetbrains.kotlin.resolve.BindingContext
 import java.nio.file.Paths
+import java.util.HashMap
 
 fun RuleSetProvider.isActive(config: Config): Boolean =
     config.subConfig(ruleSetId)
@@ -41,3 +47,15 @@ fun RuleSet.visitFile(
         it.visitFile(file, bindingContext)
         it.findings
     }
+
+typealias IdMapping = Map<RuleId, RuleSetId>
+
+@Suppress("RemoveExplicitTypeArguments") // FIXME 1.4: type inference bug
+fun associateRuleIdToRuleSetId(rules: Map<RuleSetId, List<BaseRule>>): IdMapping {
+    fun extractIds(rule: BaseRule) =
+        if (rule is MultiRule) rule.rules.map(Rule::ruleId) else listOf(rule.ruleId)
+    return rules.asSequence()
+        .associate { (key, value) -> key to value.flatMapTo(HashSet<RuleId>(), ::extractIds) }
+        .map { (key, value) -> value.associateWith { key } }
+        .fold(HashMap()) { acc, map -> acc.putAll(map); acc }
+}
diff --git a/detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/CustomRuleSetProviderSpec.kt b/detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/CustomRuleSetProviderSpec.kt
@@ -16,21 +16,20 @@ import java.nio.file.Paths
  *  3. 'cp detekt-sample-extensions/build/libs/detekt-sample-extensions-<version>.jar detekt-core/src/test/resources/sample-rule-set.jar'
  *  4. Now 'gradle build' should be green again.
  */
-@Suppress("MaxLineLength")
 class CustomRuleSetProviderSpec : Spek({
 
     describe("custom rule sets should be loadable through jars") {
 
         val sampleRuleSet = Paths.get(resource("sample-rule-set.jar"))
 
         it("should load the sample provider") {
-            val result = ProcessingSettings(
+            val providers = ProcessingSettings(
                 path,
                 excludeDefaultRuleSets = true,
                 pluginPaths = listOf(sampleRuleSet)
-            ).use { DetektFacade.create(it).run() }
+            ).use { RuleSetLocator(it).load() }
 
-            assertThat(result.findings.keys).contains("sample")
+            assertThat(providers).filteredOn { it.ruleSetId == "sample" }.hasSize(1)
         }
     }
 })
diff --git a/detekt-formatting/src/main/kotlin/io/gitlab/arturbosch/detekt/formatting/FormattingRule.kt b/detekt-formatting/src/main/kotlin/io/gitlab/arturbosch/detekt/formatting/FormattingRule.kt
@@ -26,6 +26,8 @@ import org.jetbrains.kotlin.psi.psiUtil.endOffset
  */
 abstract class FormattingRule(config: Config) : Rule(config) {
 
+    override val supportsAutoCorrect: Boolean = true
+
     abstract val wrapping: com.pinterest.ktlint.core.Rule
 
     protected fun issueFor(description: String) =

diff --git a/detekt-formatting/src/main/kotlin/io/gitlab/arturbosch/detekt/formatting/KtLintMultiRule.kt b/detekt-formatting/src/main/kotlin/io/gitlab/arturbosch/detekt/formatting/KtLintMultiRule.kt
@@ -47,6 +47,8 @@ import org.jetbrains.kotlin.psi.KtFile
  */
 class KtLintMultiRule(config: Config = Config.empty) : MultiRule() {
 
+    override val supportsAutoCorrect: Boolean = true
+
     override val rules: List<Rule> = listOf(
         AnnotationOnSeparateLine(config),
         ChainWrapping(config),