ForbiddenAnnotation rule #2719 (#5515)

* adds ForbiddenAnnotation rule #2719 * Unnecessary test removed * Fixed code snippets in ForbiddenAnnotationSpec.kt so they are valid Kotlin code * Report only annotation, parameters shouldn't be included * add more annotations to defaults * more concise message * nested annotations support * alias test * a test for expressions * a test for code blocks * fix build * minor: style in code snippets * minor: compliant code in docs * bindingContext is guaranteed to be not null Co-authored-by: Róbert Papp <papp.robert.s@gmail.com> * minor: redundant suppression Co-authored-by: Róbert Papp <papp.robert.s@gmail.com> * minor: docs * simplify * use text location where possible * explicit import to avoid confusion with java annotation * remove unnecessary check Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
detekt · Nov 29, 2022 · 028f69e · 028f69e
1 parent 5a31681
commit 028f69e
Show file tree

Hide file tree

Showing 4 changed files with 383 additions and 0 deletions.
diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml
@@ -529,6 +529,23 @@ style:
   ExpressionBodySyntax:
     active: false
     includeLineWrapping: false
+  ForbiddenAnnotation:
+    active: false
+    annotations:
+      - reason: 'it is a java annotation. Use `Suppress` instead.'
+        value: 'java.lang.SuppressWarnings'
+      - reason: 'it is a java annotation. Use `kotlin.Deprecated` instead.'
+        value: 'java.lang.Deprecated'
+      - reason: 'it is a java annotation. Use `kotlin.annotation.MustBeDocumented` instead.'
+        value: 'java.lang.annotation.Documented'
+      - reason: 'it is a java annotation. Use `kotlin.annotation.Target` instead.'
+        value: 'java.lang.annotation.Target'
+      - reason: 'it is a java annotation. Use `kotlin.annotation.Retention` instead.'
+        value: 'java.lang.annotation.Retention'
+      - reason: 'it is a java annotation. Use `kotlin.annotation.Repeatable` instead.'
+        value: 'java.lang.annotation.Repeatable'
+      - reason: 'Kotlin does not support @Inherited annotation, see https://youtrack.jetbrains.com/issue/KT-22265'
+        value: 'java.lang.annotation.Inherited'
   ForbiddenComment:
     active: true
     values:

diff --git a/...ules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenAnnotation.kt b/...ules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenAnnotation.kt
@@ -0,0 +1,110 @@
+package io.gitlab.arturbosch.detekt.rules.style
+
+import io.gitlab.arturbosch.detekt.api.CodeSmell
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.Debt
+import io.gitlab.arturbosch.detekt.api.Entity
+import io.gitlab.arturbosch.detekt.api.Issue
+import io.gitlab.arturbosch.detekt.api.Location
+import io.gitlab.arturbosch.detekt.api.Rule
+import io.gitlab.arturbosch.detekt.api.Severity
+import io.gitlab.arturbosch.detekt.api.ValueWithReason
+import io.gitlab.arturbosch.detekt.api.config
+import io.gitlab.arturbosch.detekt.api.internal.Configuration
+import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
+import io.gitlab.arturbosch.detekt.api.valuesWithReason
+import io.gitlab.arturbosch.detekt.rules.fqNameOrNull
+import org.jetbrains.kotlin.name.FqName
+import org.jetbrains.kotlin.psi.KtAnnotationEntry
+import org.jetbrains.kotlin.psi.KtElement
+import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.KtTypeReference
+import org.jetbrains.kotlin.psi.psiUtil.endOffset
+import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.types.KotlinType
+
+/**
+ * This rule allows to set a list of forbidden annotations. This can be used to discourage the use
+ * of language annotations which do not require explicit import.
+ *
+ * <noncompliant>
+ * @@SuppressWarnings("unused")
+ * class SomeClass()
+ * </noncompliant>
+ *
+ * <compliant>
+ * @@Suppress("unused")
+ * class SomeClass()
+ * </compliant>
+ */
+@RequiresTypeResolution
+class ForbiddenAnnotation(config: Config = Config.empty) : Rule(config) {
+
+    override val issue = Issue(
+        javaClass.simpleName,
+        Severity.Style,
+        "Avoid using this annotation.",
+        Debt.FIVE_MINS
+    )
+
+    @Configuration(
+        "List of fully qualified annotation classes which are forbidden."
+    )
+    private val annotations: Map<String, ValueWithReason> by config(
+        valuesWithReason(
+            "java.lang.SuppressWarnings" to "it is a java annotation. Use `Suppress` instead.",
+            "java.lang.Deprecated" to "it is a java annotation. Use `kotlin.Deprecated` instead.",
+            "java.lang.annotation.Documented" to "it is a java annotation. " +
+                "Use `kotlin.annotation.MustBeDocumented` instead.",
+            "java.lang.annotation.Target" to "it is a java annotation. Use `kotlin.annotation.Target` instead.",
+            "java.lang.annotation.Retention" to "it is a java annotation. Use `kotlin.annotation.Retention` instead.",
+            "java.lang.annotation.Repeatable" to "it is a java annotation. Use `kotlin.annotation.Repeatable` instead.",
+            "java.lang.annotation.Inherited" to "Kotlin does not support @Inherited annotation, " +
+                "see https://youtrack.jetbrains.com/issue/KT-22265",
+        )
+    ) { list ->
+        list.associateBy { it.value }
+    }
+
+    override fun visitAnnotationEntry(annotation: KtAnnotationEntry) {
+        super.visitAnnotationEntry(annotation)
+
+        annotation.typeReference?.fqNameOrNull()?.let {
+            check(annotation, it)
+        }
+    }
+
+    override fun visitExpression(expression: KtExpression) {
+        super.visitExpression(expression)
+
+        expression.expressionTypeOrNull()?.fqNameOrNull()?.let {
+            check(expression, it)
+        }
+    }
+
+    private fun check(element: KtElement, fqName: FqName) {
+        val forbidden = annotations[fqName.asString()]
+
+        if (forbidden != null) {
+            val message = if (forbidden.reason != null) {
+                "The annotation `${forbidden.value}` has been forbidden: ${forbidden.reason}"
+            } else {
+                "The annotation `${forbidden.value}` has been forbidden in the detekt config."
+            }
+            val location = Location.from(element).let { location ->
+                location.copy(
+                    text = location.text.copy(
+                        end = element.children.firstOrNull()?.endOffset ?: location.text.end
+                    )
+                )
+            }
+            report(CodeSmell(issue, Entity.from(element, location), message))
+        }
+    }
+
+    private fun KtTypeReference.fqNameOrNull(): FqName? =
+        bindingContext[BindingContext.TYPE, this]?.fqNameOrNull()
+
+    private fun KtExpression.expressionTypeOrNull(): KotlinType? =
+        bindingContext[BindingContext.EXPRESSION_TYPE_INFO, this]?.type
+}
diff --git a/...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt b/...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt
@@ -38,6 +38,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
             NoTabs(config),
             EqualsOnSignatureLine(config),
             EqualsNullCall(config),
+            ForbiddenAnnotation(config),
             ForbiddenComment(config),
             ForbiddenImport(config),
             ForbiddenMethodCall(config),

diff --git a/...-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenAnnotationSpec.kt b/...-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenAnnotationSpec.kt
@@ -0,0 +1,255 @@
+package io.gitlab.arturbosch.detekt.rules.style
+
+import io.gitlab.arturbosch.detekt.api.SourceLocation
+import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
+import io.gitlab.arturbosch.detekt.test.TestConfig
+import io.gitlab.arturbosch.detekt.test.assertThat
+import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
+import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
+import org.junit.jupiter.api.Test
+
+private const val ANNOTATIONS = "annotations"
+
+@KotlinCoreEnvironmentTest
+class ForbiddenAnnotationSpec(val env: KotlinCoreEnvironment) {
+
+    @Test
+    fun `should report SuppressWarnings usages by default`() {
+        val code = """
+        @SuppressWarnings("unused")
+        fun main() {}
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(TestConfig()).compileAndLintWithContext(env, code)
+
+        assertThat(findings)
+            .hasSize(1)
+            .hasStartSourceLocations(
+                SourceLocation(1, 1)
+            )
+            .hasTextLocations("@SuppressWarnings")
+            .extracting("message")
+            .containsExactly(
+                "The annotation `java.lang.SuppressWarnings` has been forbidden: it is a java annotation. Use `Suppress` instead.",
+            )
+    }
+
+    @Test
+    fun `should report annotations from java lang annotation package by default`() {
+        val code = """
+        import java.lang.annotation.Retention
+        import java.lang.annotation.Documented
+        import java.lang.annotation.Target
+        import java.lang.annotation.Repeatable
+        import java.lang.annotation.Inherited
+        import java.lang.annotation.RetentionPolicy
+        import java.lang.annotation.ElementType
+        import java.lang.Deprecated
+        @Deprecated
+        @Documented
+        @Retention(RetentionPolicy.RUNTIME)
+        @Target(ElementType.TYPE)
+        @Repeatable(value = SomeClass::class)
+        @Inherited
+        annotation class SomeClass(val value: Array<SomeClass>)
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(TestConfig()).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(6)
+            .hasTextLocations(
+                "@Deprecated",
+                "@Documented",
+                "@Retention",
+                "@Target",
+                "@Repeatable",
+                "@Inherited"
+            )
+    }
+
+    @Test
+    fun `should report nothing when annotations do not match`() {
+        val code = """
+        @SuppressWarnings("unused")
+        fun main() {}
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("kotlin.jvm.Transient")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).isEmpty()
+    }
+
+    @Test
+    fun `should report annotation call when the fully qualified name is used`() {
+        val code = """
+        @java.lang.SuppressWarnings("unused")
+        fun main() {}
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("java.lang.SuppressWarnings")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@java.lang.SuppressWarnings")
+    }
+
+    @Test
+    fun `should report multiple different annotations`() {
+        val code = """
+        @SuppressWarnings("unused")
+        data class SomeClass(
+            @Transient
+            @Volatile
+            var transient: String? = null
+        )
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(
+                mapOf(
+                    ANNOTATIONS to listOf(
+                        "java.lang.SuppressWarnings",
+                        "kotlin.jvm.Transient",
+                        "kotlin.jvm.Volatile"
+                    )
+                )
+            )
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(3)
+            .hasTextLocations("@SuppressWarnings", "@Transient", "@Volatile")
+    }
+
+    @Test
+    fun `should report annotation on class`() {
+        val code = """
+        @SuppressWarnings("unused")
+        class SomeClass
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("java.lang.SuppressWarnings")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@SuppressWarnings")
+    }
+
+    @Test
+    fun `should report annotation on method`() {
+        val code = """
+        class SomeClass {
+            @SuppressWarnings("unused")
+            fun someMethod(){}
+        }
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("java.lang.SuppressWarnings")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@SuppressWarnings")
+    }
+
+    @Test
+    fun `should report annotation on field`() {
+        val code = """
+        class SomeClass {
+            @SuppressWarnings("unused")
+            val someField: String = "lalala"
+        }
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("java.lang.SuppressWarnings")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@SuppressWarnings")
+    }
+
+    @Test
+    fun `should report annotation on function parameter`() {
+        val code = """
+        fun main(@SuppressWarnings("unused") args: Array<String>){}
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("java.lang.SuppressWarnings")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@SuppressWarnings")
+    }
+
+    @Test
+    fun `should report annotation on constructor`() {
+        val code = """
+        class SomeClass {
+            @SuppressWarnings("unused")
+            constructor(s: String)
+            constructor(t: Int)
+        }
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("java.lang.SuppressWarnings")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@SuppressWarnings")
+    }
+
+    @Test
+    fun `should report annotation on local variable`() {
+        val code = """
+        fun some(): Int {
+            @SuppressWarnings("unused")
+            val q = "1234"
+            return 10
+        }
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("java.lang.SuppressWarnings")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@SuppressWarnings")
+    }
+
+    @Test
+    fun `should report nested annotations`() {
+        val code = """
+        import kotlin.Deprecated
+        @Deprecated(message = "unused", replaceWith = ReplaceWith("bar"))
+        fun foo() = "1234"
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("kotlin.ReplaceWith")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("ReplaceWith")
+    }
+
+    @Test
+    fun `should report aliased annotations`() {
+        val code = """
+        typealias Dep = java.lang.Deprecated
+        @Dep
+        fun f() = Unit
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(TestConfig()).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@Dep")
+    }
+
+    @Test
+    fun `should report annotations for expressions`() {
+        val code = """
+        val x = 0 + @Suppress("UnnecessaryParentheses") (((1)+(2))) + 3
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("kotlin.Suppress")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@Suppress")
+    }
+
+    @Test
+    fun `should report annotations for blocks`() {
+        val code = """
+        fun f(list: List<String>) {
+            list.map @Suppress("x") { it.length }
+        }
+        """.trimIndent()
+        val findings = ForbiddenAnnotation(
+            TestConfig(mapOf(ANNOTATIONS to listOf("kotlin.Suppress")))
+        ).compileAndLintWithContext(env, code)
+        assertThat(findings).hasSize(1)
+            .hasTextLocations("@Suppress")
+    }
+}