From e8b4a51183ed2814eaeca8c764dd54a203c6f3cb Mon Sep 17 00:00:00 2001 From: Alexander Frolov Date: Wed, 9 Mar 2022 20:40:45 +0300 Subject: [PATCH] Improve autofix in AVOID_NULL_CHECKS rule (#1201) * Improve autofix in AVOID_NULL_CHECKS rule ### What's done: * improved autofix: now we don't use `let` and `run` in some cases * added tests (#899) * Improve autofix in AVOID_NULL_CHECKS rule ### What's done: * improved autofix: now we don't use `let` and `run` in some cases * added tests (#899) * Improve autofix in AVOID_NULL_CHECKS rule ### What's done: * improved autofix: now we don't use `let` and `run` in some cases * added tests (#899) * Improve autofix in AVOID_NULL_CHECKS rule ### What's done: * improved autofix: now we don't use `let` and `run` in some cases * added tests (#899) * Improve autofix in AVOID_NULL_CHECKS rule ### What's done: * improved autofix: now we don't use `let` and `run` in some cases * added tests (#1149) * Improve autofix in AVOID_NULL_CHECKS rule ### What's done: * improved autofix: now we don't use `let` and `run` in some cases * improved autofix: now we count statements, not lines * added tests (#1201) * Improve autofix in AVOID_NULL_CHECKS rule ### What's done: * improved autofix: now we don't use `let` and `run` in some cases * improved autofix: now we count statements, not lines * added tests (#1201) * Improve autofix in AVOID_NULL_CHECKS rule ### What's done: * improved autofix: now we don't use `let` and `run` in some cases * improved autofix: now we count statements, not lines * added tests (#1201) Co-authored-by: sanyavertolet Co-authored-by: Andrey Kuleshov --- .../plugin/gradle/DiktatJavaExecTaskBase.kt | 2 +- .../ruleset/rules/chapter4/NullChecksRule.kt | 97 +++++++++++-------- .../ruleset/chapter4/NullChecksRuleFixTest.kt | 6 ++ .../IfConditionAssignCheckExpected.kt | 45 +++++++++ .../null_checks/IfConditionAssignCheckTest.kt | 53 ++++++++++ .../IfConditionNullCheckExpected.kt | 38 +++++--- .../null_checks/IfConditionNullCheckTest.kt | 42 ++++++++ .../smoke/src/main/kotlin/Example7Expected.kt | 5 +- 8 files changed, 226 insertions(+), 62 deletions(-) create mode 100644 diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckExpected.kt create mode 100644 diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckTest.kt diff --git a/diktat-gradle-plugin/src/main/kotlin/org/cqfn/diktat/plugin/gradle/DiktatJavaExecTaskBase.kt b/diktat-gradle-plugin/src/main/kotlin/org/cqfn/diktat/plugin/gradle/DiktatJavaExecTaskBase.kt index 6ce190e1c9..0c21a4d566 100644 --- a/diktat-gradle-plugin/src/main/kotlin/org/cqfn/diktat/plugin/gradle/DiktatJavaExecTaskBase.kt +++ b/diktat-gradle-plugin/src/main/kotlin/org/cqfn/diktat/plugin/gradle/DiktatJavaExecTaskBase.kt @@ -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." } } diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter4/NullChecksRule.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter4/NullChecksRule.kt index da03cc778c..8ce43efab3 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter4/NullChecksRule.kt +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter4/NullChecksRule.kt @@ -24,6 +24,7 @@ import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType import org.jetbrains.kotlin.psi.KtBinaryExpression import org.jetbrains.kotlin.psi.KtBlockExpression import org.jetbrains.kotlin.psi.KtIfExpression +import org.jetbrains.kotlin.psi.psiUtil.blockExpressionsOrSingle /** * This rule check and fixes explicit null checks (explicit comparison with `null`) @@ -120,56 +121,66 @@ class NullChecksRule(configRules: List) : 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 -> """ - |$variableName?.let { - |${elseCodeLines.joinToString(separator = "\n")} - |} - |?: run { - |${thenCodeLines.joinToString(separator = "\n")} - |} - """.trimMargin() - } + 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 -> """ - |$variableName?.let { - |${thenCodeLines?.joinToString(separator = "\n")} - |} - |?: run { - |${elseCodeLines.joinToString(separator = "\n")} - |} - """.trimMargin() - } + thenFromExistingCode } + val elseCodeLines = if (isEqualToNull) { + thenFromExistingCode + } else { + elseFromExistingCode + } + val numberOfStatementsInElseBlock = if (isEqualToNull) { + (condition.treeParent.psi as KtIfExpression).then?.blockExpressionsOrSingle()?.count() ?: 0 + } else { + (condition.treeParent.psi as KtIfExpression).`else`?.blockExpressionsOrSingle()?.count() ?: 0 + } + + val elseEditedCodeLines = getEditedElseCodeLines(elseCodeLines, numberOfStatementsInElseBlock) + 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 getEditedElseCodeLines(elseCodeLines: List?, numberOfStatementsInElseBlock: Int): String = when { + // else { "null"/empty } -> "" + elseCodeLines == null || elseCodeLines.singleOrNull() == "null" -> "" + // else { bar() } -> ?: bar() + numberOfStatementsInElseBlock == 1 -> "?: ${elseCodeLines.joinToString(postfix = "\n", separator = "\n")}" + // else { ... } -> ?: run { ... } + else -> getDefaultCaseElseCodeLines(elseCodeLines) + } + + @Suppress("UnsafeCallOnNullableType") + private fun getEditedThenCodeLines( + variableName: String, + thenCodeLines: List?, + 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 = + "$variableName?.let {${thenCodeLines?.joinToString(prefix = "\n", postfix = "\n", separator = "\n")}}" + + private fun getDefaultCaseElseCodeLines(elseCodeLines: List): 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) diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleFixTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleFixTest.kt index 088b705ff8..f97bf3ebdb 100644 --- a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleFixTest.kt +++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleFixTest.kt @@ -25,4 +25,10 @@ class NullChecksRuleFixTest : FixTestBase("test/paragraph4/null_checks", ::NullC fun `should fix require function`() { fixAndCompare("RequireFunctionExpected.kt", "RequireFunctionTest.kt") } + + @Test + @Tag(WarningNames.AVOID_NULL_CHECKS) + fun `should fix if conditions when assigned`() { + fixAndCompare("IfConditionAssignCheckExpected.kt", "IfConditionAssignCheckTest.kt") + } } diff --git a/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckExpected.kt b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckExpected.kt new file mode 100644 index 0000000000..c60654544d --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckExpected.kt @@ -0,0 +1,45 @@ +package test.paragraph4.null_checks + +fun foo() { + val x = a?.let { +f(a) +} ?: g(a) + + val y = a ?: 0 + + x ?: println("NULL") + + val z = x ?: run { +println("NULL") +0 +} + + x?.let { +f(x) +} ?: run { +println("NULL") +g(x) +} +} + +fun bar() { + val x = a?.let { +f(a) +} ?: g(a) + + val y = a ?: 0 + + x ?: println("NULL") + + val z = x ?: run { +println("NULL") +0 +} + + x?.let { +f(x) +} ?: run { +println("NULL") +g(x) +} +} diff --git a/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckTest.kt b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckTest.kt new file mode 100644 index 0000000000..b7527a7c62 --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionAssignCheckTest.kt @@ -0,0 +1,53 @@ +package test.paragraph4.null_checks + +fun foo() { + val x = if (a != null) f(a) else g(a) + + val y = if (a != null) a else 0 + + if (x != null) { + x + } else { + println("NULL") + } + + val z = if (x != null) { + x + } else { + println("NULL") + 0 + } + + if (x != null) { + f(x) + } else { + println("NULL") + g(x) + } +} + +fun bar() { + val x = if (a == null) g(a) else f(a) + + val y = if (a == null) 0 else a + + if (x == null) { + println("NULL") + } else { + x + } + + val z = if (x == null) { + println("NULL") + 0 + } else { + x + } + + if (x == null) { + println("NULL") + g(x) + } else { + f(x) + } +} diff --git a/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckExpected.kt b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckExpected.kt index 255982c5e8..47af964fc1 100644 --- a/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckExpected.kt +++ b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckExpected.kt @@ -18,10 +18,7 @@ bar() some?.let { print("qwe") -} -?: run { -print("asd") -} +} ?: print("asd") some?.let { print("qweqwe") @@ -29,10 +26,7 @@ print("qweqwe") some?.let { print("qqq") -} -?: run { -print("www") -} +} ?: print("www") some?.let { print("ttt") @@ -40,8 +34,7 @@ print("ttt") some?.let { print("ttt") -} -?: run { +} ?: run { null value } @@ -52,11 +45,8 @@ fun foo() { while (result != 0 ) { result?.let { goo() -} -?: run { -for(i in 1..10) +} ?: for(i in 1..10) break -} } while (result != 0) { result = goo() @@ -69,3 +59,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() +} + diff --git a/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckTest.kt b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckTest.kt index f95a7d34f0..135c389548 100644 --- a/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckTest.kt +++ b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionNullCheckTest.kt @@ -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() + } +} + diff --git a/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example7Expected.kt b/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example7Expected.kt index 7887176225..6de69b3349 100644 --- a/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example7Expected.kt +++ b/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example7Expected.kt @@ -15,10 +15,7 @@ fun foo() { prop?.let { doAnotherSmth() - } - ?: run { - doSmth() - } + } ?: doSmth() } fun fooo() {