From 1ab8b53faf338b2d238f4f1a0f91048fea60cb55 Mon Sep 17 00:00:00 2001 From: Dominic Zirbel Date: Tue, 31 May 2022 16:56:58 -0700 Subject: [PATCH] UnusedImports: fix false positive for unresolved imports Fix a false positive where UnusedImports would report extension function imports whose signature cannot be resolved by the BindingContext. This can happen for autogenerated classes, etc. (especially Android UI binding classes and the like). To do this we need to generate text matchers for all the references which cannot be resolved and check if there are any unresolved matches to them, similar to the approach without type resolution. Along the way, extract mappings inside `KtImportDirective.isNotUsed()` to lazy properties to avoid recalculating them for each import directive. Also make sure to map to a set for constant rather than linear lookup time. --- .../detekt/rules/style/UnusedImports.kt | 40 +++++++++++++++---- .../detekt/rules/style/UnusedImportsSpec.kt | 32 +++++++++++++++ 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedImports.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedImports.kt index e5b1853974e..6da0c950529 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedImports.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedImports.kt @@ -54,26 +54,44 @@ class UnusedImports(config: Config) : Rule(config) { private val staticReferences = mutableSetOf() private val namedReferencesInKDoc = mutableSetOf() + private val namedReferencesAsString: Set by lazy { + namedReferences.mapTo(mutableSetOf()) { it.text.trim('`') } + } + private val staticReferencesAsString: Set by lazy { + staticReferences.mapTo(mutableSetOf()) { it.text.trim('`') } + } + private val fqNames: Set by lazy { + namedReferences.mapNotNullTo(mutableSetOf()) { it.fqNameOrNull() } + } + + /** + * All [namedReferences] whose [KtReferenceExpression.fqNameOrNull] cannot be resolved + * mapped to their text. String matches to such references shouldn't be marked as unused + * imports since they could match the unknown value being imported. + */ + @Suppress("CommentOverPrivateProperty") + private val unresolvedNamedReferencesAsString: Set by lazy { + namedReferences.mapNotNullTo(mutableSetOf()) { + if (it.fqNameOrNull() == null) it.text.trim('`') else null + } + } + fun unusedImports(): List { fun KtImportDirective.isFromSamePackage() = importedFqName?.parent() == currentPackage && alias == null @Suppress("ReturnCount") fun KtImportDirective.isNotUsed(): Boolean { - val namedReferencesAsString = namedReferences.map { it.text.trim('`') } - val staticReferencesAsString = staticReferences.map { it.text.trim('`') } if (aliasName in (namedReferencesInKDoc + namedReferencesAsString)) return false val identifier = identifier() if (identifier in namedReferencesInKDoc || identifier in staticReferencesAsString) return false return if (bindingContext == BindingContext.EMPTY) { identifier !in namedReferencesAsString } else { - val fqNames = namedReferences.mapNotNull { - val descriptor = bindingContext[BindingContext.SHORT_REFERENCE_TO_COMPANION_OBJECT, it] - ?: bindingContext[BindingContext.REFERENCE_TARGET, it] - descriptor?.getImportableDescriptor()?.fqNameOrNull() - } - importPath?.fqName?.let { it !in fqNames } == true + val fqNameUsed = importPath?.fqName?.let { it in fqNames } == true + val unresolvedNameUsed = identifier in unresolvedNamedReferencesAsString + + !fqNameUsed && !unresolvedNameUsed } } @@ -135,6 +153,12 @@ class UnusedImports(config: Config) : Rule(config) { namedReferencesInKDoc.add(str.split(".")[0]) } } + + private fun KtReferenceExpression.fqNameOrNull(): FqName? { + val descriptor = bindingContext[BindingContext.SHORT_REFERENCE_TO_COMPANION_OBJECT, this] + ?: bindingContext[BindingContext.REFERENCE_TARGET, this] + return descriptor?.getImportableDescriptor()?.fqNameOrNull() + } } companion object { diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedImportsSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedImportsSpec.kt index d9425c640d5..ca6fbf5ca67 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedImportsSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedImportsSpec.kt @@ -634,4 +634,36 @@ class UnusedImportsSpec(val env: KotlinCoreEnvironment) { assertThat(subject.lintWithContext(env, mainFile, additionalFile)).isEmpty() } + + @Test + fun `does not report imports which detekt cannot resolve but have string matches`() { + val mainFile = """ + import x.y.z.foo + import x.y.z.Bar + + fun test() { + foo() + foo("", 123) + foo + + Bar().baz() + } + """ + + assertThat(subject.lintWithContext(env, mainFile)).isEmpty() + } + + @Test + fun `reports imports which detekt cannot resolve and do not have string matches`() { + val mainFile = """ + import x.y.z.foo + import x.y.z.Bar + + fun test() { + 2 + 3 + } + """ + + assertThat(subject.lintWithContext(env, mainFile)).hasSize(2) + } }