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

Sort divergent elements deterministically #2846

Merged
merged 3 commits into from
Feb 10, 2023
Merged

Sort divergent elements deterministically #2846

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
3d54352
Sort divergent elements deterministically
IgnatBeresnev Feb 6, 2023
a301498
Preserve case sensitivity in divergent groups
IgnatBeresnev Feb 8, 2023
c1e0144
Add more details to the documentation of sorting
IgnatBeresnev Feb 9, 2023
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
51 changes: 43 additions & 8 deletions plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.*
Expand Down Expand Up @@ -524,30 +525,31 @@ 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(
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(),
Expand All @@ -571,6 +573,26 @@ 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
*
* @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<Documentable>): List<Documentable> =
vmishenev marked this conversation as resolved.
Show resolved Hide resolved
elements.takeIf { it.size > 1 } // the majority are single-element lists, but no real benchmarks done
?.sortedWith(divergentDocumentableComparator)
?: elements

private fun DocumentableContentBuilder.contentForCustomTagsBrief(documentable: Documentable) {
val customTags = documentable.customTags
if (customTags.isEmpty()) return
Expand Down Expand Up @@ -611,6 +633,19 @@ internal val Documentable.customTags: Map<String, SourceSetDependent<CustomTagWr
private val Documentable.hasSeparatePage: Boolean
get() = this !is DTypeAlias

/**
* @see DefaultPageCreator.sortDivergentElementsDeterministically for usage
*/
private val divergentDocumentableComparator =
compareBy<Documentable, String?>(nullsLast()) { it.dri.packageName }
Copy link
Member

Choose a reason for hiding this comment

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

It can be confusing that extensions from different packages are placed in the same group. Extensions from different packages have different pages, each own one, and the group title is a link that leads to the page of the first one.

It doesn't block this PR, but a consideration for the further improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I also noticed it, but couldn't come up with how it can be fixed, other than not grouping such extensions under one block.

Hopefully, one day we will get rid of the left column altogether as planned, but in the meantime I can create an issue for fixing this, if you think it's significant enough.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't seen such case of extensions from different packages in stdlib so far, but it can happen in kotlinx.coroutines once #2845 is implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Why is nullsLast for a package? should the "null" package be first?

Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

packageName is nullable, so it has to be either first or last :) By default comparison rules in this function, all nulls are last - the explicit use just locks it into place

should the "null" package be first?

If you have a reason for why it should, I don't mind

Copy link
Member

Choose a reason for hiding this comment

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

Null for package means the top level

dokka/core/src/main/kotlin/links/DRI.kt

Line 24 in 683f6bb

val topLevel = DRI()

Copy link
Member

Choose a reason for hiding this comment

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

But I have checked we have an empty string for the root package

Copy link
Member

Choose a reason for hiding this comment

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

It seems like the last question here is about null but it's unclear whether nulls are possible at all.

The reasonable outcomes might be the following:

  • Just pick nullsFirst() or nullsLast(), file an issue and investigate whether nulls are possible as packageName
  • Figure out it here and fix accordingly.

Taking into account release pressure, my vote is for the first option

Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it's unclear whether nulls are possible at all

Unless we're ready to change the API (i.e make packageName non nullable, mind 133 usages + it's very commonly used public API), I don't think it matters much - we'll still have to account for it one way or another. Unless you're proposing to add !! 🌚

So yeah, can be either first or last, I don't really care. I picked last for consistency with callable, which is also nullable and nullsLast(), but if there's a reason for why it should be first - I don't mind

Copy link
Member

@vmishenev vmishenev Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We have some stubs (as an empty string) for an unnamed package

dokka/kotlin-analysis/src/main/kotlin/org/jetbrains/dokka/analysis/DRIFactory.kt

Line 37 in a5a20cd

packageName = classes.lastOrNull()?.qualifiedName?.substringBeforeLast('.', "") ?: "",

If we do not break it, the sort should work (I hope).

So the first option is reasonable for me to merge the PR sooner.

I picked last for consistency with callable

But it is inconsistent with .thenBy(nullsFirst()) { it.dri.classNames } so I dropped attention to this.

Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But it is inconsistent with

Yes, that's why .thenBy(nullsFirst()) { it.dri.classNames } has a comment to explain the decision

.thenBy(nullsFirst()) { it.dri.classNames } // nullsFirst for top level to be first
vmishenev marked this conversation as resolved.
Show resolved Hide resolved
.thenBy(
nullsLast(
compareBy<Callable> { it.params.size }
.thenBy { it.signature() }
)
) { it.dri.callable }
Comment on lines +640 to +647
Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There's definitely room for improvement in terms of sorting rules, but it's a start. To me it makes sense to display top level declarations to be on top as they have a wider scope, and to sort vararg-like overloads (like these) in a straircase-like manner - both are implemented.

Tell me if you can think of anything else right off the bat


@Suppress("UNCHECKED_CAST")
private fun <T : Documentable> T.nameAfterClash(): String =
((this as? WithExtraProperties<out Documentable>)?.extra?.get(DriClashAwareName)?.value ?: name).orEmpty()
Expand All @@ -624,4 +659,4 @@ internal inline fun <reified T : NamedTagWrapper> GroupedTags.withTypeNamed(): M
(this[T::class] as List<Pair<DokkaSourceSet, T>>?)
?.groupByTo(linkedMapOf()) { it.second.name }
?.mapValues { (_, v) -> v.toMap() }
.orEmpty()
.orEmpty()
Loading