diff --git a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt index 19cc5f438d3..302febd20c7 100644 --- a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt +++ b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt @@ -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. diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt index 4deb5f85017..6d14998beb8 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt @@ -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 @@ -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 @@ -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 @@ -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 diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt index 2fcd4e8c5c5..ee4c85186f7 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt @@ -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." + ) + ) } } @@ -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) @@ -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) diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt index ee5c76a2431..1b4229eec8a 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt @@ -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) @@ -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( @@ -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"