Fix AnnotationExcluder (#4518)

* Implement FullQualifiedNameGuesser

* Remove bad test

* Refactor tests

* Fix AnnotationExcluder

* More exhaustive list of default import classes

* Handle PR comments
This commit is contained in:
Brais Gabín
2022-01-31 10:19:08 +01:00
committed by GitHub
parent 1ca3ab586e
commit 2d00cab368
7 changed files with 431 additions and 85 deletions

View File

@@ -1,5 +1,6 @@
package io.gitlab.arturbosch.detekt.api
import io.github.detekt.psi.internal.FullQualifiedNameGuesser
import org.jetbrains.kotlin.psi.KtAnnotationEntry
import org.jetbrains.kotlin.psi.KtFile
@@ -12,18 +13,11 @@ class AnnotationExcluder(
root: KtFile,
excludes: List<String>,
) {
private val excludes = excludes.map {
it.removePrefix("*").removeSuffix("*")
private val excludes: List<Regex> = excludes.map {
it.replace(".", "\\.").replace("*", ".*").toRegex()
}
private val resolvedAnnotations = root.importList?.run {
imports
.asSequence()
.filterNot { it.isAllUnder }
.mapNotNull { it.importedFqName?.asString() }
.map { it.substringAfterLast('.') to it }
.toMap()
}.orEmpty()
private val fullQualifiedNameGuesser = FullQualifiedNameGuesser(root)
@Deprecated("Use AnnotationExcluder(KtFile, List<String>) instead")
constructor(root: KtFile, excludes: SplitPattern) : this(root, excludes.mapAll { it })
@@ -35,8 +29,30 @@ class AnnotationExcluder(
fun shouldExclude(annotations: List<KtAnnotationEntry>): Boolean = annotations.any(::isExcluded)
private fun isExcluded(annotation: KtAnnotationEntry): Boolean {
val annotationText = annotation.typeReference?.text ?: return false
val value = resolvedAnnotations[annotationText] ?: annotationText
return excludes.any { value.contains(it, ignoreCase = true) }
val annotationText = annotation.typeReference?.text?.ifEmpty { null } ?: return false
/*
We can't know if the annotationText is a full-qualified name or not. We can have these cases:
@Component
@Component.Factory
@dagger.Component.Factory
For that reason we use a heuristic here: If the first character is lower case we assume it's a package name
*/
val possibleNames = if (!annotationText.first().isLowerCase()) {
fullQualifiedNameGuesser.getFullQualifiedName(annotationText)
} else {
listOf(annotationText)
}.flatMap(::expandFqNames)
return excludes.any { exclude -> possibleNames.any { exclude.matches(it) } }
}
}
private fun expandFqNames(fqName: String): List<String> {
return fqName
.split(".")
.dropWhile { it.first().isLowerCase() }
.reversed()
.scan("") { acc, name ->
if (acc.isEmpty()) name else "$name.$acc"
}
.drop(1) + fqName
}

View File

@@ -23,77 +23,71 @@ class AnnotationExcluderSpec {
""".trimIndent()
)
@Nested
inner class `All cases` {
@ParameterizedTest(name = "Given {0} is excluded when the {1} is found then the excluder returns {2}")
@CsvSource(
value = [
"Component,@Component,true",
"Component,@dagger.Component,true",
"Component,@Factory,false",
"Component,@Component.Factory,false",
"Component,@dagger.Component.Factory,false",
@ParameterizedTest
@CsvSource(
value = [
"Component,@Component",
"Component,@dagger.Component",
"Component,@Factory", // false positive
"Component,@Component.Factory", // false positive
"Component,@dagger.Component.Factory", // false positive
"dagger.Component,@Component",
"dagger.Component,@dagger.Component",
"dagger.Component,@Factory", // false positive
"dagger.Component,@dagger.Component.Factory", // false positive
"Component.Factory,@Factory",
"Component.Factory,@Component.Factory",
"Component.Factory,@dagger.Component.Factory",
"dagger.Component.Factory,@Factory",
"dagger.Component.Factory,@dagger.Component.Factory",
"Factory,@Factory",
"Factory,@Component.Factory",
"Factory,@dagger.Component.Factory",
"dagger.*,@Component",
"dagger.*,@dagger.Component",
"dagger.*,@Factory",
"dagger.*,@dagger.Component.Factory",
"*.Component.Factory,@Factory",
"*.Component.Factory,@dagger.Component.Factory",
"*.Component.*,@Factory",
"*.Component.*,@dagger.Component.Factory",
]
)
fun `should exclude`(exclusion: String, annotation: String) {
val excluder = AnnotationExcluder(file, listOf(exclusion))
"dagger.Component,@Component,true",
"dagger.Component,@dagger.Component,true",
"dagger.Component,@Factory,false",
"dagger.Component,@Component.Factory,false",
"dagger.Component,@dagger.Component.Factory,false",
val ktAnnotation = psiFactory.createAnnotationEntry(annotation)
assertThat(excluder.shouldExclude(listOf(ktAnnotation))).isTrue()
}
"Component.Factory,@Component,false",
"Component.Factory,@dagger.Component,false",
"Component.Factory,@Factory,true",
"Component.Factory,@Component.Factory,true",
"Component.Factory,@dagger.Component.Factory,true",
@ParameterizedTest
@CsvSource(
value = [
"dagger.Component,@Component.Factory",
"Component.Factory,@Component",
"Component.Factory,@dagger.Component",
"dagger.Component.Factory,@Component",
"dagger.Component.Factory,@dagger.Component",
"dagger.Component.Factory,@Component.Factory", // false negative
"Factory,@Component",
"Factory,@dagger.Component",
"dagger.*,@Component.Factory", // false positive
"*.Component.Factory,@Component",
"*.Component.Factory,@dagger.Component",
"*.Component.Factory,@Component.Factory", // false positive
"*.Component.*,@Component",
"*.Component.*,@dagger.Component",
"*.Component.*,@Component.Factory", // false positive
"foo.Component,@Component",
"foo.Component,@dagger.Component",
"foo.Component,@Factory",
"foo.Component,@Component.Factory",
"foo.Component,@dagger.Component.Factory",
]
)
fun `should not exclude`(exclusion: String, annotation: String) {
val excluder = AnnotationExcluder(file, listOf(exclusion))
"dagger.Component.Factory,@Component,false",
"dagger.Component.Factory,@dagger.Component,false",
"dagger.Component.Factory,@Factory,true",
"dagger.Component.Factory,@Component.Factory,true",
"dagger.Component.Factory,@dagger.Component.Factory,true",
val ktAnnotation = psiFactory.createAnnotationEntry(annotation)
assertThat(excluder.shouldExclude(listOf(ktAnnotation))).isFalse()
}
"Factory,@Component,false",
"Factory,@dagger.Component,false",
"Factory,@Factory,true",
"Factory,@Component.Factory,true",
"Factory,@dagger.Component.Factory,true",
"dagger.*,@Component,true",
"dagger.*,@dagger.Component,true",
"dagger.*,@Factory,true",
"dagger.*,@Component.Factory,true",
"dagger.*,@dagger.Component.Factory,true",
"*.Component.Factory,@Component,false",
"*.Component.Factory,@dagger.Component,false",
"*.Component.Factory,@Factory,true",
"*.Component.Factory,@Component.Factory,true",
"*.Component.Factory,@dagger.Component.Factory,true",
"*.Component.*,@Component,false",
"*Component*,@Component,true",
"*Component,@Component,true",
"*.Component.*,@dagger.Component,false",
"*.Component.*,@Factory,true",
"*.Component.*,@Component.Factory,true",
"*.Component.*,@dagger.Component.Factory,true",
"foo.Component,@Component,false",
"foo.Component,@dagger.Component,false",
"foo.Component,@Factory,false",
"foo.Component,@Component.Factory,false",
"foo.Component,@dagger.Component.Factory,false",
]
)
fun `all cases`(exclusion: String, annotation: String, shouldExclude: Boolean) {
val excluder = AnnotationExcluder(file, listOf(exclusion))
val ktAnnotation = psiFactory.createAnnotationEntry(annotation)
assertThat(excluder.shouldExclude(listOf(ktAnnotation))).isEqualTo(shouldExclude)
}
@Nested

View File

@@ -7,4 +7,9 @@ dependencies {
implementation(libs.kotlin.compilerEmbeddable)
testImplementation(libs.assertj)
testImplementation(projects.detektTest)
}
apiValidation {
ignoredPackages.add("io.github.detekt.psi.internal")
}

View File

@@ -0,0 +1,55 @@
package io.github.detekt.psi.internal
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.psi.KtImportDirective
import org.jetbrains.kotlin.utils.addIfNotNull
import kotlin.LazyThreadSafetyMode.NONE
class FullQualifiedNameGuesser internal constructor(
private val packageName: String?,
imports: List<KtImportDirective>,
) {
constructor(root: KtFile) : this(
packageName = root.packageDirective?.qualifiedName?.ifBlank { null },
imports = root.importList?.imports.orEmpty(),
)
@Suppress("ClassOrdering")
private val resolvedNames: Map<String, String> by lazy(NONE) {
imports
.asSequence()
.filterNot { it.isAllUnder }
.mapNotNull { import ->
import.importedFqName?.toString()?.let { fqImport ->
(import.alias?.name ?: fqImport.substringAfterLast('.')) to fqImport
}
}
.toMap()
}
fun getFullQualifiedName(name: String): Set<String> {
val resolvedName = findName(name)
return if (resolvedName != null) {
setOf(resolvedName)
} else {
mutableSetOf<String>()
.apply {
addIfNotNull(defaultImportClasses[name])
if (packageName != null) {
add("$packageName.$name")
}
}
}
}
private fun findName(name: String): String? {
val searchName = name.substringBefore('.')
val resolvedName = resolvedNames[searchName]
return if (name == searchName) {
resolvedName
} else {
"$resolvedName.${name.substringAfter('.')}"
}
}
}

View File

@@ -0,0 +1,199 @@
@file:Suppress("MaxLineLength")
package io.github.detekt.psi.internal
/*
The entries of this map was generate with this script:
#!/usr/bin/env bash
print_classes() {
packages=(kotlin kotlin.annotation kotlin.collections kotlin.comparisons kotlin.io kotlin.ranges kotlin.sequences kotlin.text)
for package in "${packages[@]}"; do
curl -s "https://kotlinlang.org/api/latest/jvm/stdlib/$package/" | gsed -n -E "s|<h4><a href=\".*\">([A-Z].*)</a></h4>|\"\1\" to \"$package.\1\",|p"
done
}
print_classes | sort -u | gsed '/^"[A-Z_]*"/d'
And the list of packages that should be considered was extracted from the kotlin documentation:
https://kotlinlang.org/docs/packages.html#default-imports
*/
internal val defaultImportClasses = mapOf(
"AbstractCollection" to "kotlin.collections.AbstractCollection",
"AbstractIterator" to "kotlin.collections.AbstractIterator",
"AbstractList" to "kotlin.collections.AbstractList",
"AbstractMap" to "kotlin.collections.AbstractMap",
"AbstractMutableCollection" to "kotlin.collections.AbstractMutableCollection",
"AbstractMutableList" to "kotlin.collections.AbstractMutableList",
"AbstractMutableMap" to "kotlin.collections.AbstractMutableMap",
"AbstractMutableSet" to "kotlin.collections.AbstractMutableSet",
"AbstractSet" to "kotlin.collections.AbstractSet",
"AccessDeniedException" to "kotlin.io.AccessDeniedException",
"Annotation" to "kotlin.Annotation",
"AnnotationRetention" to "kotlin.annotation.AnnotationRetention",
"AnnotationTarget" to "kotlin.annotation.AnnotationTarget",
"Any" to "kotlin.Any",
"Appendable" to "kotlin.text.Appendable",
"ArithmeticException" to "kotlin.ArithmeticException",
"Array" to "kotlin.Array",
"ArrayDeque" to "kotlin.collections.ArrayDeque",
"ArrayIndexOutOfBoundsException" to "kotlin.ArrayIndexOutOfBoundsException",
"ArrayList" to "kotlin.collections.ArrayList",
"AssertionError" to "kotlin.AssertionError",
"Boolean" to "kotlin.Boolean",
"BooleanArray" to "kotlin.BooleanArray",
"BooleanIterator" to "kotlin.collections.BooleanIterator",
"BuilderInference" to "kotlin.BuilderInference",
"Byte" to "kotlin.Byte",
"ByteArray" to "kotlin.ByteArray",
"ByteIterator" to "kotlin.collections.ByteIterator",
"ClassCastException" to "kotlin.ClassCastException",
"ClosedFloatingPointRange" to "kotlin.ranges.ClosedFloatingPointRange",
"ClosedRange" to "kotlin.ranges.ClosedRange",
"Collection" to "kotlin.collections.Collection",
"Comparable" to "kotlin.Comparable",
"Comparator" to "kotlin.Comparator",
"ConcurrentModificationException" to "kotlin.ConcurrentModificationException",
"Char" to "kotlin.Char",
"CharArray" to "kotlin.CharArray",
"CharCategory" to "kotlin.text.CharCategory",
"CharDirectionality" to "kotlin.text.CharDirectionality",
"CharIterator" to "kotlin.collections.CharIterator",
"CharProgression" to "kotlin.ranges.CharProgression",
"CharRange" to "kotlin.ranges.CharRange",
"CharSequence" to "kotlin.CharSequence",
"CharacterCodingException" to "kotlin.text.CharacterCodingException",
"Charsets" to "kotlin.text.Charsets",
"DeepRecursiveFunction" to "kotlin.DeepRecursiveFunction",
"DeepRecursiveScope" to "kotlin.DeepRecursiveScope",
"Deprecated" to "kotlin.Deprecated",
"DeprecatedSinceKotlin" to "kotlin.DeprecatedSinceKotlin",
"DeprecationLevel" to "kotlin.DeprecationLevel",
"Double" to "kotlin.Double",
"DoubleArray" to "kotlin.DoubleArray",
"DoubleIterator" to "kotlin.collections.DoubleIterator",
"DslMarker" to "kotlin.DslMarker",
"Enum" to "kotlin.Enum",
"Error" to "kotlin.Error",
"Exception" to "kotlin.Exception",
"Experimental" to "kotlin.Experimental",
"ExperimentalMultiplatform" to "kotlin.ExperimentalMultiplatform",
"ExperimentalStdlibApi" to "kotlin.ExperimentalStdlibApi",
"ExperimentalUnsignedTypes" to "kotlin.ExperimentalUnsignedTypes",
"ExtensionFunctionType" to "kotlin.ExtensionFunctionType",
"FileAlreadyExistsException" to "kotlin.io.FileAlreadyExistsException",
"FileSystemException" to "kotlin.io.FileSystemException",
"FileTreeWalk" to "kotlin.io.FileTreeWalk",
"FileWalkDirection" to "kotlin.io.FileWalkDirection",
"Float" to "kotlin.Float",
"FloatArray" to "kotlin.FloatArray",
"FloatIterator" to "kotlin.collections.FloatIterator",
"Function" to "kotlin.Function",
"Grouping" to "kotlin.collections.Grouping",
"HashMap" to "kotlin.collections.HashMap",
"HashSet" to "kotlin.collections.HashSet",
"IllegalArgumentException" to "kotlin.IllegalArgumentException",
"IllegalStateException" to "kotlin.IllegalStateException",
"IndexOutOfBoundsException" to "kotlin.IndexOutOfBoundsException",
"IndexedValue" to "kotlin.collections.IndexedValue",
"Int" to "kotlin.Int",
"IntArray" to "kotlin.IntArray",
"IntIterator" to "kotlin.collections.IntIterator",
"IntProgression" to "kotlin.ranges.IntProgression",
"IntRange" to "kotlin.ranges.IntRange",
"Iterable" to "kotlin.collections.Iterable",
"Iterator" to "kotlin.collections.Iterator",
"KotlinNullPointerException" to "kotlin.KotlinNullPointerException",
"KotlinVersion" to "kotlin.KotlinVersion",
"Lazy" to "kotlin.Lazy",
"LazyThreadSafetyMode" to "kotlin.LazyThreadSafetyMode",
"LinkedHashMap" to "kotlin.collections.LinkedHashMap",
"LinkedHashSet" to "kotlin.collections.LinkedHashSet",
"List" to "kotlin.collections.List",
"ListIterator" to "kotlin.collections.ListIterator",
"Long" to "kotlin.Long",
"LongArray" to "kotlin.LongArray",
"LongIterator" to "kotlin.collections.LongIterator",
"LongProgression" to "kotlin.ranges.LongProgression",
"LongRange" to "kotlin.ranges.LongRange",
"Map" to "kotlin.collections.Map",
"MatchGroup" to "kotlin.text.MatchGroup",
"MatchGroupCollection" to "kotlin.text.MatchGroupCollection",
"MatchNamedGroupCollection" to "kotlin.text.MatchNamedGroupCollection",
"MatchResult" to "kotlin.text.MatchResult",
"Metadata" to "kotlin.Metadata",
"MustBeDocumented" to "kotlin.annotation.MustBeDocumented",
"MutableCollection" to "kotlin.collections.MutableCollection",
"MutableIterable" to "kotlin.collections.MutableIterable",
"MutableIterator" to "kotlin.collections.MutableIterator",
"MutableList" to "kotlin.collections.MutableList",
"MutableListIterator" to "kotlin.collections.MutableListIterator",
"MutableMap" to "kotlin.collections.MutableMap",
"MutableSet" to "kotlin.collections.MutableSet",
"NoSuchElementException" to "kotlin.NoSuchElementException",
"NoSuchFileException" to "kotlin.io.NoSuchFileException",
"NoWhenBranchMatchedException" to "kotlin.NoWhenBranchMatchedException",
"NotImplementedError" to "kotlin.NotImplementedError",
"Nothing" to "kotlin.Nothing",
"NullPointerException" to "kotlin.NullPointerException",
"Number" to "kotlin.Number",
"NumberFormatException" to "kotlin.NumberFormatException",
"OnErrorAction" to "kotlin.io.OnErrorAction",
"OptIn" to "kotlin.OptIn",
"OptionalExpectation" to "kotlin.OptionalExpectation",
"OutOfMemoryError" to "kotlin.OutOfMemoryError",
"OverloadResolutionByLambdaReturnType" to "kotlin.OverloadResolutionByLambdaReturnType",
"Pair" to "kotlin.Pair",
"ParameterName" to "kotlin.ParameterName",
"PublishedApi" to "kotlin.PublishedApi",
"RandomAccess" to "kotlin.collections.RandomAccess",
"Regex" to "kotlin.text.Regex",
"RegexOption" to "kotlin.text.RegexOption",
"Repeatable" to "kotlin.annotation.Repeatable",
"ReplaceWith" to "kotlin.ReplaceWith",
"RequiresOptIn" to "kotlin.RequiresOptIn",
"Result" to "kotlin.Result",
"Retention" to "kotlin.annotation.Retention",
"RuntimeException" to "kotlin.RuntimeException",
"Sequence" to "kotlin.sequences.Sequence",
"SequenceBuilder" to "kotlin.sequences.SequenceBuilder",
"SequenceScope" to "kotlin.sequences.SequenceScope",
"Set" to "kotlin.collections.Set",
"Short" to "kotlin.Short",
"ShortArray" to "kotlin.ShortArray",
"ShortIterator" to "kotlin.collections.ShortIterator",
"SinceKotlin" to "kotlin.SinceKotlin",
"String" to "kotlin.String",
"String" to "kotlin.text.String",
"StringBuilder" to "kotlin.text.StringBuilder",
"Suppress" to "kotlin.Suppress",
"Target" to "kotlin.annotation.Target",
"Throwable" to "kotlin.Throwable",
"Throws" to "kotlin.Throws",
"Triple" to "kotlin.Triple",
"TypeCastException" to "kotlin.TypeCastException",
"Typography" to "kotlin.text.Typography",
"UByte" to "kotlin.UByte",
"UByteArray" to "kotlin.UByteArray",
"UByteIterator" to "kotlin.collections.UByteIterator",
"UInt" to "kotlin.UInt",
"UIntArray" to "kotlin.UIntArray",
"UIntIterator" to "kotlin.collections.UIntIterator",
"UIntProgression" to "kotlin.ranges.UIntProgression",
"UIntRange" to "kotlin.ranges.UIntRange",
"ULong" to "kotlin.ULong",
"ULongArray" to "kotlin.ULongArray",
"ULongIterator" to "kotlin.collections.ULongIterator",
"ULongProgression" to "kotlin.ranges.ULongProgression",
"ULongRange" to "kotlin.ranges.ULongRange",
"UShort" to "kotlin.UShort",
"UShortArray" to "kotlin.UShortArray",
"UShortIterator" to "kotlin.collections.UShortIterator",
"UninitializedPropertyAccessException" to "kotlin.UninitializedPropertyAccessException",
"Unit" to "kotlin.Unit",
"UnsafeVariance" to "kotlin.UnsafeVariance",
"UnsupportedOperationException" to "kotlin.UnsupportedOperationException",
"UseExperimental" to "kotlin.UseExperimental",
)

View File

@@ -0,0 +1,82 @@
package io.github.detekt.psi.internal
import io.github.detekt.test.utils.compileContentForTest
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Nested
import org.junit.jupiter.api.Test
class FullQualifiedNameGuesserSpec {
@Nested
class `With package` {
private val sut = FullQualifiedNameGuesser(
compileContentForTest(
"""
package foo
import kotlin.jvm.JvmField
import kotlin.jvm.JvmStatic as Static
""".trimIndent()
)
)
@Test
fun `import`() {
assertThat(sut.getFullQualifiedName("JvmField"))
.containsExactlyInAnyOrder("kotlin.jvm.JvmField")
}
@Test
fun `import with alias`() {
assertThat(sut.getFullQualifiedName("Static"))
.containsExactlyInAnyOrder("kotlin.jvm.JvmStatic")
}
@Test
fun `import with alias but using real name`() {
assertThat(sut.getFullQualifiedName("JvmStatic"))
.containsExactlyInAnyOrder("foo.JvmStatic")
}
@Test
fun `no import but maybe kotlin`() {
assertThat(sut.getFullQualifiedName("Result"))
.containsExactlyInAnyOrder("foo.Result", "kotlin.Result")
}
@Test
fun `no import but not kotlin`() {
assertThat(sut.getFullQualifiedName("Asdf"))
.containsExactlyInAnyOrder("foo.Asdf")
}
@Test
fun `import with subclass`() {
assertThat(sut.getFullQualifiedName("JvmField.Factory"))
.containsExactlyInAnyOrder("kotlin.jvm.JvmField.Factory")
}
@Test
fun `alias-import with subclass`() {
assertThat(sut.getFullQualifiedName("Static.Factory"))
.containsExactlyInAnyOrder("kotlin.jvm.JvmStatic.Factory")
}
}
@Nested
class `Without package` {
private val sut = FullQualifiedNameGuesser(compileContentForTest("import kotlin.jvm.JvmField"))
@Test
fun `no import but maybe kotlin`() {
assertThat(sut.getFullQualifiedName("Result"))
.containsExactlyInAnyOrder("kotlin.Result")
}
@Test
fun `no import and not kotlin`() {
assertThat(sut.getFullQualifiedName("Asdf"))
.isEmpty()
}
}
}

View File

@@ -35,11 +35,6 @@ class LateinitUsageSpec : Spek({
assertThat(findings).hasSize(1)
}
it("should only report lateinit properties matching kotlin.") {
val findings = LateinitUsage(TestConfig(mapOf(EXCLUDE_ANNOTATED_PROPERTIES to listOf("kotlin.")))).compileAndLint(code)
assertThat(findings).hasSize(1)
}
it("should only report lateinit properties matching kotlin.SinceKotlin") {
val config = TestConfig(mapOf(EXCLUDE_ANNOTATED_PROPERTIES to listOf("kotlin.SinceKotlin")))
val findings = LateinitUsage(config).compileAndLint(code)