Skip to content

Commit

Permalink
Fixed bug related to assigning to declared vals inside null-check bra…
Browse files Browse the repository at this point in the history 
…nches

### 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
  • Loading branch information
@DrAlexD
DrAlexD committed Dec 8, 2023
1 parent 09edb26 commit 528d0b1
Show file tree
Hide file tree
Showing 2 changed files with 177 additions and 31 deletions.
103 changes: 72 additions & 31 deletions diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<String>?, List<String>?>

/**
* This rule check and fixes explicit null checks (explicit comparison with `null`)
Expand Down Expand Up @@ -72,32 +77,59 @@ class NullChecksRule(configRules: List<RulesConfig>) : 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<String>?,
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<String>?) = 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 {
Expand Down Expand Up @@ -126,6 +158,19 @@ class NullChecksRule(configRules: List<RulesConfig>) : 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)

Expand All @@ -141,7 +186,11 @@ class NullChecksRule(configRules: List<RulesConfig>) : 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
Expand All @@ -155,28 +204,20 @@ class NullChecksRule(configRules: List<RulesConfig>) : 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<String>?,
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")
Expand Down
105 changes: 105 additions & 0 deletions ...t-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
)
}
}

0 comments on commit 528d0b1

Please sign in to comment.