Incorrect fix of null-safety rule (#857)

* Incorrect fix of null-safety rule

### What's done:
Fixed bug, changed logic
Created tests

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)
+        }
     }
 
     /**

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<RulesConfig>) : 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<RulesConfig>) : 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<RulesConfig>) : 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<RulesConfig>) : 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<RulesConfig>) : 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<RulesConfig>) : 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<String>? =
             treeParent
                 .getFirstChildWithType(type)
@@ -189,6 +239,7 @@ class NullChecksRule(configRules: List<RulesConfig>) : DiktatRule(
 
     private fun warnAndFixOnNullCheck(
         condition: KtBinaryExpression,
+        canBeAutoFixed: Boolean,
         freeText: String,
         autofix: () -> Unit) {
         AVOID_NULL_CHECKS.warnAndFix(
@@ -198,7 +249,7 @@ class NullChecksRule(configRules: List<RulesConfig>) : DiktatRule(
             freeText,
             condition.node.startOffset,
             condition.node,
-            true,
+            canBeAutoFixed,
         ) {
             autofix()
         }

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`() {

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
+        }
+    }
+}
+

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
+        }
+    }
+}
+

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
+        }
+    }
+}
+