diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt index 2c8f4a8cd5..44bbc518fb 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt @@ -206,7 +206,9 @@ enum class Warnings( canBeAutoCorrected: Boolean = this.canBeAutoCorrected, autoFix: () -> Unit) { warn(configRules, emit, canBeAutoCorrected, freeText, offset, node) - fix(configRules, isFixMode, node, autoFix) + if (canBeAutoCorrected) { + fix(configRules, isFixMode, node, autoFix) + } } /** 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 1631f8b77c..f52999d07e 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 @@ -7,6 +7,8 @@ import org.cqfn.diktat.ruleset.utils.* 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.CONDITION import com.pinterest.ktlint.core.ast.ElementType.ELSE import com.pinterest.ktlint.core.ast.ElementType.IF @@ -20,6 +22,7 @@ import com.pinterest.ktlint.core.ast.parent import org.jetbrains.kotlin.com.intellij.lang.ASTNode 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 /** @@ -67,14 +70,20 @@ class NullChecksRule(configRules: List) : DiktatRule( when (condition.operationToken) { // `==` and `===` comparison can be fixed with `?:` operator ElementType.EQEQ, ElementType.EQEQEQ -> - warnAndFixOnNullCheck(condition, - "use '.let/.also/?:/e.t.c' instead of ${condition.text}") { + warnAndFixOnNullCheck( + condition, + isFixable(node, true), + "use '.let/.also/?:/e.t.c' instead of ${condition.text}" + ) { fixNullInIfCondition(node, condition, true) } // `!==` and `!==` comparison can be fixed with `.let/also` operators ElementType.EXCLEQ, ElementType.EXCLEQEQEQ -> - warnAndFixOnNullCheck(condition, - "use '.let/.also/?:/e.t.c' instead of ${condition.text}") { + warnAndFixOnNullCheck( + condition, + isFixable(node, false), + "use '.let/.also/?:/e.t.c' instead of ${condition.text}" + ) { fixNullInIfCondition(node, condition, false) } else -> return @@ -83,6 +92,27 @@ class NullChecksRule(configRules: List) : DiktatRule( } } + @Suppress("UnsafeCallOnNullableType") + private fun isFixable(condition: ASTNode, + isEqualToNull: Boolean): Boolean { + // Handle cases with `break` word in blocks + val typePair = if (isEqualToNull) { + (ELSE to THEN) + } else { + (THEN to ELSE) + } + val isBlockInIfWithBreak = condition.getBreakNodeFromIf(typePair.first) + val isOneLineBlockInIfWithBreak = condition + .treeParent + .findChildByType(typePair.second) + ?.let { it.findChildByType(BLOCK) ?: it } + ?.let { astNode -> + astNode.hasChildOfType(BREAK) && + (astNode.psi as? KtBlockExpression)?.statements?.size != 1 + } ?: false + return (!isBlockInIfWithBreak && !isOneLineBlockInIfWithBreak) + } + @Suppress("UnsafeCallOnNullableType", "TOO_LONG_FUNCTION") private fun fixNullInIfCondition(condition: ASTNode, binaryExpression: KtBinaryExpression, @@ -92,12 +122,22 @@ class NullChecksRule(configRules: List) : DiktatRule( val elseCodeLines = condition.extractLinesFromBlock(ELSE) val text = if (isEqualToNull) { when { - elseCodeLines.isNullOrEmpty() -> "$variableName ?: run {\n${thenCodeLines?.joinToString(separator = "\n")}\n}" + 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")} @@ -108,17 +148,19 @@ class NullChecksRule(configRules: List) : DiktatRule( """.trimMargin() } } else { - if (elseCodeLines.isNullOrEmpty() || (elseCodeLines.singleOrNull() == "null")) { - "$variableName?.let {\n${thenCodeLines?.joinToString(separator = "\n")}\n}" - } else { - """ - |$variableName?.let { - |${thenCodeLines?.joinToString(separator = "\n")} - |} - |?: run { - |${elseCodeLines.joinToString(separator = "\n")} - |} - """.trimMargin() + 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() } } val tree = KotlinParser().createNode(text) @@ -145,6 +187,7 @@ class NullChecksRule(configRules: List) : DiktatRule( if (listOf(ElementType.EXCLEQ, ElementType.EXCLEQEQEQ).contains(condition.operationToken)) { warnAndFixOnNullCheck( condition, + true, "use 'requireNotNull' instead of require(${condition.text})" ) { val variableName = (binaryExprNode.psi as KtBinaryExpression).left!!.text @@ -158,6 +201,13 @@ class NullChecksRule(configRules: List) : DiktatRule( } } + private fun ASTNode.getBreakNodeFromIf(type: IElementType) = this + .treeParent + .findChildByType(type) + ?.let { it.findChildByType(BLOCK) ?: it } + ?.findAllNodesWithCondition({ it.elementType == BREAK })?.isNotEmpty() + ?: false + private fun ASTNode.extractLinesFromBlock(type: IElementType): List? = treeParent .getFirstChildWithType(type) @@ -189,6 +239,7 @@ class NullChecksRule(configRules: List) : DiktatRule( private fun warnAndFixOnNullCheck( condition: KtBinaryExpression, + canBeAutoFixed: Boolean, freeText: String, autofix: () -> Unit) { AVOID_NULL_CHECKS.warnAndFix( @@ -198,7 +249,7 @@ class NullChecksRule(configRules: List) : DiktatRule( freeText, condition.node.startOffset, condition.node, - true, + canBeAutoFixed, ) { autofix() } 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 5962baa5f9..088b705ff8 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 @@ -8,6 +8,12 @@ import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test class NullChecksRuleFixTest : FixTestBase("test/paragraph4/null_checks", ::NullChecksRule) { + @Test + @Tag(WarningNames.AVOID_NULL_CHECKS) + fun `should careful fix if conditions with break`() { + fixAndCompare("IfConditionBreakCheckExpected.kt", "IfConditionBreakCheckTest.kt") + } + @Test @Tag(WarningNames.AVOID_NULL_CHECKS) fun `should fix if conditions`() { diff --git a/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionBreakCheckExpected.kt b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionBreakCheckExpected.kt new file mode 100644 index 0000000000..e4ff1130a7 --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionBreakCheckExpected.kt @@ -0,0 +1,84 @@ +package test.paragraph4.null_checks + + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result == null) { + foo() + } else { + break + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + result?.let { +foo() +} ?: break + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + break + } else { + foo() + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + result?.let { +foo() +} ?: break + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + result ?: break + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + break + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + foo() + break + } else { + break + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + result?.let { +goo() +} ?: break + } else { + break + } + } +} + diff --git a/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionBreakCheckTest.kt b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionBreakCheckTest.kt new file mode 100644 index 0000000000..d3cf2ebd1c --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph4/null_checks/IfConditionBreakCheckTest.kt @@ -0,0 +1,92 @@ +package test.paragraph4.null_checks + + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result == null) { + foo() + } else { + break + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result == null) { + break + } else { + foo() + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + break + } else { + foo() + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + foo() + } else { + break + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result == null) { + break + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + break + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + foo() + break + } else { + break + } + } +} + +fun foo() { + var result: Int? = 19 + while(result != 0 ) { + if (result != null) { + if (result != null) { + goo() + } else { + break + } + } else { + break + } + } +} + 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 2a05252416..255982c5e8 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 @@ -47,3 +47,25 @@ value } } +fun foo() { + var result: Int? = 10 + while (result != 0 ) { + result?.let { +goo() +} +?: run { +for(i in 1..10) +break +} + } + while (result != 0) { + result = goo() + if (result != null) { + goo() + } else { + println(123) + break + } + } +} + 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 5936fcb536..f95a7d34f0 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 @@ -48,3 +48,24 @@ fun test() { } } +fun foo() { + var result: Int? = 10 + while (result != 0 ) { + if (result != null) { + goo() + } else { + for(i in 1..10) + break + } + } + while (result != 0) { + result = goo() + if (result != null) { + goo() + } else { + println(123) + break + } + } +} +