From 97628db0d7622140158ed6679b67b4e2e4355194 Mon Sep 17 00:00:00 2001 From: Ignat Beresnev Date: Wed, 24 Aug 2022 14:18:26 +0200 Subject: [PATCH] Fix incorrect handling of static members used within `@see` tag (#2627) --- .../src/main/kotlin/parsers/MarkdownParser.kt | 23 +++- .../documentables/DefaultPageCreator.kt | 18 +-- .../content/seealso/ContentForSeeAlsoTest.kt | 121 +++++++++--------- .../test/kotlin/parsers/JavadocParserTest.kt | 84 ++++++++++++ ...tDescriptorToDocumentableTranslatorTest.kt | 95 +++++++++++++- 5 files changed, 263 insertions(+), 78 deletions(-) diff --git a/plugins/base/src/main/kotlin/parsers/MarkdownParser.kt b/plugins/base/src/main/kotlin/parsers/MarkdownParser.kt index d3006f3365..ebcb1b439d 100644 --- a/plugins/base/src/main/kotlin/parsers/MarkdownParser.kt +++ b/plugins/base/src/main/kotlin/parsers/MarkdownParser.kt @@ -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 @@ -51,7 +52,7 @@ open class MarkdownParser( val dri = externalDri(referencedName) See( parseStringToDocNode(content.substringAfter(' ')), - dri?.fqName() ?: referencedName, + dri?.fqDeclarationName() ?: referencedName, dri ) } @@ -59,7 +60,7 @@ open class MarkdownParser( val dri = externalDri(content.substringBefore(' ')) Throws( parseStringToDocNode(content.substringAfter(' ')), - dri?.fqName() ?: content.substringBefore(' '), + dri?.fqDeclarationName() ?: content.substringBefore(' '), dri ) } @@ -523,7 +524,7 @@ open class MarkdownParser( val dri = pointedLink(it) Throws( parseStringToDocNode(it.getContent()), - dri?.fqName() ?: it.getSubjectName().orEmpty(), + dri?.fqDeclarationName() ?: it.getSubjectName().orEmpty(), dri, ) } @@ -531,7 +532,7 @@ open class MarkdownParser( val dri = pointedLink(it) Throws( parseStringToDocNode(it.getContent()), - dri?.fqName() ?: it.getSubjectName().orEmpty(), + dri?.fqDeclarationName() ?: it.getSubjectName().orEmpty(), dri ) } @@ -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, ) } @@ -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 } + 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 diff --git a/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt b/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt index de31e44879..44e2728a33 100644 --- a/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt +++ b/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt @@ -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 @@ -592,26 +593,26 @@ 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 -> link( - dri.classNames ?: it.name, - dri, + text = seeTag.name.removePrefix("${dri.packageName}."), + 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) } } } @@ -641,7 +642,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()) { diff --git a/plugins/base/src/test/kotlin/content/seealso/ContentForSeeAlsoTest.kt b/plugins/base/src/test/kotlin/content/seealso/ContentForSeeAlsoTest.kt index 5c1fd05415..5dee546f6d 100644 --- a/plugins/base/src/test/kotlin/content/seealso/ContentForSeeAlsoTest.kt +++ b/plugins/base/src/test/kotlin/content/seealso/ContentForSeeAlsoTest.kt @@ -517,76 +517,73 @@ class ContentForSeeAlsoTest : BaseAbstractTest() { } @Test - fun `undocumented see also from java`(){ + 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() - 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() - 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" } + } + } + } + } + } + } + } + } } } } diff --git a/plugins/base/src/test/kotlin/parsers/JavadocParserTest.kt b/plugins/base/src/test/kotlin/parsers/JavadocParserTest.kt index af8dbb94a3..f7911c082c 100644 --- a/plugins/base/src/test/kotlin/parsers/JavadocParserTest.kt +++ b/plugins/base/src/test/kotlin/parsers/JavadocParserTest.kt @@ -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() { @@ -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() + 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(), 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() + 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) + } + } + } } diff --git a/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt b/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt index e463e2ec5a..5c57c4031b 100644 --- a/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt +++ b/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt @@ -4,15 +4,15 @@ import org.jetbrains.dokka.DokkaConfiguration import org.jetbrains.dokka.base.testApi.testRunner.BaseAbstractTest import org.jetbrains.dokka.links.DRI import org.jetbrains.dokka.model.* -import org.jetbrains.dokka.model.doc.CodeBlock -import org.jetbrains.dokka.model.doc.P -import org.jetbrains.dokka.model.doc.Text +import org.jetbrains.dokka.model.doc.* import org.junit.Assert import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertNull import org.junit.jupiter.api.Disabled import org.junit.jupiter.api.Test +import utils.text import kotlin.test.assertNotNull +import kotlin.test.assertTrue class DefaultDescriptorToDocumentableTranslatorTest : BaseAbstractTest() { val configuration = dokkaConfiguration { @@ -764,6 +764,95 @@ class DefaultDescriptorToDocumentableTranslatorTest : BaseAbstractTest() { } } } + + @Test + fun `should correctly parse multiple see tags with static function and property links`() { + testInline( + """ + |/src/main/kotlin/com/example/package/CollectionExtensions.kt + |package com.example.util + | + |object CollectionExtensions { + | val property = "Hi" + | + | fun emptyList() {} + | fun emptyMap() {} + | fun emptySet() {} + |} + | + |/src/main/kotlin/com/example/foo.kt + |package com.example + | + |import com.example.util.CollectionExtensions.emptyMap + |import com.example.util.CollectionExtensions.emptyList + |import com.example.util.CollectionExtensions.emptySet + |import com.example.util.CollectionExtensions.property + | + |/** + | * @see [List] stdlib list + | * @see [Map] stdlib map + | * @see [emptyMap] static emptyMap + | * @see [emptyList] static emptyList + | * @see [emptySet] static emptySet + | * @see [property] static property + | */ + |fun foo() {} + """.trimIndent(), + configuration + ) { + fun assertSeeTag(tag: TagWrapper, expectedName: String, expectedDescription: String) { + assertTrue(tag is See) + assertEquals(expectedName, tag.name) + val description = tag.children.joinToString { it.text().trim() } + assertEquals(expectedDescription, description) + } + + documentablesMergingStage = { module -> + val testFunction = module.packages.find { it.name == "com.example" } + ?.functions + ?.single { it.name == "foo" } + checkNotNull(testFunction) + + val documentationTags = testFunction.documentation.values.single().children + assertEquals(7, documentationTags.size) + + val descriptionTag = documentationTags[0] + assertTrue(descriptionTag is Description, "Expected first tag to be empty description") + assertTrue(descriptionTag.children.isEmpty(), "Expected first tag to be empty description") + + assertSeeTag( + tag = documentationTags[1], + expectedName = "kotlin.collections.List", + expectedDescription = "stdlib list" + ) + assertSeeTag( + tag = documentationTags[2], + expectedName = "kotlin.collections.Map", + expectedDescription = "stdlib map" + ) + assertSeeTag( + tag = documentationTags[3], + expectedName = "com.example.util.CollectionExtensions.emptyMap", + expectedDescription = "static emptyMap" + ) + assertSeeTag( + tag = documentationTags[4], + expectedName = "com.example.util.CollectionExtensions.emptyList", + expectedDescription = "static emptyList" + ) + assertSeeTag( + tag = documentationTags[5], + expectedName = "com.example.util.CollectionExtensions.emptySet", + expectedDescription = "static emptySet" + ) + assertSeeTag( + tag = documentationTags[6], + expectedName = "com.example.util.CollectionExtensions.property", + expectedDescription = "static property" + ) + } + } + } } private sealed class TestSuite {