Skip to content

Commit

Permalink
Check for root of receiver in selector expression
Browse files Browse the repository at this point in the history
  • Loading branch information
@atulgpt
atulgpt committed May 7, 2024
1 parent f1b1bb1 commit 86a045c
Show file tree
Hide file tree
Showing 4 changed files with 176 additions and 19 deletions.
21 changes: 10 additions & 11 deletions ...rone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt
View file
Expand Up @@ -89,17 +89,16 @@ class NullCheckOnMutableProperty(config: Config) : Rule(
nonNullCondition.right as? KtNameReferenceExpression
} else {
nonNullCondition.left as? KtNameReferenceExpression
}?.let { referenceExpression ->
referenceExpression.getResolvedCall(bindingContext)
?.resultingDescriptor
?.let {
it.fqNameOrNull()?.takeIf(mutableProperties::contains)
}
}?.let { candidateFqName ->
// A candidate mutable property is present, so attach the current
// if-expression to it in the property candidates map.
candidateProperties.getOrPut(candidateFqName) { ArrayDeque() }.apply { add(expression) }
}
}?.getResolvedCall(bindingContext)
?.resultingDescriptor
?.fqNameOrNull()
?.takeIf(mutableProperties::contains)
?.let { candidateFqName ->
// A candidate mutable property is present, so attach the current
// if-expression to it in the property candidates map.
candidateProperties.getOrPut(candidateFqName) { ArrayDeque() }
.apply { add(expression) }
}
}
// Visit descendant expressions to see whether candidate properties
// identified in this if-expression are being referenced.
Expand Down
15 changes: 12 additions & 3 deletions detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt
View file
Expand Up @@ -12,6 +12,7 @@ import io.gitlab.arturbosch.detekt.rules.receiverIsUsed
import org.jetbrains.kotlin.descriptors.impl.ValueParameterDescriptorImpl.WithDestructuringDeclaration
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtLambdaExpression
import org.jetbrains.kotlin.psi.KtNamedDeclaration
import org.jetbrains.kotlin.psi.KtQualifiedExpression
Expand Down Expand Up @@ -47,13 +48,12 @@ class UnnecessaryLet(config: Config) : Rule(
config,
"The `let` usage is unnecessary."
) {

override fun visitCallExpression(expression: KtCallExpression) {
super.visitCallExpression(expression)

if (!expression.isCalling(letFqName, bindingContext)) return

val lambdaExpr = expression.lambdaArguments.firstOrNull()?.getLambdaExpression()

val referenceCount = lambdaExpr?.countLambdaParameterReference(bindingContext) ?: 0
if (referenceCount > 1) return

Expand Down Expand Up @@ -92,7 +92,7 @@ private fun canBeReplacedWithCall(lambdaExpr: KtLambdaExpression?): Boolean {
if (lambdaExpr == null) return false

val receiver = when (val statement = lambdaExpr.bodyExpression?.statements?.singleOrNull()) {
is KtQualifiedExpression -> statement.receiverExpression
is KtQualifiedExpression -> statement.getRootExpression()
else -> null
} ?: return false

Expand All @@ -108,6 +108,15 @@ private fun canBeReplacedWithCall(lambdaExpr: KtLambdaExpression?): Boolean {
return lambdaParameterNames.any { receiver.textMatches(it) }
}

private fun KtExpression?.getRootExpression(): KtExpression? {
// Look for the expression that was the root of a potential call chain.
var receiverExpression = this
while (receiverExpression is KtQualifiedExpression) {
receiverExpression = receiverExpression.receiverExpression
}
return receiverExpression
}

private fun KtLambdaExpression.countLambdaParameterReference(context: BindingContext): Int {
val bodyExpression = bodyExpression ?: return 0
val firstParameter = firstParameter(context) ?: return 0
Expand Down
16 changes: 11 additions & 5 deletions detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt
View file
Expand Up @@ -73,7 +73,12 @@ class VarCouldBeVal(config: Config) : Rule(
file.accept(assignmentVisitor)

assignmentVisitor.getNonReAssignedDeclarations().forEach {
report(CodeSmell(Entity.from(it), "Variable '${it.nameAsSafeName.identifier}' could be val."))
report(
CodeSmell(
Entity.from(it),
"Variable '${it.nameAsSafeName.identifier}' could be val."
)
)
}
}

Expand All @@ -95,7 +100,8 @@ class VarCouldBeVal(config: Config) : Rule(
val declarationName = nameAsSafeName.toString()
val assignments = assignments[declarationName]
if (assignments.isNullOrEmpty()) return false
val declarationDescriptor = bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, this]
val declarationDescriptor =
bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, this]
return assignments.any {
it.getResolvedCall(bindingContext)?.resultingDescriptor?.original == declarationDescriptor ||
// inside an unknown types context? (example: with-statement with unknown type)
Expand Down Expand Up @@ -138,9 +144,9 @@ class VarCouldBeVal(config: Config) : Rule(
expression.left?.let(::visitAssignment)

// Check for whether the assignment contains an object literal.
val descriptor = (expression.left as? KtNameReferenceExpression)?.let {
it.getResolvedCall(bindingContext)?.resultingDescriptor
}
val descriptor = (expression.left as? KtNameReferenceExpression)
?.getResolvedCall(bindingContext)
?.resultingDescriptor
val expressionRight = expression.right
if (descriptor != null && expressionRight != null) {
evaluateAssignmentExpression(descriptor, expressionRight)
Expand Down
143 changes: 143 additions & 0 deletions ...rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt
View file
Expand Up @@ -244,6 +244,10 @@ class UnnecessaryLetSpec(val env: KotlinCoreEnvironment) {
it.plus(1)
it.plus(2)
}
a?.let {
it.plus(1)
print(2)
}
b.let { that ->
that.plus(1)
that.plus(2)
Expand All @@ -265,6 +269,23 @@ class UnnecessaryLetSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).isEmpty()
}

@Test
fun `does not report lets with lambda body containing more than one statement with one ref count`() {
val findings = subject.compileAndLintWithContext(
env,
"""
fun f() {
val a: Int? = if (System.currentTimeMillis() > 0) 1 else null
a?.let {
it.plus(1)
print(2)
}
}
""".trimIndent()
)
assertThat(findings).isEmpty()
}

@Test
fun `does not report lets where it is used multiple times`() {
val findings = subject.compileAndLintWithContext(
Expand Down Expand Up @@ -469,6 +490,128 @@ class UnnecessaryLetSpec(val env: KotlinCoreEnvironment) {
)
assertThat(findings).hasSize(1)
}

@Nested
inner class `nested lets` {
@Test
fun `does not report nested nullable properties used with safe operator - #6373`() {
val findings = subject.compileAndLintWithContext(
env,
"""
class Dialog(val window: Int?)
val dialog: Dialog? = null
fun test() {
dialog?.window?.let {
println(it)
}
}
""".trimIndent()
)
assertThat(findings).isEmpty()
}

@Test
fun `reports nested nullable properties - #6373`() {
val findings = subject.compileAndLintWithContext(
env,
"""
class Dialog(val window: Int?)
val dialog: Dialog? = null
fun test() {
dialog?.let {
it.window?.let {
println(it)
}
}
}
""".trimIndent()
)
assertThat(findings).hasSize(1)
}

@Test
fun `does not report nested nullable properties when multiple expression are present`() {
val findings = subject.compileAndLintWithContext(
env,
"""
class Dialog(val window: Int?)
val dialog: Dialog? = null
fun test() {
dialog?.let {
it.window?.let {
val a = it + 1
println(a)
}
println(it.window)
}
}
""".trimIndent()
)
assertThat(findings).isEmpty()
}

@Test
fun `reports nested nullable properties when first let add more chain`() {
val findings = subject.compileAndLintWithContext(
env,
"""
class Dialog(val window: Int?)
val dialog: Dialog? = null
fun test() {
dialog?.let {
it.window?.inc()?.let {
println(it)
}
}
}
""".trimIndent()
)
assertThat(findings).hasSize(1)
}

@Test
fun `does not reports nested nullable properties when first let calls a fun`() {
val findings = subject.compileAndLintWithContext(
env,
"""
class Dialog(val window: Int?)
val dialog: Dialog? = null
fun inc(value: Int?) = value?.let { it + 1 }
fun test() {
dialog?.let {
inc(it.window)?.let { window ->
println(window)
}
}
dialog?.window?.inc()?.let { println(it) }
}
""".trimIndent()
)
assertThat(findings).isEmpty()
}

@Test
fun `reports double nested nullable properties`() {
val findings = subject.compileAndLintWithContext(
env,
"""
class Dialog(val window: Int?)
class ParentDialog(val dialog: Dialog?)
val parentDialog: ParentDialog? = null
fun test() {
parentDialog?.let {
it.dialog?.let { dialog ->
dialog.window?.let {
println(it)
}
}
}
}
""".trimIndent()
)
assertThat(findings).hasSize(2)
}
}
}

private const val MESSAGE_OMIT_LET = "let expression can be omitted"
Expand Down

0 comments on commit 86a045c

Please sign in to comment.