Skip to content

Commit

Permalink
Rework logic as to first find sleep call
Browse files Browse the repository at this point in the history 
Add more TCs around which failed in previous implementation
  • Loading branch information
@atulgpt
atulgpt committed Feb 25, 2023
1 parent e101083 commit cc57ddf
Show file tree
Hide file tree
Showing 2 changed files with 167 additions and 53 deletions.
90 changes: 58 additions & 32 deletions ...tines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelay.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import org.jetbrains.kotlin.builtins.isSuspendFunctionType
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.descriptors.FunctionDescriptor
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtCallableReferenceExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtFunctionLiteral
import org.jetbrains.kotlin.psi.KtLambdaArgument
import org.jetbrains.kotlin.psi.KtLambdaExpression
import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.psi.KtValueArgument
import org.jetbrains.kotlin.psi.psiUtil.forEachDescendantOfType
import org.jetbrains.kotlin.psi.psiUtil.getParentOfType
import org.jetbrains.kotlin.psi.psiUtil.getParentOfTypes
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 @@ -55,36 +55,62 @@ class SleepInsteadOfDelay(config: Config = Config.empty) : Rule(config) {
Debt.FIVE_MINS
)

override fun visitBlockExpression(expression: KtBlockExpression) {
expression.forEachDescendantOfType<KtCallExpression>(::shouldTraverseInside) {
it.verifyExpression(SUSPEND_FUN_MESSAGE)
override fun visitCallExpression(expression: KtCallExpression) {
super.visitCallExpression(expression)
checkAndReport(expression)
}

override fun visitCallableReferenceExpression(expression: KtCallableReferenceExpression) {
super.visitCallableReferenceExpression(expression)
checkAndReport(expression)
}

private fun checkAndReport(expression: KtExpression) {
if (expression.isThreadSleepFunction() && expression.isInSuspendBlock()) {
report(CodeSmell(issue, Entity.from(expression), SUSPEND_FUN_MESSAGE))
}
}

expression.forEachDescendantOfType<KtCallableReferenceExpression>(::shouldTraverseInside) {
it.verifyExpression(SUSPEND_FUN_MESSAGE)
private fun KtExpression.isThreadSleepFunction(): Boolean {
fun KtCallableReferenceExpression.isSleepCallableRef(): Boolean {
return if (this.parent is KtValueArgument) {
// Only checking if this is used as for invocation
this.callableReference.isThreadSleepFunction()
} else {
false
}
}
return if (this is KtCallableReferenceExpression) {
this.isSleepCallableRef()
} else {
getResolvedCall(bindingContext)
?.resultingDescriptor
?.fqNameOrNull()
?.asString() == FQ_NAME
}
}

@Suppress("ReturnCount")
private fun shouldTraverseInside(psiElement: PsiElement): Boolean {
return when (psiElement) {
is KtLambdaExpression -> {
val psiParent = psiElement.parent
private fun PsiElement.isSuspendingBlock(): Boolean {
return when (this) {
is KtFunctionLiteral -> {
val psiParent = this.parent.parent
if (psiParent is KtLambdaArgument) {
return psiParent.shouldTraverseInside()
psiParent.isSuspendAllowedLambdaArgument()
} else {
this.isSuspendScope()
}
bindingContext[BindingContext.EXPRESSION_TYPE_INFO, psiElement]?.let {
it.type?.isSuspendFunctionType == true
} ?: false
}
is KtNamedFunction -> {
this.isSuspendScope()
}
else -> {
true
false
}
}
}

@Suppress("ReturnCount")
private fun KtLambdaArgument.shouldTraverseInside(): Boolean {
private fun KtLambdaArgument.isSuspendAllowedLambdaArgument(): Boolean {
val callExpression = this.getParentOfType<KtCallExpression>(true)
val callDescriptor = callExpression?.getResolvedCall(bindingContext) ?: return false
val functionDescriptor = callDescriptor.resultingDescriptor as? FunctionDescriptor ?: return false
Expand All @@ -96,21 +122,21 @@ class SleepInsteadOfDelay(config: Config = Config.empty) : Rule(config) {
) || valueParameterDescriptor.returnType?.isSuspendFunctionType == true
}

private fun KtExpression.verifyExpression(message: String) {
val fqName = getResolvedCall(bindingContext)
?.resultingDescriptor
?.fqNameOrNull()
?.asString()
if (fqName == FQ_NAME) {
report(CodeSmell(issue, Entity.from(this), message))
}
@Suppress("ReturnCount")
private fun PsiElement.isSuspendScope(): Boolean {
val functionDescriptor = bindingContext[BindingContext.FUNCTION, this] ?: return false
return functionDescriptor.isSuspend
}

private fun KtCallableReferenceExpression.verifyExpression(message: String) {
if (this.parent is KtValueArgument) {
// Only checking if this is used as for invocation
this.callableReference.verifyExpression(message)
}
private fun KtExpression.isInSuspendBlock(): Boolean {
val containingBlockExpression =
this.getParentOfTypes<PsiElement>(
true,
KtLambdaArgument::class.java,
KtNamedFunction::class.java,
KtFunctionLiteral::class.java
) ?: return false
return containingBlockExpression.isSuspendingBlock()
}

companion object {
Expand Down
130 changes: 109 additions & 21 deletions ...s/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelaySpec.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `should report overloaded Thread sleep() in suspend functions`() {
val code = """
suspend fun foo() {
Thread.sleep(1000L, 1000)
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
@DisplayName("should report Thread.sleep() in CoroutineScope.launch()")
fun reportThreadSleepInCoroutineScopeLaunch() {
Expand Down Expand Up @@ -191,6 +201,66 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `should report Thread sleep() called in custom inline function inside suspend crossinline lambda`() {
val code = """
import kotlinx.coroutines.MainScope
import kotlinx.coroutines.launch
inline fun suspendBlock(crossinline lambda: suspend () -> Unit) {
MainScope().launch {
lambda()
}
}
fun test() {
suspendBlock {
Thread.sleep(1000L)
}
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `should report Thread sleep() called in custom inline function inside suspend noinline lambda`() {
val code = """
import kotlinx.coroutines.MainScope
import kotlinx.coroutines.launch
inline fun suspendBlock(noinline lambda: suspend () -> Unit) {
MainScope().launch {
lambda()
}
}
fun test() {
suspendBlock {
Thread.sleep(1000L)
}
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `should report Thread sleep() inside local function defined inside lambda`() {
val code = """
fun normalBlock(lambda: () -> Unit) {
lambda()
}
fun test() {
normalBlock {
suspend fun test() {
Thread.sleep(1000L)
}
}
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `should report Thread sleep() when called inside withContext`() {
@Suppress("DeferredResultUnused")
Expand All @@ -199,7 +269,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
fun test() {
GlobalScope.launch {
withContext(Dispatchers.IO) {
Expand Down Expand Up @@ -239,7 +309,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
fun test() {
GlobalScope.launch {
val funRef: (Long) -> Unit = Thread::sleep
Expand All @@ -257,7 +327,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
@Suppress("RedundantSuspendModifier")
suspend fun test() {
GlobalScope.launch {
Expand All @@ -279,7 +349,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
fun test() {
GlobalScope.launch {
listOf(1L, 2L, 3L).map(Thread::sleep)
Expand All @@ -297,7 +367,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
fun test() {
GlobalScope.launch {
listOf(1L, 2L, 3L).map {
Expand All @@ -316,7 +386,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
inline fun inlineFun(lambda: () -> Unit) = lambda()
suspend fun test() {
Expand All @@ -333,7 +403,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
fun test() {
GlobalScope.launch {
val localLambda: suspend () -> Unit = {
Expand All @@ -348,11 +418,9 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
@Test
fun `should not report Thread sleep() called inside non-suspend lambda variable`() {
val code = """
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
suspend fun test() {
GlobalScope.launch {
val localLambda: () -> Unit = {
Expand All @@ -364,14 +432,40 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}

@Test
fun `should report Thread sleep() called inside suspend local fun inside non-suspend lambda variable`() {
val code = """
fun test() {
val localLambda: () -> Unit = {
suspend fun test() {
Thread.sleep(1000L)
}
}
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `should report Thread sleep() called inside suspend local fun inside nested non-suspend lambda variable`() {
val code = """
val localLambda: () -> Unit = {
val localLambda2: () -> Unit = {
suspend fun test() {
Thread.sleep(1000L)
}
}
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `should report Thread sleep() only once when called inside both suspend function and suspend lambda block`() {
val code = """
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
suspend fun test() {
GlobalScope.launch {
Thread.sleep(1000)
Expand All @@ -384,11 +478,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
@Test
fun `should report Thread sleep() only once called inside nested suspend fun`() {
val code = """
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
suspend fun test() {
suspend fun test2() {
Thread.sleep(1000)
Expand All @@ -401,11 +491,9 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) {
@Test
fun `should report Thread sleep() only once when called inside nested suspend lambda`() {
val code = """
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
fun test() {
GlobalScope.launch {
GlobalScope.launch {
Expand Down

0 comments on commit cc57ddf

Please sign in to comment.