-
-
Notifications
You must be signed in to change notification settings - Fork 766
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
Split UnusedPrivateMember #5722
Conversation
@@ -700,11 +700,17 @@ style: | |||
active: false | |||
UnusedImports: | |||
active: false | |||
UnusedParameter: | |||
active: true | |||
allowedNames: '(_|ignored|expected|serialVersionUID)' |
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.
serialVersionUID
is not likely to be a parameter name we want to ignore but I wanted to make this a non-breaking change.
I would suggest a follow up issue for all of the breaking changes that will further the clean up here (including things like renaming UnusedPrivateMember
to UnusedPrivateFunction
)
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 guess we don't usually have parameters with names like ignored
, expected
, and serialVersionUID
.
*/ | ||
@ActiveByDefault(since = "1.23.0") | ||
class UnusedParameter(config: Config = Config.empty) : Rule(config) { | ||
override val defaultRuleIdAliases: Set<String> = setOf("UNUSED_VARIABLE", "UNUSED_PARAMETER", "unused") |
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 kept "UNUSED_VARIABLE" as an alias even though this rule now only handles params. This was in order to preserve existing behavior. Maybe this can be tracked in the follow-up issue for 2.0 I talked about in the other comment?
@@ -125,7 +103,8 @@ private class UnusedFunctionVisitor( | |||
KtTokens.MINUS, | |||
KtTokens.MUL, | |||
KtTokens.DIV, | |||
KtTokens.PERC -> operatorValue?.let { functionReferences["$it="] }.orEmpty() | |||
KtTokens.PERC, | |||
-> operatorValue?.let { functionReferences["$it="] }.orEmpty() |
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.
Change from running ktlint --format
@Nested | ||
inner class `parameters in primary constructors` { | ||
@Test | ||
fun `reports unused parameter`() { | ||
val code = """ | ||
class Test(unused: Any) | ||
""".trimIndent() | ||
assertThat(subject.lint(code)).hasSize(1) | ||
} | ||
|
||
@Test | ||
fun `does not report used parameter for calling super`() { | ||
val code = """ | ||
class Parent(val ignored: Any) | ||
class Test(used: Any) : Parent(used) | ||
""".trimIndent() | ||
assertThat(subject.lint(code)).isEmpty() | ||
} | ||
|
||
@Test | ||
fun `does not report used parameter in init block`() { | ||
val code = """ | ||
class Test(used: Any) { | ||
init { | ||
used.toString() | ||
} | ||
} | ||
""".trimIndent() | ||
assertThat(subject.lint(code)).isEmpty() | ||
} | ||
|
||
@Test | ||
fun `does not report used parameter to initialize property`() { | ||
val code = """ | ||
class Test(used: Any) { | ||
val usedString = used.toString() | ||
} | ||
""".trimIndent() | ||
assertThat(subject.lint(code)).isEmpty() | ||
} | ||
} | ||
|
||
@Nested | ||
inner class `secondary parameters` { | ||
@Test | ||
fun `report unused parameters in secondary constructors`() { | ||
val code = """ | ||
private class ClassWithSecondaryConstructor { | ||
constructor(used: Any, unused: Any) { | ||
used.toString() | ||
} | ||
|
||
// this is actually unused, but clashes with the other constructor | ||
constructor(used: Any) | ||
} | ||
""".trimIndent() | ||
assertThat(subject.lint(code)).hasSize(1) | ||
} | ||
} | ||
|
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.
An unfortunate consequence of this refactor is that unused primary and secondary constructor params are now detected by UnusedPrivateProperty
(they used to be detected by UnusedPrivateMember
which handled everything together).
I wanted to keep this refactor focused on mechanical splitting and not introduce logic changes that would be even harder to review. I would suggest follow up on this in a tracking issue I talk about in the other comments to see if it's possible to move the checks into UnusedParameter
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 might be reasonable given that a property can be defined from constructor's parameters, by adding val
or var
. If you can describe the behavior in the KDoc documentation it would be great
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.
Thank you @chao2zhang - that seems like good suggestion. I added to the KDoc in 5897a31
@@ -10,7 +11,7 @@ import org.junit.jupiter.api.Test | |||
|
|||
@KotlinCoreEnvironmentTest | |||
class UnusedPrivateParameterSpec(val env: KotlinCoreEnvironment) { |
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.
Can I rename this to UnusedParameterSpec
to match the new rule? It seems to detect unused params for both public and private.
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.
Yes please.
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.
Thank you, done as b563ece
Codecov Report
@@ Coverage Diff @@
## main #5722 +/- ##
============================================
- Coverage 84.52% 84.48% -0.05%
- Complexity 3731 3742 +11
============================================
Files 544 546 +2
Lines 12759 12791 +32
Branches 2232 2233 +1
============================================
+ Hits 10785 10806 +21
- Misses 864 869 +5
- Partials 1110 1116 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…roperty so it doesn't become a breaking change
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 haven't reviewed in detail, but I think after the restructuring the code looks way cleaner than before.
After the merge conflict is resolved, I think we should merge this asap.
@@ -383,7 +383,35 @@ class UnusedPrivateMemberSpec(val env: KotlinCoreEnvironment) { | |||
} | |||
|
|||
@Nested | |||
inner class `operators` { | |||
inner class `main methods` { |
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.
Hey @schalkms thank you for performing the merge 🙏 but I think this main methods
now needs to be in UnusedParameterSpec.kt
rather than in UnusedPrivateMemberSpec.kt
. That was how it was in the original patch I submitted since those test cases now cover UnusedParameter.kt
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.
@schalkms would you prefer me to add a commit to address this?
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.
Good catch. Btw, sorry for messing around with the merge.
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.
@drawers you were a few seconds faster. 😉
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.
@schalkms I added a commit ddd19eb to address this. If I take Chao's last commit before the merge as authoritative (1e4bcb8) then I do:
git diff --name-only 1e4bcb8 | grep Unused
I get:
detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateMemberSpec.kt
Then if I do:
git diff 1e4bcb8 detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateMemberSpec.kt
I get:
diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateMemberSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateMemberSpec.kt
index 2dbf7673d3..237df91e75 100644
--- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateMemberSpec.kt
+++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateMemberSpec.kt
@@ -383,7 +383,7 @@ class UnusedPrivateMemberSpec(val env: KotlinCoreEnvironment) {
}
@Nested
- inner class `operators` {
+ inner class Operators {
@Test
fun `does not report used plus operator - #1354`() {
So we end up with only the change from renaming inner class operators
to inner class Operators
and nothing else. Which I think is what we want.
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.
inner class
operators
{
to
inner class Operators {
Yes 🙏
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.
We could also rollback the merge and do a rebase. That might be easier to grasp.
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.
@schalkms yes if you would prefer me to rebase then I don't mind doing it just let me know 👍
(sorry about leaving the branch to go stale, I didn't anticipate another PR touching that test)
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.
@drawers if we are good to go after the merge, we are fine, no need to do another rebase then, IMHO.
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.
@schalkms looks okay to me now!
Fixes #3418 where it was decided to split
UnusedPrivateMember
in a non-breaking way so that after the split:I was not sure from the issue discussion whether to split into 2 or 3. I went for 3 because:
_
but not unused properties that begin with_
.UnusedPrivateMember
It's quite hard to review - the splitting of the
UnusedPrivateMember
functionality is mechanical but moving the test cases around is harder and requires more care. I guess I would recommend reviewing the individual commits.