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

Handle more corner cases for inherited accessors #2532

Merged
merged 3 commits into from Jun 17, 2022
Merged

Handle more corner cases for inherited accessors #2532

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
1f54363
Handle more corner cases for inherited accessors
IgnatBeresnev Jun 13, 2022
b1acc1f
Make java properties inherit visibility from getters
IgnatBeresnev Jun 16, 2022
5896a48
Optimize imports
IgnatBeresnev Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 31 additions & 14 deletions ...base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ import org.jetbrains.dokka.links.Callable
import org.jetbrains.dokka.model.*
import org.jetbrains.dokka.model.AnnotationTarget
import org.jetbrains.dokka.model.Nullable
import org.jetbrains.dokka.model.Visibility
import org.jetbrains.dokka.model.doc.*
import org.jetbrains.dokka.model.properties.PropertyContainer
import org.jetbrains.dokka.plugability.DokkaContext
Expand All @@ -40,9 +41,11 @@ import org.jetbrains.kotlin.descriptors.*
import org.jetbrains.kotlin.descriptors.ClassKind
import org.jetbrains.kotlin.descriptors.annotations.Annotated
import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptor
import org.jetbrains.kotlin.descriptors.java.JavaVisibilities
import org.jetbrains.kotlin.idea.kdoc.findKDoc
import org.jetbrains.kotlin.idea.kdoc.resolveKDocLink
import org.jetbrains.kotlin.js.resolve.diagnostics.findPsi
import org.jetbrains.kotlin.load.java.descriptors.JavaPropertyDescriptor
import org.jetbrains.kotlin.load.kotlin.toSourceElement
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.psi.*
Expand Down Expand Up @@ -379,7 +382,7 @@ private class DokkaDescriptorVisitor(
return coroutineScope {
val descriptorsWithKind = scope.getDescriptorsWithKind()

val (regularFunctions, accessors) = splitFunctionsAndAccessors(
val (regularFunctions, accessors) = splitFunctionsAndInheritedAccessors(
properties = descriptorsWithKind.properties,
functions = descriptorsWithKind.functions
)
Expand Down Expand Up @@ -427,11 +430,11 @@ private class DokkaDescriptorVisitor(

/**
* @param implicitAccessors getters/setters that are not part of the property descriptor, for instance
* average methods inherited from java sources
* average methods inherited from java sources that access the property
*/
private suspend fun visitPropertyDescriptor(
originalDescriptor: PropertyDescriptor,
implicitAccessors: List<FunctionDescriptor>,
implicitAccessors: DescriptorAccessorHolder?,
parent: DRIWithPlatformInfo
): DProperty {
val (dri, _) = originalDescriptor.createDRI()
Expand All @@ -451,9 +454,7 @@ private class DokkaDescriptorVisitor(
}

suspend fun getImplicitAccessorGetter() =
implicitAccessors
.firstOrNull { it.isGetterFor(originalDescriptor) }
?.let { visitFunctionDescriptor(it, parent) }
implicitAccessors?.getter?.let { visitFunctionDescriptor(it, parent) }

// example - generated setter that comes with data classes
suspend fun getDescriptorSetter() =
Expand All @@ -464,9 +465,7 @@ private class DokkaDescriptorVisitor(
}

suspend fun getImplicitAccessorSetter() =
implicitAccessors
.firstOrNull { it.isSetterFor(originalDescriptor) }
?.let { visitFunctionDescriptor(it, parent) }
implicitAccessors?.setter?.let { visitFunctionDescriptor(it, parent) }

return coroutineScope {
val generics = async { descriptor.typeParameters.parallelMap { it.toVariantTypeParameter() } }
Expand All @@ -480,7 +479,7 @@ private class DokkaDescriptorVisitor(
sources = actual,
getter = getDescriptorGetter() ?: getImplicitAccessorGetter(),
setter = getDescriptorSetter() ?: getImplicitAccessorSetter(),
visibility = descriptor.visibility.toDokkaVisibility().toSourceSetDependent(),
visibility = descriptor.getVisibility(implicitAccessors).toSourceSetDependent(),
documentation = descriptor.resolveDescriptorData(),
modifier = descriptor.modifier().toSourceSetDependent(),
type = descriptor.returnType!!.toBound(),
Expand All @@ -502,6 +501,22 @@ private class DokkaDescriptorVisitor(
}
}

private fun PropertyDescriptor.getVisibility(implicitAccessors: DescriptorAccessorHolder?): Visibility {
val isNonPublicJavaProperty = this is JavaPropertyDescriptor && !this.visibility.isPublicAPI
val visibility =
if (isNonPublicJavaProperty) {
// only try to take implicit getter's visibility if it's a java property
// because it's not guaranteed that implicit accessor will be used
// for the kotlin property, as it may have an explicit accessor of its own,
// i.e in data classes or with get() and set() are overridden
(implicitAccessors?.getter?.visibility ?: this.visibility)
} else {
this.visibility
}

return visibility.toDokkaVisibility()
}

private fun CallableMemberDescriptor.createDRI(wasOverridenBy: DRI? = null): Pair<DRI, DRI?> =
if (kind == CallableMemberDescriptor.Kind.DECLARATION || overriddenDescriptors.isEmpty())
Pair(DRI.from(this), wasOverridenBy)
Expand Down Expand Up @@ -818,12 +833,11 @@ private class DokkaDescriptorVisitor(

private suspend fun List<PropertyDescriptor>.visitProperties(
parent: DRIWithPlatformInfo,
implicitAccessors: Map<PropertyDescriptor, List<FunctionDescriptor>> = emptyMap(),
implicitAccessors: Map<PropertyDescriptor, DescriptorAccessorHolder> = emptyMap(),
): List<DProperty> {
return coroutineScope {
parallelMap {
val propertyAccessors = implicitAccessors[it] ?: emptyList()
visitPropertyDescriptor(it, propertyAccessors, parent)
visitPropertyDescriptor(it, implicitAccessors[it], parent)
}
}
}
Expand Down Expand Up @@ -1151,11 +1165,14 @@ private class DokkaDescriptorVisitor(
}) + ancestry.interfaces.map { TypeConstructorWithKind(it.typeConstructor, KotlinClassKindTypes.INTERFACE) }
}

private fun DescriptorVisibility.toDokkaVisibility(): org.jetbrains.dokka.model.Visibility = when (this.delegate) {
private fun DescriptorVisibility.toDokkaVisibility(): Visibility = when (this.delegate) {
Visibilities.Public -> KotlinVisibility.Public
Visibilities.Protected -> KotlinVisibility.Protected
Visibilities.Internal -> KotlinVisibility.Internal
Visibilities.Private -> KotlinVisibility.Private
JavaVisibilities.ProtectedAndPackage -> KotlinVisibility.Protected
JavaVisibilities.ProtectedStaticVisibility -> KotlinVisibility.Protected
JavaVisibilities.PackageVisibility -> JavaVisibility.Default
else -> KotlinVisibility.Public
}

Expand Down
94 changes: 78 additions & 16 deletions plugins/base/src/main/kotlin/translators/descriptors/DescriptorAccessorConventionUtil.kt
View file
Open in desktop
Expand Up @@ -10,29 +10,93 @@ import org.jetbrains.kotlin.load.java.propertyNamesBySetMethodName

internal data class DescriptorFunctionsHolder(
val regularFunctions: List<FunctionDescriptor>,
val accessors: Map<PropertyDescriptor, List<FunctionDescriptor>>
val accessors: Map<PropertyDescriptor, DescriptorAccessorHolder>
)

internal fun splitFunctionsAndAccessors(
internal data class DescriptorAccessorHolder(
val getter: FunctionDescriptor? = null,
val setter: FunctionDescriptor? = null
)

/**
* Separate regular Kotlin/Java functions and inherited Java accessors
* to properly display properties inherited from Java.
*
* Take this example:
* ```
* // java
* public class JavaClass {
* private int a = 1;
* public int getA() { return a; }
* public void setA(int a) { this.a = a; }
* }
*
* // kotlin
* class Bar : JavaClass() {
* fun foo() {}
* }
* ```
*
* It should result in:
* - 1 regular function `foo`
* - Map a=[`getA`, `setA`]
*/
internal fun splitFunctionsAndInheritedAccessors(
properties: List<PropertyDescriptor>,
functions: List<FunctionDescriptor>
): DescriptorFunctionsHolder {
val (javaMethods, kotlinFunctions) = functions.partition { it is JavaMethodDescriptor }
if (javaMethods.isEmpty()) {
return DescriptorFunctionsHolder(regularFunctions = kotlinFunctions, emptyMap())
}

val propertiesByName = properties.associateBy { it.name.asString() }
val regularFunctions = mutableListOf<FunctionDescriptor>()
val accessors = mutableMapOf<PropertyDescriptor, MutableList<FunctionDescriptor>>()
functions.forEach { function ->
val regularFunctions = ArrayList<FunctionDescriptor>(kotlinFunctions)
Copy link
Member

Choose a reason for hiding this comment

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

Are there reasons to use ArrayList instead of List?

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, ArrayList's constructor takes another collection as an argument and adds all of its elements into the newly created list. It's more efficient as by default even addAll still adds elements one by one, causing resizes and so on


val accessors = mutableMapOf<PropertyDescriptor, DescriptorAccessorHolder>()
javaMethods.forEach { function ->
val possiblePropertyNamesForFunction = function.toPossiblePropertyNames()
val property = possiblePropertyNamesForFunction.firstNotNullOfOrNull { propertiesByName[it] }
if (property != null && function.isAccessorFor(property)) {
accessors.getOrPut(property, ::mutableListOf).add(function)
accessors.compute(property) { prop, accessorHolder ->
if (function.isGetterFor(prop))
accessorHolder?.copy(getter = function) ?: DescriptorAccessorHolder(getter = function)
else
accessorHolder?.copy(setter = function) ?: DescriptorAccessorHolder(setter = function)
}
} else {
regularFunctions.add(function)
}
}

val accessorLookalikes = removeNonAccessorsReturning(accessors)
Copy link
Member

Choose a reason for hiding this comment

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

I would make a filter here. It is more obvious

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, one simple filter would not do :( We need to both remove elements from accessors and add removed elements to regularFunctions (so it involves mapping of elements also)

Filter would only return the elements which need to be added to regularFunctions, but it will not remove it from accessors.

This could be done in multiple steps, something like this, but IMHO it involves many excessive operations which can be substituted with a single iteration

val nonAccessibleProperties: Map<PropertyDescriptor, DescriptorAccessorHolder> =
        accessors.filterValues { it.getter == null && it.setter != null }

val functionsOfNonAccessibleProperties = nonAccessibleProperties.values.map { it.setter!! }
regularFunctions.addAll(functionsOfNonAccessibleProperties)

nonAccessibleProperties.keys.forEach { accessors.remove(it) }

regularFunctions.addAll(accessorLookalikes)

return DescriptorFunctionsHolder(regularFunctions, accessors)
}

internal fun FunctionDescriptor.toPossiblePropertyNames(): List<String> {
/**
* If a field has no getter, it's not accessible as a property from Kotlin's perspective,
* but it still might have a setter lookalike. In this case, this "setter" should be just a regular function
*
* @return removed elements
*/
private fun removeNonAccessorsReturning(
propertyAccessors: MutableMap<PropertyDescriptor, DescriptorAccessorHolder>
): List<FunctionDescriptor> {
val nonAccessors = mutableListOf<FunctionDescriptor>()
propertyAccessors.entries.removeIf { (_, accessors) ->
if (accessors.getter == null && accessors.setter != null) {
nonAccessors.add(accessors.setter)
true
} else {
false
}
}
return nonAccessors
}

private fun FunctionDescriptor.toPossiblePropertyNames(): List<String> {
val stringName = this.name.asString()
return when {
JvmAbi.isSetterName(stringName) -> propertyNamesBySetMethodName(this.name).map { it.asString() }
Expand All @@ -41,7 +105,7 @@ internal fun FunctionDescriptor.toPossiblePropertyNames(): List<String> {
}
}

internal fun propertyNamesByGetMethod(functionDescriptor: FunctionDescriptor): List<String> {
private fun propertyNamesByGetMethod(functionDescriptor: FunctionDescriptor): List<String> {
val stringName = functionDescriptor.name.asString()
// In java, the convention for boolean property accessors is as follows:
// - `private boolean active;`
Expand All @@ -63,21 +127,19 @@ internal fun propertyNamesByGetMethod(functionDescriptor: FunctionDescriptor): L
return listOfNotNull(javaPropName, kotlinPropName)
}

internal fun FunctionDescriptor.isAccessorFor(property: PropertyDescriptor): Boolean {
return this.isGetterFor(property) || this.isSetterFor(property)
private fun FunctionDescriptor.isAccessorFor(property: PropertyDescriptor): Boolean {
return (this.isGetterFor(property) || this.isSetterFor(property))
&& !property.visibility.isPublicAPI
&& this.visibility.isPublicAPI
}

internal fun FunctionDescriptor.isGetterFor(property: PropertyDescriptor): Boolean {
private fun FunctionDescriptor.isGetterFor(property: PropertyDescriptor): Boolean {
return this.returnType == property.returnType
&& this.valueParameters.isEmpty()
&& !property.visibility.isPublicAPI
&& this.visibility.isPublicAPI
}

internal fun FunctionDescriptor.isSetterFor(property: PropertyDescriptor): Boolean {
private fun FunctionDescriptor.isSetterFor(property: PropertyDescriptor): Boolean {
return this.valueParameters.size == 1
&& this.valueParameters[0].type == property.returnType
&& !property.visibility.isPublicAPI
&& this.visibility.isPublicAPI
}

33 changes: 16 additions & 17 deletions plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt
View file
Open in desktop
Expand Up @@ -17,9 +17,9 @@ import org.jetbrains.dokka.analysis.KotlinAnalysis
import org.jetbrains.dokka.analysis.PsiDocumentableSource
import org.jetbrains.dokka.analysis.from
import org.jetbrains.dokka.base.DokkaBase
import org.jetbrains.dokka.base.translators.typeConstructorsBeingExceptions
import org.jetbrains.dokka.base.translators.psi.parsers.JavaDocumentationParser
import org.jetbrains.dokka.base.translators.psi.parsers.JavadocParser
import org.jetbrains.dokka.base.translators.typeConstructorsBeingExceptions
import org.jetbrains.dokka.base.translators.unquotedValue
import org.jetbrains.dokka.links.*
import org.jetbrains.dokka.model.*
Expand All @@ -40,12 +40,7 @@ import org.jetbrains.kotlin.asJava.elements.KtLightAbstractAnnotation
import org.jetbrains.kotlin.cli.common.CLIConfigurationKeys
import org.jetbrains.kotlin.cli.jvm.config.JavaSourceRoot
import org.jetbrains.kotlin.idea.base.utils.fqname.getKotlinFqName
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
import org.jetbrains.kotlin.psi.psiUtil.getChildOfType
import org.jetbrains.kotlin.resolve.DescriptorUtils
import org.jetbrains.kotlin.utils.KotlinExceptionWithAttachments
import org.jetbrains.kotlin.utils.addToStdlib.safeAs
import java.io.File
Expand Down Expand Up @@ -108,16 +103,6 @@ class DefaultPsiToDocumentableTranslator(

private val cachedBounds = hashMapOf<String, Bound>()

private fun PsiModifierListOwner.getVisibility() = modifierList?.let {
val ml = it.children.toList()
when {
ml.any { it.text == PsiKeyword.PUBLIC } || it.hasModifierProperty("public") -> JavaVisibility.Public
ml.any { it.text == PsiKeyword.PROTECTED } || it.hasModifierProperty("protected") -> JavaVisibility.Protected
ml.any { it.text == PsiKeyword.PRIVATE } || it.hasModifierProperty("private") -> JavaVisibility.Private
else -> JavaVisibility.Default
}
} ?: JavaVisibility.Default

private val PsiMethod.hash: Int
get() = "$returnType $name$parameterList".hashCode()

Expand Down Expand Up @@ -641,7 +626,7 @@ class DefaultPsiToDocumentableTranslator(
documentation = javadocParser.parseDocumentation(psi).toSourceSetDependent(),
expectPresentInSet = null,
sources = PsiDocumentableSource(psi).toSourceSetDependent(),
visibility = psi.getVisibility().toSourceSetDependent(),
visibility = psi.getVisibility(getter).toSourceSetDependent(),
type = getBound(psi.type),
receiver = null,
setter = setter,
Expand All @@ -666,6 +651,10 @@ class DefaultPsiToDocumentableTranslator(
)
}

private fun PsiField.getVisibility(getter: DFunction?): Visibility {
return getter?.visibility?.get(sourceSetData) ?: this.getVisibility()
}

private fun Collection<PsiAnnotation>.toListOfAnnotations() =
filter { it !is KtLightAbstractAnnotation }.mapNotNull { it.toAnnotation() }

Expand Down Expand Up @@ -740,3 +729,13 @@ class DefaultPsiToDocumentableTranslator(
get() = getChildOfType<PsiJavaCodeReferenceElement>()?.resolve()
}
}

internal fun PsiModifierListOwner.getVisibility() = modifierList?.let {
val ml = it.children.toList()
when {
ml.any { it.text == PsiKeyword.PUBLIC } || it.hasModifierProperty("public") -> JavaVisibility.Public
ml.any { it.text == PsiKeyword.PROTECTED } || it.hasModifierProperty("protected") -> JavaVisibility.Protected
ml.any { it.text == PsiKeyword.PRIVATE } || it.hasModifierProperty("private") -> JavaVisibility.Private
else -> JavaVisibility.Default
}
} ?: JavaVisibility.Default