Skip to content

Commit

Permalink
Don't run rules annotated with @RequiresTypeResolution when running…
Browse files Browse the repository at this point in the history
… plain detekt (#5176)

* Improve tests

* Use reflection to disable the rules that require type solving when the BindingContext is empty

* Update detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt

Co-authored-by: marschwar <marschwar@users.noreply.github.com>

* Improve documentation

* Remove old bindingContext checks

Co-authored-by: marschwar <marschwar@users.noreply.github.com>
  • Loading branch information
BraisGabin and marschwar committed Sep 8, 2022
1 parent bda79d3 commit 5e46099
Show file tree
Hide file tree
Showing 66 changed files with 66 additions and 174 deletions.
Expand Up @@ -2,7 +2,10 @@ package io.gitlab.arturbosch.detekt.api.internal

/**
* Annotated [io.gitlab.arturbosch.detekt.api.Rule] requires type resolution to work.
*
* The detekt core will honor this annotation and it will not run any rule with this annotation if the bindingContext
* is empty.
*/
@Target(AnnotationTarget.CLASS)
@Retention(AnnotationRetention.SOURCE)
@Retention(AnnotationRetention.RUNTIME)
annotation class RequiresTypeResolution
1 change: 1 addition & 0 deletions detekt-core/build.gradle.kts
Expand Up @@ -7,6 +7,7 @@ dependencies {
api(projects.detektParser)
api(projects.detektTooling)
implementation(libs.snakeyaml)
implementation(libs.kotlin.reflection)
implementation(projects.detektMetrics)
implementation(projects.detektPsiUtils)
implementation(projects.detektReportHtml)
Expand Down
Expand Up @@ -10,6 +10,7 @@ import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.RuleSetId
import io.gitlab.arturbosch.detekt.api.RuleSetProvider
import io.gitlab.arturbosch.detekt.api.internal.CompilerResources
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import io.gitlab.arturbosch.detekt.api.internal.whichDetekt
import io.gitlab.arturbosch.detekt.api.internal.whichJava
import io.gitlab.arturbosch.detekt.api.internal.whichOS
Expand All @@ -24,6 +25,7 @@ import org.jetbrains.kotlin.config.languageVersionSettings
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.smartcasts.DataFlowValueFactoryImpl
import kotlin.reflect.full.hasAnnotation

private typealias FindingsResult = List<Map<RuleSetId, List<Finding>>>

Expand Down Expand Up @@ -113,6 +115,9 @@ internal class Analyzer(

val (correctableRules, otherRules) = activeRuleSetsToRuleSetConfigs
.flatMap { (ruleSet, _) -> ruleSet.rules.asSequence() }
.filter { rule ->
bindingContext != BindingContext.EMPTY || !rule::class.hasAnnotation<RequiresTypeResolution>()
}
.partition { isCorrectable(it) }

val result = HashMap<RuleSetId, MutableList<Finding>>()
Expand Down
Expand Up @@ -10,15 +10,20 @@ 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.RequiresTypeResolution
import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
import io.gitlab.arturbosch.detekt.test.getContextForPaths
import io.gitlab.arturbosch.detekt.test.yamlConfig
import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.Assertions.assertThatThrownBy
import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
import org.jetbrains.kotlin.psi.KtFile
import org.junit.jupiter.api.Nested
import org.junit.jupiter.api.Test
import java.util.concurrent.CompletionException

class AnalyzerSpec {
@KotlinCoreEnvironmentTest
class AnalyzerSpec(val env: KotlinCoreEnvironment) {

@Nested
inner class `exceptions during analyze()` {
Expand Down Expand Up @@ -63,16 +68,27 @@ class AnalyzerSpec {
val settings = createProcessingSettings(testFile, yamlConfig("configs/config-value-type-correct.yml"))
val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider()), emptyList())

assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }).isEmpty()
assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }.values.flatten()).isEmpty()
}

@Test
fun `with findings`() {
val testFile = path.resolve("Test.kt")
val settings = createProcessingSettings(testFile, yamlConfig("configs/config-value-type-correct.yml"))
val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider(18)), emptyList())
val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider(30)), emptyList())

assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }.values.flatten()).hasSize(1)
}

@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 analyzer = Analyzer(settings, listOf(StyleRuleSetProvider(30)), emptyList())
val ktFile = compileForTest(testFile)
val bindingContext = env.getContextForPaths(listOf(ktFile))

assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }).hasSize(1)
assertThat(settings.use { analyzer.run(listOf(ktFile), bindingContext) }.values.flatten()).hasSize(2)
}

@Test
Expand All @@ -84,7 +100,7 @@ class AnalyzerSpec {
)
val analyzer = Analyzer(settings, listOf(StyleRuleSetProvider(18)), emptyList())

assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }).isEmpty()
assertThat(settings.use { analyzer.run(listOf(compileForTest(testFile))) }.values.flatten()).isEmpty()
}

@Test
Expand Down Expand Up @@ -119,7 +135,13 @@ class AnalyzerSpec {

private class StyleRuleSetProvider(private val threshold: Int? = null) : RuleSetProvider {
override val ruleSetId: String = "style"
override fun instance(config: Config) = RuleSet(ruleSetId, listOf(MaxLineLength(config, threshold)))
override fun instance(config: Config) = RuleSet(
ruleSetId,
listOf(
MaxLineLength(config, threshold),
RequiresTypeResolutionMaxLineLength(config, threshold),
)
)
}

private class MaxLineLength(config: Config, threshold: Int?) : Rule(config) {
Expand All @@ -135,6 +157,20 @@ private class MaxLineLength(config: Config, threshold: Int?) : Rule(config) {
}
}

@RequiresTypeResolution
private class RequiresTypeResolutionMaxLineLength(config: Config, threshold: Int?) : Rule(config) {
override val issue = Issue(this::class.java.simpleName, Severity.Style, "", Debt.FIVE_MINS)
private val lengthThreshold: Int = threshold ?: valueOrDefault("maxLineLength", 100)
override fun visitKtFile(file: KtFile) {
super.visitKtFile(file)
for (line in file.text.lineSequence()) {
if (line.length > lengthThreshold) {
report(CodeSmell(issue, Entity.atPackageOrFirstDecl(file), issue.description))
}
}
}
}

private class FaultyRuleSetProvider : RuleSetProvider {
override val ruleSetId: String = "style"
override fun instance(config: Config) = RuleSet(ruleSetId, listOf(FaultyRule(config)))
Expand All @@ -156,7 +192,9 @@ private class FaultyRuleNoStackTrace(config: Config) : Rule(config) {
override val issue = Issue(this::class.java.simpleName, Severity.Style, "", Debt.FIVE_MINS)
override fun visitKtFile(file: KtFile) {
throw object : IllegalStateException("Deliberately triggered error without stack trace.") {
init { stackTrace = emptyArray() }
init {
stackTrace = emptyArray()
}
}
}
}
2 changes: 1 addition & 1 deletion detekt-core/src/test/resources/cases/Test.kt
Expand Up @@ -5,5 +5,5 @@ package cases
@Suppress("Unused")
class Test

@Target(AnnotationTarget.FILE)
@Target(AnnotationTarget.FILE, AnnotationTarget.FUNCTION)
annotation class AnAnnotation
Expand Up @@ -2,6 +2,9 @@ style:
MaxLineLength:
active: true
maxLineLength: 120
RequiresTypeResolutionMaxLineLength:
active: true
maxLineLength: 120
FaultyRule:
active: true
FaultyRuleNoStackTrace:
Expand Down
Expand Up @@ -12,7 +12,6 @@ import io.gitlab.arturbosch.detekt.api.internal.Configuration
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtLambdaArgument
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.util.getParameterForArgument
import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall

Expand Down Expand Up @@ -49,7 +48,6 @@ class NamedArguments(config: Config = Config.empty) : Rule(config) {
private val ignoreArgumentsMatchingNames: Boolean by config(defaultValue = false)

override fun visitCallExpression(expression: KtCallExpression) {
if (bindingContext == BindingContext.EMPTY) return
val valueArguments = expression.valueArguments.filterNot { it is KtLambdaArgument }
if (valueArguments.size > threshold && expression.canNameArguments()) {
val message = "This function call has ${valueArguments.size} arguments. To call a function with more " +
Expand Down
Expand Up @@ -16,7 +16,6 @@ import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import org.jetbrains.kotlin.descriptors.CallableDescriptor
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall

/**
Expand Down Expand Up @@ -66,7 +65,6 @@ class NestedScopeFunctions(config: Config = Config.empty) : Rule(config) {
}

override fun visitNamedFunction(function: KtNamedFunction) {
if (bindingContext == BindingContext.EMPTY) return
function.accept(FunctionDepthVisitor())
}

Expand Down
Expand Up @@ -9,7 +9,6 @@ import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import org.jetbrains.kotlin.psi.KtSafeQualifiedExpression
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
import org.jetbrains.kotlin.types.isNullable

Expand Down Expand Up @@ -52,8 +51,6 @@ class ReplaceSafeCallChainWithRun(config: Config = Config.empty) : Rule(config)
override fun visitSafeQualifiedExpression(expression: KtSafeQualifiedExpression) {
super.visitSafeQualifiedExpression(expression)

if (bindingContext == BindingContext.EMPTY) return

/* We want the last safe qualified expression in the chain, so if there are more in this chain then there's no
need to run checks on this one */
if (expression.parent is KtSafeQualifiedExpression) return
Expand Down
Expand Up @@ -4,7 +4,6 @@ import io.gitlab.arturbosch.detekt.api.Finding
import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
import io.gitlab.arturbosch.detekt.test.TestConfig
import io.gitlab.arturbosch.detekt.test.assertThat
import io.gitlab.arturbosch.detekt.test.compileAndLint
import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
import org.junit.jupiter.api.Test
Expand Down Expand Up @@ -94,27 +93,10 @@ class NestedScopeFunctionsSpec(private val env: KotlinCoreEnvironment) {
expectNoFindings()
}

@Test
fun `should not report when binding context is empty`() {
givenCode = """
fun f() {
1.run {
1.run { }
}
}
""".trimIndent()
whenLintRunsWithoutContext()
expectNoFindings()
}

private fun whenLintRuns() {
actual = subject.compileAndLintWithContext(env, givenCode)
}

private fun whenLintRunsWithoutContext() {
actual = subject.compileAndLint(givenCode)
}

private fun expectSourceLocation(location: Pair<Int, Int>) {
assertThat(actual).hasStartSourceLocation(location.first, location.second)
}
Expand Down
Expand Up @@ -17,7 +17,6 @@ import org.jetbrains.kotlin.psi.KtConstructorDelegationCall
import org.jetbrains.kotlin.psi.KtParameter
import org.jetbrains.kotlin.psi.KtSimpleNameExpression
import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType
import org.jetbrains.kotlin.resolve.BindingContext.EMPTY
import org.jetbrains.kotlin.resolve.calls.util.getType
import org.jetbrains.kotlin.types.typeUtil.supertypes

Expand Down Expand Up @@ -57,7 +56,7 @@ class InjectDispatcher(config: Config) : Rule(config) {

override fun visitSimpleNameExpression(expression: KtSimpleNameExpression) {
super.visitSimpleNameExpression(expression)
if (bindingContext == EMPTY || expression.getReferencedName() !in dispatcherNames) return
if (expression.getReferencedName() !in dispatcherNames) return
val type = expression.getType(bindingContext) ?: return
val isCoroutineDispatcher = type.fqNameOrNull() == COROUTINE_DISPATCHER_FQCN ||
type.supertypes().any { it.fqNameOrNull() == COROUTINE_DISPATCHER_FQCN }
Expand Down
Expand Up @@ -66,7 +66,6 @@ class RedundantSuspendModifier(config: Config) : Rule(config) {
)

override fun visitNamedFunction(function: KtNamedFunction) {
if (bindingContext == BindingContext.EMPTY) return
val suspendModifier = function.modifierList?.getModifier(KtTokens.SUSPEND_KEYWORD) ?: return
if (!function.hasBody()) return
if (function.hasModifier(KtTokens.OVERRIDE_KEYWORD)) return
Expand Down
Expand Up @@ -16,7 +16,6 @@ import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.psi.KtQualifiedExpression
import org.jetbrains.kotlin.psi.psiUtil.forEachDescendantOfType
import org.jetbrains.kotlin.psi.psiUtil.hasSuspendModifier
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameOrNull

Expand Down Expand Up @@ -50,7 +49,6 @@ class SleepInsteadOfDelay(config: Config = Config.empty) : Rule(config) {
)

override fun visitNamedFunction(function: KtNamedFunction) {
if (bindingContext == BindingContext.EMPTY) return
if (function.modifierList?.hasSuspendModifier() == true) {
function.checkDescendants(SUSPEND_FUN_MESSAGE)
}
Expand Down
Expand Up @@ -60,7 +60,6 @@ class SuspendFunWithCoroutineScopeReceiver(config: Config) : Rule(config) {
)

override fun visitNamedFunction(function: KtNamedFunction) {
if (bindingContext == BindingContext.EMPTY) return
checkReceiver(function)
}

Expand Down
Expand Up @@ -73,7 +73,6 @@ class SuspendFunWithFlowReturnType(config: Config) : Rule(config) {
)

override fun visitNamedFunction(function: KtNamedFunction) {
if (bindingContext == BindingContext.EMPTY) return
val suspendModifier = function.modifierList?.getModifier(KtTokens.SUSPEND_KEYWORD) ?: return
bindingContext[BindingContext.FUNCTION, function]
?.returnType
Expand Down
Expand Up @@ -16,7 +16,6 @@ import io.gitlab.arturbosch.detekt.rules.fqNameOrNull
import org.jetbrains.kotlin.lexer.KtTokens.EQEQEQ
import org.jetbrains.kotlin.lexer.KtTokens.EXCLEQEQEQ
import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.util.getType

/**
Expand Down Expand Up @@ -62,7 +61,6 @@ class AvoidReferentialEquality(config: Config) : Rule(config) {
}

private fun checkBinaryExpression(expression: KtBinaryExpression) {
if (bindingContext == BindingContext.EMPTY) return
if (expression.operationToken != EQEQEQ && expression.operationToken != EXCLEQEQEQ) return

val checkedType = expression.left?.getType(bindingContext)?.fqNameOrNull() ?: return
Expand Down
Expand Up @@ -11,7 +11,6 @@ import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.diagnostics.Errors
import org.jetbrains.kotlin.psi.KtNamedDeclaration
import org.jetbrains.kotlin.resolve.BindingContext

/**
* Deprecated elements are expected to be removed in the future. Alternatives should be found if possible.
Expand All @@ -30,7 +29,6 @@ class Deprecation(config: Config) : Rule(config) {
override val defaultRuleIdAliases = setOf("DEPRECATION")

override fun visitElement(element: PsiElement) {
if (bindingContext == BindingContext.EMPTY) return
if (hasDeprecationCompilerWarnings(element)) {
val entity = if (element is KtNamedDeclaration) Entity.atName(element) else Entity.from(element)
report(CodeSmell(issue, entity, "${element.text} is deprecated."))
Expand Down
Expand Up @@ -15,7 +15,6 @@ import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtIsExpression
import org.jetbrains.kotlin.psi.KtTypeReference
import org.jetbrains.kotlin.psi.KtUserType
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.util.getType

/**
Expand Down Expand Up @@ -50,16 +49,13 @@ class DontDowncastCollectionTypes(config: Config) : Rule(config) {

override fun visitIsExpression(expression: KtIsExpression) {
super.visitIsExpression(expression)
if (bindingContext == BindingContext.EMPTY) return

checkForDowncast(expression, expression.leftHandSide, expression.typeReference)
}

override fun visitBinaryWithTypeRHSExpression(expression: KtBinaryExpressionWithTypeRHS) {
super.visitBinaryWithTypeRHSExpression(expression)

if (bindingContext == BindingContext.EMPTY) return

checkForDowncast(expression, expression.left, expression.right)
}

Expand Down
Expand Up @@ -65,8 +65,6 @@ class DoubleMutabilityForCollection(config: Config = Config.empty) : Rule(config
override fun visitProperty(property: KtProperty) {
super.visitProperty(property)

if (bindingContext == BindingContext.EMPTY) return

val type = (bindingContext[BindingContext.VARIABLE, property])?.type ?: return
val standardType = type.fqNameOrNull()
if (property.isVar && standardType in mutableTypes) {
Expand Down
Expand Up @@ -10,7 +10,6 @@ import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import org.jetbrains.kotlin.cfg.WhenChecker
import org.jetbrains.kotlin.psi.KtWhenExpression
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.util.getType
import org.jetbrains.kotlin.types.KotlinType
import org.jetbrains.kotlin.types.typeUtil.isBooleanOrNullableBoolean
Expand Down Expand Up @@ -63,7 +62,6 @@ class ElseCaseInsteadOfExhaustiveWhen(config: Config = Config.empty) : Rule(conf
override fun visitWhenExpression(whenExpression: KtWhenExpression) {
super.visitWhenExpression(whenExpression)

if (bindingContext == BindingContext.EMPTY) return
checkForElseCaseInsteadOfExhaustiveWhenExpression(whenExpression)
}

Expand Down

0 comments on commit 5e46099

Please sign in to comment.