SONARKT-301 stop raising issues on abstract/overrided/open/interface methods (#395)

Author: erwan-serandour

kotlin-checks-test-sources/src/main/files/non-compiling/checks/CollectionShouldBeImmutableCheckSampleNonCompiling.kt

@@ -0,0 +1,12 @@
+package checks
+
+class CollectionShouldBeImmutableCheckSampleNonCompiling {
+
+    actual fun actualFun(list: MutableList<Int>): Unit // compliant
+    expect fun expectFun(list: MutableList<Int>): Unit // compliant
+
+    fun qualifiedStrange() {
+        val list = mutableListOf<Int>()
+        list.(add(1))
+    }
+}

kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSample.kt

@@ -78,6 +78,7 @@ class CollectionShouldBeImmutableCheckSample {
         l: MutableList<Int>, // Compliant
         m: MutableList<Int>, // Compliant
         n: MutableList<Int>, // Compliant
+        o: MutableMap<Int,Int>, // Compliant
     ): Unit {
         a.add(1)
         b.iterator()
@@ -95,6 +96,7 @@ class CollectionShouldBeImmutableCheckSample {
         l!!.doSomething()
         m?.doSomething()
         if(true){n}else{n}.doSomething()
+        o.entries
     }
 
 
@@ -219,10 +221,32 @@ class CollectionShouldBeImmutableCheckSample {
         }
     }
 
-    fun id(x : AMutableCollections): AMutableCollections = x // compliant, for now don't know how to check that is subtype of MutableCollection
+    fun id(x : AMutableCollections): AMutableCollections = x // FN,  for now don't know how to check that is subtype of MutableCollection
 
 
     fun foo(configure: (MutableMap<String, Any?>) -> Unit): Unit { // compliant
     }
 
+    interface A {
+        fun foo(list : MutableList<Int>): Unit // compliant
+        fun bar(list : MutableList<Int>): Int { // compliant
+            return list.reduce { acc, it -> acc + it}
+        }
+    }
+
+    abstract class B : A{
+        override fun foo(list: MutableList<Int>) { // compliant
+        }
+
+        abstract fun baz(list: MutableList<Int>): Unit // compliant
+
+        open fun qux(list: MutableList<Int>): Unit { // compliant
+        }
+
+    }
+
+}
+
+private fun nonCompliantParameterOnFileLevel(list: MutableList<Int>): Int { // Noncompliant
+    return list.reduce { acc, it -> acc + it}
 }

kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSampleNoSemantics.kt

@@ -77,6 +77,7 @@ class CollectionShouldBeImmutableCheckSampleNoSemantics {
         l: MutableList<Int>, // Compliant
         m: MutableList<Int>, // Compliant
         n: MutableList<Int>, // Compliant
+        o: MutableMap<Int,Int>, // Compliant
     ): Unit {
         a.add(1)
         b.iterator()
@@ -94,6 +95,7 @@ class CollectionShouldBeImmutableCheckSampleNoSemantics {
         l!!.doSomething()
         m?.doSomething()
         if(true){n}else{n}.doSomething()
+        o.entries
     }
 
 

sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/ApiExtensions.kt

@@ -311,6 +311,18 @@ fun KtNamedFunction.overrides() = modifierList?.hasModifier(KtTokens.OVERRIDE_KE
 
 fun KtNamedFunction.isAbstract() = modifierList?.hasModifier(KtTokens.ABSTRACT_KEYWORD) ?: false
 
+fun KtNamedFunction.isOpen(): Boolean {
+    return modifierList?.hasModifier(KtTokens.OPEN_KEYWORD) ?: false
+}
+
+fun KtNamedFunction.isActual(): Boolean {
+    return modifierList?.hasModifier(KtTokens.ACTUAL_KEYWORD) ?: false
+}
+
+fun KtNamedFunction.isExpect(): Boolean {
+    return modifierList?.hasModifier(KtTokens.EXPECT_KEYWORD) ?: false
+}
+
 fun KtNamedFunction.suspendModifier() = modifierList?.getModifier(KtTokens.SUSPEND_KEYWORD)
 
 fun KtQualifiedExpression.resolveReferenceTarget(bindingContext: BindingContext) =

sonar-kotlin-api/src/test/java/org/sonarsource/kotlin/api/checks/ApiExtensionsKtTest.kt

@@ -37,6 +37,7 @@ import org.jetbrains.kotlin.psi.KtExpression
 import org.jetbrains.kotlin.psi.KtFile
 import org.jetbrains.kotlin.psi.KtFunction
 import org.jetbrains.kotlin.psi.KtNameReferenceExpression
+import org.jetbrains.kotlin.psi.KtNamedFunction
 import org.jetbrains.kotlin.psi.KtPackageDirective
 import org.jetbrains.kotlin.psi.KtParameter
 import org.jetbrains.kotlin.psi.KtProperty
@@ -240,6 +241,39 @@ internal class ApiExtensionsKtTest {
         assertThat(PsiWhiteSpaceImpl(" ").getVariableType(BindingContext.EMPTY)).isNull()
     }
 
+    @Test
+    fun `test Functions modifiers`(){
+        val tree = parse(
+            """
+            abstract fun abstractFun():Unit{}
+            open fun openFun():Unit{}
+            override fun overrideFun():Unit{}
+            actual fun actualFun():Unit{}
+            expect fun expectFun():Unit{}
+            fun defaultFun():Unit{}
+            """.trimIndent()
+        )
+        val textToExpression: MutableMap<String, KtNamedFunction> = TreeMap()
+        walker(tree.psiFile) {
+            if (it is KtNamedFunction)
+                textToExpression[it.name!!] = it
+        }
+        assertThat(textToExpression["abstractFun"]!!.isAbstract()).isTrue()
+        assertThat(textToExpression["defaultFun"]!!.isAbstract()).isFalse()
+
+        assertThat(textToExpression["openFun"]!!.isOpen()).isTrue()
+        assertThat(textToExpression["defaultFun"]!!.isOpen()).isFalse()
+
+        assertThat(textToExpression["overrideFun"]!!.overrides()).isTrue()
+        assertThat(textToExpression["defaultFun"]!!.overrides()).isFalse()
+
+        assertThat(textToExpression["actualFun"]!!.isActual()).isTrue()
+        assertThat(textToExpression["defaultFun"]!!.isActual()).isFalse()
+
+        assertThat(textToExpression["expectFun"]!!.isExpect()).isTrue()
+        assertThat(textToExpression["defaultFun"]!!.isExpect()).isFalse()
+    }
+
     @Test
     fun `test KtTypeReference getType with null`() {
         assertThat((null as KtTypeReference?).getType(BindingContext.EMPTY)).isNull()

sonar-kotlin-checks/src/main/java/org/sonarsource/kotlin/checks/CollectionShouldBeImmutableCheck.kt

@@ -24,13 +24,16 @@ import org.jetbrains.kotlin.descriptors.CallableDescriptor
 import org.jetbrains.kotlin.js.descriptorUtils.getKotlinTypeFqName
 import org.jetbrains.kotlin.psi.KtBinaryExpression
 import org.jetbrains.kotlin.psi.KtCallExpression
+import org.jetbrains.kotlin.psi.KtClass
 import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
 import org.jetbrains.kotlin.psi.KtElement
 import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.KtFile
 import org.jetbrains.kotlin.psi.KtNameReferenceExpression
 import org.jetbrains.kotlin.psi.KtNamedDeclaration
 import org.jetbrains.kotlin.psi.KtNamedFunction
 import org.jetbrains.kotlin.psi.KtQualifiedExpression
+import org.jetbrains.kotlin.psi.KtReferenceExpression
 import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
 import org.jetbrains.kotlin.resolve.BindingContext
 import org.jetbrains.kotlin.resolve.BindingContext.DECLARATION_TO_DESCRIPTOR
@@ -40,6 +43,11 @@ import org.jetbrains.kotlin.types.KotlinType
 import org.sonar.check.Rule
 import org.sonarsource.kotlin.api.checks.AbstractCheck
 import org.sonarsource.kotlin.api.checks.determineTypeAsString
+import org.sonarsource.kotlin.api.checks.isAbstract
+import org.sonarsource.kotlin.api.checks.isActual
+import org.sonarsource.kotlin.api.checks.isExpect
+import org.sonarsource.kotlin.api.checks.isOpen
+import org.sonarsource.kotlin.api.checks.overrides
 import org.sonarsource.kotlin.api.frontend.KotlinFileContext
 
 @Rule(key = "S6524")
@@ -52,8 +60,24 @@ class CollectionShouldBeImmutableCheck : AbstractCheck() {
             "kotlin.collections.MutableCollection"
         )
 
+    override fun visitKtFile(file: KtFile, context: KotlinFileContext) {
+
+        file
+            .collectDescendantsOfType<KtNamedFunction>(::notAnInterface) { true }
+            .forEach { reportNamedFunction(it, context) }
+    }
+
+    private fun notAnInterface(element: PsiElement): Boolean {
+        return if (element is KtClass) {
+            !element.isInterface()
+        } else {
+            true
+        }
+    }
+
+    private fun reportNamedFunction(function: KtNamedFunction, context: KotlinFileContext) {
+        if (function.isAbstract() || function.overrides() || function.isOpen() || function.isExpect() || function.isActual()) return
 
-    override fun visitNamedFunction(function: KtNamedFunction, context: KotlinFileContext) {
         val bindingContext = context.bindingContext
 
         val referencesToMutableCollections = collectReferenceToMutatedCollections(function, bindingContext)
@@ -155,11 +179,11 @@ class CollectionShouldBeImmutableCheck : AbstractCheck() {
     }
 
     private fun KtQualifiedExpression.mutatesCollection(bindingContext: BindingContext): Boolean {
-        return if (this.selectorExpression is KtCallExpression) {
-            val selectorExpression = this.selectorExpression as KtCallExpression
-            selectorExpression.mutatesCollection(bindingContext)
-        } else {
-            false
+        return when(val selector = this.selectorExpression){
+            null -> true
+            is KtCallExpression -> selector.mutatesCollection(bindingContext)
+            is KtReferenceExpression -> selector.mutatesCollection(bindingContext)
+            else -> false
         }
     }
 

sonar-kotlin-checks/src/test/java/org/sonarsource/kotlin/checks/CollectionShouldBeImmutableCheckTest.kt

@@ -19,4 +19,5 @@
  */
 package org.sonarsource.kotlin.checks
 
-internal class CollectionShouldBeImmutableCheckTest : CheckTestWithNoSemantics(CollectionShouldBeImmutableCheck())
+internal class CollectionShouldBeImmutableCheckTest : CheckTestWithNoSemantics(CollectionShouldBeImmutableCheck()),
+    CheckTestNonCompiling by DefaultCheckTestNonCompiling(CollectionShouldBeImmutableCheck())