Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature. Auto-fixes for null-checks #681

Merged
merged 9 commits into from
Dec 31, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,17 @@ import com.pinterest.ktlint.core.Rule
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.ELSE
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.THEN
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.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.KtIfExpression

Expand Down Expand Up @@ -72,21 +75,60 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
ElementType.EQEQ, ElementType.EQEQEQ ->
warnAndFixOnNullCheck(condition, true,
"use '.let/.also/?:/e.t.c' instead of ${condition.text}") {
// todo implement fixer
fixNullInIfCondition(node, condition, true)
}
// `!==` and `!==` comparison can be fixed with `.let/also` operators
ElementType.EXCLEQ, ElementType.EXCLEQEQEQ ->
warnAndFixOnNullCheck(condition, true,
"use '.let/.also/?:/e.t.c' instead of ${condition.text}") {
// todo implement fixer
fixNullInIfCondition(node, condition, false)
}
else -> return
}
}
}
}

@Suppress("COMMENT_WHITE_SPACE")
@Suppress("UnsafeCallOnNullableType")
private fun fixNullInIfCondition(condition: ASTNode,
binaryExpression: KtBinaryExpression,
isEqualToNull: Boolean) {
val variableName = binaryExpression.left!!.text
val thenCodeLines = condition.extractLinesFromBlock(THEN)
val elseCodeLines = condition.extractLinesFromBlock(ELSE)
val text = if (isEqualToNull) {
if (elseCodeLines.isNullOrEmpty()) {
"$variableName ?: run {\n${thenCodeLines?.joinToString(separator = "\n")}\n}"
} else {
"""
|$variableName?.let {
|${elseCodeLines.joinToString(separator = "\n")}
|}
|?: run {
|${thenCodeLines?.joinToString(separator = "\n")}
|}
""".trimMargin()
}
} else {
if (elseCodeLines.isNullOrEmpty()) {
"$variableName?.let {\n${thenCodeLines?.joinToString(separator = "\n")}\n}"
} else {
"""
|$variableName?.let {
|${thenCodeLines?.joinToString(separator = "\n")}
|}
|?: run {
|${elseCodeLines.joinToString(separator = "\n")}
|}
""".trimMargin()
}
}
val tree = KotlinParser().createNode(text)
petertrr marked this conversation as resolved.
Show resolved Hide resolved
condition.treeParent.treeParent.addChild(tree, condition.treeParent)
condition.treeParent.treeParent.removeChild(condition.treeParent)
}

@Suppress("COMMENT_WHITE_SPACE", "UnsafeCallOnNullableType")
private fun nullCheckInOtherStatements(binaryExprNode: ASTNode) {
val condition = (binaryExprNode.psi as KtBinaryExpression)
if (isNullCheckBinaryExpression(condition)) {
Expand All @@ -104,17 +146,31 @@ class NullChecksRule(private val configRules: List<RulesConfig>) : Rule("null-ch
if (listOf(ElementType.EXCLEQ, ElementType.EXCLEQEQEQ).contains(condition.operationToken)) {
warnAndFixOnNullCheck(
condition,
false,
true,
"use 'requireNotNull' instead of require(${condition.text})"
) {
// todo implement fixer
val variableName = (binaryExprNode.psi as KtBinaryExpression).left!!.text
val newMethod = KotlinParser().createNode("requireNotNull($variableName)")
grandParent.treeParent.treeParent.addChild(newMethod, grandParent.treeParent)
grandParent.treeParent.treeParent.removeChild(grandParent.treeParent)
}
}
}
}
}
}

@Suppress("WRONG_INDENTATION")
private fun ASTNode.extractLinesFromBlock(type: IElementType): List<String>? =
treeParent
.getFirstChildWithType(type)
?.text
?.trim('{', '}')
?.split("\n")
?.filter { it.isNotBlank() }
?.map { it.trim() }
?.toList()

@Suppress("UnsafeCallOnNullableType")
private fun isNullCheckBinaryExpression(condition: KtBinaryExpression): Boolean =
// check that binary expression has `null` as right or left operand
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package org.cqfn.diktat.ruleset.chapter4

import org.cqfn.diktat.ruleset.rules.NullChecksRule
import org.cqfn.diktat.util.FixTestBase

import generated.WarningNames
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 fix if conditions`() {
fixAndCompare("IfConditionNullCheckExpected.kt", "IfConditionNullCheckTest.kt")
}

@Test
@Tag(WarningNames.AVOID_NULL_CHECKS)
fun `should fix require function`() {
fixAndCompare("RequireFunctionExpected.kt", "RequireFunctionTest.kt")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
| println("null")
| return
| }
| myVar ?: kotlin.run {
| println("null")
| }
| }
""".trimMargin(),
LintError(3, 11, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() +
Expand Down Expand Up @@ -183,7 +186,7 @@ class NullChecksRuleWarnTest : LintTestBase(::NullChecksRule) {
| }
""".trimMargin(),
LintError(2, 14, ruleId, Warnings.AVOID_NULL_CHECKS.warnText() +
" use 'requireNotNull' instead of require(myVar != null)", false),
" use 'requireNotNull' instead of require(myVar != null)", true),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ class DiktatSmokeTest : FixTestBase("test/smoke/src/main/kotlin",
fixAndCompare("DefaultPackageExpected.kt", "DefaultPackageTest.kt")
}

@Test
@Tag("DiktatRuleSetProvider")
fun `smoke test #7`() {
fixAndCompare("Example7Expected.kt", "Example7Test.kt")
}

@Test
@Tag("DiktatRuleSetProvider")
fun `smoke test #6`() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package test.paragraph4.null_checks

fun test() {
val some: Int? = null
some ?: run {
println("some")
bar()
}

some?.let {
println("some")
bar()
}

if (some == null && true) {
print("asd")
}

some?.let {
print("qwe")
}
?: run {
print("asd")
}

some?.let {
print("qqq")
}
?: run {
print("www")
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package test.paragraph4.null_checks

fun test() {
val some: Int? = null
if (some == null) {
println("some")
bar()
}

if (some != null) {
println("some")
bar()
}

if (some == null && true) {
print("asd")
}

if (some == null) {
print("asd")
} else {
print("qwe")
}

if (some != null) {
print("qqq")
} else {
print("www")
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package test.paragraph4.null_checks

fun test() {
val some: Int? = null

requireNotNull(some)
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package test.paragraph4.null_checks

fun test() {
val some: Int? = null

require(some != null)
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.cqfn.diktat

fun foo() {
val prop: Int = 0

prop ?: run {
println("prop is null")
bar()
}

prop?.let {
baz()
gaz()
}

prop?.let {
doAnotherSmth()
}
?: run {
doSmth()
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package org.cqfn.diktat

fun foo() {
val prop: Int? = null

if (prop == null) {
println("prop is null")
bar()
}

if (prop != null) {
baz()
gaz()
}

if (prop == null) {
doSmth()
} else {
doAnotherSmth()
}
}