Keep imports from the same package if the name is overloaded #414
Conversation
facebook-github-bot
added
the
CLA Signed
|
@hick209 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
core/src/main/java/com/facebook/ktfmt/format/RedundantImportDetector.kt
Outdated
Show resolved
Hide resolved
| val hasAlias = it.alias != null | ||
| val isOverload = requireNotNull(identifierCounts[it.identifier]) > 1 | ||
| // Remove if... | ||
| !isUsed || (isFromThisPackage && !hasAlias && !isOverload) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
core/src/main/java/com/facebook/ktfmt/format/RedundantImportDetector.kt
Outdated
Show resolved
Hide resolved
|
@hick209 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
* upstream/main: Add unit tests to capture line break behavior on type specifiers Plugin doesn't work with if "Only VCS changed text" is selected from code-reformat settings (facebook#386) Bump version to 0.47-SNAPSHOT Bump version to 0.46 Fix indentation of trailing comments in a bunch of cases (facebook#418) Adjust .editorconfig for kotlinlang style for IntelliJ to better align with ktfmt (facebook#412) Bump Kotlin version to 1.8.22 Bump version to 0.46-SNAPSHOT Bump version to 0.45 Bump word-wrap from 1.2.3 to 1.2.4 in /website (facebook#410) Use inExpression in a nullsafe way (facebook#417) Update ktfmt component on FBS:master Back out "Improve argsfile support" Improve argsfile support Fix double indentation in Elvis chains (facebook#416) Daily `arc lint --take KTFMT` Remove TypeNameClassifier Support context receivers (facebook#400) Added link to live playground directly into README file Keep imports from the same package if the name is overloaded (facebook#414)
No description provided.