From 3d54352dd9e5aa02601d9f3b7a880e779ac3f22e Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Mon, 6 Feb 2023 13:05:58 +0100 Subject: [PATCH 1/3] Sort divergent elements deterministically Fixes #2784 --- .../documentables/DefaultPageCreator.kt | 46 ++- .../kotlin/pageMerger/PageNodeMergerTest.kt | 276 +++++++++++++++--- 2 files changed, 268 insertions(+), 54 deletions(-) diff --git a/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt b/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt index b8bf87a47f..e6bb068b7c 100644 --- a/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt +++ b/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt @@ -9,6 +9,7 @@ import org.jetbrains.dokka.base.transformers.documentables.ClashingDriIdentifier import org.jetbrains.dokka.base.transformers.pages.comments.CommentsToContentConverter import org.jetbrains.dokka.base.transformers.pages.tags.CustomTagContentProvider import org.jetbrains.dokka.base.translators.documentables.PageContentBuilder.DocumentableContentBuilder +import org.jetbrains.dokka.links.Callable import org.jetbrains.dokka.links.DRI import org.jetbrains.dokka.model.* import org.jetbrains.dokka.model.doc.* @@ -526,28 +527,29 @@ open class DefaultPageCreator( .mapValues { if (it.value.any { it is DClasslike }) it.value.filter { it !is DTypeAlias } else it.value } .toSortedMap(compareBy(nullsLast(String.CASE_INSENSITIVE_ORDER)) { it }) .forEach { (elementName, elements) -> // This groupBy should probably use LocationProvider + val sortedElements = sortDivergentElementsDeterministically(elements) row( - dri = elements.map { it.dri }.toSet(), - sourceSets = elements.flatMap { it.sourceSets }.toSet(), + dri = sortedElements.map { it.dri }.toSet(), + sourceSets = sortedElements.flatMap { it.sourceSets }.toSet(), kind = kind, styles = emptySet(), extra = elementName?.let { name -> extra + SymbolAnchorHint(name, kind) } ?: extra ) { link( text = elementName.orEmpty(), - address = elements.first().dri, + address = sortedElements.first().dri, kind = kind, styles = setOf(ContentStyle.RowTitle), - sourceSets = elements.sourceSets.toSet(), + sourceSets = sortedElements.sourceSets.toSet(), extra = extra ) divergentGroup( ContentDivergentGroup.GroupID(name), - elements.map { it.dri }.toSet(), + sortedElements.map { it.dri }.toSet(), kind = kind, extra = extra ) { - elements.map { + sortedElements.map { instance( setOf(it.dri), it.sourceSets.toSet(), @@ -571,6 +573,23 @@ open class DefaultPageCreator( } } + /** + * Divergent elements, such as extensions for the same receiver, can have identical signatures + * if they are declared in different places. If such elements are shown on the same page together, + * they need to be rendered deterministically to have reproducible builds. + * + * For example, you can have three identical extensions, if they are declared as: + * 1) top-level in package A + * 2) top-level in package B + * 3) inside a companion object in package A/B + * + * @see divergentBlock + */ + private fun sortDivergentElementsDeterministically(elements: List): List = + elements.takeIf { it.size > 1 } + ?.sortedWith(divergentDocumentableComparator) + ?: elements + private fun DocumentableContentBuilder.contentForCustomTagsBrief(documentable: Documentable) { val customTags = documentable.customTags if (customTags.isEmpty()) return @@ -611,6 +630,19 @@ internal val Documentable.customTags: Map(nullsLast()) { it.dri.packageName } + .thenBy(nullsFirst()) { it.dri.classNames } // nullsFirst for top level to be first + .thenBy( + nullsLast( + compareBy { it.params.size } + .thenBy { it.signature() } + ) + ) { it.dri.callable } + @Suppress("UNCHECKED_CAST") private fun T.nameAfterClash(): String = ((this as? WithExtraProperties)?.extra?.get(DriClashAwareName)?.value ?: name).orEmpty() @@ -624,4 +656,4 @@ internal inline fun GroupedTags.withTypeNamed(): M (this[T::class] as List>?) ?.groupByTo(linkedMapOf()) { it.second.name } ?.mapValues { (_, v) -> v.toMap() } - .orEmpty() \ No newline at end of file + .orEmpty() diff --git a/plugins/base/src/test/kotlin/pageMerger/PageNodeMergerTest.kt b/plugins/base/src/test/kotlin/pageMerger/PageNodeMergerTest.kt index 46b13763b8..7c52bd363c 100644 --- a/plugins/base/src/test/kotlin/pageMerger/PageNodeMergerTest.kt +++ b/plugins/base/src/test/kotlin/pageMerger/PageNodeMergerTest.kt @@ -1,52 +1,28 @@ package pageMerger -import org.jetbrains.dokka.pages.ClasslikePageNode -import org.jetbrains.dokka.pages.ContentPage -import org.jetbrains.dokka.pages.PageNode import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Disabled import org.junit.jupiter.api.Test import org.jetbrains.dokka.base.testApi.testRunner.BaseAbstractTest +import org.jetbrains.dokka.model.dfs +import org.jetbrains.dokka.model.withDescendants +import org.jetbrains.dokka.pages.* +import org.junit.jupiter.api.RepeatedTest +import java.lang.IllegalArgumentException +import kotlin.test.assertEquals class PageNodeMergerTest : BaseAbstractTest() { - /* object SameNameStrategy : DokkaPlugin() { - val strategy by extending { CoreExtensions.pageMergerStrategy with SameMethodNamePageMergerStrategy } - } - - class DefaultStrategy(val strList: MutableList = mutableListOf()) : DokkaPlugin(), DokkaLogger { - val strategy by extending { CoreExtensions.pageMergerStrategy with DefaultPageMergerStrategy(this@DefaultStrategy) } - - override var warningsCount: Int = 0 - override var errorsCount: Int = 0 - - override fun debug(message: String) = TODO() - - override fun info(message: String) = TODO() - - override fun progress(message: String) = TODO() - - override fun warn(message: String) { - strList += message + private val defaultConfiguration = dokkaConfiguration { + sourceSets { + sourceSet { + sourceRoots = listOf("src/main/kotlin") + } } - - override fun error(message: String) = TODO() - - override fun report() = TODO() } - */ @Test fun sameNameStrategyTest() { - - val configuration = dokkaConfiguration { - sourceSets { - sourceSet { - sourceRoots = listOf("src/main/kotlin/pageMerger/Test.kt") - } - } - } - testInline( """ |/src/main/kotlin/pageMerger/Test.kt @@ -60,8 +36,7 @@ class PageNodeMergerTest : BaseAbstractTest() { | fun test(str: String): String = str |} """.trimMargin(), - configuration/*, - pluginOverrides = listOf(SameNameStrategy)*/ + defaultConfiguration ) { pagesTransformationStage = { val allChildren = it.childrenRec().filterIsInstance() @@ -82,14 +57,6 @@ class PageNodeMergerTest : BaseAbstractTest() { fun defaultStrategyTest() { val strList: MutableList = mutableListOf() - val configuration = dokkaConfiguration { - sourceSets { - sourceSet { - sourceRoots = listOf("src/main/kotlin/pageMerger/Test.kt") - } - } - } - testInline( """ |/src/main/kotlin/pageMerger/Test.kt @@ -103,8 +70,7 @@ class PageNodeMergerTest : BaseAbstractTest() { | fun test(str: String): String = str |} """.trimMargin(), - configuration/*, - pluginOverrides = listOf(DefaultStrategy(strList)) */ + defaultConfiguration ) { pagesTransformationStage = { root -> val allChildren = root.childrenRec().filterIsInstance() @@ -187,4 +153,220 @@ class PageNodeMergerTest : BaseAbstractTest() { } } } + + @RepeatedTest(3) + fun `should deterministically render same name property extensions`() { + testInline( + """ + |/src/main/kotlin/test/Test.kt + |package test + | + |class ExtensionReceiver + | + |/** + | * Top level val extension + | */ + |val ExtensionReceiver.foo: String get() = "bar" + | + |class Obj { + | companion object { + | /** + | * Companion val extension + | */ + | val ExtensionReceiver.foo: String get() = "bar" + | } + |} + | + |/src/main/kotlin/test/nestedpackage/Pckg.kt + |package test.nestedpackage + | + |import test.ExtensionReceiver + | + |/** + | * From nested package int val extension + | */ + |val ExtensionReceiver.foo: Int get() = 42 + """.trimMargin(), + defaultConfiguration + ) { + renderingStage = { rootPageNode, _ -> + val extensions = rootPageNode.findExtensionsOfClass("ExtensionReceiver") + + extensions.assertContainsKDocsInOrder( + "Top level val extension", + "Companion val extension", + "From nested package int val extension" + ) + } + } + } + + @RepeatedTest(3) + fun `should deterministically render parameterless same name function extensions`() { + testInline( + """ + |/src/main/kotlin/test/Test.kt + |package test + | + |class ExtensionReceiver + | + |/** + | * Top level fun extension + | */ + |fun ExtensionReceiver.bar(): String = "bar" + | + |class Obj { + | + | companion object { + | /** + | * Companion fun extension + | */ + | fun ExtensionReceiver.bar(): String = "bar" + | } + |} + | + |/src/main/kotlin/test/nestedpackage/Pckg.kt + |package test.nestedpackage + | + |import test.ExtensionReceiver + | + |/** + | * From nested package fun extension + | */ + |fun ExtensionReceiver.bar(): String = "bar" + """.trimMargin(), + defaultConfiguration + ) { + renderingStage = { rootPageNode, _ -> + val extensions = rootPageNode.findExtensionsOfClass("ExtensionReceiver") + extensions.assertContainsKDocsInOrder( + "Top level fun extension", + "Companion fun extension", + "From nested package fun extension" + ) + } + } + } + + @RepeatedTest(3) + fun `should deterministically render same name function extensions with parameters`() { + testInline( + """ + |/src/main/kotlin/test/Test.kt + |package test + | + |class ExtensionReceiver + | + |/** + | * Top level fun extension with one string param + | */ + |fun ExtensionReceiver.bar(one: String): String = "bar" + | + |/** + | * Top level fun extension with one int param + | */ + |fun ExtensionReceiver.bar(one: Int): Int = 42 + | + |class Obj { + | + | companion object { + | /** + | * Companion fun extension with two params + | */ + | fun ExtensionReceiver.bar(one: String, two: String): String = "bar" + | } + |} + | + |/src/main/kotlin/test/nestedpackage/Pckg.kt + |package test.nestedpackage + | + |import test.ExtensionReceiver + | + |/** + | * From nested package fun extension with two params + | */ + |fun ExtensionReceiver.bar(one: String, two: String): String = "bar" + | + |/** + | * From nested package fun extension with three params + | */ + |fun ExtensionReceiver.bar(one: String, two: String, three: String): String = "bar" + | + |/** + | * From nested package fun extension with four params + | */ + |fun ExtensionReceiver.bar(one: String, two: String, three: String, four: String): String = "bar" + """.trimMargin(), + defaultConfiguration + ) { + renderingStage = { rootPageNode, _ -> + val extensions = rootPageNode.findExtensionsOfClass("ExtensionReceiver") + extensions.assertContainsKDocsInOrder( + "Top level fun extension with one int param", + "Top level fun extension with one string param", + "Companion fun extension with two params", + "From nested package fun extension with two params", + "From nested package fun extension with three params", + "From nested package fun extension with four params" + ) + } + } + } + + @RepeatedTest(3) + fun `should deterministically render same name function extensions with different receiver and return type`() { + testInline( + """ + |/src/main/kotlin/test/Test.kt + |package test + | + |/** + | * Top level fun extension string + | */ + |fun Int.bar(): String = "bar" + | + |/** + | * Top level fun extension int + | */ + |fun String.bar(): Int = 42 + """.trimMargin(), + defaultConfiguration + ) { + renderingStage = { rootPageNode, _ -> + val extensions = rootPageNode.findPackageExtensions("bar") + extensions.assertContainsKDocsInOrder( + "Top level fun extension string", + "Top level fun extension int" + ) + } + } + } + + private fun RootPageNode.findExtensionsOfClass(name: String): ContentDivergentGroup { + val extensionReceiverPage = this.dfs { it is ClasslikePageNode && it.name == name } as ClasslikePageNode + return extensionReceiverPage.content + .dfs { it is ContentDivergentGroup && it.groupID.name == "Extensions" } as ContentDivergentGroup + } + + private fun RootPageNode.findPackageExtensions(extensionName: String): ContentDivergentGroup { + val extensionReceiverPage = this.dfs { it is MemberPageNode && it.name == extensionName } as MemberPageNode + return extensionReceiverPage.content + .dfs { it is ContentDivergentGroup && it.groupID.name == "member" } as ContentDivergentGroup + } + + private fun ContentDivergentGroup.assertContainsKDocsInOrder(vararg expectedKDocs: String) { + expectedKDocs.forEachIndexed { index, expectedKDoc -> + assertEquals(expectedKDoc, this.getElementKDocText(index)) + } + } + + private fun ContentDivergentGroup.getElementKDocText(index: Int): String { + val element = this.children.getOrNull(index) ?: throw IllegalArgumentException("No element with index $index") + val commentNode = element.after + ?.withDescendants() + ?.singleOrNull { it is ContentText && it.dci.kind == ContentKind.Comment } + ?: throw IllegalStateException("Expected the element to contain a single paragraph of text / comment") + + return (commentNode as ContentText).text + } } From a3014986df828bc3f07ef011acc78553cd7a583e Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Wed, 8 Feb 2023 03:25:56 +0100 Subject: [PATCH 2/3] Preserve case sensitivity in divergent groups --- .../documentables/DefaultPageCreator.kt | 2 +- .../kotlin/pageMerger/PageNodeMergerTest.kt | 56 +++++++++++++++++-- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt b/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt index e6bb068b7c..6cfa5164e4 100644 --- a/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt +++ b/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt @@ -525,7 +525,7 @@ open class DefaultPageCreator( .groupBy { it.name } // This groupBy should probably use LocationProvider // This hacks displaying actual typealias signatures along classlike ones .mapValues { if (it.value.any { it is DClasslike }) it.value.filter { it !is DTypeAlias } else it.value } - .toSortedMap(compareBy(nullsLast(String.CASE_INSENSITIVE_ORDER)) { it }) + .entries.sortedBy { it.key } .forEach { (elementName, elements) -> // This groupBy should probably use LocationProvider val sortedElements = sortDivergentElementsDeterministically(elements) row( diff --git a/plugins/base/src/test/kotlin/pageMerger/PageNodeMergerTest.kt b/plugins/base/src/test/kotlin/pageMerger/PageNodeMergerTest.kt index 7c52bd363c..05ff55ee4c 100644 --- a/plugins/base/src/test/kotlin/pageMerger/PageNodeMergerTest.kt +++ b/plugins/base/src/test/kotlin/pageMerger/PageNodeMergerTest.kt @@ -333,8 +333,11 @@ class PageNodeMergerTest : BaseAbstractTest() { defaultConfiguration ) { renderingStage = { rootPageNode, _ -> - val extensions = rootPageNode.findPackageExtensions("bar") - extensions.assertContainsKDocsInOrder( + val packageFunctionBlocks = rootPageNode.findPackageFunctionBlocks(packageName = "test") + assertEquals(1, packageFunctionBlocks.size, "Expected to find only one group for the functions") + + val functionsBlock = packageFunctionBlocks[0] + functionsBlock.assertContainsKDocsInOrder( "Top level fun extension string", "Top level fun extension int" ) @@ -342,16 +345,57 @@ class PageNodeMergerTest : BaseAbstractTest() { } } + @Test + fun `should not ignore case when grouping by name`() { + testInline( + """ + |/src/main/kotlin/test/Test.kt + |package test + | + |/** + | * Top level fun bAr + | */ + |fun Int.bAr(): String = "bar" + | + |/** + | * Top level fun BaR + | */ + |fun String.BaR(): Int = 42 + """.trimMargin(), + defaultConfiguration + ) { + renderingStage = { rootPageNode, _ -> + val packageFunctionBlocks = rootPageNode.findPackageFunctionBlocks(packageName = "test") + assertEquals(2, packageFunctionBlocks.size, "Expected two separate function groups") + + val firstGroup = packageFunctionBlocks[0] + firstGroup.assertContainsKDocsInOrder( + "Top level fun BaR", + ) + + val secondGroup = packageFunctionBlocks[1] + secondGroup.assertContainsKDocsInOrder( + "Top level fun bAr", + ) + } + } + } + private fun RootPageNode.findExtensionsOfClass(name: String): ContentDivergentGroup { val extensionReceiverPage = this.dfs { it is ClasslikePageNode && it.name == name } as ClasslikePageNode return extensionReceiverPage.content .dfs { it is ContentDivergentGroup && it.groupID.name == "Extensions" } as ContentDivergentGroup } - private fun RootPageNode.findPackageExtensions(extensionName: String): ContentDivergentGroup { - val extensionReceiverPage = this.dfs { it is MemberPageNode && it.name == extensionName } as MemberPageNode - return extensionReceiverPage.content - .dfs { it is ContentDivergentGroup && it.groupID.name == "member" } as ContentDivergentGroup + private fun RootPageNode.findPackageFunctionBlocks(packageName: String): List { + val packagePage = this.dfs { it is PackagePage && it.name == packageName } as PackagePage + val packageFunctionTable = packagePage.content.dfs { + it is ContentTable && it.dci.kind == ContentKind.Functions + } as ContentTable + + return packageFunctionTable.children.map { packageGroup -> + packageGroup.dfs { it is ContentDivergentGroup } as ContentDivergentGroup + } } private fun ContentDivergentGroup.assertContainsKDocsInOrder(vararg expectedKDocs: String) { From c1e0144857d7694e7f315a24d64c1f3aefb294da Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Thu, 9 Feb 2023 22:22:15 +0100 Subject: [PATCH 3/3] Add more details to the documentation of sorting --- .../kotlin/translators/documentables/DefaultPageCreator.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt b/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt index 6cfa5164e4..25f3c4502d 100644 --- a/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt +++ b/plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt @@ -584,9 +584,12 @@ open class DefaultPageCreator( * 3) inside a companion object in package A/B * * @see divergentBlock + * + * @param elements can contain types (annotation/class/interface/object/typealias), functions and properties + * @return the original list if it has one or zero elements */ private fun sortDivergentElementsDeterministically(elements: List): List = - elements.takeIf { it.size > 1 } + elements.takeIf { it.size > 1 } // the majority are single-element lists, but no real benchmarks done ?.sortedWith(divergentDocumentableComparator) ?: elements