From 528d0b172ba009a5d97047a8deba88c26a1d99aa Mon Sep 17 00:00:00 2001 From: DrAlexD Date: Fri, 8 Dec 2023 18:26:51 +0300 Subject: [PATCH 1/2] Fixed bug related to assigning to declared vals inside null-check branches ### What's done: - Fixed bug related to case when `if-else` block inside `init` or 'run', 'with', 'apply' blocks and there is more than one statement inside 'else' block. This case leads to kotlin compilation error after adding 'run' block instead of 'else' block. Read https://youtrack.jetbrains.com/issue/KT-64174 for more information. - Added new warning tests. Closes #1844 --- .../ruleset/rules/chapter4/NullChecksRule.kt | 103 +++++++++++------ .../chapter4/NullChecksRuleWarnTest.kt | 105 ++++++++++++++++++ 2 files changed, 177 insertions(+), 31 deletions(-) diff --git a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt index 4a19d0afa5..d7ecec1bb3 100644 --- a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt +++ b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt @@ -9,6 +9,8 @@ import com.saveourtool.diktat.ruleset.utils.parent import org.jetbrains.kotlin.KtNodeTypes.BINARY_EXPRESSION import org.jetbrains.kotlin.KtNodeTypes.BLOCK import org.jetbrains.kotlin.KtNodeTypes.BREAK +import org.jetbrains.kotlin.KtNodeTypes.CALL_EXPRESSION +import org.jetbrains.kotlin.KtNodeTypes.CLASS_INITIALIZER import org.jetbrains.kotlin.KtNodeTypes.CONDITION import org.jetbrains.kotlin.KtNodeTypes.ELSE import org.jetbrains.kotlin.KtNodeTypes.IF @@ -26,6 +28,9 @@ import org.jetbrains.kotlin.psi.KtBlockExpression import org.jetbrains.kotlin.psi.KtIfExpression import org.jetbrains.kotlin.psi.KtPsiUtil import org.jetbrains.kotlin.psi.psiUtil.blockExpressionsOrSingle +import org.jetbrains.kotlin.psi.psiUtil.parents + +typealias ThenAndElseLines = Pair?, List?> /** * This rule check and fixes explicit null checks (explicit comparison with `null`) @@ -72,32 +77,59 @@ class NullChecksRule(configRules: List) : DiktatRule( private fun conditionInIfStatement(node: ASTNode) { node.findAllDescendantsWithSpecificType(BINARY_EXPRESSION).forEach { binaryExprNode -> val condition = (binaryExprNode.psi as KtBinaryExpression) + if (isNullCheckBinaryExpression(condition)) { - when (condition.operationToken) { + val isEqualToNull = when (condition.operationToken) { // `==` and `===` comparison can be fixed with `?:` operator - KtTokens.EQEQ, KtTokens.EQEQEQ -> - warnAndFixOnNullCheck( - condition, - isFixable(node, true), - "use '.let/.also/?:/e.t.c' instead of ${condition.text}" - ) { - fixNullInIfCondition(node, condition, true) - } + KtTokens.EQEQ, KtTokens.EQEQEQ -> true // `!==` and `!==` comparison can be fixed with `.let/also` operators - KtTokens.EXCLEQ, KtTokens.EXCLEQEQEQ -> - warnAndFixOnNullCheck( - condition, - isFixable(node, false), - "use '.let/.also/?:/e.t.c' instead of ${condition.text}" - ) { - fixNullInIfCondition(node, condition, false) - } + KtTokens.EXCLEQ, KtTokens.EXCLEQEQEQ -> false else -> return } + + val (_, elseCodeLines) = getThenAndElseLines(node, isEqualToNull) + val (numberOfStatementsInElseBlock, isAssignmentInNewElseBlock) = getInfoAboutElseBlock(node, isEqualToNull) + + // if `if-else` block inside `init` or 'run', 'with', 'apply' blocks and there is more than one statement inside 'else' block, then + // we don't have to make any fixes, because this leads to kotlin compilation error after adding 'run' block instead of 'else' block + // read https://youtrack.jetbrains.com/issue/KT-64174 for more information + if (isShouldBeFixed(node, elseCodeLines, numberOfStatementsInElseBlock, isAssignmentInNewElseBlock)) { + warnAndFixOnNullCheck( + condition, + isFixable(node, isEqualToNull), + "use '.let/.also/?:/e.t.c' instead of ${condition.text}" + ) { + fixNullInIfCondition(node, condition, isEqualToNull) + } + } } } } + private fun isShouldBeFixed( + condition: ASTNode, + elseCodeLines: List?, + numberOfStatementsInElseBlock: Int, + isAssignment: Boolean, + ): Boolean = when { + // else { "null"/empty } -> "" + isNullOrEmptyElseBlock(elseCodeLines) -> true + // else { bar() } -> ?: bar() + isOnlyOneNonAssignmentStatementInElseBlock(numberOfStatementsInElseBlock, isAssignment) -> true + // else { ... } -> ?: run { ... } + else -> isNotInsideWrongBlock(condition) + } + + private fun isOnlyOneNonAssignmentStatementInElseBlock(numberOfStatementsInElseBlock: Int, isAssignment: Boolean) = numberOfStatementsInElseBlock == 1 && !isAssignment + + private fun isNullOrEmptyElseBlock(elseCodeLines: List?) = elseCodeLines == null || elseCodeLines.singleOrNull() == "null" + + private fun isNotInsideWrongBlock(condition: ASTNode): Boolean = condition.parents().none { + it.elementType == CLASS_INITIALIZER || + (it.elementType == CALL_EXPRESSION && + it.findChildByType(REFERENCE_EXPRESSION)?.text in listOf("run", "with", "apply")) + } + @Suppress("UnsafeCallOnNullableType") private fun isFixable(condition: ASTNode, isEqualToNull: Boolean): Boolean { @@ -126,6 +158,19 @@ class NullChecksRule(configRules: List) : DiktatRule( isEqualToNull: Boolean ) { val variableName = binaryExpression.left!!.text + val (thenCodeLines, elseCodeLines) = getThenAndElseLines(condition, isEqualToNull) + val (numberOfStatementsInElseBlock, isAssignmentInNewElseBlock) = getInfoAboutElseBlock(condition, isEqualToNull) + + val elseEditedCodeLines = getEditedElseCodeLines(elseCodeLines, numberOfStatementsInElseBlock, isAssignmentInNewElseBlock) + val thenEditedCodeLines = getEditedThenCodeLines(variableName, thenCodeLines, elseEditedCodeLines) + + val text = "$thenEditedCodeLines $elseEditedCodeLines" + val tree = KotlinParser().createNode(text) + val ifNode = condition.treeParent + ifNode.treeParent.replaceChild(ifNode, tree) + } + + private fun getThenAndElseLines(condition: ASTNode, isEqualToNull: Boolean): ThenAndElseLines { val thenFromExistingCode = condition.extractLinesFromBlock(THEN) val elseFromExistingCode = condition.extractLinesFromBlock(ELSE) @@ -141,7 +186,11 @@ class NullChecksRule(configRules: List) : DiktatRule( elseFromExistingCode } - val (numberOfStatementsInElseBlock, isAssignmentInNewElseBlock) = (condition.treeParent.psi as? KtIfExpression) + return Pair(thenCodeLines, elseCodeLines) + } + + private fun getInfoAboutElseBlock(condition: ASTNode, isEqualToNull: Boolean) = + ((condition.treeParent.psi as? KtIfExpression) ?.let { if (isEqualToNull) { it.then @@ -155,28 +204,20 @@ class NullChecksRule(configRules: List) : DiktatRule( KtPsiUtil.isAssignment(element) } } - ?: Pair(0, false) - - val elseEditedCodeLines = getEditedElseCodeLines(elseCodeLines, numberOfStatementsInElseBlock, isAssignmentInNewElseBlock) - val thenEditedCodeLines = getEditedThenCodeLines(variableName, thenCodeLines, elseEditedCodeLines) - - val text = "$thenEditedCodeLines $elseEditedCodeLines" - val tree = KotlinParser().createNode(text) - val ifNode = condition.treeParent - ifNode.treeParent.replaceChild(ifNode, tree) - } + ?: Pair(0, false)) + @Suppress("UnsafeCallOnNullableType") private fun getEditedElseCodeLines( elseCodeLines: List?, numberOfStatementsInElseBlock: Int, isAssignment: Boolean, ): String = when { // else { "null"/empty } -> "" - elseCodeLines == null || elseCodeLines.singleOrNull() == "null" -> "" + isNullOrEmptyElseBlock(elseCodeLines) -> "" // else { bar() } -> ?: bar() - numberOfStatementsInElseBlock == 1 && !isAssignment -> "?: ${elseCodeLines.joinToString(postfix = "\n", separator = "\n")}" + isOnlyOneNonAssignmentStatementInElseBlock(numberOfStatementsInElseBlock, isAssignment) -> "?: ${elseCodeLines!!.joinToString(postfix = "\n", separator = "\n")}" // else { ... } -> ?: run { ... } - else -> getDefaultCaseElseCodeLines(elseCodeLines) + else -> getDefaultCaseElseCodeLines(elseCodeLines!!) } @Suppress("UnsafeCallOnNullableType") diff --git a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt index e510491694..43122523ba 100644 --- a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt +++ b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt @@ -215,4 +215,109 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) { """.trimMargin() ) } + + @Test + @Tag(WarningNames.AVOID_NULL_CHECKS) + fun `don't trigger inside 'init' block when more than one statement in 'else' block`() { + lintMethod( + """ + |class Demo { + | val one: Int + | val two: String + | + | init { + | val number = get() + | if (number != null) { + | one = number.toInt() + | two = number + | } else { + | one = 0 + | two = "0" + | } + | } + | + | private fun get(): String? = if (Math.random() > 0.5) { "1" } else { null } + |} + """.trimMargin() + ) + } + + @Test + @Tag(WarningNames.AVOID_NULL_CHECKS) + fun `trigger inside 'init' block when only one statement in 'else' block`() { + lintMethod( + """ + |class Demo { + | val one: Int = 0 + | val two: String = "" + | + | init { + | val number = get() + | if (number != null) { + | print(number + 1) + | } else { + | print(null) + | } + | } + | + | private fun get(): String? = if (Math.random() > 0.5) { "1" } else { null } + |} + """.trimMargin(), + DiktatError(7, 13, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() + + " use '.let/.also/?:/e.t.c' instead of number != null", true), + ) + } + + @Test + @Tag(WarningNames.AVOID_NULL_CHECKS) + fun `trigger inside 'init' block when no 'else' block`() { + lintMethod( + """ + |class Demo { + | val one: Int = 0 + | val two: String = "" + | + | init { + | val number = get() + | if (number != null) { + | print(number) + | } + | } + | + | private fun get(): String? = if (Math.random() > 0.5) { "1" } else { null } + |} + """.trimMargin(), + DiktatError(7, 13, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() + + " use '.let/.also/?:/e.t.c' instead of number != null", true), + ) + } + + @Test + @Tag(WarningNames.AVOID_NULL_CHECKS) + fun `don't trigger inside 'run', 'with', 'apply' scope functions when more than one statement in 'else' block`() { + lintMethod( + """ + |class Demo { + | + | private fun set() { + | val one: Int + | val two: String + | + | run { + | val number: String? = get() + | if (number != null) { + | one = number.toInt() + | two = number + | } else { + | one = 0 + | two = "0" + | } + | } + | } + | + | private fun get(): String? = if (Math.random() > 0.5) { "1" } else { null } + |} + """.trimMargin() + ) + } } From 3217542f1738a43034504d16ab68107f44f4a8ad Mon Sep 17 00:00:00 2001 From: DrAlexD Date: Mon, 11 Dec 2023 11:38:41 +0300 Subject: [PATCH 2/2] - fixes to resolve conversations --- .../ruleset/rules/chapter4/NullChecksRule.kt | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt index d7ecec1bb3..9d40395f1b 100644 --- a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt +++ b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt @@ -82,7 +82,7 @@ class NullChecksRule(configRules: List) : DiktatRule( val isEqualToNull = when (condition.operationToken) { // `==` and `===` comparison can be fixed with `?:` operator KtTokens.EQEQ, KtTokens.EQEQEQ -> true - // `!==` and `!==` comparison can be fixed with `.let/also` operators + // `!=` and `!==` comparison can be fixed with `.let/also` operators KtTokens.EXCLEQ, KtTokens.EXCLEQEQEQ -> false else -> return } @@ -93,10 +93,10 @@ class NullChecksRule(configRules: List) : DiktatRule( // if `if-else` block inside `init` or 'run', 'with', 'apply' blocks and there is more than one statement inside 'else' block, then // we don't have to make any fixes, because this leads to kotlin compilation error after adding 'run' block instead of 'else' block // read https://youtrack.jetbrains.com/issue/KT-64174 for more information - if (isShouldBeFixed(node, elseCodeLines, numberOfStatementsInElseBlock, isAssignmentInNewElseBlock)) { + if (shouldBeWarned(node, elseCodeLines, numberOfStatementsInElseBlock, isAssignmentInNewElseBlock)) { warnAndFixOnNullCheck( condition, - isFixable(node, isEqualToNull), + canBeAutoFixed(node, isEqualToNull), "use '.let/.also/?:/e.t.c' instead of ${condition.text}" ) { fixNullInIfCondition(node, condition, isEqualToNull) @@ -106,7 +106,10 @@ class NullChecksRule(configRules: List) : DiktatRule( } } - private fun isShouldBeFixed( + /** + * Checks whether it is necessary to warn about null-check + */ + private fun shouldBeWarned( condition: ASTNode, elseCodeLines: List?, numberOfStatementsInElseBlock: Int, @@ -130,9 +133,12 @@ class NullChecksRule(configRules: List) : DiktatRule( it.findChildByType(REFERENCE_EXPRESSION)?.text in listOf("run", "with", "apply")) } + /** + * Checks whether null-check can be auto fixed + */ @Suppress("UnsafeCallOnNullableType") - private fun isFixable(condition: ASTNode, - isEqualToNull: Boolean): Boolean { + private fun canBeAutoFixed(condition: ASTNode, + isEqualToNull: Boolean): Boolean { // Handle cases with `break` word in blocks val typePair = if (isEqualToNull) { (ELSE to THEN) @@ -164,10 +170,10 @@ class NullChecksRule(configRules: List) : DiktatRule( val elseEditedCodeLines = getEditedElseCodeLines(elseCodeLines, numberOfStatementsInElseBlock, isAssignmentInNewElseBlock) val thenEditedCodeLines = getEditedThenCodeLines(variableName, thenCodeLines, elseEditedCodeLines) - val text = "$thenEditedCodeLines $elseEditedCodeLines" - val tree = KotlinParser().createNode(text) + val newTextForReplacement = "$thenEditedCodeLines $elseEditedCodeLines" + val newNodeForReplacement = KotlinParser().createNode(newTextForReplacement) val ifNode = condition.treeParent - ifNode.treeParent.replaceChild(ifNode, tree) + ifNode.treeParent.replaceChild(ifNode, newNodeForReplacement) } private fun getThenAndElseLines(condition: ASTNode, isEqualToNull: Boolean): ThenAndElseLines {