Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion app/config/detekt/detekt.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,9 @@ naming:
ignoreAnnotated:
- 'Composable'
OtusRuleSet:
ComposeModifierRule:
ComposeModifierMissingRule:
active: true
GlobalScopeRule:
active: true
TopLevelCoroutineInSuspendFunRule:
active: true
14 changes: 11 additions & 3 deletions app/src/main/java/ru/otus/example/SampleViewModel.kt
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package ru.otus.example

import android.util.Log
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.launch

class SampleViewModel : ViewModel() {
Expand All @@ -16,8 +16,16 @@ class SampleViewModel : ViewModel() {
}

suspend fun coroutineLaunchInSuspendViolation() {
viewModelScope.launch {
val globalScope = GlobalScope
globalScope.launch { }
GlobalScope.async {

}
coroutineScope {

}
viewModelScope.launch {
println()
}
}
}
24 changes: 23 additions & 1 deletion detekt-rules/src/main/kotlin/ru/otus/detekt/GlobalScopeRule.kt
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
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

class GlobalScopeRule(config: Config) : Rule(config) {
override val issue: Issue = Issue(
Expand All @@ -14,5 +17,24 @@ class GlobalScopeRule(config: Config) : Rule(config) {
debt = Debt.FIVE_MINS
)

// TODO
override fun visitDotQualifiedExpression(expression: KtDotQualifiedExpression) {
super.visitDotQualifiedExpression(expression)
val isGlobalScopeUsing = expression.firstChild.text == "GlobalScope"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вместо firstChild и lastChild более идиоматичным будет использование receiverExpression и selectorExpression

if (isGlobalScopeUsing) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Предпочитайте ранние возвраты из функций, чтобы не делать вложенные if

val callingFun = expression.lastChild.firstChild
if (callingFun != null) {
val funText = callingFun.text
if (funText == "launch" || funText == "async") {
report(
CodeSmell(
issue = issue,
entity = Entity.from(expression),
message = "Avoid using GlobalScope " +
"More: https://elizarov.medium.com/the-reason-to-avoid-globalscope-835337445abc"
)
)
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,100 @@
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.KtNodeTypes
import org.jetbrains.kotlin.descriptors.impl.LocalVariableDescriptor
import org.jetbrains.kotlin.fir.analysis.forEachChildOfType
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.psi.KtReferenceExpression
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameOrNull
import org.jetbrains.kotlin.serialization.deserialization.descriptors.DeserializedClassDescriptor
import org.jetbrains.kotlin.serialization.deserialization.descriptors.DeserializedSimpleFunctionDescriptor

class TopLevelCoroutineInSuspendFunRule(config: Config) : Rule(config) {
override val issue: Issue = Issue(
id = javaClass.simpleName,
severity = Severity.CodeSmell,
description = "Avoid running top level coroutines inside suspend functions",
debt = Debt.FIVE_MINS
debt = Debt.FIVE_MINS,
)

// TODO
override fun visitNamedFunction(function: KtNamedFunction) {
super.visitNamedFunction(function)
val isSuspend = function.firstChild.text == "suspend"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Модификатор suspend лучше проверять через список модификаторов: function.modifierList?.hasSuspendModifier()

if (isSuspend) {
function.bodyBlockExpression?.forEachChildOfType(
types = setOf(KtNodeTypes.DOT_QUALIFIED_EXPRESSION)
) { dotQualifiedExpression ->
var reference = dotQualifiedExpression.firstChild
if (reference is KtCallExpression) {
reference = reference.firstChild
}
val callingFun = dotQualifiedExpression.lastChild.firstChild
if (bindingContext != BindingContext.EMPTY) {
val descriptor = bindingContext[
BindingContext.REFERENCE_TARGET,
reference as KtReferenceExpression
]

// e.g. GlobalScope.launch {
val isCoroutineScopeClass = (descriptor as? DeserializedClassDescriptor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

В целом здесь можно было не разделять на разные дескрипторы, достаточно было получить тип у receiverExpression и проверить, что его fqName равен kotlinx.coroutines.CoroutineScope, но так тоже можно

?.isCoroutineScope()
// e.g. scopeVal.launch {
val isCoroutineScopeValue = (descriptor as? LocalVariableDescriptor)
?.isCoroutineScope()
// e.g. CoroutineScope(Dispatchers.IO).launch {
val isCoroutineScopeSimpleFunction =
(descriptor as? DeserializedSimpleFunctionDescriptor)
?.isCoroutineScope()

if (isCoroutineScopeClass == true || isCoroutineScopeValue == true ||
isCoroutineScopeSimpleFunction == true
) {
if (callingFun != null) {
val funText = callingFun.text
if (funText == "launch" || funText == "async") {
report(
CodeSmell(
issue = issue,
entity = Entity.from(dotQualifiedExpression),
message = "Avoid running top level coroutines inside suspend functions" +
"More: " +
"https://elizarov.medium.com/structured-concurrency-722d765aa952"
)
)
}
}
}
}
}
}
}

private val coroutineScopeFqName = FqName("kotlinx.coroutines.CoroutineScope")

private fun DeserializedSimpleFunctionDescriptor.isCoroutineScope() =
this.returnType?.fqNameOrNull() == FqName("kotlinx.coroutines.CoroutineScope")

private fun DeserializedClassDescriptor.isCoroutineScope() =
this.fqNameOrNull() == coroutineScopeFqName ||
this.typeConstructor.supertypes.any { type ->
type.fqNameOrNull() == coroutineScopeFqName
}

private fun LocalVariableDescriptor.isCoroutineScope() =
this.fqNameOrNull() == coroutineScopeFqName ||
this.type.constructor
.supertypes.any { type ->
type.fqNameOrNull() == coroutineScopeFqName
}
}
73 changes: 73 additions & 0 deletions detekt-rules/src/test/kotlin/ru/otus/detekt/GlobalScopeRuleTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
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)

@Test
fun `should report GlobalScope launch`() {
val code = """
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.async
import kotlinx.coroutines.launch

fun test() {
GlobalScope.launch { }
}
"""
val findings = rule.compileAndLintWithContext(env, code)
findings shouldHaveSize 1
}

@Test
fun `should report GlobalScope async`() {
val code = """
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.async
import kotlinx.coroutines.launch

fun test() {
GlobalScope.async { }
}
"""
val findings = rule.compileAndLintWithContext(env, code)
findings shouldHaveSize 1
}

@Test
fun `should not report GlobalScope toString`() {
val code = """
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.async
import kotlinx.coroutines.launch

fun test() {
GlobalScope.toString()
}
"""
val findings = rule.compileAndLintWithContext(env, code)
findings shouldHaveSize 0
}

@Test
fun `should not report GlobalScope string`() {
val code = """
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.async
import kotlinx.coroutines.launch

fun test() {
printLn("GlobalScope.launch")
}
"""
val findings = rule.compileAndLintWithContext(env, code)
findings shouldHaveSize 0
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
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 TopLevelCoroutineInSuspendFunRuleTest(private val env: KotlinCoreEnvironment) {
private val rule = TopLevelCoroutineInSuspendFunRule(Config.empty)

@Test
fun `should detect coroutine launch inside suspend function`() {
val code = """
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.async
import kotlinx.coroutines.launch
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.supervisorScope

suspend fun test() {
CoroutineScope(Dispatchers.Main).launch { }
GlobalScope.launch { }
GlobalScope.async { }
CoroutineScope.launch { }

coroutineScope { }
}
"""
val findings = rule.compileAndLintWithContext(env, code)
findings shouldHaveSize 4
}

@Test
fun `should detect coroutine launch using scope value`() {
val code = """
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.async
import kotlinx.coroutines.launch

suspend fun test() {
println("suspend")
val g = GlobalScope
g.launch { }

var g1 = GlobalScope
g1.launch { }
}
"""
val findings = rule.compileAndLintWithContext(env, code)
findings shouldHaveSize 2
}

@Test
fun `should not detect coroutine launch in ordinary function`() {
val code = """
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.async
import kotlinx.coroutines.launch

fun test3() {
GlobalScope.launch { }
}
"""
val findings = rule.compileAndLintWithContext(env, code)
findings shouldHaveSize 0
}
}