IgnoredReturnValue: add option returnValueTypes to enable rule fo…

…r particular types (#4922)

* Add hasSourceLocation assertion for FindingAssert

* Add returnValueTypes option to declare types that should not be ignored

* Replace restrictToAnnotatedMethods with restrictToConfig

detekt-core/src/main/resources/default-detekt-config.yml

@@ -435,12 +435,16 @@ potential-bugs:
     active: true
   IgnoredReturnValue:
     active: true
-    restrictToAnnotatedMethods: true
+    restrictToConfig: true
     returnValueAnnotations:
       - '*.CheckResult'
       - '*.CheckReturnValue'
     ignoreReturnValueAnnotations:
       - '*.CanIgnoreReturnValue'
+    returnValueTypes:
+      - 'kotlin.sequences.Sequence'
+      - 'kotlinx.coroutines.flow.*Flow'
+      - 'java.util.stream.*Stream'
     ignoreFunctionCall: []
   ImplicitDefaultLocale:
     active: true

detekt-core/src/main/resources/deprecation.properties

@@ -1,5 +1,6 @@
 complexity>LongParameterList>threshold=Use `functionThreshold` and `constructorThreshold` instead
 empty-blocks>EmptyFunctionBlock>ignoreOverriddenFunctions=Use `ignoreOverridden` instead
+potential-bugs>IgnoredReturnValue>restrictToAnnotatedMethods=Use `restrictToConfig` instead
 potential-bugs>LateinitUsage>excludeAnnotatedProperties=Use `ignoreAnnotated` instead
 naming>FunctionParameterNaming>ignoreOverriddenFunctions=Use `ignoreOverridden` instead
 naming>MemberNameEqualsClassName>ignoreOverriddenFunction=Use `ignoreOverridden` instead

detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt

@@ -8,15 +8,20 @@ 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.UnstableApi
 import io.gitlab.arturbosch.detekt.api.config
+import io.gitlab.arturbosch.detekt.api.configWithFallback
 import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
 import io.gitlab.arturbosch.detekt.api.internal.Configuration
 import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
 import io.gitlab.arturbosch.detekt.api.simplePatternToRegex
+import io.gitlab.arturbosch.detekt.rules.fqNameOrNull
 import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptor
+import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.psi.KtCallExpression
 import org.jetbrains.kotlin.resolve.bindingContextUtil.isUsedAsExpression
 import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
+import org.jetbrains.kotlin.types.KotlinType
 import org.jetbrains.kotlin.types.typeUtil.isUnit
 
 /**
@@ -48,8 +53,14 @@ class IgnoredReturnValue(config: Config = Config.empty) : Rule(config) {
     )
 
     @Configuration("if the rule should check only annotated methods")
+    @Deprecated("Use `restrictToConfig` instead")
     private val restrictToAnnotatedMethods: Boolean by config(defaultValue = true)
 
+    @Suppress("DEPRECATION")
+    @OptIn(UnstableApi::class)
+    @Configuration("If the rule should check only methods matching to configuration, or all methods")
+    private val restrictToConfig: Boolean by configWithFallback(::restrictToAnnotatedMethods, defaultValue = true)
+
     @Configuration("List of glob patterns to be used as inspection annotation")
     private val returnValueAnnotations: List<Regex> by config(listOf("*.CheckResult", "*.CheckReturnValue")) {
         it.map(String::simplePatternToRegex)
@@ -60,6 +71,15 @@ class IgnoredReturnValue(config: Config = Config.empty) : Rule(config) {
         it.map(String::simplePatternToRegex)
     }
 
+    @Configuration("List of return types that should not be ignored")
+    private val returnValueTypes: List<Regex> by config(
+        listOf(
+            "kotlin.sequences.Sequence",
+            "kotlinx.coroutines.flow.*Flow",
+            "java.util.stream.*Stream",
+        ),
+    ) { it.map(String::simplePatternToRegex) }
+
     @Configuration(
         "List of function signatures which should be ignored by this rule. " +
             "Specifying fully-qualified function signature with name only (i.e. `java.time.LocalDate.now`) will " +
@@ -84,7 +104,8 @@ class IgnoredReturnValue(config: Config = Config.empty) : Rule(config) {
 
         val annotations = resultingDescriptor.annotations
         if (annotations.any { it in ignoreReturnValueAnnotations }) return
-        if (restrictToAnnotatedMethods &&
+        if (restrictToConfig &&
+            resultingDescriptor.returnType !in returnValueTypes &&
             (annotations + resultingDescriptor.containingDeclaration.annotations).none { it in returnValueAnnotations }
         ) return
 
@@ -98,9 +119,11 @@ class IgnoredReturnValue(config: Config = Config.empty) : Rule(config) {
         )
     }
 
-    @Suppress("UnusedPrivateMember")
-    private operator fun List<Regex>.contains(annotation: AnnotationDescriptor): Boolean {
-        val fqName = annotation.fqName?.asString() ?: return false
-        return any { it.matches(fqName) }
+    private operator fun List<Regex>.contains(type: KotlinType?) = contains(type?.fqNameOrNull())
+    private operator fun List<Regex>.contains(annotation: AnnotationDescriptor) = contains(annotation.fqName)
+
+    private operator fun List<Regex>.contains(fqName: FqName?): Boolean {
+        val name = fqName?.asString() ?: return false
+        return any { it.matches(name) }
     }
 }

detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValueSpec.kt

@@ -771,8 +771,8 @@ class IgnoredReturnValueSpec(private val env: KotlinCoreEnvironment) {
     }
 
     @Nested
-    inner class `restrict to annotated methods config` {
-        val subject = IgnoredReturnValue(TestConfig(mapOf("restrictToAnnotatedMethods" to false)))
+    inner class `restrict to config` {
+        val subject = IgnoredReturnValue(TestConfig(mapOf("restrictToConfig" to false)))
 
         @Test
         fun `reports when a function is annotated with a custom annotation`() {
@@ -811,6 +811,25 @@ class IgnoredReturnValueSpec(private val env: KotlinCoreEnvironment) {
             assertThat(findings[0]).hasMessage("The call listOfChecked is returning a value that is ignored.")
         }
 
+        @Test
+        fun `reports when a function returns type that should not be ignored`() {
+            val code = """
+                import kotlinx.coroutines.flow.MutableStateFlow
+
+                fun flowOfChecked(value: String) = MutableStateFlow(value)
+
+                fun foo() : Int {
+                    flowOfChecked("hello")
+                    return 42
+                }
+            """
+            val findings = subject.compileAndLintWithContext(env, code)
+            assertThat(findings)
+                .singleElement()
+                .hasSourceLocation(6, 5)
+                .hasMessage("The call flowOfChecked is returning a value that is ignored.")
+        }
+
         @Test
         fun `does not report when a function has _@CanIgnoreReturnValue_`() {
             val code = """
@@ -849,7 +868,7 @@ class IgnoredReturnValueSpec(private val env: KotlinCoreEnvironment) {
                 TestConfig(
                     mapOf(
                         "ignoreReturnValueAnnotations" to listOf("*.CustomIgnoreReturn"),
-                        "restrictToAnnotatedMethods" to false
+                        "restrictToConfig" to false
                     )
                 )
             )
@@ -873,12 +892,78 @@ class IgnoredReturnValueSpec(private val env: KotlinCoreEnvironment) {
                 TestConfig(
                     mapOf(
                         "ignoreFunctionCall" to listOf("foo.listOfChecked"),
-                        "restrictToAnnotatedMethods" to false
+                        "restrictToConfig" to false
                     )
                 )
             )
             val findings = rule.compileAndLintWithContext(env, code)
             assertThat(findings).isEmpty()
         }
     }
+
+    @Nested
+    inner class `return value types default config` {
+        private val subject = IgnoredReturnValue()
+
+        @Test
+        fun `reports when result of function returning Flow is ignored`() {
+            val code = """
+                import kotlinx.coroutines.flow.flowOf
+                
+                fun foo() {
+                    flowOf(1, 2, 3)
+                }
+            """
+            val findings = subject.compileAndLintWithContext(env, code)
+
+            assertThat(findings)
+                .singleElement()
+                .hasSourceLocation(line = 4, column = 5)
+                .hasMessage("The call flowOf is returning a value that is ignored.")
+        }
+
+        @Test
+        fun `reports when a function returned result is used in a chain that returns a Flow`() {
+            val code = """
+                import kotlinx.coroutines.flow.*
+                
+                fun foo() {
+                    flowOf(1, 2, 3)
+                        .onEach { println(it) }
+                }
+            """
+            val findings = subject.compileAndLintWithContext(env, code)
+
+            assertThat(findings)
+                .singleElement()
+                .hasSourceLocation(line = 5, column = 10)
+                .hasMessage("The call onEach is returning a value that is ignored.")
+        }
+
+        @Test
+        fun `does not report when a function returned value is used to be returned`() {
+            val code = """
+                import kotlinx.coroutines.flow.flowOf
+                
+                fun foo() = flowOf(1, 2, 3)
+            """
+            val findings = subject.compileAndLintWithContext(env, code)
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun `does not report when a function returned value is consumed in a chain that returns an Unit`() {
+            val code = """
+                import kotlinx.coroutines.flow.*
+                
+                suspend fun foo() {
+                    flowOf(1, 2, 3)
+                        .onEach { println(it) }
+                        .collect()
+                }
+            """
+            val findings = subject.compileAndLintWithContext(env, code)
+            assertThat(findings).isEmpty()
+        }
+    }
 }

detekt-test/api/detekt-test.api

@@ -2,6 +2,7 @@ public final class io/gitlab/arturbosch/detekt/test/FindingAssert : org/assertj/
 	public fun <init> (Lio/gitlab/arturbosch/detekt/api/Finding;)V
 	public final fun getActual ()Lio/gitlab/arturbosch/detekt/api/Finding;
 	public final fun hasMessage (Ljava/lang/String;)Lio/gitlab/arturbosch/detekt/test/FindingAssert;
+	public final fun hasSourceLocation (II)Lio/gitlab/arturbosch/detekt/test/FindingAssert;
 }
 
 public final class io/gitlab/arturbosch/detekt/test/FindingsAssert : org/assertj/core/api/AbstractListAssert {

detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt

@@ -117,6 +117,15 @@ class FindingsAssert(actual: List<Finding>) :
 }
 
 class FindingAssert(val actual: Finding?) : AbstractAssert<FindingAssert, Finding>(actual, FindingAssert::class.java) {
+
+    fun hasSourceLocation(line: Int, column: Int) = apply {
+        val expectedLocation = SourceLocation(line, column)
+        val actualLocation = actual.location.source
+        if (actualLocation != expectedLocation) {
+            failWithMessage("Expected source location to be $expectedLocation but was $actualLocation")
+        }
+    }
+
     fun hasMessage(expectedMessage: String) = apply {
         if (expectedMessage.isNotBlank() && actual.message.isBlank()) {
             failWithMessage("Expected message <$expectedMessage> but finding has no message")