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

Fixed bug related to assigning to declared vals inside null-check branches #1851

Merged
merged 5 commits into from
Dec 11, 2023
Merged

Fixed bug related to assigning to declared vals inside null-check branches #1851

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
528d0b1
Fixed bug related to assigning to declared vals inside null-check bra…
DrAlexD Dec 8, 2023
47dd69c
Merge branch 'master' into bugfix/assign-val
DrAlexD Dec 8, 2023
3217542
- fixes to resolve conversations
DrAlexD Dec 11, 2023
d9d97c9
Merge branch 'master' into bugfix/assign-val
DrAlexD Dec 11, 2023
b9f2e99
Merge branch 'master' into bugfix/assign-val
DrAlexD Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 72 additions & 31 deletions diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter4/NullChecksRule.kt
View file
Open in desktop
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
kgevorkyan marked this conversation as resolved.
Show resolved Hide resolved
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(
kgevorkyan marked this conversation as resolved.
Show resolved Hide resolved
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,
kgevorkyan marked this conversation as resolved.
Show resolved Hide resolved
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"
kgevorkyan marked this conversation as resolved.
Show resolved Hide resolved
val tree = KotlinParser().createNode(text)
kgevorkyan marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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()
)
}
}
Loading