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
Fix gathering inherited properties #2353
Changes from 7 commits
a6b4650
23c95db
50d00e7
dd7dc6b
efdd42f
3097193
bb746c9
255925b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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.isJvmName() |
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 losely 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can be extracted into a variable with a name that explains intention (something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will look into that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -430,7 +430,16 @@ private class DokkaDescriptorVisitor( | |
originalDescriptor: PropertyDescriptor, | ||
parent: DRIWithPlatformInfo | ||
): DProperty { | ||
val (dri, inheritedFrom) = originalDescriptor.createDRI() | ||
val (dri, _) = originalDescriptor.createDRI() | ||
/** | ||
* `createDRI` returns the DRI of the exact element and potential DRI of an element that is overriding it | ||
* (It can be also FAKE_OVERRIDE which is in fact just inheritance of the symbol) | ||
* | ||
* Looking at what PSIs do, they give the DRI of the element within the classnames where it is actually | ||
* declared and inheritedFrom as the same DRI but truncated callable part. | ||
* Therefore, we set callable to null and take the DRI only if it is indeed coming from different class. | ||
*/ | ||
val inheritedFrom = dri.copy(callable = null).takeIf { parent.dri.classNames != dri.classNames } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is about classes with the same name but from the different packages? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Should it be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it should. |
||
val descriptor = originalDescriptor.getConcreteDescriptor() | ||
val isExpect = descriptor.isExpect | ||
val isActual = descriptor.isActual | ||
|
@@ -448,10 +457,10 @@ private class DokkaDescriptorVisitor( | |
}, | ||
sources = actual, | ||
getter = descriptor.accessors.filterIsInstance<PropertyGetterDescriptor>().singleOrNull()?.let { | ||
visitPropertyAccessorDescriptor(it, descriptor, dri) | ||
visitPropertyAccessorDescriptor(it, descriptor, dri, inheritedFrom) | ||
}, | ||
setter = descriptor.accessors.filterIsInstance<PropertySetterDescriptor>().singleOrNull()?.let { | ||
visitPropertyAccessorDescriptor(it, descriptor, dri) | ||
visitPropertyAccessorDescriptor(it, descriptor, dri, inheritedFrom) | ||
}, | ||
visibility = descriptor.visibility.toDokkaVisibility().toSourceSetDependent(), | ||
documentation = descriptor.resolveDescriptorData(), | ||
|
@@ -468,7 +477,7 @@ private class DokkaDescriptorVisitor( | |
(descriptor.getAnnotationsWithBackingField() + descriptor.fileLevelAnnotations()).toSourceSetDependent() | ||
.toAnnotations(), | ||
descriptor.getDefaultValue()?.let { DefaultValue(it) }, | ||
InheritedMember(inheritedFrom.toSourceSetDependent()), | ||
inheritedFrom?.let { InheritedMember(it.toSourceSetDependent()) }, | ||
) | ||
) | ||
) | ||
|
@@ -485,7 +494,12 @@ private class DokkaDescriptorVisitor( | |
originalDescriptor: FunctionDescriptor, | ||
parent: DRIWithPlatformInfo | ||
): DFunction { | ||
val (dri, inheritedFrom) = originalDescriptor.createDRI() | ||
val (dri, _) = originalDescriptor.createDRI() | ||
/** | ||
* To avoid redundant docs, please visit [visitPropertyDescriptor] inheritedFrom | ||
* local val documentation. | ||
*/ | ||
val inheritedFrom = dri.copy(callable = null).takeIf { parent.dri.classNames != dri.classNames } | ||
val descriptor = originalDescriptor.getConcreteDescriptor() | ||
val isExpect = descriptor.isExpect | ||
val isActual = descriptor.isActual | ||
|
@@ -515,7 +529,7 @@ private class DokkaDescriptorVisitor( | |
sourceSets = setOf(sourceSet), | ||
isExpectActual = (isExpect || isActual), | ||
extra = PropertyContainer.withAll( | ||
InheritedMember(inheritedFrom.toSourceSetDependent()), | ||
inheritedFrom?.let { InheritedMember(it.toSourceSetDependent()) }, | ||
descriptor.additionalExtras().toSourceSetDependent().toAdditionalModifiers(), | ||
(descriptor.getAnnotations() + descriptor.fileLevelAnnotations()).toSourceSetDependent() | ||
.toAnnotations(), | ||
|
@@ -594,7 +608,8 @@ private class DokkaDescriptorVisitor( | |
private suspend fun visitPropertyAccessorDescriptor( | ||
descriptor: PropertyAccessorDescriptor, | ||
propertyDescriptor: PropertyDescriptor, | ||
parent: DRI | ||
parent: DRI, | ||
inheritedFrom: DRI? = null | ||
): DFunction { | ||
val dri = parent.copy(callable = Callable.from(descriptor)) | ||
val isGetter = descriptor is PropertyGetterDescriptor | ||
|
@@ -647,13 +662,34 @@ private class DokkaDescriptorVisitor( | |
return coroutineScope { | ||
val generics = async { descriptor.typeParameters.parallelMap { it.toVariantTypeParameter() } } | ||
|
||
/** | ||
* Workaround for problem with inheriting TagWrappers. | ||
* There is an issue if one declare documentation in the class header for | ||
* property using this syntax: `@property` | ||
* The compiler will propagate it withing this tag to property and to its getters and setters. | ||
* | ||
* Actually, the problem impacts more of these tags, yet this particular tag was blocker for | ||
* some opens-source plugin creators. | ||
* TODO: Should rethink if we could fix it globally in dokka or in compiler itself. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What solution do you suggest? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be fixed in the compiler. On the other hand, the compiler returns corresponding KDoc for some property etc. that is based on the positions of the original KDoc string, and therefore it is wrapped in the |
||
*/ | ||
fun SourceSetDependent<DocumentationNode>.translatePropertyToDescription(): SourceSetDependent<DocumentationNode> { | ||
BarkingBad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return this.mapValues { (_, value) -> | ||
value.copy(children = value.children.map { | ||
when (it) { | ||
is Property -> Description(it.root) | ||
else -> it | ||
} | ||
}) | ||
} | ||
} | ||
|
||
DFunction( | ||
dri, | ||
name, | ||
isConstructor = false, | ||
parameters = parameters, | ||
visibility = descriptor.visibility.toDokkaVisibility().toSourceSetDependent(), | ||
documentation = descriptor.resolveDescriptorData(), | ||
documentation = descriptor.resolveDescriptorData().translatePropertyToDescription(), | ||
type = descriptor.returnType!!.toBound(), | ||
generics = generics.await(), | ||
modifier = descriptor.modifier().toSourceSetDependent(), | ||
|
@@ -669,7 +705,8 @@ private class DokkaDescriptorVisitor( | |
isExpectActual = (isExpect || isActual), | ||
extra = PropertyContainer.withAll( | ||
descriptor.additionalExtras().toSourceSetDependent().toAdditionalModifiers(), | ||
descriptor.getAnnotations().toSourceSetDependent().toAnnotations() | ||
descriptor.getAnnotations().toSourceSetDependent().toAnnotations(), | ||
inheritedFrom?.let { InheritedMember(it.toSourceSetDependent()) } | ||
) | ||
) | ||
} | ||
|
@@ -904,9 +941,9 @@ private class DokkaDescriptorVisitor( | |
) | ||
} ?: getJavaDocs())?.takeIf { it.children.isNotEmpty() } | ||
|
||
private fun DeclarationDescriptor.getJavaDocs() = (this as? CallableDescriptor) | ||
?.overriddenDescriptors | ||
?.mapNotNull { it.findPsi() as? PsiNamedElement } | ||
private fun DeclarationDescriptor.getJavaDocs() = ( | ||
((this as? CallableDescriptor)?.overriddenDescriptors ?: emptyList()) + listOf(this) | ||
)?.mapNotNull { it.findPsi() as? PsiNamedElement } | ||
?.firstOrNull() | ||
?.let { javadocParser.parseDocumentation(it) } | ||
|
||
|
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