Repository: analysis-dev/diktat (kotlin)

Issue:
Comparison with null yields error in diktat 0.1.5
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
```

_Originally posted by @petertrr in https://github.com/cqfn/diKTat/issues/532#issuecomment-729714302_

Pull request:
### What's done:
1) Now it is not trigered on simple cases like val a = b ==null
2) added logic for requireNotNull

Patch:
diff --git a/detekt-config.yml b/detekt-config.yml
index f75c59e824..38827a04a2 100644
--- a/detekt-config.yml
+++ b/detekt-config.yml
@@ -110,7 +110,7 @@ complexity:
   NestedBlockDepth:
     excludes: ['**/resources/**'] 
     active: true
-    threshold: 4
+    threshold: 5
   StringLiteralDuplication:
     active: false
     excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/jsTest/**', '**/iosTest/**']
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 14449b4a9b..a5c03c6d02 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
@@ -109,7 +109,7 @@ enum class Warnings(val canBeAutoCorrected: Boolean, val ruleId: String, private
     GENERIC_VARIABLE_WRONG_DECLARATION(true, "4.3.2", "variable should have explicit type declaration"),
     // FixMe: change float literal to BigDecimal? Or kotlin equivalent?
     FLOAT_IN_ACCURATE_CALCULATIONS(false, "4.1.1", "floating-point values shouldn't be used in accurate calculations"),
-    AVOID_NULL_CHECKS(false, "4.3.3", "Try to avoid explicit null-checks. Use '.let/.also/?:/e.t.c' instead of"),
+    AVOID_NULL_CHECKS(false, "4.3.3", "Try to avoid explicit null-checks"),
 
     // ======== chapter 5 ========
     TOO_LONG_FUNCTION(false, "5.1.1", "function is too long: split it or make more primitive"),
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 d4ea4ea891..e2448ff234 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
@@ -8,6 +8,9 @@ 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.ElementType.REFERENCE_EXPRESSION
+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.parent
 import org.cqfn.diktat.common.config.rules.RulesConfig
 import org.cqfn.diktat.ruleset.constants.Warnings.AVOID_NULL_CHECKS
@@ -34,8 +37,8 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
             node.parent(IF)?.let {
                 // 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)
+                    // this can be autofixed as the condition stays in simple if-statement
+                    conditionInIfStatement(node)
                 }
             }
         }
@@ -65,11 +68,14 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
             if (isNullCheckBinaryExpession(condition)) {
                 when (condition.operationToken) {
                     // `==` and `===` comparison can be fixed with `?:` operator
-                    ElementType.EQEQ, ElementType.EQEQEQ -> warnAndFixOnNullCheck(condition, true) {}
+                    ElementType.EQEQ, ElementType.EQEQEQ ->
+                        warnAndFixOnNullCheck(condition, true,
+                                "use '.let/.also/?:/e.t.c' instead of ${condition.text}") {}
                     // `!==` and `!==` comparison can be fixed with `.let/also` operators
-                    ElementType.EXCLEQ, ElementType.EXCLEQEQEQ -> warnAndFixOnNullCheck(condition, true) {}
-                    else -> {
-                    }
+                    ElementType.EXCLEQ, ElementType.EXCLEQEQEQ ->
+                        warnAndFixOnNullCheck(condition, true,
+                                "use '.let/.also/?:/e.t.c' instead of ${condition.text}") {}
+                    else -> return
                 }
             }
         }
@@ -78,34 +84,55 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
     private fun nullCheckInOtherStatements(binaryExprNode: ASTNode) {
         val condition = (binaryExprNode.psi as KtBinaryExpression)
         if (isNullCheckBinaryExpession(condition)) {
-            warnAndFixOnNullCheck(condition, false) {}
+
+            // require(a != null) is incorrect, Kotlin has special method: `requireNotNull` - need to use it instead
+            // hierarchy is the following:
+            //              require(a != null)
+            //                 /            \
+            //      REFERENCE_EXPRESSION    VALUE_ARGUMENT_LIST
+            //                |                       |
+            //          IDENTIFIER(require)     VALUE_ARGUMENT
+            val parent = binaryExprNode.treeParent
+            if (parent != null && parent.elementType == VALUE_ARGUMENT) {
+                val grandParent = parent.treeParent
+                if (grandParent != null && grandParent.elementType == VALUE_ARGUMENT_LIST && isRequireFun(grandParent.treePrev)) {
+                    if (listOf(ElementType.EXCLEQ, ElementType.EXCLEQEQEQ).contains(condition.operationToken)) {
+                        warnAndFixOnNullCheck(
+                                condition,
+                                false,
+                                "use 'requireNotNull' instead of require(${condition.text})"
+                        ) {}
+                    }
+                }
+            }
         }
     }
 
     @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) &&
-        // no need to raise warning or fix null checks in complex expressions
-        !condition.isComplexCondition()
+            // check that binary expression 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
-    */
+     * 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) {
+    private fun warnAndFixOnNullCheck(condition: KtBinaryExpression, canBeAutoFixed: Boolean, freeText: String, autofix: () -> Unit) {
         AVOID_NULL_CHECKS.warnAndFix(
                 configRules,
                 emitWarn,
                 isFixMode,
-                condition.text,
+                freeText,
                 condition.node.startOffset,
                 condition.node,
                 canBeAutoFixed,
@@ -113,4 +140,8 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
             autofix()
         }
     }
+
+    private fun isRequireFun(referenceExpression: ASTNode) =
+            referenceExpression.elementType == REFERENCE_EXPRESSION && referenceExpression.firstChildNode.text == "require"
+
 }
diff --git a/info/guide/guide-chapter-8.md b/info/guide/guide-chapter-8.md
index 7c19482263..61f8b08862 100644
--- a/info/guide/guide-chapter-8.md
+++ b/info/guide/guide-chapter-8.md
@@ -81,3 +81,6 @@ if (myVar != null) {
 ```kotlin
 if (myVar == null || otherValue == 5 && isValid) {}
 ```
+
+Please also note, that instead of using `require(a != null)` with a not null check - you should use a special Kotlin function called `requireNotNull(a)`.
+


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 c34596ea5b..cd50363ff2 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
@@ -26,7 +26,7 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 |     }
                 | }
                 """.trimMargin(),
-                LintError(3, 10, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar == null", true),
+                LintError(3, 10, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} use '.let/.also/?:/e.t.c' instead of myVar == null", true),
         )
     }
 
@@ -43,7 +43,8 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 |     }
                 | }
                 """.trimMargin(),
-                LintError(3, 11, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar == null", true),
+                LintError(3, 11, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() +
+                        " use '.let/.also/?:/e.t.c' instead of myVar == null", true),
         )
     }
 
@@ -59,7 +60,8 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 |     }
                 | }
                 """.trimMargin(),
-                LintError(2, 10, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar != null", true),
+                LintError(2, 10, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() +
+                        " use '.let/.also/?:/e.t.c' instead of myVar != null", true),
         )
     }
 
@@ -77,7 +79,8 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 |                      }
                 | }
                 """.trimMargin(),
-                LintError(2, 27, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar != null", true),
+                LintError(2, 27, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() +
+                        " use '.let/.also/?:/e.t.c' instead of myVar != null", true),
         )
     }
 
@@ -94,7 +97,8 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 |     }
                 | }
                 """.trimMargin(),
-                LintError(2, 10, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar !== null", true),
+                LintError(2, 10, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() +
+                        " use '.let/.also/?:/e.t.c' instead of myVar !== null", true),
         )
     }
 
@@ -107,13 +111,13 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 |     if (true) {
                 |         fun foo() {
                 |             var myVar: Int? = null
+                |             val myVal = myVar == null
                 |             foo1(myVar == null)
                 |             println("null")
                 |         }
                 |      }
                 | }
                 """.trimMargin(),
-                LintError(5, 19, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar == null", false),
         )
     }
 
@@ -164,7 +168,22 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
                 |      }
                 | }
                 """.trimMargin(),
-                LintError(2, 10, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} myVar != null", true),
+                LintError(2, 10, ruleId, "${Warnings.AVOID_NULL_CHECKS.warnText()} use '.let/.also/?:/e.t.c'" +
+                        " instead of myVar != null", true),
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.AVOID_NULL_CHECKS)
+    fun `require statements - adding `() {
+        lintMethod(
+                """
+                | fun foo0(myVar: String?) {
+                |     require(myVar != null)
+                | }
+                """.trimMargin(),
+                LintError(2, 14, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() +
+                        " use 'requireNotNull' instead of require(myVar != null)", false),
         )
     }
 }