Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AnnotationExcluder #4518

Merged
merged 6 commits into from Jan 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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

Expand All @@ -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 })
Expand All @@ -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
}
Expand Up @@ -23,77 +23,71 @@ class AnnotationExcluderSpec {
""".trimIndent()
)

@Nested
inner class `All cases` {

@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))

val ktAnnotation = psiFactory.createAnnotationEntry(annotation)
assertThat(excluder.shouldExclude(listOf(ktAnnotation))).isTrue()
}
@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 = [
"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))

val ktAnnotation = psiFactory.createAnnotationEntry(annotation)
assertThat(excluder.shouldExclude(listOf(ktAnnotation))).isFalse()
}
"dagger.Component,@Component,true",
"dagger.Component,@dagger.Component,true",
"dagger.Component,@Factory,false",
"dagger.Component,@Component.Factory,false",
"dagger.Component,@dagger.Component.Factory,false",

"Component.Factory,@Component,false",
"Component.Factory,@dagger.Component,false",
"Component.Factory,@Factory,true",
"Component.Factory,@Component.Factory,true",
"Component.Factory,@dagger.Component.Factory,true",

"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",

"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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non blocking comment: I hate parameterized tests like this. That is why I split it into 2 tests. But I know there are more schools of thought here ;) But that is something for our style guide. How many parameters may a test have before it is considered unreadable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this kind of tests either. I would prefer to have way less. But they were needed. There were really random cases that were not covered. And while I was implementing this I broke some of them while fixing others.

And I think that it's better to have all the cases together because they are in blocks of 5. Maybe I could add a new line between each "block of tests" to make it easier to read. But I can reverse the change and split them in two if you think it's better.


val ktAnnotation = psiFactory.createAnnotationEntry(annotation)
assertThat(excluder.shouldExclude(listOf(ktAnnotation))).isEqualTo(shouldExclude)
}

@Nested
Expand Down
5 changes: 5 additions & 0 deletions detekt-psi-utils/build.gradle.kts
Expand Up @@ -7,4 +7,9 @@ dependencies {
implementation(libs.kotlin.compilerEmbeddable)

testImplementation(libs.assertj)
testImplementation(projects.detektTest)
}

apiValidation {
ignoredPackages.add("io.github.detekt.psi.internal")
}
@@ -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('.')}"
}
}
}