Fix gathering inherited properties #2353
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| package org.jetbrains.dokka.model | ||
|
|
||
| import org.jetbrains.dokka.links.DRI | ||
|
|
||
| const val JVM_FIELD_PACKAGE_NAME = "kotlin.jvm" | ||
| const val JVM_FIELD_CLASS_NAMES = "JvmField" | ||
|
|
||
| fun DRI.isJvmField(): Boolean = packageName == JVM_FIELD_PACKAGE_NAME && classNames == JVM_FIELD_CLASS_NAMES | ||
|
|
||
| fun Annotations.Annotation.isJvmField(): Boolean = dri.isJvmField() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| package org.jetbrains.dokka.base.transformers.documentables | ||
|
|
||
| import org.jetbrains.dokka.model.* | ||
| import org.jetbrains.dokka.transformers.documentation.PreMergeDocumentableTransformer | ||
| import org.jetbrains.kotlin.load.java.JvmAbi | ||
| import org.jetbrains.kotlin.load.java.propertyNameByGetMethodName | ||
| import org.jetbrains.kotlin.load.java.propertyNamesBySetMethodName | ||
| import org.jetbrains.kotlin.name.Name | ||
|
|
||
| /** | ||
| * This transformer is used to merge the backing fields and accessors (getters and setters) | ||
| * obtained from Java sources. This way, we could generate more coherent documentation, | ||
| * since the model is now aware of the relationship between accessors and the fields. | ||
| * This way if we generate Kotlin output we get rid of spare getters and setters, | ||
| * and from Kotlin-as-Java perspective we can collect accessors of each property. | ||
| */ | ||
| class PropertiesMergerTransformer : PreMergeDocumentableTransformer { | ||
BarkingBad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| override fun invoke(modules: List<DModule>) = | ||
| modules.map { it.copy(packages = it.packages.map { | ||
| it.mergeAccessorsAndField().copy( | ||
| classlikes = it.classlikes.map { it.mergeAccessorsAndField() } | ||
| ) | ||
| }) } | ||
|
|
||
| private fun <T : WithScope> T.mergeAccessorsAndField(): T { | ||
| val (functions, properties) = mergePotentialAccessorsAndField(this.functions, this.properties) | ||
| return when (this) { | ||
| is DClass -> { | ||
| this.copy(functions = functions, properties = properties) | ||
| } | ||
| is DEnum -> { | ||
| this.copy(functions = functions, properties = properties) | ||
| } | ||
| is DInterface -> { | ||
| this.copy(functions = functions, properties = properties) | ||
| } | ||
| is DObject -> { | ||
| this.copy(functions = functions, properties = properties) | ||
| } | ||
| is DAnnotation -> { | ||
| this.copy(functions = functions, properties = properties) | ||
| } | ||
| is DPackage -> { | ||
| this.copy(functions = functions, properties = properties) | ||
| } | ||
| else -> this | ||
| } as T | ||
| } | ||
|
|
||
| /** | ||
| * This is copied from here | ||
| * [org.jetbrains.dokka.base.translators.psi.DefaultPsiToDocumentableTranslator.DokkaPsiParser.getPropertyNameForFunction] | ||
| * we should consider if we could unify that. | ||
| * TODO: Revisit that | ||
| */ | ||
| private fun DFunction.getPropertyNameForFunction() = | ||
| when { | ||
| JvmAbi.isGetterName(name) -> propertyNameByGetMethodName(Name.identifier(name))?.asString() | ||
| JvmAbi.isSetterName(name) -> propertyNamesBySetMethodName(Name.identifier(name)).firstOrNull() | ||
| ?.asString() | ||
| else -> null | ||
| } | ||
|
|
||
| /** | ||
| * This is loosely copied from here | ||
| * [org.jetbrains.dokka.base.translators.psi.DefaultPsiToDocumentableTranslator.DokkaPsiParser.splitFunctionsAndAccessors] | ||
| * we should consider if we could unify that. | ||
| * TODO: Revisit that | ||
| */ | ||
| private fun mergePotentialAccessorsAndField( | ||
| functions: List<DFunction>, | ||
| fields: List<DProperty> | ||
| ): Pair<List<DFunction>, List<DProperty>> { | ||
| val fieldNames = fields.associateBy { it.name } | ||
|
|
||
| // Regular methods are methods that are not getters or setters | ||
| val regularMethods = mutableListOf<DFunction>() | ||
| // Accessors are methods that are getters or setters | ||
| val accessors = mutableMapOf<DProperty, MutableList<DFunction>>() | ||
| functions.forEach { method -> | ||
| val field = method.getPropertyNameForFunction()?.let { name -> fieldNames[name] } | ||
| if (field != null) { | ||
| accessors.getOrPut(field, ::mutableListOf).add(method) | ||
| } else { | ||
| regularMethods.add(method) | ||
| } | ||
| } | ||
|
|
||
| // Properties are triples of field and its getters and/or setters. | ||
| // They are wrapped up in DProperty class, | ||
| // so we copy accessors into its dedicated DProperty data class properties | ||
| val propertiesWithAccessors = accessors.map { (dProperty, dFunctions) -> | ||
| if (dProperty.visibility.values.all { it is KotlinVisibility.Private }) { | ||
| dFunctions.flatMap { it.visibility.values }.toSet().singleOrNull()?.takeIf { | ||
| it in listOf(KotlinVisibility.Public, KotlinVisibility.Protected) | ||
|
There was a problem hiding this comment. I think this can be extracted into a variable with a name that explains intention (something like There was a problem hiding this comment. Will look into that There was a problem hiding this comment. It's not necesarily overridability that we put emphasis on here, but rather we struggle with the proper visibility for property. The problem is, the java field will most likely be private and its accessors will be public/protected. We have to replace the visibility of the DProperty as it would be not visible in Kotlin. On the other hand I am now thinking about situation where we have public getter and private setter. That way we would like to have our DProperty to be public, yet be presented as val. Probably will have to rewrite the logic here... There was a problem hiding this comment. Do you need time to investigate it before we merge this PR? Can what you're talking about break something or introduce bugs? nice comments btw, thanks There was a problem hiding this comment. I think we should hardcode the configurations like these manually... I don't know if I have time to do it this week. |
||
| }?.let { visibility -> | ||
| dProperty.copy( | ||
| getter = dFunctions.firstOrNull { it.type == dProperty.type }, | ||
| setter = dFunctions.firstOrNull { it.parameters.isNotEmpty() }, | ||
| visibility = dProperty.visibility.mapValues { visibility } | ||
| ) | ||
| } ?: dProperty | ||
| } else { | ||
| dProperty | ||
| } | ||
| } | ||
|
|
||
| // The above logic is driven by accessors list | ||
| // Therefore, if there was no getter or setter, we missed processing the field itself. | ||
| // To include them, we collect all fields that have no accessors | ||
| val remainingFields = fields.toSet().minus(accessors.keys.toSet()) | ||
|
|
||
| val allProperties = propertiesWithAccessors + remainingFields | ||
|
|
||
| return regularMethods.toList() to allProperties | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this transformer only applicable to java sources? If so:
JavaPropertiesMergerTransformer?KotlinVisibility?My main concern is that if we don't limit it, then kotlin classes might satisfy the same conditions as Java and it'll introduce bugs (i.e not display getters/setters of a kotlin class for explicit declarations). If it's only java classes that are processed (somehow), should be safer.
But maybe this can never happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it can happen, I am not sure how I could limit it to only Java classes. Do you have any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you mean how is the source language defined https://github.com/Kotlin/dokka/blob/master/plugins/base/src/main/kotlin/translators/documentables/documentableLanguage.kt#L1