Skip to content

Commit

Permalink
Changing functionality for AVOID_NULL_CHECK (#564)
Browse files Browse the repository at this point in the history 
Changing functionality for AVOID_NULL_CHECK

### What's done:
1) Now it is not trigered on simple cases like val a = b ==null
2) added logic for requireNotNull
  • Loading branch information
@orchestr7
orchestr7 authored Nov 30, 2020
1 parent 6d4a8ef commit fefe2fd
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 27 deletions.
2 changes: 1 addition & 1 deletion detekt-config.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ complexity:
NestedBlockDepth:
excludes: ['**/resources/**']
active: true
threshold: 4
threshold: 5
StringLiteralDuplication:
active: false
excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/jsTest/**', '**/iosTest/**']
Expand Down
2 changes: 1 addition & 1 deletion diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
67 changes: 49 additions & 18 deletions diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/NullChecksRule.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
}
}
Expand Down Expand Up @@ -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
}
}
}
Expand All @@ -78,39 +84,64 @@ 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,
) {
autofix()
}
}

private fun isRequireFun(referenceExpression: ASTNode) =
referenceExpression.elementType == REFERENCE_EXPRESSION && referenceExpression.firstChildNode.text == "require"

}
33 changes: 26 additions & 7 deletions diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter4/NullChecksRuleWarnTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
}

Expand All @@ -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),
)
}

Expand All @@ -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),
)
}

Expand All @@ -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),
)
}

Expand All @@ -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),
)
}

Expand All @@ -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),
)
}

Expand Down Expand Up @@ -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),
)
}
}
3 changes: 3 additions & 0 deletions info/guide/guide-chapter-8.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)`.

0 comments on commit fefe2fd

Please sign in to comment.