From 95556b3d3fe24330e5c89faa1d0d3f5680b2546d Mon Sep 17 00:00:00 2001 From: erwan-serandour-sonarsource Date: Wed, 29 Nov 2023 09:19:37 +0100 Subject: [PATCH] SONARKT-301 stop raising issues on abstract/overrided/open/interface methods (#395) --- ...houldBeImmutableCheckSampleNonCompiling.kt | 12 +++++++ .../CollectionShouldBeImmutableCheckSample.kt | 26 +++++++++++++- ...ShouldBeImmutableCheckSampleNoSemantics.kt | 2 ++ .../kotlin/api/checks/ApiExtensions.kt | 12 +++++++ .../kotlin/api/checks/ApiExtensionsKtTest.kt | 34 ++++++++++++++++++ .../CollectionShouldBeImmutableCheck.kt | 36 +++++++++++++++---- .../CollectionShouldBeImmutableCheckTest.kt | 3 +- 7 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 kotlin-checks-test-sources/src/main/files/non-compiling/checks/CollectionShouldBeImmutableCheckSampleNonCompiling.kt diff --git a/kotlin-checks-test-sources/src/main/files/non-compiling/checks/CollectionShouldBeImmutableCheckSampleNonCompiling.kt b/kotlin-checks-test-sources/src/main/files/non-compiling/checks/CollectionShouldBeImmutableCheckSampleNonCompiling.kt new file mode 100644 index 000000000..9d17de2fd --- /dev/null +++ b/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): Unit // compliant + expect fun expectFun(list: MutableList): Unit // compliant + + fun qualifiedStrange() { + val list = mutableListOf() + list.(add(1)) + } +} \ No newline at end of file diff --git a/kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSample.kt b/kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSample.kt index c6a460ca3..4102258d1 100644 --- a/kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSample.kt +++ b/kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSample.kt @@ -78,6 +78,7 @@ class CollectionShouldBeImmutableCheckSample { l: MutableList, // Compliant m: MutableList, // Compliant n: MutableList, // Compliant + o: MutableMap, // 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) -> Unit): Unit { // compliant } + interface A { + fun foo(list : MutableList): Unit // compliant + fun bar(list : MutableList): Int { // compliant + return list.reduce { acc, it -> acc + it} + } + } + + abstract class B : A{ + override fun foo(list: MutableList) { // compliant + } + + abstract fun baz(list: MutableList): Unit // compliant + + open fun qux(list: MutableList): Unit { // compliant + } + + } + +} + +private fun nonCompliantParameterOnFileLevel(list: MutableList): Int { // Noncompliant + return list.reduce { acc, it -> acc + it} } \ No newline at end of file diff --git a/kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSampleNoSemantics.kt b/kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSampleNoSemantics.kt index a7202c085..5c68cada8 100644 --- a/kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSampleNoSemantics.kt +++ b/kotlin-checks-test-sources/src/main/kotlin/checks/CollectionShouldBeImmutableCheckSampleNoSemantics.kt @@ -77,6 +77,7 @@ class CollectionShouldBeImmutableCheckSampleNoSemantics { l: MutableList, // Compliant m: MutableList, // Compliant n: MutableList, // Compliant + o: MutableMap, // 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 } diff --git a/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/ApiExtensions.kt b/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/ApiExtensions.kt index c1e15f002..fcd563cbc 100644 --- a/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/ApiExtensions.kt +++ b/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) = diff --git a/sonar-kotlin-api/src/test/java/org/sonarsource/kotlin/api/checks/ApiExtensionsKtTest.kt b/sonar-kotlin-api/src/test/java/org/sonarsource/kotlin/api/checks/ApiExtensionsKtTest.kt index 02f494021..c6a6e8aa5 100644 --- a/sonar-kotlin-api/src/test/java/org/sonarsource/kotlin/api/checks/ApiExtensionsKtTest.kt +++ b/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 = 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() diff --git a/sonar-kotlin-checks/src/main/java/org/sonarsource/kotlin/checks/CollectionShouldBeImmutableCheck.kt b/sonar-kotlin-checks/src/main/java/org/sonarsource/kotlin/checks/CollectionShouldBeImmutableCheck.kt index e81184972..d723539d8 100644 --- a/sonar-kotlin-checks/src/main/java/org/sonarsource/kotlin/checks/CollectionShouldBeImmutableCheck.kt +++ b/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(::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 } } diff --git a/sonar-kotlin-checks/src/test/java/org/sonarsource/kotlin/checks/CollectionShouldBeImmutableCheckTest.kt b/sonar-kotlin-checks/src/test/java/org/sonarsource/kotlin/checks/CollectionShouldBeImmutableCheckTest.kt index 6bb503eac..9a0e0a2c6 100644 --- a/sonar-kotlin-checks/src/test/java/org/sonarsource/kotlin/checks/CollectionShouldBeImmutableCheckTest.kt +++ b/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()) \ No newline at end of file +internal class CollectionShouldBeImmutableCheckTest : CheckTestWithNoSemantics(CollectionShouldBeImmutableCheck()), + CheckTestNonCompiling by DefaultCheckTestNonCompiling(CollectionShouldBeImmutableCheck()) \ No newline at end of file