From 905310655842df7bfe677834026720c8ef10f115 Mon Sep 17 00:00:00 2001 From: Andrzej Ratajczak Date: Wed, 2 Mar 2022 12:12:47 +0100 Subject: [PATCH] Apply requested changes. --- core/api/core.api | 2 + core/src/main/kotlin/model/JvmField.kt | 7 +- .../PropertiesMergerTransformer.kt | 98 ++++++++++++------- ...faultDescriptorToDocumentableTranslator.kt | 30 +++++- .../psi/DefaultPsiToDocumentableTranslator.kt | 4 +- .../kotlin/superFields/PsiSuperFieldsTest.kt | 4 +- 6 files changed, 102 insertions(+), 43 deletions(-) diff --git a/core/api/core.api b/core/api/core.api index 2ec59ccabf..0cafa57dae 100644 --- a/core/api/core.api +++ b/core/api/core.api @@ -1839,6 +1839,8 @@ public final class org/jetbrains/dokka/model/JavaVisibility$Public : org/jetbrai } public final class org/jetbrains/dokka/model/JvmFieldKt { + public static final field JVM_FIELD_CLASS_NAMES Ljava/lang/String; + public static final field JVM_FIELD_PACKAGE_NAME Ljava/lang/String; public static final fun isJvmField (Lorg/jetbrains/dokka/links/DRI;)Z public static final fun isJvmField (Lorg/jetbrains/dokka/model/Annotations$Annotation;)Z } diff --git a/core/src/main/kotlin/model/JvmField.kt b/core/src/main/kotlin/model/JvmField.kt index 841dd16c9a..623115e0bd 100644 --- a/core/src/main/kotlin/model/JvmField.kt +++ b/core/src/main/kotlin/model/JvmField.kt @@ -2,6 +2,9 @@ package org.jetbrains.dokka.model import org.jetbrains.dokka.links.DRI -fun DRI.isJvmField(): Boolean = packageName == "kotlin.jvm" && classNames == "JvmField" +const val JVM_FIELD_PACKAGE_NAME = "kotlin.jvm" +const val JVM_FIELD_CLASS_NAMES = "JvmField" -fun Annotations.Annotation.isJvmField(): Boolean = dri.isJvmName() +fun DRI.isJvmField(): Boolean = packageName == JVM_FIELD_PACKAGE_NAME && classNames == JVM_FIELD_CLASS_NAMES + +fun Annotations.Annotation.isJvmField(): Boolean = dri.isJvmField() diff --git a/plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt b/plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt index e225748ca1..77c88285ef 100644 --- a/plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt +++ b/plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt @@ -7,43 +7,53 @@ 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 { override fun invoke(modules: List) = modules.map { it.copy(packages = it.packages.map { - it.mergeBeansAndField().copy( - classlikes = it.classlikes.map { it.mergeBeansAndField() } + it.mergeAccessorsAndField().copy( + classlikes = it.classlikes.map { it.mergeAccessorsAndField() } ) }) } - private fun T.mergeBeansAndField(): T = when (this) { - is DClass -> { - val (functions, properties) = mergePotentialBeansAndField(this.functions, this.properties) - this.copy(functions = functions, properties = properties) - } - is DEnum -> { - val (functions, properties) = mergePotentialBeansAndField(this.functions, this.properties) - this.copy(functions = functions, properties = properties) - } - is DInterface -> { - val (functions, properties) = mergePotentialBeansAndField(this.functions, this.properties) - this.copy(functions = functions, properties = properties) - } - is DObject -> { - val (functions, properties) = mergePotentialBeansAndField(this.functions, this.properties) - this.copy(functions = functions, properties = properties) - } - is DAnnotation -> { - val (functions, properties) = mergePotentialBeansAndField(this.functions, this.properties) - this.copy(functions = functions, properties = properties) - } - is DPackage -> { - val (functions, properties) = mergePotentialBeansAndField(this.functions, this.properties) - this.copy(functions = functions, properties = properties) - } - else -> this - } as T + private fun 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() @@ -52,13 +62,22 @@ class PropertiesMergerTransformer : PreMergeDocumentableTransformer { else -> null } - private fun mergePotentialBeansAndField( + /** + * 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, fields: List ): Pair, List> { val fieldNames = fields.associateBy { it.name } - val accessors = mutableMapOf>() + + // Regular methods are methods that are not getters or setters val regularMethods = mutableListOf() + // Accessors are methods that are getters or setters + val accessors = mutableMapOf>() functions.forEach { method -> val field = method.getPropertyNameForFunction()?.let { name -> fieldNames[name] } if (field != null) { @@ -67,7 +86,11 @@ class PropertiesMergerTransformer : PreMergeDocumentableTransformer { regularMethods.add(method) } } - return regularMethods.toList() to accessors.map { (dProperty, dFunctions) -> + + // 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) @@ -81,6 +104,15 @@ class PropertiesMergerTransformer : PreMergeDocumentableTransformer { } else { dProperty } - } + fields.toSet().minus(accessors.keys.toSet()) + } + + // 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 } } \ No newline at end of file diff --git a/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt b/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt index c362e4e9cb..c80aeba2d4 100644 --- a/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt +++ b/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt @@ -423,7 +423,15 @@ private class DokkaDescriptorVisitor( private suspend fun visitPropertyDescriptor(originalDescriptor: PropertyDescriptor, ): DProperty { val (dri, _) = originalDescriptor.createDRI() - val inheritedFrom = dri.copy(callable = null).takeIf { parent.dri.classNames != dri.classNames } + /** + * `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 || parent.dri.packageName != dri.packageName } val descriptor = originalDescriptor.getConcreteDescriptor() val isExpect = descriptor.isExpect val isActual = descriptor.isActual @@ -476,7 +484,11 @@ private class DokkaDescriptorVisitor( private suspend fun visitFunctionDescriptor(originalDescriptor: FunctionDescriptor): DFunction { val (dri, _) = originalDescriptor.createDRI() - val inheritedFrom = dri.copy(callable = null).takeIf { parent.dri.classNames != dri.classNames } + /** + * To avoid redundant docs, please visit [visitPropertyDescriptor] inheritedFrom + * local val documentation. + */ + val inheritedFrom = dri.copy(callable = null).takeIf { parent.dri.classNames != dri.classNames || parent.dri.packageName != dri.packageName } val descriptor = originalDescriptor.getConcreteDescriptor() val isExpect = descriptor.isExpect val isActual = descriptor.isActual @@ -639,7 +651,17 @@ private class DokkaDescriptorVisitor( return coroutineScope { val generics = async { descriptor.typeParameters.parallelMap { it.toVariantTypeParameter() } } - fun SourceSetDependent.translateParamToDescription(): SourceSetDependent { + /** + * 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 the text wrapped in 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. + */ + fun SourceSetDependent.translatePropertyTagToDescription(): SourceSetDependent { return this.mapValues { (_, value) -> value.copy(children = value.children.map { when (it) { @@ -656,7 +678,7 @@ private class DokkaDescriptorVisitor( isConstructor = false, parameters = parameters, visibility = descriptor.visibility.toDokkaVisibility().toSourceSetDependent(), - documentation = descriptor.resolveDescriptorData().translateParamToDescription(), + documentation = descriptor.resolveDescriptorData().translatePropertyTagToDescription(), type = descriptor.returnType!!.toBound(), generics = generics.await(), modifier = descriptor.modifier().toSourceSetDependent(), diff --git a/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt b/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt index 4d0b19b645..f22bf8d9d2 100644 --- a/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt +++ b/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt @@ -480,9 +480,9 @@ class DefaultPsiToDocumentableTranslator( * Workaround for getting JvmField Kotlin annotation in PSIs */ private fun Collection.getJvmFieldAnnotation() = filter { - it.qualifiedName == "kotlin.jvm.JvmField" + it.qualifiedName == "$JVM_FIELD_PACKAGE_NAME.$JVM_FIELD_CLASS_NAMES" }.map { - Annotations.Annotation(DRI("kotlin.jvm", "JvmField"), emptyMap()) + Annotations.Annotation(DRI(JVM_FIELD_PACKAGE_NAME, JVM_FIELD_CLASS_NAMES), emptyMap()) }.distinct() private fun PsiTypeParameter.annotations(): PropertyContainer = this.annotations.toList().toListOfAnnotations().annotations() diff --git a/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt b/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt index 7d45f91099..257bd94683 100644 --- a/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt +++ b/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt @@ -4,6 +4,7 @@ import org.jetbrains.dokka.base.testApi.testRunner.BaseAbstractTest import org.jetbrains.dokka.links.DRI import org.jetbrains.dokka.model.Annotations import org.jetbrains.dokka.model.InheritedMember +import org.jetbrains.dokka.model.isJvmField import org.junit.jupiter.api.Disabled import org.junit.jupiter.api.Assertions.assertNotNull import org.junit.jupiter.api.Assertions.assertNull @@ -126,8 +127,7 @@ class PsiSuperFieldsTest : BaseAbstractTest() { assertNull(this.getter) assertNull(this.setter) assertNotNull(this.extra[Annotations]?.directAnnotations?.values?.single()?.find { - it.dri.packageName == "kotlin.jvm" && - it.dri.classNames == "JvmField" + it.isJvmField() }) this.extra[InheritedMember]?.inheritedFrom?.values?.single()?.run { assertEquals(