Add new rule UseEmptyCounterpart #2864
Conversation
| * emptySet() | ||
| * </compliant> | ||
| */ | ||
| class UseEmptyCounterpart(config: Config) : Rule(config) { |
There was a problem hiding this comment.
I originally was going to go with "UseEmptyCollection", but decided the name would be too confusing. Even though this rule does apply to "collections" of elements, the name would imply that this rule only applies to objects that implement the Collection interface (which they don't).
Codecov Report
@@ Coverage Diff @@
## master #2864 +/- ##
============================================
+ Coverage 80.15% 80.18% +0.02%
- Complexity 2429 2434 +5
============================================
Files 421 422 +1
Lines 7384 7405 +21
Branches 1354 1358 +4
============================================
+ Hits 5919 5938 +19
Misses 764 764
- Partials 701 703 +2
Continue to review full report at Codecov.
|
detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseEmptyCounterpartSpec.kt
Outdated
Show resolved
Hide resolved
|
|
||
| it("does not report $argInstantiation with arguments") { | ||
| val findings = rule.compileAndLint(argInstantiation.argExample) | ||
| assertThat(findings).isEmpty() |
There was a problem hiding this comment.
For future regressions it's a good idea to test some more negative cases.
What happens if the user names a variable or a function setOf? An intensive test suite can prevent errors in the future. Maybe you can come up with more test cases like this.
There was a problem hiding this comment.
I added your recommended test in 9ba2c35 but have found that it does not pass. This is because I only compare the text of the function instead of the fully qualified name.
I've done some poking around, but can't find the API to retrieve the fully qualified name of the function. Do you have any pointers?
There was a problem hiding this comment.
You would need to use type and symbol solving for this. BindingContext is the keyword here.
There was a problem hiding this comment.
Thanks, that quickly put me on the right track.
Fixed in 920ecd0.
|
After #2865 we should move tis rule to the correct module: |
...les-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseEmptyCounterpartSpec.kt
Show resolved
Hide resolved
|
|
||
| private val exprsWithEmptyCounterparts = mapOf( | ||
| "arrayOf" to "emptyArray", | ||
| "listOf" to "emptyList", |
There was a problem hiding this comment.
I think it should be safe to add also:
"listOfNotNull" to "emptyList",
Though there is a type restriction (listOfNotNull<Int?> and emptyList<Int> are of different type), but I think it should be safe anyway to flag the usage of a listOfNotNull() with no parameter.
There was a problem hiding this comment.
I like this idea, though I have an alternative proposal that would flag incorrect usages of listOfNotNull() and listOfNotNull(elem, …).
If we extend the rule UselessCallOnNotNull to also flag invocations of listOfNotNull with all non-nullable arguments, then a no-arg invocation of listOfNotNull would also be flagged. The user can then replace listOfNotNull with listOf for all these incorrect usages. Then UseEmptyCounterpart would flag all the no-arg listOf invocations as a result of this.
WDYT?
There was a problem hiding this comment.
If we extend the rule UselessCallOnNotNull to also flag invocations of listOfNotNull with all non-nullable arguments
If possible I'll keep this out of the scope of this PR.
then a no-arg invocation of listOfNotNull would also be flagged. The user can then replace listOfNotNull with listOf for all these incorrect usages. Then UseEmptyCounterpart would flag all the no-arg listOf invocations as a result of this.
Ideally we want to provide immediate feedback with a potential solution. Having the user go through listOfNotNull() -> listOf -> emptyList() and two Detekt runs seems cumbersome to me.
There was a problem hiding this comment.
Yeah that makes sense. Just to note that when Kotlin 1.4 is released, setOfNotNull will also be introduced, so we should add support for this function as well.
Fixed in 8587d4f.
| val calleeExpression = expression.calleeExpression ?: return | ||
| val emptyCounterpart = exprsWithEmptyCounterparts[calleeExpression.text] | ||
|
|
||
| if (emptyCounterpart != null && expression.valueArguments.isEmpty()) { |
There was a problem hiding this comment.
Instead of emptyCounterpart != null probably guard clause can be used like the following:
val emptyCounterpart = exprsWithEmptyCounterparts[calleeExpression.text] ?: return
There was a problem hiding this comment.
Good point! Fixed in 920ecd0.
|
Hey @veyndan , can you take a look why a testcase is crashing? |
|
Hey @arturbosch, the build is currently failing because of #2864 (comment). I've been swamped for the past couple of weeks, but I should have some time this coming weekend to try and fix the tests. |
Closes #2850.