Add list of functions to skip in IgnoredReturnValue rule

[artem-zinnatullin]

Hey Detekt team, I was configuring IgnoredReturnValue rule in our project and found it extremely useful — fixed few actual bugs!

In fact, with this rule my old feature request filed to Kotlin team 6 years ago is now fulfilled heh https://youtrack.jetbrains.com/issue/KT-12719

However the rule does find quite a lot of false-positives that are outside of our or Detekt control.

Thus I'm proposing a configuration option for the IgnoredReturnValue rule that will allow user to list fully qualified names of the functions return values of which they don't expect to be consumed — thus skipped by the rule.

potential-bugs:
  IgnoredReturnValue:
    active: true
    restrictToAnnotatedMethods: false
    skipFunctions:
      - io.ktor.routing.post
      - io.ktor.routing.get

---

Examples:

In Ktor some routing DSL methods return values (idk why, seems like bad design, but oh well) and DSL doesn't rely on them to be used:

embeddedServer(Netty, port = 8000) {
	routing {
		// Here get is a DSL configuration function, you never consume its result..
		get ("/") {
			call.respondText("Hello, world!")
		}
	}
}

Or some stdlib functions like use:

// Here use() returns a value but often you rely only on lambda..
file.outputStream().use { fos ->
        inputStream.use {
            it.copyTo(fos)
        }
}

and so on.

[codecov]

Codecov Report

Merging #4434 (da96af0) into main (d993ce6) will decrease coverage by 0.00%.
The diff coverage is 66.66%.

❗ Current head da96af0 differs from pull request most recent head d575313. Consider uploading reports for the commit d575313 to get more accurate results

@@             Coverage Diff              @@
##               main    #4434      +/-   ##
============================================
- Coverage     84.32%   84.32%   -0.01%     
- Complexity     3288     3294       +6     
============================================
  Files           475      472       -3     
  Lines         10502    10525      +23     
  Branches       1882     1886       +4     
============================================
+ Hits           8856     8875      +19     
- Misses          670      671       +1     
- Partials        976      979       +3     

Impacted Files	Coverage Δ
...arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt	82.85% <66.66%> (-1.52%)	⬇️
...b/arturbosch/detekt/core/tooling/AnalysisFacade.kt	62.85% <0.00%> (-3.81%)	⬇️
...sch/detekt/rules/style/UnnecessaryAbstractClass.kt	86.66% <0.00%> (-0.29%)	⬇️
...gitlab/arturbosch/detekt/core/tooling/Lifecycle.kt	100.00% <0.00%> (ø)
...ab/arturbosch/detekt/core/config/Configurations.kt	84.37% <0.00%> (ø)
...detekt/tooling/api/DefaultConfigurationProvider.kt	100.00% <0.00%> (ø)
...bosch/detekt/core/tooling/DefaultConfigProvider.kt	80.00% <0.00%> (ø)
...kt/core/config/DefaultPropertiesConfigValidator.kt	100.00% <0.00%> (ø)
...lab/arturbosch/detekt/core/config/DefaultConfig.kt
.../detekt/core/baseline/DelegatingXMLStreamWriter.kt
... and 5 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 d993ce6...d575313. Read the comment docs.

[schalkms]

This is a nice addition to this rule. I like it. Thanks for submitting!

[artem-zinnatullin]

Thanks :)

[chao2zhang]

Thanks for providing a PR to address this issue. I will follow up on completing the docs, but we have ignoreAnnotated universal option added. This means that you probably don't necessarily need this PR (or wait for a new release) to work around this issue.

• Changelog
• Sample configuration guide

I am wondering if we should differentiate the two following scenarios:

• In the app code, apply ignoreAnnotated at the caller side.
• In the detekt configuration file, use a new option like skipFunctions in this PR to ignore specific functions globally.

@BraisGabin what do you think?

[artem-zinnatullin]

@chao2zhang I don't think ignoreAnnotated will work for cases I listed, functions in question are in 3rd-party code beyond my/yours control, thus they don't usually have annotations on them.

[schalkms]

@artem-zinnatullin I think @chao2zhang meant the following comment in the changelog. ignoreAnnotated doesn't help in this scenario, but ignoreFunction should do the job. I also didn't know that detekt already supports that. 😉

Similarly, we now offer also an ignoreFunction configuration key that you can use to suppress findings if inside a function with a given name - #4148

[chao2zhang]

Good callout! I forgot about ignoreFunction even it is called out in the release note 😅. It should help in this case.

[BraisGabin]

I think that ignoreFunction would not work on this scenario. That Suppressor ignores justthe issues finded on the function declaration. Here the issue is thrown in the function call so I think that it would not work.

But I agree that this feature could be handled by a Suppressor so it could be used on any rule. But some open questions:

1. Should we use the same Suppressor for both cases (issues in the definition and the call side)?
2. If we decide it should be in different Suppressors how do we call this new one? ignoreFunctionCall? Naming is hard.

[BraisGabin]

I think that you should use FunctionMatcher. It provides more porwerful matching (you can match only by name or name + param types. Here a rule using it:

detekt/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenMethodCall.kt

Lines 46 to 58 in ff7824d

	@Configuration(
	"List of fully qualified method signatures which are forbidden. " +
	"Methods can be defined without full signature (i.e. `java.time.LocalDate.now`) which will report " +
	"calls of all methods with this name or with full signature " +
	"(i.e. `java.time.LocalDate(java.time.Clock)`) which would report only call " +
	"with this concrete signature."
	)
	private val methods: List<FunctionMatcher> by config(
	listOf(
	"kotlin.io.print",
	"kotlin.io.println",
	)
	) { it.map(FunctionMatcher::fromFunctionSignature) }

[artem-zinnatullin]

@BraisGabin spot on suggestion with a FunctionMatcher and naming!

I was wondering on distinguishing function overloads, I've changed the code to use the matcher and renamed the property to ignoreFunctionCall which is better in terms of consistency I think.

Regarding the ignoreFunction suggested above, from how I understand it:

Similarly, we now offer also an ignoreFunction configuration key that you can use to suppress findings if inside a function with a given name

I think it won't work, "inside a function with a given name" while I'm trying to ignore external function calls.

[BraisGabin]

@chao2zhang What do you think about this? It would be nice to have a Suppressor for this. But we can add it later without any breaking change.