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.

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

Render member extensions correctly #3374

Merged
merged 4 commits into from
Dec 27, 2023
Merged

Render member extensions correctly #3374

merged 4 commits into from
Dec 27, 2023

Conversation

whyoleg
Copy link
Contributor

@whyoleg whyoleg commented Nov 24, 2023

Fixes #3187

Context
During building Classlike page, separate tabs for members and members+extensions are configured inside divergentBlock. Functions/Properties are divided in members and extensions by calling Documentable.isExtension (returns if function/property is extension or not based on existence of receiver).

Problem
Kotlin also has member extensions, which should be shown on members page, but Documentable.isExtension for them returns true because, you know, they are extensions.

To better illustrate the problem, here is a sample code:

interface Receiver {
    // 1. just ordinary member function
    // receiver = null
    fun memberFunction()

    // 2. member extension function
    // receiver = Int
    fun Int.intExtensionFunction()

    // 3. member extension function (yes it's valid Kotlin code)
    // receiver = Receiver
    fun Receiver.recieverExtensionFunction()

    companion object {
        // 4. extension function inside companion
        // receiver = Receiver
        fun Receiver.receiverCompanionExtensionFunction(): String = "bar"
    }
}

// 5. top-level extension function
// receiver = Receiver
fun Receiver.receiverTopLevelExtensionFunction(): String = "bar"

Expected behaviour
Functions 1, 2, 3 should be shown on members tab, and on members+extensions additionally shown 4 and 5

Current behaviour
Only function 1 is shown on members tab, and on members+extensions additionally shown 1, 2, 3, 4, 5.

Code problem
DocumentableContentBuilder.divergentBlock only accepts List<Documentable> (where Documentable could be function, property or type), and having only this we can't distinguish, is this function/property is member extension in type Receiver or it is external extension with receiver of type Receiver.
But we have this information upper in chain:

  1. for Classlikes, member extensions are already present in functions/properties and extensions are coming from CallableExtensions extra
  2. for packages, we can check receiver != null the same way as in Documentable.isExtension. We don't really render top-level declarations and top-level extensions differently in HTML or other formats, but we have tests for it :)

Another minor problem(inconvenience) that now processing types, functions and properties is done via single function (DocumentableContentBuilder.divergentBlock), and so it already has multiple checks, if we are processing different kinds of documentables and change behaviour accordingly

Solution

  • Path extensions and members explicitly as separate lists and handle them correctly after it
  • Split functions/properties and types logic, so that only related code for each kind of declarations is executed and code flow is easier to understand

Questions

  • Do we really need Documentable.isExtension in core? (it was used only in dokka-bases DefaultPageCreator.kt)
  • Is it ok to break DefaultPageCreator ABI? Now half of members there protected open and half private. In our codebase the only usage is in kotlin-as-java to override 1 function.
    On GitHub there is also only one usage of it in island-time - only contentForModule is overridden there (the only change is headers of table - looks like it is to fix rendering of packages in GHM format)

@whyoleg whyoleg self-assigned this Nov 24, 2023
@vmishenev
Copy link
Contributor

After the grooming:

I do not know if this function will be useful

fun <T> T.isTopLevelExtension() where T : Documentable, T : Callable = receiver != null && dri.classNames.isNullOrBlank()

@whyoleg whyoleg marked this pull request as ready for review December 4, 2023 18:31
@whyoleg
Copy link
Contributor Author

whyoleg commented Dec 4, 2023

I've added some tests for pages for checking tab types to cover both working and not working cases, to check if everything's works fine before/after refactoring.
Here is what happens if to run those tests without changes in this PR:

image

This specific error shows, that before member extension function was treated like EXTENSION_FUNCTION and so was placed to different tab. Other errors have similar error message.

@whyoleg
Copy link
Contributor Author

whyoleg commented Dec 4, 2023

I do not know if this function will be useful

For now, I deprecated function isExtension in core which was rather ambiguous in meaning and now isn't used anywhere in our code (and not sure, that we need to rely on it). If user will want this functionality, they will be able to implement it in one line

Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

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

Well done with the tests! And it's nice to see that typesBlock was extracted from divergentBlock, I think it makes it much easier to understand what's going on in different tabs 👍

setOf(element.dri),
element.sourceSets.toSet(),
extra = PropertyContainer.withAll(
SymbolAnchorHint(element.name ?: "", rowKind)
Copy link
Member

Choose a reason for hiding this comment

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

Did this anchor get lost accidentally? Don't see it in the new code

Dokka's extras are tricky here, they get propagated down, so maybe it's all good and this anchor came down from row's extras and exists in the output, just not in the code. Although up above it uses group.name, whereas before it used just name from the function parameters, so not sure how it works now

Would you be able to compare the HTML output of before and after this change? To see if the layout in general (divs/spans) and the styles/css classes are the same, and it's only the content that changed in specific scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's passed above in https://github.com/Kotlin/dokka/pull/3374/files#diff-1b193984c3c1257fc41e713fa16d6decba9266439b235ac447e1400367951f21R810

group.name is the same as element.name as group is grouped by name :)

So, element is function/property object and group is a group of functions/properties with the same name - so no changes here

I will recheck if it make any difference in the output

@@ -24,4 +24,8 @@ public fun DTypeParameter.filter(filteredSet: Set<DokkaSourceSet>): DTypeParamet
)
}

@Deprecated(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make it in #3365 to have one decision for such a function. Moreover, it was introduced recently here #2764 (comment)

not sure, that we need to rely on it

What is you concern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will revert changes here. My main concern is that the body of this function is rather straightforward code to write, but having such as an extension on Documentable feels strange and not obvious + we don't have a use-case for it and no-one asked for it. Not sure, that we need to make such functions our ABI.

@whyoleg whyoleg merged commit 2b1a366 into master Dec 27, 2023
11 of 12 checks passed
@whyoleg whyoleg deleted the member-extensions branch December 27, 2023 12:23
vmishenev pushed a commit that referenced this pull request Mar 20, 2024
* Add a lot of tests for extensions rendering in different cases
* Add documentation for unclear things
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No member extensions in the index
3 participants