-
-
Notifications
You must be signed in to change notification settings - Fork 755
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add SuspendFunWithCoroutineScopeReceiverRule
- Loading branch information
Showing
4 changed files
with
341 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
108 changes: 108 additions & 0 deletions
108
...tlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiver.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
package io.gitlab.arturbosch.detekt.rules.coroutines | ||
|
||
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.api.internal.RequiresTypeResolution | ||
import io.gitlab.arturbosch.detekt.rules.fqNameOrNull | ||
import org.jetbrains.kotlin.builtins.getReceiverTypeFromFunctionType | ||
import org.jetbrains.kotlin.builtins.isSuspendFunctionType | ||
import org.jetbrains.kotlin.lexer.KtTokens | ||
import org.jetbrains.kotlin.psi.KtNamedFunction | ||
import org.jetbrains.kotlin.psi.KtParameter | ||
import org.jetbrains.kotlin.resolve.BindingContext | ||
import org.jetbrains.kotlin.types.KotlinType | ||
import org.jetbrains.kotlin.types.typeUtil.supertypes | ||
|
||
/** | ||
* Suspend functions that use `CoroutineScope` as receiver should not be marked as `suspend`. | ||
* A `CoroutineScope` provides structured concurrency via its `coroutineContext`. A `suspend` | ||
* function also has its own `coroutineContext`, which is now ambiguous and mixed with the | ||
* receiver`s. | ||
* | ||
* See https://kotlinlang.org/docs/coroutines-basics.html#scope-builder-and-concurrency | ||
* | ||
* <noncompliant> | ||
* suspend fun CoroutineScope.foo() { | ||
* launch { | ||
* delay(1.seconds) | ||
* } | ||
* } | ||
* </noncompliant> | ||
* | ||
* <compliant> | ||
* fun CoroutineScope.foo() { | ||
* launch { | ||
* delay(1.seconds) | ||
* } | ||
* } | ||
* | ||
* // Alternative | ||
* suspend fun foo() = coroutineScope { | ||
* launch { | ||
* delay(1.seconds) | ||
* } | ||
* } | ||
* </compliant> | ||
* | ||
*/ | ||
@RequiresTypeResolution | ||
class SuspendFunWithCoroutineScopeReceiver(config: Config) : Rule(config) { | ||
|
||
override val issue = Issue( | ||
id = "SuspendFunWithCoroutineScopeReceiver", | ||
severity = Severity.Minor, | ||
description = "The `suspend` modifier should not be used for functions that use a " + | ||
"CoroutinesScope as receiver. You should use suspend functions without the receiver or use plain " + | ||
"functions and use coroutineScope { } instead.", | ||
debt = Debt.TEN_MINS | ||
) | ||
|
||
override fun visitNamedFunction(function: KtNamedFunction) { | ||
if (bindingContext == BindingContext.EMPTY) return | ||
checkReceiver(function) | ||
checkLambdaParameters(function.valueParameters) | ||
} | ||
|
||
private fun checkLambdaParameters(parameters: List<KtParameter>) { | ||
for (it in parameters) { | ||
val type = bindingContext[BindingContext.VALUE_PARAMETER, it] | ||
?.type?.takeIf { it.isSuspendFunctionType } ?: continue | ||
if (type.getReceiverTypeFromFunctionType()?.isCoroutineScope() == true) { | ||
report( | ||
CodeSmell( | ||
issue = issue, | ||
entity = Entity.Companion.from(it), | ||
message = "`suspend` function uses CoroutineScope as receiver." | ||
) | ||
) | ||
} | ||
} | ||
} | ||
|
||
private fun checkReceiver(function: KtNamedFunction) { | ||
val suspendModifier = function.modifierList?.getModifier(KtTokens.SUSPEND_KEYWORD) ?: return | ||
val receiver = bindingContext[BindingContext.FUNCTION, function] | ||
?.extensionReceiverParameter?.value?.type ?: return | ||
if (receiver.isCoroutineScope()) { | ||
report( | ||
CodeSmell( | ||
issue = issue, | ||
entity = Entity.from(suspendModifier), | ||
message = "`suspend` function uses CoroutineScope as receiver." | ||
) | ||
) | ||
} | ||
} | ||
|
||
private fun KotlinType.isCoroutineScope() = sequence { | ||
yield(this@isCoroutineScope) | ||
yieldAll(this@isCoroutineScope.supertypes()) | ||
} | ||
.mapNotNull { it.fqNameOrNull()?.asString() } | ||
.contains("kotlinx.coroutines.CoroutineScope") | ||
} |
229 changes: 229 additions & 0 deletions
229
.../io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiverSpec.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,229 @@ | ||
package io.gitlab.arturbosch.detekt.rules.coroutines | ||
|
||
import io.gitlab.arturbosch.detekt.api.Config | ||
import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest | ||
import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext | ||
import org.assertj.core.api.Assertions.assertThat | ||
import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment | ||
import org.junit.jupiter.api.Nested | ||
import org.junit.jupiter.api.Test | ||
|
||
@KotlinCoreEnvironmentTest | ||
class SuspendFunWithCoroutineScopeReceiverSpec(val env: KotlinCoreEnvironment) { | ||
|
||
val subject = SuspendFunWithCoroutineScopeReceiver(Config.empty) | ||
|
||
@Nested | ||
inner class `SuspendFunWithCoroutineScopeReceiver rule` { | ||
|
||
@Test | ||
fun `reports when top-level suspend function has explicit CoroutineScope receiver type`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
suspend fun CoroutineScope.foo() { | ||
launch { | ||
delay(timeMillis = 1000) | ||
} | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) | ||
} | ||
|
||
@Test | ||
fun `reports when top-level suspend function has explicit CoroutineScope receiver type and star import used`() { | ||
val code = """ | ||
import kotlinx.coroutines.* | ||
suspend fun CoroutineScope.foo() { | ||
launch { | ||
delay(timeMillis = 1000) | ||
} | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) | ||
} | ||
|
||
@Test | ||
fun `reports when top-level suspend function has explicit FQN CoroutineScope receiver type`() { | ||
val code = """ | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
suspend fun kotlinx.coroutines.CoroutineScope.foo() { | ||
launch { | ||
delay(timeMillis = 1000) | ||
} | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) | ||
} | ||
|
||
@Test | ||
fun `reports when suspend function has a receiver which inherits from CoroutineScope`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
interface TestScope: CoroutineScope | ||
suspend fun TestScope.foo() { | ||
launch { | ||
delay(timeMillis = 1000) | ||
} | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) | ||
} | ||
|
||
@Test | ||
fun `no reports when plain function has a CoroutineScope as receiver`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
fun CoroutineScope.foo() { | ||
launch { | ||
delay(timeMillis = 1000) | ||
} | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() | ||
} | ||
|
||
@Test | ||
fun `no reports when suspend function has no receiver`() { | ||
val code = """ | ||
import kotlinx.coroutines.coroutineScope | ||
import kotlinx.coroutines.delay | ||
import kotlin.time.Duration.Companion.seconds | ||
suspend fun foo() = coroutineScope { | ||
launch { | ||
delay(timeMillis = 1000) | ||
} | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() | ||
} | ||
|
||
@Test | ||
fun `no reports when suspend function has Long as receiver`() { | ||
val code = """ | ||
import kotlinx.coroutines.delay | ||
suspend fun Long.foo() = coroutineScope { | ||
launch { | ||
delay(timeMillis = this@foo) | ||
} | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() | ||
} | ||
} | ||
|
||
@Nested | ||
inner class SuspendFunWithCoroutineScopeLambda { | ||
|
||
@Test | ||
fun `reports when lambda parameter has suspend and explicit CoroutineScope receiver type`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
import kotlinx.coroutines.coroutineScope | ||
suspend fun foo(action: suspend CoroutineScope.() -> Unit) = coroutineScope { | ||
action() | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) | ||
} | ||
|
||
@Test | ||
fun `reports when lambda parameter has suspend and inherited CoroutineScope receiver type`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
import kotlinx.coroutines.coroutineScope | ||
interface TestScope: CoroutineScope | ||
suspend fun foo(action: suspend TestScope.() -> Unit) = coroutineScope { | ||
val scope = object: TestScope, CoroutineScope by this { } | ||
scope.action() | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) | ||
} | ||
|
||
@Test | ||
fun `no report when lambda parameter has only CoroutineScope receiver type`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
import kotlinx.coroutines.coroutineScope | ||
suspend fun foo(action: CoroutineScope.() -> Unit) = coroutineScope { | ||
action() | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() | ||
} | ||
|
||
@Test | ||
fun `no report when suspend lambda parameter has no CoroutineScope receiver type`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
import kotlinx.coroutines.coroutineScope | ||
suspend fun foo(action: suspend Int.() -> Unit) { | ||
1.action() | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() | ||
} | ||
|
||
@Test | ||
fun `no report when suspend lambda parameter has no receiver`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
import kotlinx.coroutines.coroutineScope | ||
suspend fun foo(action: suspend () -> Unit) { | ||
action() | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() | ||
} | ||
} | ||
|
||
@Nested | ||
inner class SuspendCoroutineFunWithCoroutineScopeLambda { | ||
|
||
@Test | ||
fun `reports when lambda parameter has suspend CoroutineScope receiver type and its lambda too`() { | ||
val code = """ | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.delay | ||
import kotlinx.coroutines.coroutineScope | ||
suspend fun CoroutineScope.foo(action: suspend CoroutineScope.() -> Unit) { | ||
action() | ||
} | ||
""" | ||
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(2) | ||
} | ||
} | ||
} |