Skip to content

Commit

Permalink
UnusedPrivateProperty improvements and refactoring out new UnusedVari…
Browse files Browse the repository at this point in the history 
…able rule (#7089)

* wip: fix for unused private property in same file

* minor refactor

* remove not applicable tests

* remove '_' from allowed names

* compilation fixes

* replace compileAndLintWithContext with lintWithContext

* UnusedVariable rule

* tests for lamda

* type resolution annotation

* unused property parameter false +ve

* Apply suggestions from code review

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

* moved top level property check to UnusedPrivateProperty

* Apply suggestions from code review

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

* Apply suggestions from code review

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

* add support for destructuring declearation

---------

Co-authored-by: Brais Gabín <braisgabin@gmail.com>
  • Loading branch information
@psuzn @BraisGabin
psuzn and BraisGabin committed Apr 14, 2024
1 parent 7b5d42d commit dfda4e0
Show file tree
Hide file tree
Showing 6 changed files with 875 additions and 329 deletions.
5 changes: 4 additions & 1 deletion detekt-core/src/main/resources/default-detekt-config.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,10 @@ style:
allowedNames: ''
UnusedPrivateProperty:
active: true
allowedNames: '_|ignored|expected|serialVersionUID'
allowedNames: 'ignored|expected|serialVersionUID'
UnusedVariable:
active: true
allowedNames: 'ignored|_'
UseAnyOrNoneInsteadOfFind:
active: true
UseArrayLiteralsInAnnotations:
Expand Down
1 change: 1 addition & 0 deletions ...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
::UnusedPrivateClass,
::UnusedPrivateMember,
::UnusedPrivateProperty,
::UnusedVariable,
::ExpressionBodySyntax,
::NestedClassesVisibility,
::RedundantVisibilityModifier,
Expand Down
171 changes: 112 additions & 59 deletions ...es-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,35 @@ import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.Configuration
import io.gitlab.arturbosch.detekt.api.DetektVisitor
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.RequiresTypeResolution
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.config
import io.gitlab.arturbosch.detekt.rules.isActual
import io.gitlab.arturbosch.detekt.rules.isExpect
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.descriptors.ClassConstructorDescriptor
import org.jetbrains.kotlin.descriptors.DeclarationDescriptor
import org.jetbrains.kotlin.descriptors.PropertyDescriptor
import org.jetbrains.kotlin.descriptors.Visibilities
import org.jetbrains.kotlin.descriptors.isTopLevelInPackage
import org.jetbrains.kotlin.js.resolve.diagnostics.findPsi
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtConstructor
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.psi.KtImportDirective
import org.jetbrains.kotlin.psi.KtNameReferenceExpression
import org.jetbrains.kotlin.psi.KtNamedDeclaration
import org.jetbrains.kotlin.psi.KtPackageDirective
import org.jetbrains.kotlin.psi.KtParameter
import org.jetbrains.kotlin.psi.KtPrimaryConstructor
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtReferenceExpression
import org.jetbrains.kotlin.psi.KtSecondaryConstructor
import org.jetbrains.kotlin.psi.KtValueArgumentList
import org.jetbrains.kotlin.psi.psiUtil.containingClassOrObject
import org.jetbrains.kotlin.psi.psiUtil.getChildrenOfType
import org.jetbrains.kotlin.psi.psiUtil.isPrivate
import org.jetbrains.kotlin.psi.psiUtil.isPropertyParameter
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.bindingContextUtil.getReferenceTargets

/**
* An unused private property can be removed to simplify the source file.
Expand All @@ -46,113 +58,154 @@ import org.jetbrains.kotlin.psi.psiUtil.isPrivate
* }
* </compliant>
*/
@RequiresTypeResolution
@ActiveByDefault(since = "1.23.0")
class UnusedPrivateProperty(config: Config) : Rule(
config,
"Property is unused and should be removed."
) {

override val defaultRuleIdAliases: Set<String> =
setOf("UNUSED_VARIABLE", "UNUSED_PARAMETER", "unused", "UnusedPrivateMember")
setOf("UNUSED_PARAMETER", "unused", "UnusedPrivateMember")

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

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

@Suppress("unused")
private class UnusedPrivatePropertyVisitor(private val allowedNames: Regex) : DetektVisitor() {
private class UnusedPrivatePropertyVisitor(
private val allowedNames: Regex,
private val bindingContext: BindingContext,
) : DetektVisitor() {

private val properties = mutableSetOf<KtNamedDeclaration>()
private val nameAccesses = mutableSetOf<String>()
private val topLevelProperties = hashSetOf<KtNamedDeclaration>()
private val usedTopLevelProperties = hashSetOf<PsiElement>()

private val classProperties = hashSetOf<KtNamedDeclaration>()
private val usedClassProperties = hashSetOf<PsiElement>()

private val constructorParameters = hashSetOf<KtNamedDeclaration>()
private val usedConstructorParameters = hashSetOf<PsiElement>()

fun getUnusedReports(): List<CodeSmell> {
return properties
.filter { it.nameAsSafeName.identifier !in nameAccesses }
val propertiesReport = classProperties
.filter { it.psiOrParent !in usedClassProperties }
.filter { !allowedNames.matches(it.nameAsSafeName.identifier) }
.map {
CodeSmell(
Entity.atName(it),
"Private property `${it.nameAsSafeName.identifier}` is unused.",
entity = Entity.atName(it),
message = "Private property `${it.nameAsSafeName.identifier}` is unused."
)
}
}

override fun visitParameter(parameter: KtParameter) {
super.visitParameter(parameter)
if (parameter.isLoopParameter) {
val destructuringDeclaration = parameter.destructuringDeclaration
if (destructuringDeclaration != null) {
for (variable in destructuringDeclaration.entries) {
maybeAddUnusedProperty(variable)
}
} else {
maybeAddUnusedProperty(parameter)
val constructorParametersReport = constructorParameters
.filter { it.psiOrParent !in usedConstructorParameters }
.filter { !allowedNames.matches(it.nameAsSafeName.identifier) }
.map {
CodeSmell(
entity = Entity.atName(it),
message = "Constructor parameter `${it.nameAsSafeName.identifier}` is unused.",
)
}
}

val topLevelPropertyReport = topLevelProperties
.filter { it.psiOrParent !in usedTopLevelProperties }
.filter { !allowedNames.matches(it.nameAsSafeName.identifier) }
.map {
CodeSmell(
entity = Entity.atName(it),
message = "Private top level property `${it.nameAsSafeName.identifier}` is unused.",
)
}

return propertiesReport + constructorParametersReport + topLevelPropertyReport
}

override fun visitPrimaryConstructor(constructor: KtPrimaryConstructor) {
super.visitPrimaryConstructor(constructor)

constructor.valueParameters
.filter {
(it.isPrivate() || (!it.hasValOrVar() && !constructor.isActual())) &&
it.containingClassOrObject?.isExpect() == false &&
isConstructorForDataOrValueClass(constructor).not()
(it.isPrivate() || !it.isPropertyParameter()) &&
!constructor.isExpectClassConstructor() &&
!constructor.isDataOrValueClassConstructor()
}
.forEach { valueParameter ->
if (valueParameter.isPropertyParameter()) {
classProperties.add(valueParameter)
} else {
constructorParameters.add(valueParameter)
}
}
.forEach { maybeAddUnusedProperty(it) }
}

private fun isConstructorForDataOrValueClass(constructor: PsiElement): Boolean {
val parent = constructor.parent as? KtClass ?: return false
return parent.isData() || parent.isValue() || parent.isInline()
}

override fun visitSecondaryConstructor(constructor: KtSecondaryConstructor) {
super.visitSecondaryConstructor(constructor)
constructor.valueParameters.forEach { maybeAddUnusedProperty(it) }
constructorParameters += constructor.valueParameters
}

private fun maybeAddUnusedProperty(it: KtNamedDeclaration) {
if (!allowedNames.matches(it.nameAsSafeName.identifier)) {
properties.add(it)
override fun visitProperty(property: KtProperty) {
super.visitProperty(property)

if (!property.isPrivate()) {
return
}
}

override fun visitProperty(property: KtProperty) {
if (property.isPrivate() && property.isMemberOrTopLevel() || property.isLocal) {
maybeAddUnusedProperty(property)
if (property.isTopLevel) {
topLevelProperties.add(property)
} else {
classProperties.add(property)
}
super.visitProperty(property)
}

private fun KtProperty.isMemberOrTopLevel() = isMember || isTopLevel

override fun visitReferenceExpression(expression: KtReferenceExpression) {
if (!skipNode(expression)) {
nameAccesses.add(expression.text.removeSurrounding("`"))
}
super.visitReferenceExpression(expression)
}
val references = when (expression) {
is KtNameReferenceExpression -> expression.getReferenceTargets(bindingContext)
is KtCallExpression -> expression.getChildrenOfType<KtValueArgumentList>()
.flatMap { it.arguments }
.flatMap {
it.getArgumentExpression()?.getReferenceTargets(bindingContext).orEmpty()
}

private fun skipNode(expression: KtReferenceExpression): Boolean {
return when {
expression.isPackageDirective() -> true
expression.isImportDirective() -> true
else -> false
else -> return
}

references
.filter {
it.containingDeclaration is ClassConstructorDescriptor || it.isPrivateProperty()
}
.forEach { descriptor ->
val psi = descriptor.findPsi() ?: return@forEach
when {
descriptor.isTopLevelInPackage() -> usedTopLevelProperties.add(psi)
descriptor.isPropertyParameter() -> usedClassProperties.add(psi)
else -> usedConstructorParameters.add(psi)
}
}
}
}

private fun PsiElement.isPackageDirective(): Boolean {
return this is KtPackageDirective || parent?.isPackageDirective() == true
}
private fun KtConstructor<*>.isExpectClassConstructor() =
containingClassOrObject?.isExpect() == true

private fun PsiElement.isImportDirective(): Boolean {
return this is KtImportDirective || parent?.isImportDirective() == true
private fun KtConstructor<*>.isDataOrValueClassConstructor(): Boolean {
val parent = parent as? KtClass ?: return false
return parent.isData() || parent.isValue() || parent.isInline()
}

fun DeclarationDescriptor.isPrivateProperty() =
this is PropertyDescriptor && visibility.name == Visibilities.Private.name

private fun DeclarationDescriptor.isPropertyParameter() =
this is PropertyDescriptor || (findPsi() as? KtParameter)?.isPropertyParameter() ?: false
Loading

0 comments on commit dfda4e0

Please sign in to comment.