Extend VarCouldBeVal to include analysis of file- and class-level properties #4424
Conversation
…l properties; Added checks in VarCouldBeVal to avoid flagging escaped object literals
Codecov Report
@@ Coverage Diff @@
## main #4424 +/- ##
=========================================
Coverage 84.34% 84.34%
+ Complexity 3299 3297 -2
=========================================
Files 473 473
Lines 10532 10580 +48
Branches 1885 1906 +21
=========================================
+ Hits 8883 8924 +41
Misses 671 671
- Partials 978 985 +7
Continue to review full report at Codecov.
|
|
First issue identified: I'm not sure what kind of "tell" there would be for this one, given it's marked as |
I guess |
| if (bindingContext == BindingContext.EMPTY) return | ||
| super.visitKtFile(file) |
There was a problem hiding this comment.
| if (bindingContext == BindingContext.EMPTY) return | |
| super.visitKtFile(file) | |
| super.visitKtFile(file) | |
| if (bindingContext == BindingContext.EMPTY) return |
There was a problem hiding this comment.
If there's no bindingContext available, the super method isn't called.
There was a problem hiding this comment.
Maybe that's desirable, right? Faster rule.
I think that we should even move this checks earlier in the life cycle. But that's another topic.
There was a problem hiding this comment.
In this case, you don't gain any performance benefit, because KtFile's are usually not nested.
I agree with the following statement. In the best case, you don't even load the rule if it's annotated with the corresponding annotation. As far as I remember, there's an issue floating around, which discusses this topic. Anyways, as you said, this is out of scope for this PR.
I think that we should even move this checks earlier in the life cycle. But that's another topic.
There was a problem hiding this comment.
It's cargo-culting from other files that I copied from when I started out working on this code base. I think it and the test wording show that there might need to be some more definitions in the contribution guide about style and things to add or avoid.
| it("does not report non-private variables") { | ||
| val code = """ | ||
| var a = 1 | ||
| """.trimIndent() |
There was a problem hiding this comment.
nitpick: if you don't test for characters or reports at a specific position in the code snippet, it's optional to include the trimIndent() call.
There was a problem hiding this comment.
My IDE automatically appends .trimIndent() when I create these string blocks. If that's a setting that's generally better left off, I'll disable it, but I'd rather keep things as-is if they don't cause any harm.
| @@ -13,6 +13,82 @@ class VarCouldBeValSpec : Spek({ | |||
| val env: KotlinCoreEnvironment by memoized() | |||
| val subject by memoized { VarCouldBeVal() } | |||
|
|
|||
| describe("file-level declarations") { | |||
| it("does not report non-private variables") { | |||
There was a problem hiding this comment.
| it("does not report non-private variables") { | |
| it("does not report public variables") { |
There was a problem hiding this comment.
public != non-private - there are protected and internal scopes as well. I didn't do it because I felt it'd be a bit excessive, but I could put a few more tests to cover the other scopes.
| } | ||
|
|
||
| describe("class-level declarations") { | ||
| it("does not report non-private variables in non-private classes") { |
There was a problem hiding this comment.
I guess the following is a bit easier to read and grasp.
| it("does not report non-private variables in non-private classes") { | |
| it("does not report public variables in public classes") { |
| assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() | ||
| } | ||
|
|
||
| it("does not report non-private variables in non-private objects") { |
| context("should not report anonymous objects that escape") { | ||
| it("when initializing a variable directly") { |
There was a problem hiding this comment.
minor: I think it might be beneficial to apply the same test description style as above. I guess it's easier to grasp the many different cases when reading and editing this rule in the future.
e.g.:
context("anonymous objects that escape") {
it("does not report when initializing a variable directly") {
}
it("does not report")...
Other option is to add an ingnoreAnnotated and ignore the params that had the |
| import org.jetbrains.kotlin.resolve.BindingContext | ||
| import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall | ||
|
|
||
| private val unaryAssignmentOperators = setOf(KtTokens.MINUSMINUS, KtTokens.PLUSPLUS) | ||
| import org.jetbrains.kotlin.util.containingNonLocalDeclaration | ||
|
|
||
| /** | ||
| * Reports var declarations (locally-scoped variables) that could be val, as they are not re-assigned. |
There was a problem hiding this comment.
| * Reports var declarations (locally-scoped variables) that could be val, as they are not re-assigned. | |
| * Reports var declarations (both local variables and private class properties) that could be val, as they are not re-assigned. |
| private fun KtProperty.isDeclarationCandidate(): Boolean { | ||
| return when { | ||
| !isVar -> false | ||
| isLocal || isPrivate() -> true |
There was a problem hiding this comment.
It might be good to follow up with hanlding expect / actual properties.
|
Thanks for the contribution! |
Second poor soul attempting to conquer #1330. Also, I noticed what could be a potential false-positive issue for
varproperties declared in object literals that escape their containing declaration, so this code includes checks for that.