New rule: SuspendFunWithFlowReturnType by mhernand40 · Pull Request #3098 · detekt/detekt

mhernand40 · 2020-09-25T04:11:15Z

A Coroutines rule that reports any functions marked as suspend that also return a Flow.

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.

Addresses #3086

codecov · 2020-09-25T04:45:29Z

Codecov Report

Merging #3098 into master will increase coverage by 0.15%.
The diff coverage is 86.95%.

@@             Coverage Diff              @@
##             master    #3098      +/-   ##
============================================
+ Coverage     79.28%   79.43%   +0.15%     
- Complexity     2577     2591      +14     
============================================
  Files           435      437       +2     
  Lines          7766     7813      +47     
  Branches       1482     1484       +2     
============================================
+ Hits           6157     6206      +49     
  Misses          819      819              
+ Partials        790      788       -2

Impacted Files	Coverage Δ	Complexity Δ
...t/rules/coroutines/SuspendFunWithFlowReturnType.kt	`85.71% <85.71%> (ø)`	`7.00 <7.00> (?)`
...osch/detekt/rules/coroutines/CoroutinesProvider.kt	`100.00% <100.00%> (ø)`	`3.00 <0.00> (ø)`
...urbosch/detekt/rules/bugs/ImplicitDefaultLocale.kt	`74.19% <0.00%> (ø)`	`9.00% <0.00%> (ø%)`
...osch/detekt/rules/complexity/ComplexityProvider.kt	`100.00% <0.00%> (ø)`	`3.00% <0.00%> (ø%)`
...kt/rules/complexity/ReplaceSafeCallChainWithRun.kt	`86.66% <0.00%> (ø)`	`7.00% <0.00%> (?%)`
...rbosch/detekt/rules/complexity/TooManyFunctions.kt	`98.43% <0.00%> (+1.66%)`	`25.00% <0.00%> (ø%)`
...lab/arturbosch/detekt/api/internal/Suppressions.kt	`76.00% <0.00%> (+2.08%)`	`0.00% <0.00%> (ø%)`
...rturbosch/detekt/rules/style/UnusedPrivateClass.kt	`71.42% <0.00%> (+5.25%)`	`4.00% <0.00%> (ø%)`
...gitlab/arturbosch/detekt/api/AnnotationExcluder.kt	`71.42% <0.00%> (+35.06%)`	`7.00% <0.00%> (ø%)`

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 e52f6f2...2b969b6. Read the comment docs.

mhernand40 · 2020-09-25T05:23:04Z

        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")


FYI, AssertJ already declared on line 12.

mhernand40 · 2020-09-25T06:03:20Z

        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")
+        api("org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.8")


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)

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

arturbosch · 2020-09-25T13:44:34Z

+ * 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


Can we link to a specific section in this document?

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.

arturbosch · 2020-09-25T13:45:33Z

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


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

Sure. Does this work? 8cfc906

schalkms

👍 awesome first contribution

…uspend-fun-with-flow-return-type

mhernand40 · 2020-09-25T23:11:55Z

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


TOL: "Coroutines Flow" or "Coroutine Flow"? 🤔

Coroutines Flow interface?

Hmm… the rule will also detect subtypes of Flow, e.g., StateFlow, MutableStateFlow, and eventually SharedFlow. I am not so sure that putting an emphasis on "interface" adds much value to the description. For example, RxJava's Observable is actually an abstract class. Flow could have been designed as an abstract class and yet the rule description, as it currently is, would still apply. However, if you still feel strongly about it, I can make that change. 🙂

Okay, I decided to reword it a bit in 2b969b6. Let me know what you think. 🙂

mhernand40 · 2020-09-25T23:13:32Z

+            }
+    }
+
+    private fun KotlinType.isCoroutineFlow(): Boolean {


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. 🤔

I would choose isCoroutinesFlow() :)

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

tinder-mhernandez and others added 6 commits September 24, 2020 21:06

Add new coroutines rule - SuspendFunWithFlowReturnType

ec0fa79

Run generateDocumentation task

edbdda3

Add Marcelo Hernandez to Contributors list

ed306e7

Downgrade Coroutines from 1.3.9 to 1.3.8

a02869d

Remove Coroutines dependency declaration in detekt-test-utils

62a99b9

Reformat CodeSmell instantiation

6bed5c8

mhernand40 added 3 commits September 24, 2020 21:52

Add test for non-suspend, Flow-returning functions

b445068

Delete tests that fail when compile-test-snippets=true

d04cc26

Revert changes to index.md files

dbb35d9

mhernand40 commented Sep 25, 2020

View reviewed changes

mhernand40 mentioned this pull request Sep 25, 2020

Coroutines - Add rule that ensures that functions that return Flow are not marked as suspend #3086

Closed

mhernand40 commented Sep 25, 2020

View reviewed changes

arturbosch reviewed Sep 25, 2020

View reviewed changes

schalkms approved these changes Sep 25, 2020

View reviewed changes

mhernand40 added 5 commits September 25, 2020 15:27

Add link to specific "Flows are cold" section in Flow documentation

27dea52

Extend the issue description by providing the 'why'

8cfc906

Merge remote-tracking branch 'upstream/master' into coroutines-rule-s…

1ca8248

…uspend-fun-with-flow-return-type

Use apply when adding JvmClasspathRoots in KtTestCompiler

30baa22

Update documentation to reflect updated link

697fcc3

mhernand40 commented Sep 25, 2020

View reviewed changes

arturbosch added this to the 1.14.0 milestone Sep 26, 2020

arturbosch approved these changes Sep 26, 2020

View reviewed changes

mhernand40 added 2 commits September 25, 2020 23:56

Rename private isCoroutineFlow() extension to isCoroutinesFlow()

ad3d7bf

Update issue description

2b969b6

arturbosch merged commit cbbca85 into detekt:master Sep 26, 2020

mhernand40 deleted the coroutines-rule-suspend-fun-with-flow-return-type branch September 26, 2020 07:48

mhernand40 mentioned this pull request Sep 28, 2020

SuspendFunWithFlowReturnType introduced in 1.14.0 does not seem to work when activated in project #3116

Closed

Uh oh!

Uh oh!

Conversation

mhernand40 commented Sep 25, 2020

Uh oh!

codecov Bot commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mhernand40 Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schalkms left a comment

Choose a reason for hiding this comment

Uh oh!

mhernand40 Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 25, 2020 •

edited

Loading

mhernand40 Sep 25, 2020 •

edited

Loading

mhernand40 Sep 25, 2020 •

edited

Loading