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 CastNullableToNonNullableType
rule
#5653
Add CastNullableToNonNullableType
rule
#5653
Conversation
Hi @BraisGabin I have created this PR for #3484 but not able to add you as reviewer |
...rone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/CastNullableToNonNullableType.kt
Outdated
Show resolved
Hide resolved
...rone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/CastNullableToNonNullableType.kt
Show resolved
Hide resolved
2d9c764
to
073014a
Compare
073014a
to
a1baee6
Compare
detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/TypeUtils.kt
Outdated
Show resolved
Hide resolved
To make ci happy you will need to run |
detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/TypeUtils.kt
Outdated
Show resolved
Hide resolved
* problems in your code. The compliant code would be that which will correctly check | ||
* for two things (nullability and type) and not just one (cast). |
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.
* problems in your code. The compliant code would be that which will correctly check | |
* for two things (nullability and type) and not just one (cast). | |
* problems in your code. The compliant code would either us smart-casting or would check the type before performing the cast. |
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.
Hi, @cortinico changing the message to * problems in your code. The compliant code would either us smart-casting or would check the type before performing the cast.
doesn't actually reflect the intention of the rule.
Original code bar as String
asserts two things(nullability as well as type assertions) at once. And suggested fix is to assert both the cases differently or handle both the cases
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'm unsure I get the point of this rule at this point.
The problem of bar as String
is that it will fail at runtime if the two types are incompatible. Using a if (bar is String) ... else ...
would not instead.
Your rule is instead suggesting to fail for two different scenarios (null or non compatible types) with different error messages, right?.
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.
The point of this rule is that a cast can hide a !!
behind it. That's a potential error and what this rule does is to point it out to ensure that this is not an error but a decission by the devolver. If you really want to cast directly from Any?
to String
you should be fine with foo!! as String
. If you didn't want to do that, well, then you need to rewrite your code in any other way that doesn't produces a null pointer exception.
To me this rule should not be too opinated. Just point to the potential error and its up to the dev to decide how to fix it. If the problem here are the compilant part we can add 3 different ways to make the code compilant because it depends a lot to what the user wants on each case.
If you are casting your code is smelly. This rule at least ensure that the cast doesn't hide any extra surprise.
* | ||
* <compliant> | ||
* fun foo(bar: Any?) { | ||
* val x = (bar ?: error("null assertion message")) as String |
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.
If would suggest something like:
val x = if (bar is String) ... else ...
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.
Hi @cortinico wouldn't val x = if (bar is String) ... else ...
defeat the purpose of the rule? As if type of bar
is Any?
then the check bar is String
can be false due to two reasons
bar
being nullbar
is not the type ofString
hence there will be no clear and separate handling of null
and wrong type. The initial idea was The idea is to write code that shows that it could fail for two things (nullability and type) and not just one (cast).
at the issue description
@BraisGabin let me know your thoughts as well
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 would vote to add two alternatives:
val x = bar!! as String
val x = bar as String?
Or if !!
is so bad (I really like this operator):
val x = checkNotNull(bar) as String
val x = bar as String?
The idea of this rule is to make clear that there is a cast from nullable to non-nullable. The idea is to avoid it use as String?
or make it completely clear. For that second part I think that !!
does its job.
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.
Hi, @BraisGabin I personally use (bar ?: error("null assertion message")) as String
as I always prefer some custom error message. But I am also okay with val x = checkNotNull(bar) as String
I don't like !!
much also this also gets flagged by UnsafeCallOnNullableType
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.
hence there will be no clear and separate handling of null and wrong type
Which might very much be outside the scope of the rule.
The user receives a bar: Any?
and wants to know if it can be used as a String
.
Your code suggestion is too opinionated in failing on null
state, which the user might not want to do.
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.
Hi @BraisGabin / @cortinico added below compliant block
* <compliant>
* fun foo(bar: Any?) {
* val x = checkNotNull(bar) as String
* }
*
* // Alternative
* fun foo(bar: Any?) {
* val x = (bar ?: error("null assertion message")) as String
* }
* </compliant>
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.
That works for me
Move isNullable and descriptor common utility that was common across NullableToStringCall.kt and CastNullableToNonNullableType.kt rules
Handle null compilerResources at the call site
9b50142
to
8afd51c
Compare
LGTM |
(sorry for the late review) |
CastNullableToNonNullableType
rule
Fixes #3484