Add UnnecessaryNotNullCheck rule
#5218
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5218 +/- ##
=========================================
Coverage 84.96% 84.97%
- Complexity 3640 3647 +7
=========================================
Files 502 503 +1
Lines 11988 12008 +20
Branches 2259 2263 +4
=========================================
+ Hits 10186 10204 +18
Misses 690 690
- Partials 1112 1114 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
chao2zhang
left a comment
chao2zhang
left a comment
There was a problem hiding this comment.
💯 on adding this new rule. Thank you for your contribution
| if (bindingContext == BindingContext.EMPTY) return | ||
|
|
||
| val callName = expression.getCallNameExpression()?.text | ||
| if (callName == "requireNotNull" || callName == "checkNotNull") { |
There was a problem hiding this comment.
Since we have binding context present here, I would recommend match the full qualified name. You may find UseCheckNotNull as a good example
| override val issue = Issue( | ||
| "UnnecessaryNotNullCheck", | ||
| Severity.Defect, | ||
| "Unnecessary not-null check detected.", |
There was a problem hiding this comment.
| "Unnecessary not-null check detected.", | |
| "Remove unnecessary not-null checks on non-null types.", |
I would recommend using a more elaborative description since our description has been historically vague.
| active: true | ||
| UnconditionalJumpStatementInLoop: | ||
| active: false | ||
| UnnecessaryNotNullCheck: |
There was a problem hiding this comment.
I don't think using UnnecessaryNotNullCheck would be catching error-prone bugs. What do you think about style rules instead of potential-bugs section (Define the rule in :detekt-rules-style instead of `:detekt-rules-errorprone)?
There was a problem hiding this comment.
It looks like similar rules like UnnecessaryNotNullOperator and UnnecessarySafeCall are in potential-bugs as well. I think the reason for that is probably that treating a non-null value as nullable shows a potential misuse of code and while not terribly likely to cause bugs, it still could. For example, consider the following:
fun getSomeValue(id: Int): Int {
// ...
}
// somewhere else
val value = requireNotNull(getSomeValue(0)) { "Value not found" }This use of requireNotNull is probably assuming that getSomeValue returns null when it can't find the value, but we can see that isn't the case; it could be returning -1 or even throw an exception. The former might and the latter definitely would cause a bug as the exception wouldn't be handled.
| CodeSmell( | ||
| issue = issue, | ||
| entity = Entity.from(expression), | ||
| message = "`${expression.text}` contains an unnecessary `$callName`", |
There was a problem hiding this comment.
Nitpick: Could we improve the message by
Using $callNameon non-nullcalleeExpression is unnecessary to make it more clear to user?
| } | ||
|
|
||
| @Nested | ||
| inner class `check valid not null check usage` { |
There was a problem hiding this comment.
Do you think it's worth adding one test case in this PR?
- Platform type - Similar to
shouldDetectWhenCallingPrimitiveJavaMethodbut on a non-primitive - Definitely not null generic type. Doc
If this sounds more expensive, we could definitely follow up.
There was a problem hiding this comment.
I already have the first one with shouldIgnoreWhenCallingObjectJavaMethod, I'll add the second one.
4 participants
This adds the
UnnecessaryNotNullCheckrule as discussed in #5204. It detects usages ofrequireNotNullandcheckNotNullon non-null values.