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

ktlint incorrectly removes a used import #2821

Closed
TheBeruriahIncident opened this issue Oct 2, 2024 · 2 comments · Fixed by #2822
Closed

ktlint incorrectly removes a used import #2821

TheBeruriahIncident opened this issue Oct 2, 2024 · 2 comments · Fixed by #2822
Milestone

Comments

@TheBeruriahIncident
Copy link

TheBeruriahIncident commented Oct 2, 2024

Summary

ktlintFormat thinks a used import is unused, it removes the import, and my build no longer works.

The weird situation is that it's an import from the same package, which normally is unnecessary. However, it overloads (in a more specific way) another method that is used in the class in question, so it must be imported for the Kotlin compiler to choose the most specific overload.

Expected Behavior

ktlintFormat should not remove used imports and generally should not change behavior of the codebase

Observed Behavior

The import is removed. In my case, tests now fail, because methods are resolved differently.

Steps to Reproduce

The motivation here was trying to make unit testing with kotlinx.coroutines.flow.Flow cleaner.

TestUtils.kt:

package mypackage

import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.runBlocking
import org.junit.jupiter.api.Assertions

fun <T> assertEquals(
    expected: T,
    actual: Flow<T>,
) {
    Assertions.assertEquals(expected, actual.load())
}

private fun <T> Flow<T>.load(): T {
    val flow = this
    return runBlocking { flow.first() }
}

MyTest.kt

package mypackage

import mypackage.assertEquals // This import gets removed
import kotlinx.coroutines.flow.Flow
import org.junit.jupiter.api.Assertions.assertEquals

fun minimalExample(myFlow: Flow<String>) {
  assertEquals("some value", myFlow) // This resolves to my utility method that "unwraps" the flow and calls the normal assertEquals
  assertEquals("other value", "other value") // This resolves to the normal assertEquals
}

After ktlint removes the not-actually-unused import, the first assertEquals then resolves to the normal assertEquals rather than my helper method. This means that the String is directly compared to the Flow<String>, which of course fails.

Your Environment

  • Version of ktlint used: Whichever version the latest Gradle plugin brings in
  • Relevant parts of the .editorconfig settings: ktlint_code_style = ktlint_official
  • Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): 12.1.1 (most recent) of the Gradle plugin wrapper (org.jlleitschuh.gradle.ktlint)
  • Version of Gradle used (if applicable): 8.10.1
  • Operating System and version: Windows 10 Home

This is not blocking me

In my case, I decided that this kind of complication was a sign that I was trying to be too clever, and I renamed my helper method. However, it does appear to be a bug, so I figured I should report it.

@paul-dingemans
Copy link
Collaborator

paul-dingemans commented Oct 3, 2024

In my case, I decided that this kind of complication was a sign that I was trying to be too clever, and I renamed my helper method. However, it does appear to be a bug, so I figured I should report it.

Lol. Relocating your method to another package, would have worked as well. None of imports below will be removed:

import mypackage.sub.assertEquals
import org.junit.jupiter.api.Assertions.assertEquals
import otherpackage.assertEquals

The no-unused-imports rule indeed marks imports from the current package (fully qualified path) as unnecessary. This can be a useful warning, but in this case it is harmfull. Ktlint can not do more advanced checking whether the import from the current package is actually used like in your example. Given your example, I am inclined to remove the unnecessary check entirely as removing the import leads to compilation issues.

@paul-dingemans paul-dingemans added this to the 1.4.0 milestone Oct 3, 2024
paul-dingemans added a commit that referenced this issue Oct 3, 2024
… to the package name

In rare cases this could lead to code which no longer can be compiled

Closes #2821
@TheBeruriahIncident
Copy link
Author

Thanks, Paul!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants