diff --git a/detekt-rules/build.gradle.kts b/detekt-rules/build.gradle.kts index cc3f792..6a8d38c 100644 --- a/detekt-rules/build.gradle.kts +++ b/detekt-rules/build.gradle.kts @@ -15,6 +15,7 @@ dependencies { testImplementation(libs.detekt.test) testImplementation(libs.kotest) testImplementation(libs.jupiter) + testImplementation(kotlin("test")) } java { diff --git a/detekt-rules/src/main/kotlin/ru/otus/detekt/GlobalScopeRule.kt b/detekt-rules/src/main/kotlin/ru/otus/detekt/GlobalScopeRule.kt index 8258aec..b5bcf2c 100644 --- a/detekt-rules/src/main/kotlin/ru/otus/detekt/GlobalScopeRule.kt +++ b/detekt-rules/src/main/kotlin/ru/otus/detekt/GlobalScopeRule.kt @@ -1,10 +1,14 @@ package ru.otus.detekt +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 org.jetbrains.kotlin.psi.KtDotQualifiedExpression +import org.jetbrains.kotlin.resolve.calls.util.getCalleeExpressionIfAny class GlobalScopeRule(config: Config) : Rule(config) { override val issue: Issue = Issue( @@ -14,5 +18,18 @@ class GlobalScopeRule(config: Config) : Rule(config) { debt = Debt.FIVE_MINS ) - // TODO + override fun visitDotQualifiedExpression(expression: KtDotQualifiedExpression) { + super.visitDotQualifiedExpression(expression) + if (expression.receiverExpression.text.contains("GlobalScope") && + expression.getCalleeExpressionIfAny()?.text in listOf("launch", "async") + ) { + report( + CodeSmell( + issue = issue, + entity = Entity.from(expression), + message = "Avoid using GlobalScope for Coroutines" + ) + ) + } + } } diff --git a/detekt-rules/src/main/kotlin/ru/otus/detekt/TopLevelCoroutineInSuspendFunRule.kt b/detekt-rules/src/main/kotlin/ru/otus/detekt/TopLevelCoroutineInSuspendFunRule.kt index abd3717..4bbeae1 100644 --- a/detekt-rules/src/main/kotlin/ru/otus/detekt/TopLevelCoroutineInSuspendFunRule.kt +++ b/detekt-rules/src/main/kotlin/ru/otus/detekt/TopLevelCoroutineInSuspendFunRule.kt @@ -1,10 +1,24 @@ package ru.otus.detekt +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.rules.fqNameOrNull +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.name.FqName +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtDotQualifiedExpression +import org.jetbrains.kotlin.psi.KtFunction +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.psiUtil.getCallNameExpression +import org.jetbrains.kotlin.psi.psiUtil.hasSuspendModifier +import org.jetbrains.kotlin.psi.psiUtil.parents +import org.jetbrains.kotlin.resolve.calls.util.getType +import org.jetbrains.kotlin.types.typeUtil.supertypes class TopLevelCoroutineInSuspendFunRule(config: Config) : Rule(config) { override val issue: Issue = Issue( @@ -14,5 +28,63 @@ class TopLevelCoroutineInSuspendFunRule(config: Config) : Rule(config) { debt = Debt.FIVE_MINS ) - // TODO + override fun visitCallExpression(expression: KtCallExpression) { + super.visitCallExpression(expression) + + if (expression.containingFunction()?.modifierList?.hasSuspendModifier() != true) return + + val coroutineBuilders = setOf("launch", "async") + val callName = expression.getCallNameExpression()?.text + if (callName !in coroutineBuilders) return + + var parent = expression.parent + while (parent != null) { + when (parent) { + is KtDotQualifiedExpression -> { + val receiver = parent.receiverExpression + val receiverType = receiver.getType(bindingContext)?.fqNameOrNull() + if ( + receiverType == FqName("kotlinx.coroutines.CoroutineScope") || + receiver.text == "viewModelScope" + ) { + if (isInsideSuspendFunction(parent)) { + reportCodeSmell(expression) + return + } + } + } + is KtCallExpression -> { + if ( + parent.getType(bindingContext) + ?.supertypes() + ?.any { it.fqNameOrNull() == FqName("kotlinx.coroutines.CoroutineScope") } == true + ) { + if (isInsideSuspendFunction(parent)) { + reportCodeSmell(expression) + return + } + } + } + } + parent = parent.parent + } + } + + private fun KtCallExpression.containingFunction(): KtFunction? = + this.parents.filterIsInstance().firstOrNull() + + private fun isInsideSuspendFunction(element: PsiElement): Boolean { + return element.parents.filterIsInstance().firstOrNull()?.modifierList?.hasSuspendModifier() == true + } + + private fun reportCodeSmell(expression: KtCallExpression) { + report( + CodeSmell( + issue = issue, + entity = Entity.from(expression), + message = "Avoid running top level coroutines inside suspend functions" + ) + ) + } + } diff --git a/detekt-rules/src/test/kotlin/ru/otus/detekt/GlobalScopeRuleTest.kt b/detekt-rules/src/test/kotlin/ru/otus/detekt/GlobalScopeRuleTest.kt new file mode 100644 index 0000000..b55bfe5 --- /dev/null +++ b/detekt-rules/src/test/kotlin/ru/otus/detekt/GlobalScopeRuleTest.kt @@ -0,0 +1,63 @@ +package ru.otus.detekt + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest +import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext +import io.kotest.matchers.collections.shouldHaveSize +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.junit.jupiter.api.Test + +@KotlinCoreEnvironmentTest +internal class GlobalScopeRuleTest(private val env: KotlinCoreEnvironment) { + private val rule = GlobalScopeRule(Config.empty) + + private fun assertFindings(code: String, expectedSize: Int) { + val findings = rule.compileAndLintWithContext(env, code.trimIndent()) + findings shouldHaveSize expectedSize + } + + @Test + fun `should report GlobalScope launch`() = assertFindings(""" + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.launch + fun loadInfo() { + GlobalScope.launch { } + } + """, 1) + + @Test + fun `should report GlobalScope async`() = assertFindings(""" + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.async + fun loadInfo() { + GlobalScope.async { } + } + """, 1) + + @Test + fun `should not report 'val scope = GlobalScope'`() = assertFindings(""" + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.GlobalScope + fun loadInfo(scope: CoroutineScope) = Unit + fun fetchData() { + val scope = GlobalScope + loadInfo(scope) + } + """, 0) + + @Test + fun `should not report CoroutineScope launch`() = assertFindings(""" + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.launch + fun example(scope: CoroutineScope) { + scope.launch { println("This is a coroutine") } + } + """, 0) + + @Test + fun `should not report non-coroutine code`() = assertFindings(""" + fun example() { + println("This is not a coroutine") + } + """, 0) +} diff --git a/detekt-rules/src/test/kotlin/ru/otus/detekt/TopLevelCoroutineRuleTest.kt b/detekt-rules/src/test/kotlin/ru/otus/detekt/TopLevelCoroutineRuleTest.kt new file mode 100644 index 0000000..60bbbae --- /dev/null +++ b/detekt-rules/src/test/kotlin/ru/otus/detekt/TopLevelCoroutineRuleTest.kt @@ -0,0 +1,158 @@ +package ru.otus.detekt + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest +import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext +import io.kotest.matchers.collections.shouldHaveSize +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.junit.jupiter.api.Test + +@KotlinCoreEnvironmentTest +internal class TopLevelCoroutineRuleTest(private val env: KotlinCoreEnvironment) { + private val rule = TopLevelCoroutineInSuspendFunRule(Config.empty) + + private fun assertFindings( + code: String, + expectedSize: Int + ) { + val findings = rule.compileAndLintWithContext(env, code.trimIndent()) + findings shouldHaveSize expectedSize + } + + @Test + fun `reports launch in suspend fun`() = assertFindings( + """ + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + suspend fun loadInfo() { + CoroutineScope(Dispatchers.Default).launch { } + } + """, + 1 + ) + + @Test + fun `reports launch and async in suspend fun with apply`() = assertFindings( + """ + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + suspend fun loadInfo() { + CoroutineScope(Dispatchers.Default).apply { + launch { } + async { } + } + } + """, + 2 + ) + + @Test + fun `reports launch in suspend fun with val scope`() = assertFindings( + """ + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + suspend fun loadInfo() { + val scope = CoroutineScope(Dispatchers.Default) + scope.launch { } + } + """, + 1 + ) + + @Test + fun `reports launch in suspend fun with viewModelScope`() = assertFindings( + """ + import androidx.lifecycle.viewModelScope + import kotlinx.coroutines.launch + suspend fun loadInfo() { + viewModelScope.launch { } + } + """, + 1 + ) + + @Test + fun `reports launch in suspend fun with scope from parameter`() = assertFindings( + """ + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + suspend fun loadInfo(scope: CoroutineScope) { + scope.launch { } + } + """, + 1 + ) + + @Test + fun `reports async in suspend fun`() = assertFindings( + """ + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + suspend fun loadInfo() { + CoroutineScope(Dispatchers.Default).async { } + } + """, + 1 + ) + + @Test + fun `no report launch in coroutineScope`() = assertFindings( + """ + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + import kotlinx.coroutines.coroutineScope + suspend fun loadInfo() { + coroutineScope { + launch {} + async {} + } + } + """, + 0 + ) + + @Test + fun `no report launch in supervisor scope`() = assertFindings( + """ + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + import kotlinx.coroutines.supervisorScope + suspend fun loadInfo() { + supervisorScope { + launch {} + async {} + } + } + """, + 0 + ) + + @Test + fun `no reports launch in not suspend fun`() = assertFindings( + """ + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + fun loadInfo() { + CoroutineScope(Dispatchers.Default).launch { } + } + """, + 0 + ) + + @Test + fun `no reports async in not suspend fun`() = assertFindings( + """ + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + fun loadInfo() { + CoroutineScope(Dispatchers.Default).async { } + } + """, + 0 + ) + +}