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 UnnecessaryNotNullCheck
rule
#5218
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
@@ -470,6 +470,8 @@ potential-bugs: | |||
active: true | |||
UnconditionalJumpStatementInLoop: | |||
active: false | |||
UnnecessaryNotNullCheck: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Could we improve the message by
Using
$callNameon non-null
calleeExpression 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's worth adding one test case in this PR?
- Platform type - Similar to
shouldDetectWhenCallingPrimitiveJavaMethod
but 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already have the first one with shouldIgnoreWhenCallingObjectJavaMethod
, I'll add the second one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! Thanks
This adds the
UnnecessaryNotNullCheck
rule as discussed in #5204. It detects usages ofrequireNotNull
andcheckNotNull
on non-null values.