Skip to content
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

Merged
merged 29 commits into from
Feb 5, 2023
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
968683f
Run ktlint --format on files about to be touched
drawers Jan 23, 2023
7cb6c0f
Make UnusedPrivateMember focus on private functions
drawers Jan 23, 2023
806a415
Extract UnusedParameter
drawers Jan 23, 2023
429142c
Extract UnusedParameter
drawers Jan 23, 2023
09a7c6b
Extract UnusedPrivateProperty
drawers Jan 23, 2023
7c50787
Move first batch of property tests from inside UnusedPrivateMemberSpec
drawers Jan 23, 2023
eb11585
Move second batch of property tests from inside UnusedPrivateMemberSpec
drawers Jan 23, 2023
a86e950
Readjust expectations in test now that subject only detects unused pr…
drawers Jan 23, 2023
7ff9c21
Move over next batch of property tests from UnusedPrivateMemberSpec
drawers Jan 23, 2023
39491ae
Move one test from "unused class declarations which are allowed"
drawers Jan 23, 2023
49df7cb
Move "nested class declarations" from UnusedPrivateMemberSpec
drawers Jan 23, 2023
939c679
Split "error messages"
drawers Jan 23, 2023
f6ffe46
Move "suppress unused property warning annotations"
drawers Jan 23, 2023
2406a24
Move "main methods" inner test class
drawers Jan 23, 2023
8048a00
Move "backtick identifiers" inner test class
drawers Jan 23, 2023
4376627
Move "backtick identifiers" inner test class for unused private param…
drawers Jan 23, 2023
93f090d
Split "highlights declaration name"
drawers Jan 23, 2023
99a350b
Move "parameter with the same name as named argument"
drawers Jan 23, 2023
636b3ad
Split "actual functions and classes"
drawers Jan 23, 2023
18830f8
Move "properties in primary constructors"
drawers Jan 23, 2023
5227986
Fix typo
drawers Jan 23, 2023
9241ae4
Fix disabled tests
drawers Jan 23, 2023
8c4c509
Add alias "UnusedPrivateMember" to UnusedParameter and UnusedPrivateP…
drawers Jan 23, 2023
b563ece
Rename "UnusedPrivateParameterSpec" to "UnusedParameterSpec"
drawers Jan 25, 2023
5a3a901
Adjust KDoc to say "source file" instead of "class etc." for clarity
drawers Jan 25, 2023
5897a31
Adjust KDoc to explain how UnusedPrivateProperty can detect unused co…
drawers Jan 25, 2023
1e4bcb8
inline method
chao2zhang Jan 28, 2023
8f9a9e5
Merge branch 'main' into fix-3418-split
schalkms Feb 5, 2023
ddd19eb
Remove duplicate `main methods` inner class after merge
drawers Feb 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -703,11 +703,17 @@ style:
active: false
UnusedImports:
active: false
UnusedParameter:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
Copy link
Contributor Author

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)

Copy link
Member

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.

UnusedPrivateClass:
active: true
UnusedPrivateMember:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
UnusedPrivateProperty:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
UseAnyOrNoneInsteadOfFind:
active: true
UseArrayLiteralsInAnnotations:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,10 @@ class StyleGuideProvider : DefaultRuleSetProvider {
DataClassShouldBeImmutable(config),
UseDataClass(config),
UnusedImports(config),
UnusedParameter(config),
UnusedPrivateClass(config),
UnusedPrivateMember(config),
UnusedPrivateProperty(config),
ExpressionBodySyntax(config),
NestedClassesVisibility(config),
RedundantVisibilityModifierRule(config),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.gitlab.arturbosch.detekt.api.CodeSmell
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.Debt
import io.gitlab.arturbosch.detekt.api.DetektVisitor
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.config
import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
import io.gitlab.arturbosch.detekt.api.internal.Configuration
import io.gitlab.arturbosch.detekt.rules.isAbstract
import io.gitlab.arturbosch.detekt.rules.isActual
import io.gitlab.arturbosch.detekt.rules.isExpect
import io.gitlab.arturbosch.detekt.rules.isExternal
import io.gitlab.arturbosch.detekt.rules.isMainFunction
import io.gitlab.arturbosch.detekt.rules.isOpen
import io.gitlab.arturbosch.detekt.rules.isOperator
import io.gitlab.arturbosch.detekt.rules.isOverride
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtClassOrObject
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.psi.KtParameter
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtReferenceExpression
import org.jetbrains.kotlin.psi.KtValueArgumentName
import org.jetbrains.kotlin.psi.psiUtil.isProtected

/**
* An unused parameter can be removed to simplify the signature of the function.
*
* <noncompliant>
* fun foo(unused: String) {
* println()
* }
* </noncompliant>
*
* <compliant>
* fun foo(used: String) {
* println(used)
* }
* </compliant>
*/
@ActiveByDefault(since = "1.23.0")
class UnusedParameter(config: Config = Config.empty) : Rule(config) {
override val defaultRuleIdAliases: Set<String> =
setOf("UNUSED_VARIABLE", "UNUSED_PARAMETER", "unused", "UnusedPrivateMember")

override val issue: Issue = Issue(
"UnusedParameter",
Severity.Maintainability,
"Function parameter is unused and should be removed.",
Debt.FIVE_MINS,
)

@Configuration("unused parameter names matching this regex are ignored")
private val allowedNames: Regex by config("(_|ignored|expected|serialVersionUID)", String::toRegex)

override fun visit(root: KtFile) {
super.visit(root)
val visitor = UnusedParameterVisitor(allowedNames)
root.accept(visitor)
visitor.getUnusedReports(issue).forEach { report(it) }
}
}

private class UnusedParameterVisitor(private val allowedNames: Regex) : DetektVisitor() {

private val unusedParameters: MutableSet<KtParameter> = mutableSetOf()

fun getUnusedReports(issue: Issue): List<CodeSmell> {
return unusedParameters.map {
CodeSmell(issue, Entity.atName(it), "Function parameter `${it.nameAsSafeName.identifier}` is unused.")
}
}

override fun visitClassOrObject(klassOrObject: KtClassOrObject) {
if (klassOrObject.isExpect()) return

super.visitClassOrObject(klassOrObject)
}

override fun visitClass(klass: KtClass) {
if (klass.isInterface()) return
if (klass.isExternal()) return

super.visitClass(klass)
}

override fun visitNamedFunction(function: KtNamedFunction) {
if (!function.isRelevant()) {
return
}

collectParameters(function)

super.visitNamedFunction(function)
}

private fun collectParameters(function: KtNamedFunction) {
val parameters = mutableMapOf<String, KtParameter>()
function.valueParameterList?.parameters?.forEach { parameter ->
val name = parameter.nameAsSafeName.identifier
if (!allowedNames.matches(name)) {
parameters[name] = parameter
}
}

function.accept(object : DetektVisitor() {
override fun visitProperty(property: KtProperty) {
if (property.isLocal) {
val name = property.nameAsSafeName.identifier
parameters.remove(name)
}
super.visitProperty(property)
}

override fun visitReferenceExpression(expression: KtReferenceExpression) {
if (expression.parent !is KtValueArgumentName) {
parameters.remove(expression.text.removeSurrounding("`"))
}
super.visitReferenceExpression(expression)
}
})

unusedParameters.addAll(parameters.values)
}

private fun KtNamedFunction.isRelevant() = !isAllowedToHaveUnusedParameters()

private fun KtNamedFunction.isAllowedToHaveUnusedParameters() =
isAbstract() || isOpen() || isOverride() || isOperator() || isMainFunction() || isExternal() ||
isExpect() || isActual() || isProtected()
}