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

Keep imports from the same package if the name is overloaded #414

Closed
wants to merge 2 commits into from
Closed

Keep imports from the same package if the name is overloaded #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76052ae
Keep imports from the same package if the name is overloaded
nreid260 Aug 17, 2023
0127e4f
Merge branch 'main' into imports
hick209 Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions core/src/main/java/com/facebook/ktfmt/format/RedundantImportDetector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,10 @@ internal class RedundantImportDetector(val enabled: Boolean) {
importCleanUpCandidates =
importList.imports
.filter { import ->
val identifier = import.identifier ?: return@filter false
import.isValidImport &&
!import.isAllUnder &&
hick209 marked this conversation as resolved.
Show resolved Hide resolved
import.identifier != null &&
requireNotNull(import.identifier) !in OPERATORS &&
!COMPONENT_OPERATOR_REGEX.matches(import.identifier.orEmpty())
identifier !in OPERATORS &&
!COMPONENT_OPERATOR_REGEX.matches(identifier)
}
.toSet()

Expand Down Expand Up @@ -160,20 +159,23 @@ internal class RedundantImportDetector(val enabled: Boolean) {
fun getRedundantImportElements(): List<PsiElement> {
if (!enabled) return emptyList()

val redundantImports = mutableListOf<PsiElement>()

// Collect unused imports
for (import in importCleanUpCandidates) {
val isUnused = import.aliasName !in usedReferences && import.identifier !in usedReferences
val isFromSamePackage = import.importedFqName?.parent() == thisPackage && import.alias == null
if (isUnused || isFromSamePackage) {
redundantImports += import
val identifierCounts =
importCleanUpCandidates
.groupBy { it.identifier }
.mapValues { it.value.size }

return importCleanUpCandidates
.filter{
val isUsed = it.identifier in usedReferences
val isFromThisPackage = it.importedFqName?.parent() == thisPackage
val hasAlias = it.alias != null
val isOverload = requireNotNull(identifierCounts[it.identifier]) > 1
// Remove if...
!isUsed || (isFromThisPackage && !hasAlias && !isOverload)
Copy link
Contributor

Choose a reason for hiding this comment

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

We might actually want to pivot this to just remove if it's unused, which keeping things from the same package always.

We have internal reasons to want this change, but this is something we likely will do in the future and if you state that this is not something you'd want we could put it under a config. Please let us know.

Things are fine for this PR as they are though, just to be clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do generally prefer the behaviour of removing imports for the local package, since it keeps things consistent. Otherwise, some local package elements will be imported, and others won't be, depending on the whims of various authors.

The case I'm trying to fix has to do with overload resolution for Truth. The issue seems to be that importing Truth.assertThat causes kotlinc to always use an overload from the Truth class, even when there's a local package function named assertThat, which has narrower typing. Part of the problem here is there's a definition Truth.assertThat(x: Object) which matches ~everything, which is a bad but entrenched design.

}
}

return redundantImports
}

/** The imported short name, possibly an alias name, if any. */
private inline val KtImportDirective.identifier: String?
get() = importPath?.importedName?.identifier?.trim('`')
}
92 changes: 91 additions & 1 deletion core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,7 +1179,7 @@ class FormatterTest {
}

@Test
fun `imports from the same package are removed`() {
fun `used imports from this package are removed`() {
val code =
"""
|package com.example
Expand Down Expand Up @@ -1208,6 +1208,96 @@ class FormatterTest {
assertThatFormatting(code).isEqualTo(expected)
}

@Test
fun `potentially unused imports from this package are kept if they are overloaded`() {
val code =
"""
|package com.example
|
|import com.example.a
|import com.example.b
|import com.example.c
|import com.notexample.a
|import com.notexample.b
|import com.notexample.notC as c
|
|fun test() {
| a("hello")
| c("hello")
|}
|"""
.trimMargin()
val expected =
"""
|package com.example
|
|import com.example.a
|import com.example.c
|import com.notexample.a
|import com.notexample.notC as c
|
|fun test() {
| a("hello")
| c("hello")
|}
|"""
.trimMargin()
assertThatFormatting(code).isEqualTo(expected)
}

@Test
fun `used imports from this package are kept if they are aliased`() {
val code =
"""
|package com.example
|
|import com.example.b as a
|import com.example.c
|
|fun test() {
| a("hello")
|}
|"""
.trimMargin()
val expected =
"""
|package com.example
|
|import com.example.b as a
|
|fun test() {
| a("hello")
|}
|"""
.trimMargin()
assertThatFormatting(code).isEqualTo(expected)
}

@Test
fun `unused imports are computed using only the alias name if present`() {
val code =
"""
|package com.example
|
|import com.notexample.a as b
|
|fun test() {
| a("hello")
|}
|"""
.trimMargin()
val expected =
"""
|package com.example
|
|fun test() {
| a("hello")
|}
|"""
.trimMargin()
assertThatFormatting(code).isEqualTo(expected)
}

@Test
fun `keep import elements only mentioned in kdoc`() {
val code =
Expand Down