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

[K2] Fix order of KDoc link candidates #3447

Merged
merged 3 commits into from
Jan 25, 2024
Merged

[K2] Fix order of KDoc link candidates #3447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dceda9f
[K2] Fix order of KDoc link candidates
vmishenev Jan 13, 2024
953152e
Add more tests
vmishenev Jan 18, 2024
a645424
Add tests for overloads
vmishenev Jan 18, 2024
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
19 changes: 17 additions & 2 deletions ...mbols/src/main/kotlin/org/jetbrains/dokka/analysis/kotlin/symbols/kdoc/ResolveKDocLink.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import org.jetbrains.dokka.analysis.kotlin.symbols.translators.getDRIFromSymbol
import org.jetbrains.dokka.links.DRI
import org.jetbrains.dokka.utilities.DokkaLogger
import org.jetbrains.kotlin.analysis.api.KtAnalysisSession
import org.jetbrains.kotlin.analysis.api.symbols.KtSymbol
import org.jetbrains.kotlin.analysis.api.symbols.*
import org.jetbrains.kotlin.idea.references.mainReference
import org.jetbrains.kotlin.kdoc.psi.api.KDoc
import org.jetbrains.kotlin.kdoc.psi.impl.KDocLink
Expand Down Expand Up @@ -78,5 +78,20 @@ internal fun KtAnalysisSession.resolveKDocLinkToDRI(link: KDocLink): DRI? {

private fun KtAnalysisSession.resolveToSymbol(kDocLink: KDocLink): KtSymbol? {
val lastNameSegment = kDocLink.children.filterIsInstance<KDocName>().lastOrNull()
return lastNameSegment?.mainReference?.resolveToSymbols()?.firstOrNull()
return lastNameSegment?.mainReference?.resolveToSymbols()?.sortedWith(linkCandidatesComparator)?.firstOrNull()
}

/**
* The order is like in [K1](https://github.com/JetBrains/intellij-community/blob/84f54ed97da66d6e24e6572345869bf1071945b6/plugins/kotlin/base/fe10/kdoc/src/org/jetbrains/kotlin/idea/kdoc/resolveKDocLink.kt#L104)
*
* TODO KT-64190
*/
private var linkCandidatesComparator: Comparator<KtSymbol> = compareBy{
when(it) {
is KtClassifierSymbol -> 1
is KtPackageSymbol -> 2
is KtFunctionSymbol -> 3
is KtVariableSymbol-> 4
else -> 5
}
}
Comment on lines +89 to +97
Copy link
Member

@IgnatBeresnev IgnatBeresnev Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please cover all cases with tests? I see a test for the initial bug where class < function, which is good, and it would be very helpful to make sure other cases don't break either. Especially once KT-64190 is fixed - it will help us verify that it was implemented the way we expected

And a side question: can there be two symbols of the same type? For example, two KtClassifierSymbol? Asking in case there's a non-obvious corner case related to source sets, expect/actual, internal visibility, java interop or something else? If there is, does it make sense to add a second comparison like thenBy { it.name } (but with something that's different between the two), to make it predictable?

Copy link
Member Author

Choose a reason for hiding this comment

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

can there be two symbols of the same type?

It is possible for KtFunctionSymbol when there are overloads. In this case, the order is not specified at all and it does not affect a result URL. It is not easy to make the order like in K1 on Dokka's side and consistent with IDEA. See added tests for overloads

For the other types of symbols, it is difficult to come up with a case when we have the same name in the same scope.

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 think a stable order of overloads can be addressed in #3250

Loading
Loading