Add AlwaysNullReadOnlyVariableDetector to detect unnecessary null initialization #358
Conversation
|
Thanks for the contribution! Before we can merge this, we need @henni99 to sign the Salesforce Inc. Contributor License Agreement. |
| """ | ||
| package foo | ||
|
|
||
| val nullableKtString: String? = null | ||
|
|
||
| class Test { | ||
| val nullableString: String? = null | ||
| val nullableFloat: Float? = null | ||
|
|
||
| fun method() { | ||
| val nullableLocalString: String? = null | ||
| } | ||
|
|
||
| companion object { | ||
| val nullableCompanionString: String? = null | ||
| } | ||
| } | ||
| """ |
There was a problem hiding this comment.
Let's clean up this huge leading indent. Same with the expect block below
| Issue.create( | ||
| "AvoidNullInitializationForReadOnlyVariables", | ||
| "Avoid initializing read-only variable with null", | ||
| "Avoid unnecessary null initialization for read-only variables.", |
There was a problem hiding this comment.
This explanation should be a bit more longer. i.e. explaining why it's bad
|
@ZacSweers I would greatly appreciate your review 😊 |
| import org.jetbrains.uast.kotlin.isKotlin | ||
| import slack.lint.util.sourceImplementation | ||
|
|
||
| class NotNullReadOnlyVariableDetector : Detector(), SourceCodeScanner { |
There was a problem hiding this comment.
This name seems wrong. By definition the variable is nullable, the issue is that we want to point out when a nullable type can only ever be null
There was a problem hiding this comment.
Thank you for your great review! I think the naming needs some refinement, so I'll take more time to consider it!
There was a problem hiding this comment.
I suggest the naming 'AlwaysNullReadOnlyVariableDetector'.
henni99
changed the title
| import com.android.tools.lint.checks.infrastructure.TestMode | ||
| import org.junit.Test | ||
|
|
||
| class AlwaysNullReadOnlyVariableDetectorTest : BaseSlackLintTest() { |
There was a problem hiding this comment.
Can you add some cases that should not warn to these tests? i.e. mutable variables
There was a problem hiding this comment.
I have added test code.
While exploring more cases, I thought it would be good to consider scenarios where a read-only property's custom getter returns null, so I have made adjustments accordingly.
I would appreciate your feedback.
Thank you!
|
@henni99 are you planning to address the CI failures? |
|
@ZacSweers I plan to proceed with it tomorrow! (complete) |
henni99
force-pushed
the
main
branch
2 times, most recently
from
d7599ae to
2704205
Compare
|
@ZacSweers Are you planning to review it? |
|
Sorry I missed a notification that this was updated, I'll try to look today |
|
Any Updates? |
Summary
try to resolve #344 !
Requirements (place an
xin each[ ])-> happen 404 error