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.

Sign up for GitHub

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

Jump to bottom

Fix AnnotationExcluder #4518

Merged
merged 6 commits into from Jan 31, 2022
Merged

Fix AnnotationExcluder #4518

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f72028b
Implement FullQualifiedNameGuesser
BraisGabin Dec 9, 2021
95e44e2
Remove bad test
BraisGabin Jan 24, 2022
3d9a804
Refactor tests
BraisGabin Jan 28, 2022
1e36938
Fix AnnotationExcluder
BraisGabin Jan 24, 2022
7d35ecf
More exhaustive list of default import classes
BraisGabin Jan 30, 2022
fab3a88
Handle PR comments
BraisGabin Jan 31, 2022
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
40 changes: 27 additions & 13 deletions detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt
View file
Open in desktop
@@ -1,5 +1,6 @@
package io.gitlab.arturbosch.detekt.api

import io.github.detekt.psi.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,28 @@ 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 { fqName ->
fqName
.split(".")
.dropWhile { it.first().isLowerCase() }
.reversed()
.scan("") { acc, name ->
if (acc.isEmpty()) name else "$name.$acc"
}
.drop(1) + fqName
BraisGabin marked this conversation as resolved.
Show resolved Hide resolved
}
return excludes.any { exclude -> possibleNames.any { exclude.matches(it) } }
}
}
124 changes: 54 additions & 70 deletions detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluderSpec.kt
View file
Open in desktop
Expand Up @@ -23,77 +23,61 @@ 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
@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))
@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",
"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.*,@dagger.Component,false",
"*.Component.*,@Factory,true",
"*.Component.*,@Component.Factory,true",
"*.Component.*,@dagger.Component.Factory,true",
BraisGabin marked this conversation as resolved.
Show resolved Hide resolved
"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))).isFalse()
}
val ktAnnotation = psiFactory.createAnnotationEntry(annotation)
assertThat(excluder.shouldExclude(listOf(ktAnnotation))).isEqualTo(shouldExclude)
}

@Nested
Expand Down
5 changes: 5 additions & 0 deletions detekt-psi-utils/api/detekt-psi-utils.api
View file
Open in desktop
Expand Up @@ -20,6 +20,11 @@ public final class io/github/detekt/psi/FilePath$Companion {
public final fun fromRelative (Ljava/nio/file/Path;Ljava/nio/file/Path;)Lio/github/detekt/psi/FilePath;
}

public final class io/github/detekt/psi/FullQualifiedNameGuesser {
public fun <init> (Lorg/jetbrains/kotlin/psi/KtFile;)V
public final fun getFullQualifiedName (Ljava/lang/String;)Ljava/util/Set;
}

BraisGabin marked this conversation as resolved.
Show resolved Hide resolved
public final class io/github/detekt/psi/KeysKt {
public static final fun getBASE_PATH ()Lorg/jetbrains/kotlin/com/intellij/openapi/util/Key;
public static final fun getLINE_SEPARATOR ()Lorg/jetbrains/kotlin/com/intellij/openapi/util/Key;
Expand Down
1 change: 1 addition & 0 deletions detekt-psi-utils/build.gradle.kts
View file
Open in desktop
Expand Up @@ -7,4 +7,5 @@ dependencies {
implementation(libs.kotlin.compilerEmbeddable)

testImplementation(libs.assertj)
testImplementation(projects.detektTest)
}
55 changes: 55 additions & 0 deletions detekt-psi-utils/src/main/kotlin/io/github/detekt/psi/FullQualifiedNameGuesser.kt
View file
Open in desktop
@@ -0,0 +1,55 @@
package io.github.detekt.psi

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")
cortinico marked this conversation as resolved.
Show resolved Hide resolved
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('.')}"
}
}
}