Skip to content

Commit

Permalink
SONARKT-304 - simplify report message when the replacement suggested …
Browse files Browse the repository at this point in the history 
…is verbose (#391)
  • Loading branch information
@ADarko22
ADarko22 authored Nov 23, 2023
1 parent ea15223 commit ccb81ef
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 64 deletions.
85 changes: 57 additions & 28 deletions ...-test-sources/src/main/kotlin/checks/UnsuitedFindFunctionWithNullComparisonCheckSample.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,29 @@ class UnsuitedFindFunctionWithNullComparisonCheckSample {

fun onCollections(list: List<Int>, set: Set<Int>, array: Array<Int>, map: Map<Int, Int>) {

list.find { it > 5 } != null // Noncompliant {{Replace `list.find { it > 5 } != null` with `list.any { it > 5 }`.}}
list.find { it > 5 } != null // Noncompliant {{Replace with `list.any { it > 5 }`.}}
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

list.findLast { it > 5 } == null // Noncompliant {{Replace `list.findLast { it > 5 } == null` with `list.none { it > 5 }`.}}
list.firstOrNull { it > 5 } == null // Noncompliant {{Replace `list.firstOrNull { it > 5 } == null` with `list.none { it > 5 }`.}}
list.lastOrNull { it > 5 } == null // Noncompliant {{Replace `list.lastOrNull { it > 5 } == null` with `list.none { it > 5 }`.}}
list.findLast { it > 5 } == null // Noncompliant {{Replace with `list.none { it > 5 }`.}}
list.firstOrNull { it > 5 } == null // Noncompliant {{Replace with `list.none { it > 5 }`.}}
list.lastOrNull { it > 5 } == null // Noncompliant {{Replace with `list.none { it > 5 }`.}}

list.find { it == 5 } != null // Noncompliant {{Replace `list.find { it == 5 } != null` with `list.contains(5)`.}}
list.find { 5 == it } != null // Noncompliant {{Replace `list.find { 5 == it } != null` with `list.contains(5)`.}}
list.find { it != 5 } == null // Noncompliant {{Replace `list.find { it != 5 } == null` with `list.none { it != 5 }`.}}
list.find { 5 != it } == null // Noncompliant {{Replace `list.find { 5 != it } == null` with `list.none { 5 != it }`.}}
list.find { it == 5 } != null // Noncompliant {{Replace with `list.contains(5)`.}}
list.find { 5 == it } != null // Noncompliant {{Replace with `list.contains(5)`.}}
list.find { it != 5 } == null // Noncompliant {{Replace with `list.none { it != 5 }`.}}
list.find { 5 != it } == null // Noncompliant {{Replace with `list.none { 5 != it }`.}}

list.find { it == 5 } == null // Noncompliant {{Replace `list.find { it == 5 } == null` with `!list.contains(5)`.}}
list.find { 5 == it } == null // Noncompliant {{Replace `list.find { 5 == it } == null` with `!list.contains(5)`.}}
list.find { it != 5 } != null // Noncompliant {{Replace `list.find { it != 5 } != null` with `list.any { it != 5 }`.}}
list.find { 5 != it } != null // Noncompliant {{Replace `list.find { 5 != it } != null` with `list.any { 5 != it }`.}}
list.find { it == 5 } == null // Noncompliant {{Replace with `!list.contains(5)`.}}
list.find { 5 == it } == null // Noncompliant {{Replace with `!list.contains(5)`.}}
list.find { it != 5 } != null // Noncompliant {{Replace with `list.any { it != 5 }`.}}
list.find { 5 != it } != null // Noncompliant {{Replace with `list.any { 5 != it }`.}}

fun five() = 5
val five = 5
list.find { it > five } == null // Noncompliant {{Replace `list.find { it > five } == null` with `list.none { it > five }`.}}
list.find { it > five() } == null // Noncompliant {{Replace `list.find { it > five() } == null` with `list.none { it > five() }`.}}
list.find { it == five } != null // Noncompliant {{Replace `list.find { it == five } != null` with `list.contains(five)`.}}
list.find { it == five() } == null // Noncompliant {{Replace `list.find { it == five() } == null` with `!list.contains(five())`.}}
list.find { it > five } == null // Noncompliant {{Replace with `list.none { it > five }`.}}
list.find { it > five() } == null // Noncompliant {{Replace with `list.none { it > five() }`.}}
list.find { it == five } != null // Noncompliant {{Replace with `list.contains(five)`.}}
list.find { it == five() } == null // Noncompliant {{Replace with `!list.contains(five())`.}}

list.find { it > 5 }
list.findLast { it > 5 }
Expand All @@ -53,22 +53,22 @@ class UnsuitedFindFunctionWithNullComparisonCheckSample {
map.entries.find { it.key == five } != null // Noncompliant
map.entries.find { it.key > five() } == null // Noncompliant

list.map { it + 1 }.filter { it > 1 }.find { it > 5 } != null // Noncompliant {{Replace `list.map { it + 1 }.filter { it > 1 }.find { it > 5 } != null` with `list.map { it + 1 }.filter { it > 1 }.any { it > 5 }`.}}
list.map { it + 1 }.filter { it > 1 }.find { it > 5 } != null // Noncompliant {{Replace with `any`.}}
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

list.findLast { `my var` -> `my var` == 5 } != null // Noncompliant {{Replace `list.findLast { `my var` -> `my var` == 5 } != null` with `list.contains(5)`.}}
list.findLast { `my var` -> `my var` == 5 } != null // Noncompliant {{Replace with `list.contains(5)`.}}


null != list.find { it > 5 } // Noncompliant {{Replace `null != list.find { it > 5 }` with `list.any { it > 5 }`.}}
null != list.find { it > 5 } // Noncompliant {{Replace with `list.any { it > 5 }`.}}

((((list.find { it > 5 })))) != null // Noncompliant {{Replace `((((list.find { it > 5 })))) != null` with `list.any { it > 5 }`.}}
((((list.find { it > 5 })))) != null // Noncompliant {{Replace with `list.any { it > 5 }`.}}

with(list) {
if (find { it > 5 } != null) { // Noncompliant {{Replace `find { it > 5 } != null` with `any { it > 5 }`.}}
if (find { it > 5 } != null) { // Noncompliant {{Replace with `any { it > 5 }`.}}

}

if (find { it == 5 } != null) { // Noncompliant {{Replace `find { it == 5 } != null` with `contains(5)`.}}
if (find { it == 5 } != null) { // Noncompliant {{Replace with `contains(5)`.}}

}

Expand All @@ -79,18 +79,47 @@ class UnsuitedFindFunctionWithNullComparisonCheckSample {

5.let {
with(list) {
findLast { it > 5 } != null // Noncompliant {{Replace `findLast { it > 5 } != null` with `any { it > 5 }`.}}
findLast { it > 5 } != null // Noncompliant {{Replace with `any { it > 5 }`.}}
}
}

list.find { x -> x == 5 } != null // Noncompliant {{Replace `list.find { x -> x == 5 } != null` with `list.contains(5)`.}}
list.find { x -> x == five } != null // Noncompliant {{Replace `list.find { x -> x == five } != null` with `list.contains(five)`.}}
list.find { x -> five() == 5 } != null // Noncompliant {{Replace `list.find { x -> five() == 5 } != null` with `list.any { x -> five() == 5 }`.}}
list.filter { x -> x == 5 && (list.firstOrNull { it is Int } as Int?) == null } // Noncompliant {{Replace with `list.none { it is Int }`.}}

list.filter { x -> // Noncompliant {{Replace with `any`.}}
if(x == 5) false else true // filter
}.map {
it.inc() // mapping
}.find {
it > 5
} != null

list.map {// Noncompliant {{Replace with `contains`.}}
it.inc() // mapping
}.find {
it == 5
} != null

list.find {// Noncompliant {{Replace with `list.contains(5)`.}}
it == 5
} != null

val nullableList :List<Int>? = null

nullableList?.findLast { it == 5 } != null // Noncompliant {{Replace with `nullableList?.contains(5)`.}}
nullableList?.firstOrNull { it >= 5 } != null // Noncompliant {{Replace with `nullableList?.any { it >= 5 }`.}}
nullableList?.find { it >= 5 } == null // Noncompliant {{Replace with `nullableList?.none { it >= 5 }`.}}

nullableList!!.findLast { it == 5 } != null // Noncompliant {{Replace with `nullableList!!.contains(5)`.}}

list.find { x -> x == 5 } != null // Noncompliant {{Replace with `list.contains(5)`.}}
list.find { x -> x == five } != null // Noncompliant {{Replace with `list.contains(five)`.}}
list.find { x -> five() == 5 } != null // Noncompliant {{Replace with `list.any { x -> five() == 5 }`.}}
list.find { x -> x == five && five == 5 } != null // Noncompliant
list.find { x -> x == five && x == 5 } != null // Noncompliant
list.find { x -> 1 == 5 } != null // Noncompliant
list.find { x -> { x > 5 }() } != null // Noncompliant {{Replace `list.find { x -> { x > 5 }() } != null` with `list.any { x -> { x > 5 }() }`.}}
list.findLast { x -> { x == 5 }() } != null // Noncompliant {{Replace `list.findLast { x -> { x == 5 }() } != null` with `list.any { x -> { x == 5 }() }`.}}
list.find { x -> { x > 5 }() } != null // Noncompliant {{Replace with `list.any { x -> { x > 5 }() }`.}}
list.findLast { x -> { x == 5 }() } != null // Noncompliant {{Replace with `list.any { x -> { x == 5 }() }`.}}
list.findLast { x -> { if(x >= 5 && 5 > 4) false else true }() } == null // Noncompliant {{Replace with `none`.}}
}

class NonKotlinCollections {
Expand Down
78 changes: 42 additions & 36 deletions ...rc/main/java/org/sonarsource/kotlin/checks/UnsuitedFindFunctionWithNullComparisonCheck.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ import org.jetbrains.kotlin.lexer.KtTokens.EQEQ
import org.jetbrains.kotlin.lexer.KtTokens.EXCLEQ
import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtLambdaExpression
import org.jetbrains.kotlin.psi.KtNameReferenceExpression
import org.jetbrains.kotlin.psi.KtQualifiedExpression
import org.jetbrains.kotlin.psi.KtSafeQualifiedExpression
import org.jetbrains.kotlin.psi.psiUtil.getChildOfType
import org.jetbrains.kotlin.psi.psiUtil.isNull
import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall
Expand All @@ -47,68 +48,73 @@ import org.sonarsource.kotlin.api.reporting.message
@Rule(key = "S6528")
class UnsuitedFindFunctionWithNullComparisonCheck : CallAbstractCheck() {

override val functionsToVisit = listOf(
FunMatcher(qualifier = "kotlin.collections") {
withNames("find", "findLast", "firstOrNull", "lastOrNull")
withArguments("kotlin.Function1")
}
)
override val functionsToVisit = listOf(FunMatcher(qualifier = "kotlin.collections") {
withNames("find", "findLast", "firstOrNull", "lastOrNull")
withArguments("kotlin.Function1")
})

override fun visitFunctionCall(callExpression: KtCallExpression, resolvedCall: ResolvedCall<*>, kotlinFileContext: KotlinFileContext) {
callExpression.findClosestAncestorOfType<KtBinaryExpression>()
?.takeIf { it.right!!.isNull() || it.left!!.isNull() }
callExpression.findClosestAncestorOfType<KtBinaryExpression>()?.takeIf { it.right!!.isNull() || it.left!!.isNull() }
?.let { report(it, callExpression, kotlinFileContext) }
}

private fun report(nullComparisonExpr: KtBinaryExpression, callExpression: KtCallExpression, kotlinFileContext: KotlinFileContext) {
// callExpression has argument a lambda expression with a single parameter, due to the functionsToVisit FunMatchers
val lambda = callExpression.valueArguments[0].getArgumentExpression() as KtLambdaExpression

val lambdaBinaryExpr = lambda.bodyExpression!!.firstChild as? KtBinaryExpression
// Note that lambda.bodyExpression!!.children[0] and lambda.bodyExpression!!.firstChild behave differently in case of a block body
// when there is a comment before the expression, the firstChild is the comment, while children[0] is the expression
val lambdaBinaryExpr = lambda.bodyExpression!!.children[0] as? KtBinaryExpression
// the lambda either has the implicit "it" parameter or a single parameter
val lambdaParameterName = if (lambda.valueParameters.isEmpty()) "it" else lambda.valueParameters[0].name!!

// the functionsToVisit can be applied directly on the collection, or indirectly, for example using "with(collection)"
val dotExpressionBeforeFind =
callExpression.findClosestAncestorOfType<KtDotQualifiedExpression>(stopCondition = { it is KtBinaryExpression })?.receiverExpression
// in case the function is called on the collection directly, we build the replacement on the dot expression before the call
val beforeFindTxt = dotExpressionBeforeFind?.text?.plus(".") ?: ""
val qualifiedExprBeforeFind =
callExpression.findClosestAncestorOfType<KtQualifiedExpression>(stopCondition = { it is KtBinaryExpression })
val qualifiedExpressionAccessor = if (qualifiedExprBeforeFind is KtSafeQualifiedExpression) "?." else "."
// in case the function is called on the collection directly, we build the replacement on the qualified expression before the call
val beforeFindTxt = qualifiedExprBeforeFind?.receiverExpression?.text?.plus(qualifiedExpressionAccessor) ?: ""

// case in which the lambda predicate is an equality check: "it == something" or "x -> x == something"
if (lambdaBinaryExpr != null && lambdaBinaryExpr.isSingleEquality() && lambdaBinaryExpr.references(lambdaParameterName)) {
val negateContains = (lambdaBinaryExpr.operationToken == EQEQ) xor (nullComparisonExpr.operationToken == EXCLEQ)
// the binary expression operand that is not the lambda parameter
val elementTxt = lambdaBinaryExpr.nonParameterOperand(lambdaParameterName).text
val replacementExpr = (if (negateContains) "!" else "") + "${beforeFindTxt}contains($elementTxt)"

kotlinFileContext.reportIssue(nullComparisonExpr, message(nullComparisonExpr, replacementExpr))
// suggest the replacement only if the message is not too long and does not contain a line break
val message = if (replacementExpr.isVerbose()) message("contains") else message(replacementExpr)

kotlinFileContext.reportIssue(nullComparisonExpr, message)
} else {
val isAnyReplacement = nullComparisonExpr.operationToken == EXCLEQ
val replacementExpr = "${beforeFindTxt}${if (isAnyReplacement) "any" else "none"} ${lambda.text}"
val anyOrNone = if (isAnyReplacement) "any" else "none"
val replacementExpr = "${beforeFindTxt}${anyOrNone} ${lambda.text}"

// suggest the replacement only if the message is not too long and does not contain a line break
val message = if (replacementExpr.isVerbose()) message(anyOrNone) else message(replacementExpr)

kotlinFileContext.reportIssue(nullComparisonExpr, message(nullComparisonExpr, replacementExpr))
kotlinFileContext.reportIssue(nullComparisonExpr, message)
}
}

// checks that the binary expression has no binary expression as child, and it is an equality comparison
private fun KtBinaryExpression.isSingleEquality(): Boolean =
this.getChildOfType<KtBinaryExpression>() == null && this.operationToken == EQEQ
private fun message(replacementExpr: String): Message = message {
+"Replace with "
code(replacementExpr)
+"."
}
}

private fun KtBinaryExpression.references(reference: String): Boolean =
this.left!!.references(reference) || this.right!!.references(reference)
// checks that the binary expression has no binary expression as child, and it is an equality comparison
private fun KtBinaryExpression.isSingleEquality(): Boolean =
this.getChildOfType<KtBinaryExpression>() == null && this.operationToken == EQEQ

private fun KtBinaryExpression.nonParameterOperand(parameter: String): KtExpression =
if (!this.left!!.references(parameter)) this.left!! else this.right!!
private fun KtBinaryExpression.references(reference: String): Boolean =
this.left!!.references(reference) || this.right!!.references(reference)

private fun KtExpression.references(reference: String): Boolean =
this.let { it as? KtNameReferenceExpression }?.getReferencedName() == reference
private fun KtBinaryExpression.nonParameterOperand(parameter: String): KtExpression =
if (!this.left!!.references(parameter)) this.left!! else this.right!!

private fun message(nullComparisonExpr: KtBinaryExpression, replacementExpr: String): Message =
message {
+"Replace "
code(nullComparisonExpr.text)
+" with "
code(replacementExpr)
+"."
}
}
private fun KtExpression.references(reference: String): Boolean =
this.let { it as? KtNameReferenceExpression }?.getReferencedName() == reference

private fun String.isVerbose(): Boolean = this.length >= 40 || this.contains("\\n")

0 comments on commit ccb81ef

Please sign in to comment.