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.

Sign up for GitHub

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

Jump to bottom

Check for root of receiver in selector expression #7220

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Check for root of receiver in selector expression #7220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 10 additions & 11 deletions ...rone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt
View file
Open in desktop
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) }
}
Comment on lines +92 to +101
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as other comment. Looks like this change is unnecessary (let's move it to another PR?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this rule, this code is started flagging. As we have written the code as <expression>?.let { it.<someOtherNullableChain> } that could be simplified as <expression>?.<someOtherNullableChain>

}
// 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
Open in desktop
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
Open in desktop
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
Comment on lines +147 to +149
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment

With this rule, this code is started flagging. As we have written the code as <expression>?.let { it.<someOtherNullableChain> } that could be simplified as <expression>?.<someOtherNullableChain>

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
Open in desktop
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