Add SuspendFunWithCoroutineScopeReceiver Rule

[hfhbd]

Fixes #4587

[BraisGabin]

Really nice PR! :)

To make CI happy you need to run ./gradlew generateDocumentation and commit the changes that it will add in the default-detekt-config file.

[hfhbd]

Ah, there is the default configuration. I've already searched for it :D

[BraisGabin]

LGTM. Thanks for the contribution!

[hfhbd]

I forgot the embedded Kotlin compiler provided by Gradle uses a different language API than the actual compiler executing the tests. So you can't use Duration, an inline class with compile-test-snippets=true.

[BraisGabin]

I just found this on the project I work: fun foo(block: suspend TestCoroutineScope.() -> Unit) {}. This is basically the same issue. Should we handle it in this rule too?

[hfhbd]

@BraisGabin Sure, added

[codecov]

Codecov Report

Merging #4616 (d8b4bd8) into main (481b3b2) will increase coverage by 84.54%.
The diff coverage is 84.21%.

@@             Coverage Diff             @@
##             main    #4616       +/-   ##
===========================================
+ Coverage        0   84.54%   +84.54%     
- Complexity      0     3381     +3381     
===========================================
  Files           0      483      +483     
  Lines           0    11242    +11242     
  Branches        0     2050     +2050     
===========================================
+ Hits            0     9504     +9504     
- Misses          0      698      +698     
- Partials        0     1040     +1040     

Impacted Files	Coverage Δ
...coroutines/SuspendFunWithCoroutineScopeReceiver.kt	83.33% <83.33%> (ø)
...osch/detekt/rules/coroutines/CoroutinesProvider.kt	100.00% <100.00%> (ø)
...b/arturbosch/detekt/rules/naming/FunctionNaming.kt	100.00% <0.00%> (ø)
...detekt/rules/exceptions/ThrowingExceptionInMain.kt	90.90% <0.00%> (ø)
...itlab/arturbosch/detekt/rules/naming/EnumNaming.kt	93.75% <0.00%> (ø)
...tyle/DestructuringDeclarationWithTooManyEntries.kt	100.00% <0.00%> (ø)
.../gitlab/arturbosch/detekt/rules/ThrowExtensions.kt	50.00% <0.00%> (ø)
...ab/arturbosch/detekt/cli/DetektProgressListener.kt	90.00% <0.00%> (ø)
...lab/arturbosch/detekt/formatting/FormattingRule.kt	98.07% <0.00%> (ø)
...lab/arturbosch/detekt/rules/style/UnusedImports.kt	88.60% <0.00%> (ø)
... and 474 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 481b3b2...d8b4bd8. Read the comment docs.

[BraisGabin]

Oh, it seems that #4630 is making this PR to fail now. I suppose that they are just the indentation of """.

[hfhbd]

Yeah, fixed it.

[hfhbd]

@BraisGabin What about changing compile-test-snippets to true by default, to run this check with gradlew build too?

[BraisGabin]

This is kind of a pain for sure. If we have enable by default we are testing the test code every time. And this task is slow. For that reason we don't have it enable by default.

[hfhbd]

@BraisGabin BTW are you sure about flagging lambdas too? In IntelliJ, only the function with the receiver is marked, but I don't know if this is a bug of IntelliJ...

import kotlinx.coroutines.*
import kotlin.time.Duration.Companion.seconds

suspend fun main() {
    coroutineScope {
        receiver()
    }
    block {
        launch {

        }
    }
}

suspend fun CoroutineScope.receiver(block: suspend CoroutineScope.() -> Unit) { // Ambiguous coroutineContext due to CoroutineScope receiver of suspend function
    delay(1.seconds)
    launch { // Ambiguous coroutineContext due to CoroutineScope receiver of suspend function
        delay(1.seconds)
        block()
    }
}

suspend fun block(block: suspend CoroutineScope.() -> Unit) {
    delay(1.seconds)
    coroutineScope {
        delay(1.seconds)
        block()
    }
} 

[BraisGabin]

block {
  launch { } // Which scope am I using the one from suspend or the receiver? 
}

I think that the issue is the same. But can you open an issue to intellij to confirm that is a missing part on their end?

[hfhbd]

Sure.
CoroutineScope: the inner one wins
coroutineContext: this is the problem, because the outer one wins.

My concern about lambdas is its usage inside coroutine-core itself.

[hfhbd]

Honestly, after reading https://youtrack.jetbrains.com/issue/KTIJ-12776 and https://youtrack.jetbrains.com/issue/KTIJ-13692/Ambiguous-calls-to-coroutineContext-should-be-reported, I still don't know, when we should mark the coroutine builder (suspend lambda with coroutinescope).
https://youtrack.jetbrains.com/issue/KT-52042/Clarifying-suspend-lambda-and-function-with-coroutineScope-as-pa

[BraisGabin]

We can wait to see what they say in the new issue. But for me the issue is the same. In the body of those lambdas is confusing what coroutineContext mean. And that's the main reason to use forbid it in funs.

[hfhbd]

@BraisGabin Got an interesting answer. https://youtrack.jetbrains.com/issue/KT-52042/Clarifying-suspend-lambda-and-function-with-coroutineScope-as-pa#focus=Comments-27-6029021.0-0
So I would revert marking the lambda parameter too and focus on suspend fun CoroutineScope.foo only.