Skip to content

Commit

Permalink
Improve autofix in AVOID_NULL_CHECKS rule
Browse files Browse the repository at this point in the history 
### What's done:
 * improved autofix: now we don't use `let` and `run` in some cases
 * added tests

(#1149)
  • Loading branch information
sanyavertolet committed Feb 25, 2022
1 parent abbe9ea commit f3c4a21
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 67 deletions.
2 changes: 1 addition & 1 deletion diktat-gradle-plugin/src/main/kotlin/org/cqfn/diktat/plugin/gradle/DiktatJavaExecTaskBase.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ open class DiktatJavaExecTaskBase @Inject constructor(
// validate configuration
require(inputs == null && excludes == null) {
"`inputs` and `excludes` arguments for diktat task are deprecated and now should be changed for `inputs {}` " +
"with configuration for PatternFilterable. Please check https://github.com/diktat-static-analysis/diktat/README.md for more info."
"with configuration for PatternFilterable. Please check https://github.com/analysis-dev/diktat/README.md for more info."
}
}

Expand Down
93 changes: 45 additions & 48 deletions diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter4/NullChecksRule.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import com.pinterest.ktlint.core.ast.ElementType
import com.pinterest.ktlint.core.ast.ElementType.BINARY_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.BLOCK
import com.pinterest.ktlint.core.ast.ElementType.BREAK
import com.pinterest.ktlint.core.ast.ElementType.CALL_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.CONDITION
import com.pinterest.ktlint.core.ast.ElementType.ELSE
import com.pinterest.ktlint.core.ast.ElementType.IF
Expand All @@ -18,6 +19,7 @@ import com.pinterest.ktlint.core.ast.ElementType.REFERENCE_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.THEN
import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT
import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT_LIST
import com.pinterest.ktlint.core.ast.children
import com.pinterest.ktlint.core.ast.parent
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType
Expand Down Expand Up @@ -120,65 +122,60 @@ class NullChecksRule(configRules: List<RulesConfig>) : DiktatRule(
isEqualToNull: Boolean
) {
val variableName = binaryExpression.left!!.text
val thenCodeLines = condition.extractLinesFromBlock(THEN)
val elseCodeLines = condition.extractLinesFromBlock(ELSE)
val text = if (isEqualToNull) {
when {
elseCodeLines.isNullOrEmpty() ->
if (condition.getBreakNodeFromIf(THEN)) {
"$variableName ?: break"
} else {
"$variableName ?: run {${thenCodeLines?.joinToString(prefix = "\n", postfix = "\n", separator = "\n")}}"
}
thenCodeLines!!.singleOrNull() == "null" -> """
|$variableName?.let {
|${elseCodeLines.joinToString(separator = "\n")}
|}
""".trimMargin()
thenCodeLines.singleOrNull() == "break" -> """
|$variableName?.let {
|${elseCodeLines.joinToString(separator = "\n")}
|} ?: break
""".trimMargin()
else -> getDefaultCaseNullCheck(variableName, elseCodeLines, thenCodeLines)
}
val thenFromExistingCode = condition.extractLinesFromBlock(THEN)
val elseFromExistingCode = condition.extractLinesFromBlock(ELSE)

// if (a == null) { foo() } else { bar() } -> if (a != null) { bar() } else { foo() }
val thenCodeLines = if (isEqualToNull) {
elseFromExistingCode
} else {
when {
elseCodeLines.isNullOrEmpty() || (elseCodeLines.singleOrNull() == "null") ->
"$variableName?.let {${thenCodeLines?.joinToString(prefix = "\n", postfix = "\n", separator = "\n")}}"
elseCodeLines.singleOrNull() == "break" ->
"$variableName?.let {${thenCodeLines?.joinToString(prefix = "\n", postfix = "\n", separator = "\n")}} ?: break"
else -> getDefaultCaseNullCheck(variableName, thenCodeLines, elseCodeLines)
}
thenFromExistingCode
}
val elseCodeLines = if (isEqualToNull) {
thenFromExistingCode
} else {
elseFromExistingCode
}

val elseEditedCodeLines = getEditedElseCodeLines(elseCodeLines)
val thenEditedCodeLines = getEditedThenCodeLines(variableName, thenCodeLines, elseEditedCodeLines)

val text = "$thenEditedCodeLines $elseEditedCodeLines"
val tree = KotlinParser().createNode(text)
condition.treeParent.treeParent.addChild(tree, condition.treeParent)
condition.treeParent.treeParent.removeChild(condition.treeParent)
}

private fun getDefaultCaseNullCheck(
private fun getEditedElseCodeLines(elseCodeLines: List<String>?): String = when {
// else { "null"/empty } -> ""
elseCodeLines == null || elseCodeLines.singleOrNull() == "null" -> ""
// else { bar() } -> ?: bar()
elseCodeLines.singleOrNull() != null -> "?: ${elseCodeLines.first()}"
// else { ... } -> ?: run { ... }
else -> getDefaultCaseElseCodeLines(elseCodeLines)
}

private fun getEditedThenCodeLines(
variableName: String,
thenCodeLines: List<String>?,
elseCodeLines: List<String>?
): String {
val editedThenPart = if (variableName == thenCodeLines?.singleOrNull()) {
variableName
} else {
"""
|$variableName?.let {
|${thenCodeLines?.joinToString(separator = "\n")}
|}
|""".trimMargin()
}

val editedElsePart = elseCodeLines?.singleOrNull()
?: """
|run {
|${elseCodeLines?.joinToString(separator = "\n")}
|}""".trimMargin()
return "$editedThenPart ?: $editedElsePart"
elseEditedCodeLines: String
): String = when {
// if (a != null) { } -> a ?: editedElse
(thenCodeLines.isNullOrEmpty() && elseEditedCodeLines.isNotEmpty()) ||
// if (a != null) { a } else { ... } -> a ?: editedElse
(thenCodeLines?.singleOrNull() == variableName && elseEditedCodeLines.isNotEmpty()) -> variableName
// if (a != null) { a.foo() } -> a?.foo()
thenCodeLines?.singleOrNull()?.startsWith("$variableName.") ?: false -> "$variableName?${thenCodeLines?.firstOrNull()!!.removePrefix(variableName)}"
// if (a != null) { break } -> a?.let { ... }
// if (a != null) { foo() } -> a?.let { ... }
else -> getDefaultCaseThenCodeLines(variableName, thenCodeLines)
}

private fun getDefaultCaseThenCodeLines(variableName: String, thenCodeLines: List<String>?): String =
"$variableName?.let {${thenCodeLines?.joinToString(prefix = "\n", postfix = "\n", separator = "\n")}}"

private fun getDefaultCaseElseCodeLines(elseCodeLines: List<String>): String = "?: run {${elseCodeLines.joinToString(prefix = "\n", postfix = "\n", separator = "\n")}}"

@Suppress("COMMENT_WHITE_SPACE", "UnsafeCallOnNullableType")
private fun nullCheckInOtherStatements(binaryExprNode: ASTNode) {
val condition = (binaryExprNode.psi as KtBinaryExpression)
Expand Down
12 changes: 4 additions & 8 deletions ...at-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckExpected.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ package test.paragraph4.null_checks
fun foo() {
val x = a?.let {
f(a)
}
?: g(a)
} ?: g(a)

val y = a ?: 0

Expand All @@ -17,8 +16,7 @@ println("NULL")

x?.let {
f(x)
}
?: run {
} ?: run {
println("NULL")
g(x)
}
Expand All @@ -27,8 +25,7 @@ g(x)
fun bar() {
val x = a?.let {
f(a)
}
?: g(a)
} ?: g(a)

val y = a ?: 0

Expand All @@ -41,8 +38,7 @@ println("NULL")

x?.let {
f(x)
}
?: run {
} ?: run {
println("NULL")
g(x)
}
Expand Down
32 changes: 24 additions & 8 deletions diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckExpected.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,26 +18,23 @@ bar()

some?.let {
print("qwe")
}
?: print("asd")
} ?: print("asd")

some?.let {
print("qweqwe")
}

some?.let {
print("qqq")
}
?: print("www")
} ?: print("www")

some?.let {
print("ttt")
}

some?.let {
print("ttt")
}
?: run {
} ?: run {
null
value
}
Expand All @@ -48,8 +45,7 @@ fun foo() {
while (result != 0 ) {
result?.let {
goo()
}
?: run {
} ?: run {
for(i in 1..10)
break
}
Expand All @@ -65,3 +61,23 @@ break
}
}

fun checkSmartCases() {
val x = a?.toString() ?: "Null"
val y = a.b.c?.toString() ?: a.b.toString()
a?.let {
print()
}
a?.let {
foo()
} ?: boo()
}

fun reversedCheckSmartCases() {
val x = a?.toString() ?: "Null"
val y = a.b.c?.toString() ?: a.b.toString()
a ?: print()
a?.let {
foo()
} ?: boo()
}

42 changes: 42 additions & 0 deletions diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,45 @@ fun foo() {
}
}

fun checkSmartCases() {
val x = if (a != null) {
a.toString()
} else {
"Null"
}
val y = if (a.b.c != null) {
a.b.c.toString()
} else {
a.b.toString()
}
if (a != null) {
print()
}
if (a != null) {
foo()
} else {
boo()
}
}

fun reversedCheckSmartCases() {
val x = if (a == null) {
"Null"
} else {
a.toString()
}
val y = if (a.b.c == null) {
a.b.toString()
} else {
a.b.c.toString()
}
if (a == null) {
print()
}
if (a == null) {
boo()
} else {
foo()
}
}

3 changes: 1 addition & 2 deletions diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example7Expected.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ fun foo() {

prop?.let {
doAnotherSmth()
}
?: doSmth()
} ?: doSmth()
}

fun fooo() {
Expand Down

0 comments on commit f3c4a21

Please sign in to comment.