diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 69ddbbbab94..764ba811aa5 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/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 diff --git a/detekt-core/src/main/resources/deprecation.properties b/detekt-core/src/main/resources/deprecation.properties index c855a20cd2f..7d9a994a4b1 100644 --- a/detekt-core/src/main/resources/deprecation.properties +++ b/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 diff --git a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt index fde27821f27..443a57f1385 100644 --- a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt +++ b/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 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 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.contains(annotation: AnnotationDescriptor): Boolean { - val fqName = annotation.fqName?.asString() ?: return false - return any { it.matches(fqName) } + private operator fun List.contains(type: KotlinType?) = contains(type?.fqNameOrNull()) + private operator fun List.contains(annotation: AnnotationDescriptor) = contains(annotation.fqName) + + private operator fun List.contains(fqName: FqName?): Boolean { + val name = fqName?.asString() ?: return false + return any { it.matches(name) } } } diff --git a/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValueSpec.kt b/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValueSpec.kt index 03d2f7bf591..49be8502a4a 100644 --- a/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValueSpec.kt +++ b/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,7 +892,7 @@ class IgnoredReturnValueSpec(private val env: KotlinCoreEnvironment) { TestConfig( mapOf( "ignoreFunctionCall" to listOf("foo.listOfChecked"), - "restrictToAnnotatedMethods" to false + "restrictToConfig" to false ) ) ) @@ -881,4 +900,70 @@ class IgnoredReturnValueSpec(private val env: KotlinCoreEnvironment) { 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() + } + } } diff --git a/detekt-test/api/detekt-test.api b/detekt-test/api/detekt-test.api index 587575cc696..51940bfa5d9 100644 --- a/detekt-test/api/detekt-test.api +++ b/detekt-test/api/detekt-test.api @@ -2,6 +2,7 @@ public final class io/gitlab/arturbosch/detekt/test/FindingAssert : org/assertj/ public fun (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 { diff --git a/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt b/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt index 3297cf7c5d7..cd69600c8be 100644 --- a/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt +++ b/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt @@ -117,6 +117,15 @@ class FindingsAssert(actual: List) : } class FindingAssert(val actual: Finding?) : AbstractAssert(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")