Skip to content

Commit

Permalink
SONARKT-303 - Implement rule S6527: Function chain using "filter" sho…
Browse files Browse the repository at this point in the history
…uld be simplified (#380)
  • Loading branch information
ADarko22 authored Nov 13, 2023
1 parent 39377ab commit 0c6d145
Show file tree
Hide file tree
Showing 8 changed files with 330 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package checks

class SimplifyFilteringBeforeTerminalOperationCheckNoSemanticsSample {
fun test(list: List<Int>, set: Set<Int>, array: Array<Int>) {
list.filter { it > 5 }.any()
list.any { it > 5 }

set.filter { it > 5 }.any()
set.any { it > 5 }

array.filter { it > 5 }.any()
array.any { it > 5 }

list.filter { it > 5 }.none()
list.none { it > 5 }

list.filter { it > 5 }.count()
list.count { it > 5 }

list.filter { it > 5 }.last()
list.last { it > 5 }

list.filter { it > 5 }.lastOrNull()
list.lastOrNull { it > 5 }

list.filter { it > 5 }.first()
list.first { it > 5 }

list.filter { it > 5 }.firstOrNull()
list.firstOrNull { it > 5 }

list.filter { it > 5 }.single()
list.single { it > 5 }

list.filter { it > 5 }.singleOrNull()
list.singleOrNull { it > 5 }

list.map { it + 1 }.filter { it < 10 }.filter { it > 5 }.any()
list.map { it + 1 }.filter { it < 10 }.filter { it > 5 }.map { it }.any()

with(list) {
filter { it > 5 }.any()
}

(list.filter { it > 5 }).any()

list.any()
list.none()
list.count()
list.last()
list.lastOrNull()
list.first()
list.firstOrNull()
list.single()
list.singleOrNull()

list.filter { it < 10 }.any { it > 5 }
list.filter { it > 5 }.apply { any() }
list.filter { it > 5 }.let { it.any() }
}

fun onMap(map: Map<Int, Int>) {
map.filter { it.value > 5 }.any()
map.any { it.value > 5 }

map.filter { it.value > 5 }.none()
map.none { it.value > 5 }

map.filter { it.value > 5 }.count()
map.count { it.value > 5 }

map.filter { (key, value) -> key > 5 && value > 5 }.any()
map.any { (key, value) -> key > 5 && value > 5 }

map.filter { (key, value) -> key > 5 && value > 5 }.none()
map.none { (key, value) -> key > 5 && value > 5 }

map.filter { (key, value) -> key > 5 && value > 5 }.count()
map.count { (key, value) -> key > 5 && value > 5 }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package checks

class SimplifyFilteringBeforeTerminalOperationCheckSample {
fun test(list: List<Int>, set: Set<Int>, array: Array<Int>) {
list.filter { it > 5 }.any() // Noncompliant {{Remove "filter { it > 5 }" and replace "any()" with "any { it > 5 }".}}
// ^^^^^^^^^^^^^^^^^
list.any { it > 5 }

set.filter { it > 5 }.any() // Noncompliant
set.any { it > 5 }

array.filter { it > 5 }.any() // Noncompliant
array.any { it > 5 }

list.filter { it > 5 }.none() // Noncompliant
list.none { it > 5 }

list.filter { it > 5 }.count() // Noncompliant
list.count { it > 5 }

list.filter { it > 5 }.last() // Noncompliant
list.last { it > 5 }

list.filter { it > 5 }.lastOrNull() // Noncompliant
list.lastOrNull { it > 5 }

list.filter { it > 5 }.first() // Noncompliant
list.first { it > 5 }

list.filter { it > 5 }.firstOrNull() // Noncompliant
list.firstOrNull { it > 5 }

list.filter { it > 5 }.single() // Noncompliant
list.single { it > 5 }

list.filter { it > 5 }.singleOrNull() // Noncompliant
list.singleOrNull { it > 5 }

list.map { it + 1 }.filter { it < 10 }.filter { it > 5 }.any() // Noncompliant
list.map { it + 1 }.filter { it < 10 }.filter { it > 5 }.map { it }.any()

with(list) {
filter { it > 5 }.any() // Noncompliant {{Remove "filter { it > 5 }" and replace "any()" with "any { it > 5 }".}}
// ^^^^^^^^^^^^^^^^^
}

(list.filter { it > 5 }).any() // Noncompliant {{Remove "filter { it > 5 }" and replace "any()" with "any { it > 5 }".}}
// ^^^^^^^^^^^^^^^^^

list.any()
list.none()
list.count()
list.last()
list.lastOrNull()
list.first()
list.firstOrNull()
list.single()
list.singleOrNull()

list.filter { it < 10 }.any { it > 5 }
list.filter { it > 5 }.apply { any() }
list.filter { it > 5 }.let { it.any() }
}

fun onMap(map: Map<Int, Int>) {
map.filter { it.value > 5 }.any() // Noncompliant
map.any { it.value > 5 }

map.filter { it.value > 5 }.none() // Noncompliant
map.none { it.value > 5 }

map.filter { it.value > 5 }.count() // Noncompliant
map.count { it.value > 5 }

map.filter { (key, value) -> key > 5 && value > 5 }.any() // Noncompliant
map.any { (key, value) -> key > 5 && value > 5 }

map.filter { (key, value) -> key > 5 && value > 5 }.none() // Noncompliant
map.none { (key, value) -> key > 5 && value > 5 }

map.filter { (key, value) -> key > 5 && value > 5 }.count() // Noncompliant
map.count { (key, value) -> key > 5 && value > 5 }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* SonarSource Kotlin
* Copyright (C) 2018-2023 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonarsource.kotlin.checks

import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
import org.jetbrains.kotlin.resolve.calls.callUtil.getCall
import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall
import org.jetbrains.kotlin.utils.IDEAPluginsCompatibilityAPI
import org.sonar.check.Rule
import org.sonarsource.kotlin.api.checks.CallAbstractCheck
import org.sonarsource.kotlin.api.checks.FunMatcher
import org.sonarsource.kotlin.api.frontend.KotlinFileContext

private const val KOTLIN_COLLECTIONS_QUALIFIER = "kotlin.collections"
private val FILTER_MATCHER = FunMatcher(qualifier = KOTLIN_COLLECTIONS_QUALIFIER, name = "filter") { withArguments("kotlin.Function1") }

@Rule(key = "S6527")
class SimplifyFilteringBeforeTerminalOperationCheck : CallAbstractCheck() {
override val functionsToVisit = listOf(
FunMatcher(qualifier = KOTLIN_COLLECTIONS_QUALIFIER) {
withNames("any", "none", "count", "last", "lastOrNull", "first", "firstOrNull", "single", "singleOrNull")
withNoArguments()
}
)

@OptIn(IDEAPluginsCompatibilityAPI::class)
override fun visitFunctionCall(callExpression: KtCallExpression, resolvedCall: ResolvedCall<*>, kotlinFileContext: KotlinFileContext) {
val chainedCallBeforeTerminalOperationCall = callExpression.parent
.let { it as? KtDotQualifiedExpression }
?.receiverExpression
.let { it?.getCall(kotlinFileContext.bindingContext) }

if (chainedCallBeforeTerminalOperationCall != null
&& FILTER_MATCHER.matches(chainedCallBeforeTerminalOperationCall, kotlinFileContext.bindingContext)
) {
val filterCallText = chainedCallBeforeTerminalOperationCall.callElement.text
val filterPredicateText = chainedCallBeforeTerminalOperationCall.valueArguments[0].asElement().text
val terminalOperationCallText = callExpression.text
val terminalOperationWithPredicate = "${callExpression.calleeExpression!!.text} $filterPredicateText"

val message = "Remove \"$filterCallText\" and replace \"$terminalOperationCallText\" with \"$terminalOperationWithPredicate\"."

kotlinFileContext.reportIssue(chainedCallBeforeTerminalOperationCall.callElement, message)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* SonarSource Kotlin
* Copyright (C) 2018-2023 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonarsource.kotlin.checks

import org.junit.jupiter.api.Test
import org.sonarsource.kotlin.testapi.KotlinVerifier

internal class SimplifyFilteringBeforeTerminalOperationCheckTest : CheckTest(SimplifyFilteringBeforeTerminalOperationCheck()) {

@Test
fun `with no semantics`() {
KotlinVerifier(check) {
this.fileName = "SimplifyFilteringBeforeTerminalOperationCheckNoSemanticsSample.kt"
this.classpath = emptyList()
this.deps = emptyList()
this.isAndroid = false
}.verifyNoIssue()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ import org.sonarsource.kotlin.checks.ScheduledThreadPoolExecutorZeroCheck
import org.sonarsource.kotlin.checks.SelfAssignmentCheck
import org.sonarsource.kotlin.checks.ServerCertificateCheck
import org.sonarsource.kotlin.checks.SimplifiedPreconditionsCheck
import org.sonarsource.kotlin.checks.SimplifyFilteringBeforeTerminalOperationCheck
import org.sonarsource.kotlin.checks.SimplifySizeExpressionCheck
import org.sonarsource.kotlin.checks.SingletonPatternCheck
import org.sonarsource.kotlin.checks.StreamNotConsumedCheck
Expand Down Expand Up @@ -236,6 +237,7 @@ val KOTLIN_CHECKS = listOf(
SelfAssignmentCheck::class.java,
ServerCertificateCheck::class.java,
SimplifiedPreconditionsCheck::class.java,
SimplifyFilteringBeforeTerminalOperationCheck::class.java,
SingletonPatternCheck::class.java,
SimplifySizeExpressionCheck::class.java,
StreamNotConsumedCheck::class.java,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<h2>Why is this an issue?</h2>
<p>The <code>filter(predicate)</code> function is used to extract a subset of elements from a collection that match a given predicate. Many collection
functions such as <code>any()</code>, <code>count()</code>, <code>first()</code>, and more, come with an optional condition predicate.</p>
<p>It is not recommended to invoke the <code>filter(predicate)</code> function prior to these terminal operations. Instead, the predicate variant of
the terminal operation should be used as a replacement.</p>
<h3>What is the potential impact?</h3>
<p>Using <code>filter(predicate)</code> before terminal operations can result in unnecessary iterations over the collection, which could negatively
impact the performance of the code, especially with large collections. By directly using the predicate variant of the function, you can streamline the
code and improve its efficiency and readability.</p>
<h2>How to fix it</h2>
<p>Replace the <code>filter(predicate)</code> call with the predicate variant of the terminal operation. As of Kotlin API version 1.8, the list of
terminal operations supporting a predicate is:</p>
<ul>
<li> <code>any()</code> </li>
<li> <code>none()</code> </li>
<li> <code>count()</code> </li>
<li> <code>first()</code>, <code>firstOrNull()</code> </li>
<li> <code>last()</code>, <code>lastOrNull()</code> </li>
<li> <code>single()</code>, <code>singleOrNull()</code> </li>
</ul>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
val list = listOf(5,2,9,6,8,2,5,7,3)
val hasElementsGreater5 = list.filter { it &gt; 5 }.any() // Noncompliant
val countElementsGreater5 = list.filter { it &gt; 5 }.count() // Noncompliant
val lastElementGreater5 = list.filter { it &gt; 5 }.lastOrNull() // Noncompliant
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
val list = listOf(5,2,9,6,8,2,5,7,3)
val hasElementsGreater5 = list.any { it &gt; 5 } // Compliant
val countElementsGreater5 = list.count { it &gt; 5 } // Compliant
val lastElementGreater5 = list.lastOrNull { it &gt; 5 } // Compliant
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/">Kotlin API Docs, Package kotlin.collections</a> </li>
</ul>

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"title": "Function chain using \"filter\" should be simplified",
"type": "CODE_SMELL",
"code": {
"impacts": {
"MAINTAINABILITY": "LOW"
},
"attribute": "EFFICIENT"
},
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-6527",
"sqKey": "S6527",
"scope": "All",
"quickfix": "unknown"
}
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
"S6517",
"S6518",
"S6519",
"S6527",
"S6529",
"S6530",
"S6531",
Expand Down

0 comments on commit 0c6d145

Please sign in to comment.