Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New rule: SuspendFunWithFlowReturnType #3098

Merged
merged 16 commits into from
Sep 26, 2020
Merged
Show file tree
Hide file tree
Changes from 9 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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ If you contributed to detekt but your name is not in the list, please feel free
- [Veyndan Stuart](https://github.com/veyndan) - New rule: UseEmptyCounterpart; Rule improvement: UselessCallOnNotNull
- [Parimatch Tech](https://github.com/parimatchtech) - New rule: LibraryEntitiesShouldNotBePublic
- [Chao Zhang](https://github.com/chao2zhang) - Rule improvement: ImplicitDefaultLocale, ModifierOrder.
- [Marcelo Hernandez](https://github.com/mhernand40) - New rule: SuspendFunWithFlowReturnType

### Mentions

Expand Down
2 changes: 1 addition & 1 deletion detekt-bom/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ dependencies {
api("com.pinterest.ktlint:ktlint-core:${version.ktlint}")
api("com.pinterest.ktlint:ktlint-ruleset-experimental:${version.ktlint}")
api("org.jetbrains.kotlinx:kotlinx-html-jvm:0.7.2")
api("org.assertj:assertj-core:3.16.1")
Copy link
Contributor Author

@mhernand40 mhernand40 Sep 25, 2020

Choose a reason for hiding this comment

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

FYI, AssertJ already declared on line 12.

api("org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.8")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, when using Coroutines 1.3.9 which is currently the latest version, :detekt-gradle-plugin:test seems to fail with the following error:

org.gradle.api.internal.tasks.testing.TestSuiteExecutionException: Could not complete execution for Gradle Test Executor 5.
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:63)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy2.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.stop(TestWorker.java:133)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164)
	at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:414)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56)
	at java.lang.Thread.run(Thread.java:748)
Caused by: org.junit.platform.commons.JUnitException: TestEngine with ID 'spek2' failed to discover tests
	at org.junit.platform.launcher.core.DefaultLauncher.discoverEngineRoot(DefaultLauncher.java:189)
	at org.junit.platform.launcher.core.DefaultLauncher.discoverRoot(DefaultLauncher.java:168)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:132)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
	... 25 more
Caused by: java.lang.NoSuchMethodError: kotlin.jvm.internal.FunctionReferenceImpl.<init>(ILjava/lang/Class;Ljava/lang/String;Ljava/lang/String;I)V
	at kotlinx.coroutines.flow.internal.SafeCollectorKt$emitFun$1.<init>(SafeCollector.kt)
	at kotlinx.coroutines.flow.internal.SafeCollectorKt.<clinit>(SafeCollector.kt:15)
	at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:73)
	at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:55)
	at org.spekframework.spek2.runtime.SpekRuntime$filterScopes$2.invokeSuspend(SpekRuntime.kt:39)
	at ???(Coroutine boundary.?(?)
	at org.spekframework.spek2.runtime.SpekRuntime$discover$time$1$1.invokeSuspend(SpekRuntime.kt:49)
	at org.spekframework.spek2.runtime.ExecutorsKt$doRunBlocking$1.invokeSuspend(executors.kt:8)
Caused by: java.lang.NoSuchMethodError: kotlin.jvm.internal.FunctionReferenceImpl.<init>(ILjava/lang/Class;Ljava/lang/String;Ljava/lang/String;I)V
	at kotlinx.coroutines.flow.internal.SafeCollectorKt$emitFun$1.<init>(SafeCollector.kt)
	at kotlinx.coroutines.flow.internal.SafeCollectorKt.<clinit>(SafeCollector.kt:15)
	at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:73)
	at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:55)
	at org.spekframework.spek2.runtime.SpekRuntime$filterScopes$2.invokeSuspend(SpekRuntime.kt:39)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:738)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

Copy link
Member

Choose a reason for hiding this comment

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

Interesting this should be the same incompatibility we see in the intellij plugin with Kotlin 1.4 vs 1.3

}
}

Expand Down
2 changes: 2 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ coroutines:
active: false
RedundantSuspendModifier:
active: false
SuspendFunWithFlowReturnType:
active: false

empty-blocks:
active: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ class CoroutinesProvider : DefaultRuleSetProvider {
ruleSetId,
listOf(
GlobalCoroutineUsage(config),
RedundantSuspendModifier(config)
RedundantSuspendModifier(config),
SuspendFunWithFlowReturnType(config)
)
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
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 org.jetbrains.kotlin.js.descriptorUtils.getJetTypeFqName
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.types.KotlinType
import org.jetbrains.kotlin.types.typeUtil.supertypes

/**
* Functions that return `Flow` from `kotlinx.coroutines.flow` should not be marked as `suspend`.
* `Flows` are intended to be cold observable streams. The act of simply invoking a function that
* returns a `Flow`, should not have any side effects. Only once collection begins against the
* returned `Flow`, should work actually be done.
*
* See https://kotlinlang.org/docs/reference/coroutines/flow.html
Copy link
Member

Choose a reason for hiding this comment

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

Can we link to a specific section in this document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this? 27dea52

There is no specific section in the document discouraging suspend functions that return Flow. However when it comes to other Reactive Stream libraries, for example RxJava, it is typically discouraged to "do work" outside of the subscription.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect!

*
* <noncompliant>
* suspend fun observeSignals(): Flow<Unit> {
* val pollingInterval = getPollingInterval() // Done outside of the flow builder block.
* return flow {
* while (true) {
* delay(pollingInterval)
* emit(Unit)
* }
* }
* }
*
* private suspend fun getPollingInterval(): Long {
* // Return the polling interval from some repository
* // in a suspending manner.
* }
* </noncompliant>
*
* <compliant>
* fun observeSignals(): Flow<Unit> {
* return flow {
* val pollingInterval = getPollingInterval() // Moved into the flow builder block.
* while (true) {
* delay(pollingInterval)
* emit(Unit)
* }
* }
* }
*
* private suspend fun getPollingInterval(): Long {
* // Return the polling interval from some repository
* // in a suspending manner.
* }
* </compliant>
*
* @requiresTypeResolution
*/
class SuspendFunWithFlowReturnType(config: Config) : Rule(config) {

override val issue = Issue(
id = "SuspendFunWithFlowReturnType",
severity = Severity.Minor,
description = "`suspend` modifier should not be used for functions returning Coroutines Flow",
Copy link
Member

Choose a reason for hiding this comment

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

Can you extend the description by providing the why for the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Does this work? 8cfc906

debt = Debt.TEN_MINS
)

override fun visitNamedFunction(function: KtNamedFunction) {
if (bindingContext == BindingContext.EMPTY) return
val suspendModifier = function.modifierList?.getModifier(KtTokens.SUSPEND_KEYWORD) ?: return
bindingContext[BindingContext.FUNCTION, function]
?.returnType
?.takeIf { it.isCoroutineFlow() }
?.also {
report(
CodeSmell(
issue = issue,
entity = Entity.from(suspendModifier),
message = "`suspend` function returns Coroutines Flow."
)
)
}
}

private fun KotlinType.isCoroutineFlow(): Boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TOL: isCoroutineFlow() or isCoroutinesFlow()? This is somewhat inconsistent with "Coroutines Flow" in the issue description. It is not always clear when to prefer plural over singular. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I would choose isCoroutinesFlow() :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. For consistency with the issue description, I will make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return sequence {
yield(this@isCoroutineFlow)
yieldAll(this@isCoroutineFlow.supertypes())
}
.map { it.getJetTypeFqName(printTypeArguments = false) }
.contains("kotlinx.coroutines.flow.Flow")
}
}
Loading