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

Add list of functions to skip in IgnoredReturnValue rule #4434

Merged
merged 9 commits into from
Jan 28, 2022

Conversation

artem-zinnatullin
Copy link
Contributor

@artem-zinnatullin artem-zinnatullin commented Jan 1, 2022

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
Copy link

codecov bot commented Jan 1, 2022

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
Impacted file tree graph

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

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

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

@artem-zinnatullin
Copy link
Contributor Author

Thanks :)

Copy link
Member

@chao2zhang chao2zhang left a comment

Choose a reason for hiding this comment

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

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.

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
Copy link
Contributor Author

@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
Copy link
Member

schalkms commented Jan 2, 2022

@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
Copy link
Member

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

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

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.

"List of fully-qualified function names that should be skipped by this check. " +
"Example: 'package.class.fun1'"
)
private val skipFunctions: List<String> by config(emptyList())
Copy link
Member

Choose a reason for hiding this comment

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

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:

@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
Copy link
Contributor Author

@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
Copy link
Member

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

@cortinico cortinico added this to the 1.20.0 milestone Jan 5, 2022
@cortinico cortinico added the rules label Jan 5, 2022
…etekt/rules/bugs/IgnoredReturnValue.kt

Co-authored-by: Chao Zhang <zhangchao6865@gmail.com>
@BraisGabin BraisGabin merged commit 90ae9ec into detekt:main Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants