Add LateinitUsageInClassLevel check #8125
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8125 +/- ##
============================================
- Coverage 85.09% 85.06% -0.03%
- Complexity 4223 4257 +34
============================================
Files 575 576 +1
Lines 13431 13509 +78
Branches 2531 2557 +26
============================================
+ Hits 11429 11492 +63
- Misses 805 807 +2
- Partials 1197 1210 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
schalkms
left a comment
There was a problem hiding this comment.
Thanks for the amazing 1st contribution! 👍
I really like this rule. Truth to be hold, we had this rule in the backlog for a while. Great that the rule finally finds its way into the default rule set.
Sorry that it took so long to finally review the PR. Just a few comments to tidy up a bit. Afterwards, I'd say LGTM.
| // Note: The nested class test is currently skipped because the current implementation | ||
| // doesn't support detecting lateinit vars in nested classes with qualified expressions. | ||
| // This is a limitation of the current implementation and should be addressed in the future. | ||
| // @Test | ||
| // fun `reports lateinit var assigned to class-level val in nested class`() { | ||
| // val code = """ | ||
| // class Outer { | ||
| // lateinit var unsafe: String | ||
| // | ||
| // class Inner { | ||
| // val someValue: String = this@Outer.unsafe | ||
| // } | ||
| // } | ||
| // """.trimIndent() | ||
| // println("[DEBUG_LOG] Nested class test code: $code") | ||
| // val findings = subject.lintWithContext(env, code) | ||
| // println("[DEBUG_LOG] Findings for nested class test: $findings") | ||
| // assertThat(findings).hasSize(1) | ||
| // } |
There was a problem hiding this comment.
I'd prefer to put this test code in a new issue describing the potential improvements of this rule. It's easy to overlook commented code, even so when refactoring the codebase.
| } | ||
| """.trimIndent() | ||
| val findings = subject.lintWithContext(env, code) | ||
| assertThat(findings).hasSize(1) // Expecting 1 finding because the lateinit var is conditionally initialized |
There was a problem hiding this comment.
I think the comment does not provide any further value, since you already described in detail the test case intention in line 74. 👍
| assertThat(findings).hasSize(1) // Expecting 1 finding because the lateinit var is conditionally initialized | |
| assertThat(findings).hasSize(1) |
| } | ||
| """.trimIndent() | ||
| val findings = subject.lintWithContext(env, code) | ||
| assertThat(findings).hasSize(1) // Expecting 1 finding because the lateinit var is not initialized in all branches |
There was a problem hiding this comment.
I think the comment does not provide any further value, since you already described in detail the test case intention in line 93. 👍
| assertThat(findings).hasSize(1) // Expecting 1 finding because the lateinit var is not initialized in all branches | |
| assertThat(findings).hasSize(1) |
| } | ||
| """.trimIndent() | ||
| val findings = subject.lintWithContext(env, code) | ||
| assertThat(findings).hasSize(1) // Expecting 1 finding because the lateinit var is not initialized in all branches |
There was a problem hiding this comment.
I think the comment does not provide any further value, since you already described in detail the test case intention in line 113. 👍
| assertThat(findings).hasSize(1) // Expecting 1 finding because the lateinit var is not initialized in all branches | |
| assertThat(findings).hasSize(1) |
| } | ||
| """.trimIndent() | ||
| val findings = subject.lintWithContext(env, code) | ||
| assertThat(findings).isEmpty() // Expecting 0 findings because the lateinit var is properly initialized in all branches |
| } | ||
| """.trimIndent() | ||
| val findings = subject.lintWithContext(env, code) | ||
| assertThat(findings).isEmpty() // Expecting 0 findings because the lateinit var is properly initialized |
| import org.junit.jupiter.api.Test | ||
|
|
||
| @KotlinCoreEnvironmentTest | ||
| class LateinitUsageInClassLevelValSpec(val env: KotlinEnvironmentContainer) { |
| * Using a lateinit var to initialize a class-level val is problematic because the initialization | ||
| * of the val happens during object construction, while the lateinit var might be initialized later. | ||
| * This can lead to runtime exceptions or unexpected behavior. | ||
| * |
There was a problem hiding this comment.
Shall we put a hint here that the rule currently provides the following limitation? What's your take on this?
// Note: The nested class test is currently skipped because the current implementation doesn't support detecting lateinit vars in nested classes with qualified expressions.
| val propertyDescriptor = resolvedCall.resultingDescriptor | ||
|
|
||
| // Check if the property is lateinit using the descriptor | ||
| // This is a brittle approach, but it's the best we can do without access to the original declaration. | ||
| // We're looking for "lateinit var" in the descriptor's string representation to be more specific. | ||
| val isLateinit = propertyDescriptor.toString().contains("lateinit var") |
There was a problem hiding this comment.
I'm not sure. Can we somehow check if the resolved call is a declaration of KtProperty or derived classes? KtProperty provides property to check for lateinit var.
BraisGabin
left a comment
There was a problem hiding this comment.
Fully agree with comments from schalkms. Once addressed I would also approve this great contribution.
Really good test coverage :)
...prone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/LateinitUsageInClassLevelVal.kt
Outdated
Show resolved
Hide resolved
|
@alex28sh do you plan to update this PR to address the feedback? I haven't reviewed but based on the reviews that were done this looks like a great contribution. |
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
Fixes for #1431