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

Fix incorrect handling of static members used within @see tag #2627

Merged
merged 2 commits into from Aug 24, 2022
Merged

Fix incorrect handling of static members used within @see tag #2627

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d17a2fe
Fix incorrect handling of static members used within `@see` tag
IgnatBeresnev Aug 17, 2022
b45f195
Remove confusing comment
IgnatBeresnev Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 18 additions & 5 deletions plugins/base/src/main/kotlin/parsers/MarkdownParser.kt
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ import org.intellij.markdown.flavours.gfm.GFMFlavourDescriptor
import org.intellij.markdown.flavours.gfm.GFMTokenTypes
import org.jetbrains.dokka.base.parsers.factories.DocTagsFromIElementFactory
import org.jetbrains.dokka.links.DRI
import org.jetbrains.dokka.links.PointingToDeclaration
import org.jetbrains.dokka.model.doc.*
import org.jetbrains.dokka.model.doc.Suppress
import org.jetbrains.kotlin.kdoc.parser.KDocKnownTag
Expand Down Expand Up @@ -51,15 +52,15 @@ open class MarkdownParser(
val dri = externalDri(referencedName)
See(
parseStringToDocNode(content.substringAfter(' ')),
dri?.fqName() ?: referencedName,
dri?.fqDeclarationName() ?: referencedName,
dri
)
}
"throws", "exception" -> {
val dri = externalDri(content.substringBefore(' '))
Throws(
parseStringToDocNode(content.substringAfter(' ')),
dri?.fqName() ?: content.substringBefore(' '),
dri?.fqDeclarationName() ?: content.substringBefore(' '),
dri
)
}
Expand Down Expand Up @@ -523,15 +524,15 @@ open class MarkdownParser(
val dri = pointedLink(it)
Throws(
parseStringToDocNode(it.getContent()),
dri?.fqName() ?: it.getSubjectName().orEmpty(),
dri?.fqDeclarationName() ?: it.getSubjectName().orEmpty(),
dri,
)
}
KDocKnownTag.EXCEPTION -> {
val dri = pointedLink(it)
Throws(
parseStringToDocNode(it.getContent()),
dri?.fqName() ?: it.getSubjectName().orEmpty(),
dri?.fqDeclarationName() ?: it.getSubjectName().orEmpty(),
dri
)
}
Expand All @@ -545,7 +546,7 @@ open class MarkdownParser(
val dri = pointedLink(it)
See(
parseStringToDocNode(it.getContent()),
dri?.fqName() ?: it.getSubjectName().orEmpty(),
dri?.fqDeclarationName() ?: it.getSubjectName().orEmpty(),
dri,
)
}
Expand All @@ -567,8 +568,20 @@ open class MarkdownParser(
}

//Horrible hack but since link resolution is passed as a function i am not able to resolve them otherwise
@kotlin.Suppress("DeprecatedCallableAddReplaceWith")
@Deprecated("This function makes wrong assumptions and is missing a lot of corner cases related to generics, " +
"parameters and static members. This is not supposed to be public API and will not be supported in the future")
fun DRI.fqName(): String? = "$packageName.$classNames".takeIf { packageName != null && classNames != null }
Copy link
Member Author

Choose a reason for hiding this comment

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

The overall problem is basically here.

For com.example.Object.function() it would return com.example.Object, so if you have multiple @see tags that point to multiple different functions from the same object - there would be name collisions, and some members would be discarded by groupBy further down the road.

This function is overall very wrong, and it would take a lot of effort to make it right


private fun DRI.fqDeclarationName(): String? {
if (this.target !is PointingToDeclaration) {
return null
}
return listOfNotNull(this.packageName, this.classNames, this.callable?.name)
.joinToString(separator = ".")
.takeIf { it.isNotBlank() }
}

private fun findParent(kDoc: PsiElement): PsiElement =
if (kDoc.canHaveParent()) findParent(kDoc.parent) else kDoc

Expand Down
23 changes: 15 additions & 8 deletions plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@ import org.jetbrains.dokka.base.transformers.pages.comments.CommentsToContentCon
import org.jetbrains.dokka.base.transformers.pages.tags.CustomTagContentProvider
import org.jetbrains.dokka.base.translators.documentables.PageContentBuilder.DocumentableContentBuilder
import org.jetbrains.dokka.links.DRI
import org.jetbrains.dokka.links.PointingToDeclaration
import org.jetbrains.dokka.model.*
import org.jetbrains.dokka.model.doc.*
import org.jetbrains.dokka.model.properties.PropertyContainer
Expand Down Expand Up @@ -590,26 +591,31 @@ open class DefaultPageCreator(
availablePlatforms.forEach { platform ->
val possibleFallbacks = sourceSets.getPossibleFallbackSourcesets(platform)
seeAlsoTags.forEach { (_, see) ->
(see[platform] ?: see.fallback(possibleFallbacks))?.let {
(see[platform] ?: see.fallback(possibleFallbacks))?.let { seeTag ->
row(
sourceSets = setOf(platform),
kind = ContentKind.Comment,
styles = this@group.mainStyles,
) {
it.address?.let { dri ->
seeTag.address?.let { dri ->
// only declaration links are expected to be fully qualified
val linkText = dri.takeIf { it.target is PointingToDeclaration }
?.let { seeTag.name.removePrefix("${it.packageName}.") }
?: seeTag.name
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out, @see can also be used with local params (we have tests for it) and generics. And with functions, of course, so using classNames was wrong here, my bad


link(
dri.classNames ?: it.name,
dri,
text = linkText,
address = dri,
kind = ContentKind.Comment,
styles = mainStyles + ContentStyle.RowTitle
)
} ?: text(
it.name,
text = seeTag.name,
kind = ContentKind.Comment,
styles = mainStyles + ContentStyle.RowTitle
)
if (it.isNotEmpty()) {
comment(it.root)
if (seeTag.isNotEmpty()) {
comment(seeTag.root)
}
}
}
Expand Down Expand Up @@ -639,7 +645,8 @@ open class DefaultPageCreator(
row(sourceSets = setOf(sourceset)) {
group(styles = mainStyles + ContentStyle.RowTitle) {
throws.exceptionAddress?.let {
link(text = it.classNames ?: entry.key, address = it)
val className = it.takeIf { it.target is PointingToDeclaration }?.classNames
link(text = className ?: entry.key, address = it)
} ?: text(entry.key)
}
if (throws.isNotEmpty()) {
Expand Down
121 changes: 59 additions & 62 deletions plugins/base/src/test/kotlin/content/seealso/ContentForSeeAlsoTest.kt
View file
Open in desktop
Expand Up @@ -517,76 +517,73 @@ class ContentForSeeAlsoTest : BaseAbstractTest() {
}

@Test
fun `undocumented see also from java`(){
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved two tests to JavadocParserTest as they are not about content, but verify documentables model.

fun `should prefix static function and property links with class name`() {
testInline(
"""
|/src/main/java/example/Source.java
|package example;
|/src/main/kotlin/com/example/package/CollectionExtensions.kt
|package com.example.util
|
|public interface Source {
| String getProperty(String k, String v);
|
| /**
| * @see #getProperty(String, String)
| */
| String getProperty(String k);
|object CollectionExtensions {
| val property = "Hi"
| fun emptyList() {}
|}
""".trimIndent(), testConfiguration
) {
documentablesTransformationStage = { module ->
val functionWithSeeTag = module.packages.flatMap { it.classlikes }.flatMap { it.functions }.find { it.name == "getProperty" && it.parameters.count() == 1 }
val seeTag = functionWithSeeTag?.docs()?.firstIsInstanceOrNull<See>()
val expectedLinkDestinationDRI = DRI(
packageName = "example",
classNames = "Source",
callable = Callable(
name = "getProperty",
params = listOf(JavaClassReference("java.lang.String"), JavaClassReference("java.lang.String"))
)
)

kotlin.test.assertNotNull(seeTag)
assertEquals("getProperty(String, String)", seeTag.name)
assertEquals(expectedLinkDestinationDRI, seeTag.address)
assertEquals(emptyList(), seeTag.children)
}
}
}

@Test
fun `documented see also from java`(){
testInline(
"""
|/src/main/java/example/Source.java
|package example;
|
|public interface Source {
| String getProperty(String k, String v);
|/src/main/kotlin/com/example/foo.kt
|package com.example
|
| /**
| * @see #getProperty(String, String) this is a reference to a method that is present on the same class.
| */
| String getProperty(String k);
|}
""".trimIndent(), testConfiguration
|import com.example.util.CollectionExtensions.property
|import com.example.util.CollectionExtensions.emptyList
|
|/**
| * @see [property] static property
| * @see [emptyList] static emptyList
| */
|fun function() {}
""".trimIndent(),
testConfiguration
) {
documentablesTransformationStage = { module ->
val functionWithSeeTag = module.packages.flatMap { it.classlikes }.flatMap { it.functions }.find { it.name == "getProperty" && it.parameters.size == 1 }
val seeTag = functionWithSeeTag?.docs()?.firstIsInstanceOrNull<See>()
val expectedLinkDestinationDRI = DRI(
packageName = "example",
classNames = "Source",
callable = Callable(
name = "getProperty",
params = listOf(JavaClassReference("java.lang.String"), JavaClassReference("java.lang.String"))
)
)
pagesTransformationStage = { module ->
val page = module.children.single { it.name == "com.example" }
.children.single { it.name == "function" } as ContentPage

kotlin.test.assertNotNull(seeTag)
assertEquals("getProperty(String, String)", seeTag.name)
assertEquals(expectedLinkDestinationDRI, seeTag.address)
assertEquals("this is a reference to a method that is present on the same class.", seeTag.children.first().text().trim())
assertEquals(1, seeTag.children.size)
page.content.assertNode {
group {
header(1) { +"function" }
}
divergentGroup {
divergentInstance {
divergent {
bareSignature(
annotations = emptyMap(),
visibility = "",
modifier = "",
keywords = emptySet(),
name = "function",
returnType = null,
)
}
after {
header(4) { +"See also" }
group {
table {
group {
link { +"CollectionExtensions.property" }
group {
group { +"static property" }
}
}
group {
link { +"CollectionExtensions.emptyList" }
group {
group { +"static emptyList" }
}
}
}
}
}
}
}
}
}
}
}
Expand Down
84 changes: 84 additions & 0 deletions plugins/base/src/test/kotlin/parsers/JavadocParserTest.kt
View file
Open in desktop
Expand Up @@ -2,12 +2,18 @@ package parsers

import com.jetbrains.rd.util.first
import org.jetbrains.dokka.base.testApi.testRunner.BaseAbstractTest
import org.jetbrains.dokka.links.Callable
import org.jetbrains.dokka.links.DRI
import org.jetbrains.dokka.links.JavaClassReference
import org.jetbrains.dokka.model.DEnum
import org.jetbrains.dokka.model.DModule
import org.jetbrains.dokka.model.doc.*
import org.jetbrains.kotlin.utils.addToStdlib.firstIsInstanceOrNull
import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Test
import utils.docs
import utils.text
import kotlin.test.assertNotNull

class JavadocParserTest : BaseAbstractTest() {

Expand Down Expand Up @@ -428,4 +434,82 @@ class JavadocParserTest : BaseAbstractTest() {
}
}
}

@Test
fun `undocumented see also from java`(){
testInline(
"""
|/src/main/java/example/Source.java
|package example;
|
|public interface Source {
| String getProperty(String k, String v);
|
| /**
| * @see #getProperty(String, String)
| */
| String getProperty(String k);
|}
""".trimIndent(), configuration
) {
documentablesTransformationStage = { module ->
val functionWithSeeTag = module.packages.flatMap { it.classlikes }.flatMap { it.functions }.find { it.name == "getProperty" && it.parameters.count() == 1 }
val seeTag = functionWithSeeTag?.docs()?.firstIsInstanceOrNull<See>()
val expectedLinkDestinationDRI = DRI(
packageName = "example",
classNames = "Source",
callable = Callable(
name = "getProperty",
params = listOf(JavaClassReference("java.lang.String"), JavaClassReference("java.lang.String"))
)
)

assertNotNull(seeTag)
assertEquals("getProperty(String, String)", seeTag.name)
assertEquals(expectedLinkDestinationDRI, seeTag.address)
assertEquals(emptyList<DocTag>(), seeTag.children)
}
}
}

@Test
fun `documented see also from java`(){
testInline(
"""
|/src/main/java/example/Source.java
|package example;
|
|public interface Source {
| String getProperty(String k, String v);
|
| /**
| * @see #getProperty(String, String) this is a reference to a method that is present on the same class.
| */
| String getProperty(String k);
|}
""".trimIndent(), configuration
) {
documentablesTransformationStage = { module ->
val functionWithSeeTag = module.packages.flatMap { it.classlikes }.flatMap { it.functions }.find { it.name == "getProperty" && it.parameters.size == 1 }
val seeTag = functionWithSeeTag?.docs()?.firstIsInstanceOrNull<See>()
val expectedLinkDestinationDRI = DRI(
packageName = "example",
classNames = "Source",
callable = Callable(
name = "getProperty",
params = listOf(JavaClassReference("java.lang.String"), JavaClassReference("java.lang.String"))
)
)

assertNotNull(seeTag)
assertEquals("getProperty(String, String)", seeTag.name)
assertEquals(expectedLinkDestinationDRI, seeTag.address)
assertEquals(
"this is a reference to a method that is present on the same class.",
seeTag.children.first().text().trim()
)
assertEquals(1, seeTag.children.size)
}
}
}
}