Repository: cqfn/diKTat
Pull request #532 (2020-11-16 21:18:21)

Removing complex cases from the warning related to null checks
### What's done:
Null check will not trigger on the following code:
1) if (myVar == null || otherValue == 5 && isValid) {}
2) if (myVar != null) {
       println("not null")
   } else if (true) {
       println("Other condition")
   }


codecov[bot]: # [Codecov](https://codecov.io/gh/cqfn/diKTat/pull/532?src=pr&el=h1) Report
> Merging [#532](https://codecov.io/gh/cqfn/diKTat/pull/532?src=pr&el=desc) (6b50112) into [master](https://codecov.io/gh/cqfn/diKTat/commit/fe2ad6f89b9e3ed8f47ded5fdd395ec22dcbed17?el=desc) (fe2ad6f) will **decrease** coverage by `0.04%`.
> The diff coverage is `55.55%`.

[![Impacted file tree graph](https://codecov.io/gh/cqfn/diKTat/pull/532/graphs/tree.svg?width=650&height=150&src=pr&token=P2quRDffpt)](https://codecov.io/gh/cqfn/diKTat/pull/532?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master     #532      +/-   ##
============================================
- Coverage     81.88%   81.83%   -0.05%     
- Complexity     1635     1639       +4     
============================================
  Files            78       78              
  Lines          4123     4129       +6     
  Branches       1302     1306       +4     
============================================
+ Hits           3376     3379       +3     
  Misses          219      219              
- Partials        528      531       +3     
```

| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| unittests | `81.83% <55.55%> (-0.05%)` | `0.00 <5.00> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/cqfn/diKTat/pull/532?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...in/org/cqfn/diktat/ruleset/rules/NullChecksRule.kt](https://codecov.io/gh/cqfn/diKTat/pull/532/diff?src=pr&el=tree#diff-ZGlrdGF0LXJ1bGVzL3NyYy9tYWluL2tvdGxpbi9vcmcvY3Fmbi9kaWt0YXQvcnVsZXNldC9ydWxlcy9OdWxsQ2hlY2tzUnVsZS5rdA==) | `77.77% <55.55%> (-5.56%)` | `18.00 <5.00> (+4.00)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/cqfn/diKTat/pull/532?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/cqfn/diKTat/pull/532?src=pr&el=footer). Last update [fe2ad6f...b3e374f](https://codecov.io/gh/cqfn/diKTat/pull/532?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

petertrr: I discovered that this rule currently reports simple comparisons as errors too:
```kotlin
val isDefault = configuration == null
```
yields
```
[AVOID_NULL_CHECKS] Try to avoid explicit null-checks. Use '.let/.also/?:/e.t.c' instead of: configuration == null
```

Patch:
diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/NullChecksRule.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/NullChecksRule.kt
index dc31c9d7..d4ea4ea8 100644
--- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/NullChecksRule.kt
+++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/NullChecksRule.kt
@@ -6,6 +6,7 @@ import com.pinterest.ktlint.core.ast.ElementType
 import com.pinterest.ktlint.core.ast.ElementType.BINARY_EXPRESSION
 import com.pinterest.ktlint.core.ast.ElementType.CONDITION
 import com.pinterest.ktlint.core.ast.ElementType.IF
+import com.pinterest.ktlint.core.ast.ElementType.IF_KEYWORD
 import com.pinterest.ktlint.core.ast.ElementType.NULL
 import com.pinterest.ktlint.core.ast.parent
 import org.cqfn.diktat.common.config.rules.RulesConfig
@@ -13,6 +14,7 @@ import org.cqfn.diktat.ruleset.constants.Warnings.AVOID_NULL_CHECKS
 import org.cqfn.diktat.ruleset.utils.*
 import org.jetbrains.kotlin.com.intellij.lang.ASTNode
 import org.jetbrains.kotlin.psi.KtBinaryExpression
+import org.jetbrains.kotlin.psi.KtIfExpression
 
 /**
  * This rule check and fixes explicit null checks (explicit comparison with `null`)
@@ -30,8 +32,11 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
 
         if (node.elementType == CONDITION) {
             node.parent(IF)?.let {
-                // this can be autofixed as the condition stays in if-statement
-                conditionInIfStatement(node)
+                // excluding complex cases with else-if statements, because they look better with explicit null-check
+                if (!isComplexIfStatement(it)) {
+                        // this can be autofixed as the condition stays in simple if-statement
+                        conditionInIfStatement(node)
+                }
             }
         }
 
@@ -44,6 +49,16 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
         }
     }
 
+    /**
+     * checks that if-statement is a complex condition
+     * You can name a statement - "complex if-statement" if it has other if in the else branch (else-if structure)
+     */
+    private fun isComplexIfStatement(parentIf: ASTNode): Boolean {
+        val parentIfPsi = parentIf.psi
+        require(parentIfPsi is KtIfExpression)
+        return (parentIfPsi.`else`?.node?.firstChildNode?.elementType == IF_KEYWORD)
+    }
+
     private fun conditionInIfStatement(node: ASTNode) {
         node.findAllNodesWithSpecificType(BINARY_EXPRESSION).forEach { binaryExprNode ->
             val condition = (binaryExprNode.psi as KtBinaryExpression)
@@ -69,11 +84,21 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
 
     @Suppress("UnsafeCallOnNullableType")
     private fun isNullCheckBinaryExpession(condition: KtBinaryExpression): Boolean =
-            // check that binary expession has `null` as right or left operand
-            setOf(condition.right, condition.left).map { it!!.node.elementType }.contains(NULL) &&
-                    // checks that it is the comparison condition
-                    setOf(ElementType.EQEQ, ElementType.EQEQEQ, ElementType.EXCLEQ, ElementType.EXCLEQEQEQ).contains(condition.operationToken)
+        // check that binary expession has `null` as right or left operand
+        setOf(condition.right, condition.left).map { it!!.node.elementType }.contains(NULL) &&
+        // checks that it is the comparison condition
+        setOf(ElementType.EQEQ, ElementType.EQEQEQ, ElementType.EXCLEQ, ElementType.EXCLEQEQEQ).contains(condition.operationToken) &&
+        // no need to raise warning or fix null checks in complex expressions
+        !condition.isComplexCondition()
 
+    /**
+    * checks if condition is a complex expression. For example:
+    * (a == 5) - is not a complex condition, but (a == 5 && b != 6) is a complex condition
+    */
+    private fun KtBinaryExpression.isComplexCondition(): Boolean {
+        // KtBinaryExpression is complex if it has a parent that is also a binary expression
+        return this.parent is KtBinaryExpression
+    }
 
     private fun warnAndFixOnNullCheck(condition: KtBinaryExpression, canBeAutoFixed: Boolean, autofix: () -> Unit) {
         AVOID_NULL_CHECKS.warnAndFix(
diff --git a/info/guide/guide-chapter-8.md b/info/guide/guide-chapter-8.md
index b133365b..7c194822 100644
--- a/info/guide/guide-chapter-8.md
+++ b/info/guide/guide-chapter-8.md
@@ -65,3 +65,19 @@ myVar?.let {
     println("null")
 } ?: run { println("not null") }
 ```
+
+**Exceptions:** in case of complex expressions like multiple `else-if` structures, or a long conditional statement there is a common sense to use explicit comparison with `null`.
+
+**Valid examples:**
+
+```kotlin
+if (myVar != null) {
+    println("not null")
+} else if (anotherCondition) {
+    println("Other condition")
+}
+``` 
+
+```kotlin
+if (myVar == null || otherValue == 5 && isValid) {}
+```


Test patch:
diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt
index 0cd9ebc1..c34596ea 100644
--- a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt
+++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt
@@ -37,7 +37,7 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 """
                 | fun foo() {
                 |     var myVar: Int? = null
-                |     if ((myVar == null) && (true)) {
+                |     if ((myVar == null) && (true) || isValid) {
                 |         println("null")
                 |         return
                 |     }
@@ -116,4 +116,55 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 LintError(5, 19, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar == null", false),
         )
     }
+
+    @Test
+    @Tag(WarningNames.AVOID_NULL_CHECKS)
+    fun `equals to null, but in complex else-if statement`() {
+        lintMethod(
+                """
+                | fun foo0() {
+                |     if (myVar != null) {
+                |        println("not null")
+                |      } else if (true) {
+                |        println()
+                |      }
+                | }
+                """.trimMargin()
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.AVOID_NULL_CHECKS)
+    fun `equals to null, but in complex else-if statement with dummy comment`() {
+        lintMethod(
+                """
+                | fun foo0() {
+                |     if (myVar != null) {
+                |        println("not null")
+                |      } else /* test comment */ if (true) {
+                |        println()
+                |      }
+                | }
+                """.trimMargin()
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.AVOID_NULL_CHECKS)
+    fun `equals to null, but the expression is not a else-if`() {
+        lintMethod(
+                """
+                | fun foo0() {
+                |     if (myVar != null) {
+                |        println("not null")
+                |      } else {
+                |           if (true) {
+                |                println()
+                |           }
+                |      }
+                | }
+                """.trimMargin(),
+                LintError(2, 10, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar != null", true),
+        )
+    }
 }