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

Fix incorrectly labeling java properties as val/var #2540

Merged
merged 1 commit into from Jun 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions core/api/core.api
Expand Up @@ -1770,6 +1770,13 @@ public final class org/jetbrains/dokka/model/Invariance : org/jetbrains/dokka/mo
public fun toString ()Ljava/lang/String;
}

public final class org/jetbrains/dokka/model/IsVar : org/jetbrains/dokka/model/properties/ExtraProperty, org/jetbrains/dokka/model/properties/ExtraProperty$Key {
public static final field INSTANCE Lorg/jetbrains/dokka/model/IsVar;
public fun getKey ()Lorg/jetbrains/dokka/model/properties/ExtraProperty$Key;
public synthetic fun mergeStrategyFor (Ljava/lang/Object;Ljava/lang/Object;)Lorg/jetbrains/dokka/model/properties/MergeStrategy;
public fun mergeStrategyFor (Lorg/jetbrains/dokka/model/IsVar;Lorg/jetbrains/dokka/model/IsVar;)Lorg/jetbrains/dokka/model/properties/MergeStrategy;
}

public final class org/jetbrains/dokka/model/JavaClassKindTypes : java/lang/Enum, org/jetbrains/dokka/model/ClassKind {
public static final field ANNOTATION_CLASS Lorg/jetbrains/dokka/model/JavaClassKindTypes;
public static final field CLASS Lorg/jetbrains/dokka/model/JavaClassKindTypes;
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/kotlin/model/documentableProperties.kt
Expand Up @@ -39,6 +39,16 @@ object ObviousMember : ExtraProperty<Documentable>, ExtraProperty.Key<Documentab
override val key: ExtraProperty.Key<Documentable, *> = this
}

/**
* Whether this [DProperty] is `var` or `val`, should be present both in Kotlin and in Java properties
*
* In case of properties that came from `Java`, [IsVar] is added if
* the java field has no accessors at all (plain field) or has a setter
*/
object IsVar : ExtraProperty<DProperty>, ExtraProperty.Key<DProperty, IsVar> {
override val key: ExtraProperty.Key<DProperty, *> = this
}
IgnatBeresnev marked this conversation as resolved.
Show resolved Hide resolved

data class CheckedExceptions(val exceptions: SourceSetDependent<List<DRI>>) : ExtraProperty<Documentable>, ExtraProperty.Key<Documentable, ObviousMember> {
companion object : ExtraProperty.Key<Documentable, CheckedExceptions> {
override fun mergeStrategyFor(left: CheckedExceptions, right: CheckedExceptions) =
Expand Down
Expand Up @@ -261,7 +261,7 @@ class KotlinSignatureProvider(ctcc: CommentsToContentConverter, logger: DokkaLog
if (it is JavaModifier.Empty) KotlinModifier.Open else it
}?.name?.let { keyword("$it ") }
p.modifiers()[sourceSet]?.toSignatureString()?.let { keyword(it) }
p.setter?.let { keyword("var ") } ?: keyword("val ")
if (p.isMutable()) keyword("var ") else keyword("val ")
list(p.generics, prefix = "<", suffix = "> ",
separatorStyles = mainStyles + TokenStyle.Punctuation,
surroundingCharactersStyle = mainStyles + TokenStyle.Operator) {
Expand All @@ -279,6 +279,10 @@ class KotlinSignatureProvider(ctcc: CommentsToContentConverter, logger: DokkaLog
}
}

private fun DProperty.isMutable(): Boolean {
return this.extra[IsVar] != null || this.setter != null
}

private fun PageContentBuilder.DocumentableContentBuilder.highlightValue(expr: Expression) = when (expr) {
is IntegerConstant -> constant(expr.value.toString())
is FloatConstant -> constant(expr.value.toString() + "f")
Expand Down
Expand Up @@ -469,6 +469,8 @@ private class DokkaDescriptorVisitor(

return coroutineScope {
val generics = async { descriptor.typeParameters.parallelMap { it.toVariantTypeParameter() } }
val getter = getDescriptorGetter() ?: getImplicitAccessorGetter()
val setter = getDescriptorSetter() ?: getImplicitAccessorSetter()

DProperty(
dri = dri,
Expand All @@ -477,8 +479,8 @@ private class DokkaDescriptorVisitor(
visitReceiverParameterDescriptor(it, DRIWithPlatformInfo(dri, actual))
},
sources = actual,
getter = getDescriptorGetter() ?: getImplicitAccessorGetter(),
setter = getDescriptorSetter() ?: getImplicitAccessorSetter(),
getter = getter,
setter = setter,
visibility = descriptor.getVisibility(implicitAccessors).toSourceSetDependent(),
documentation = descriptor.resolveDescriptorData(),
modifier = descriptor.modifier().toSourceSetDependent(),
Expand All @@ -495,12 +497,26 @@ private class DokkaDescriptorVisitor(
.toAnnotations(),
descriptor.getDefaultValue()?.let { DefaultValue(it.toSourceSetDependent()) },
inheritedFrom?.let { InheritedMember(it.toSourceSetDependent()) },
takeIf { descriptor.isVar(getter, setter) }?.let { IsVar },
)
)
)
}
}

private fun PropertyDescriptor.isVar(getter: DFunction?, setter: DFunction?): Boolean {
return if (this is JavaPropertyDescriptor) {
// in Java, concepts of extensibility and mutability are mixed into a single `final` modifier
// in Kotlin, it's different - val/var controls mutability and open modifier controls extensibility
// so when inheriting Java properties, you can end up with a final var - non extensible mutable prop
val isMutable = this.isVar
// non-final java property should be var if it has no accessors at all or has a setter
(isMutable && getter == null && setter == null) || (getter != null && setter != null)
} else {
this.isVar
}
}

private fun PropertyDescriptor.getVisibility(implicitAccessors: DescriptorAccessorHolder?): Visibility {
val isNonPublicJavaProperty = this is JavaPropertyDescriptor && !this.visibility.isPublicAPI
val visibility =
Expand Down
Expand Up @@ -620,6 +620,12 @@ class DefaultPsiToDocumentableTranslator(

private fun parseField(psi: PsiField, getter: DFunction?, setter: DFunction?, inheritedFrom: DRI? = null): DProperty {
val dri = DRI.from(psi)

// non-final java field without accessors should be a var
// setter should be not null when inheriting kotlin vars
val isMutable = !psi.hasModifierProperty("final")
val isVar = (isMutable && getter == null && setter == null) || (getter != null && setter != null)

return DProperty(
dri = dri,
name = psi.name,
Expand All @@ -645,7 +651,8 @@ class DefaultPsiToDocumentableTranslator(
PropertyContainer.withAll(
inheritedFrom?.let { inheritedFrom -> InheritedMember(inheritedFrom.toSourceSetDependent()) },
it.toSourceSetDependent().toAdditionalModifiers(),
annotations.toSourceSetDependent().toAnnotations()
annotations.toSourceSetDependent().toAnnotations(),
takeIf { isVar }?.let { IsVar }
)
}
)
Expand Down
Expand Up @@ -275,7 +275,7 @@ class FunctionalTypeConstructorsSignatureTest : BaseAbstractTest() {
) {
renderingStage = { _, _ ->
writerPlugin.writer.renderedContent("root/example/-java-class/index.html").lastSignature().match(
"open val ", A("javaFunction"), ": (", A("Integer"), ") -> ", A("String"), Span(),
"open var ", A("javaFunction"), ": (", A("Integer"), ") -> ", A("String"), Span(),
ignoreSpanWithTokenStyle = true
)
}
Expand All @@ -301,7 +301,7 @@ class FunctionalTypeConstructorsSignatureTest : BaseAbstractTest() {
) {
renderingStage = { _, _ ->
writerPlugin.writer.renderedContent("root/example/-java-class/index.html").lastSignature().match(
"open val ", A("kotlinFunction"), ": (", A("Integer"), ") -> ", A("String"), Span(),
"open var ", A("kotlinFunction"), ": (", A("Integer"), ") -> ", A("String"), Span(),
ignoreSpanWithTokenStyle = true
)
}
Expand Down
Expand Up @@ -213,7 +213,7 @@ class InheritedAccessorsSignatureTest : BaseAbstractTest() {

val property = signatures[4]
property.match(
"val ", A("a"), ":", A("Int"), Span(),
"var ", A("a"), ":", A("Int"), Span(),
ignoreSpanWithTokenStyle = true
)
}
Expand Down Expand Up @@ -421,7 +421,7 @@ class InheritedAccessorsSignatureTest : BaseAbstractTest() {

val property = signatures[2]
property.match(
"protected val ", A("protectedProperty"), ":", A("Int"), Span(),
"protected var ", A("protectedProperty"), ":", A("Int"), Span(),
ignoreSpanWithTokenStyle = true
)
}
Expand Down
44 changes: 44 additions & 0 deletions plugins/base/src/test/kotlin/signatures/SignatureTest.kt
Expand Up @@ -951,4 +951,48 @@ class SignatureTest : BaseAbstractTest() {
}
}
}

@Test
fun `java property without accessors should be var`() {
val writerPlugin = TestOutputWriterPlugin()
testInline(
"""
|/src/test/JavaClass.java
|package test;
|public class JavaClass {
| public int property = 0;
|}
|
|/src/test/KotlinClass.kt
|package test
|open class KotlinClass : JavaClass() { }
""".trimIndent(),
configuration,
pluginOverrides = listOf(writerPlugin)
) {
renderingStage = { _, _ ->
writerPlugin.writer.renderedContent("root/test/-kotlin-class/index.html").let { kotlinClassContent ->
val signatures = kotlinClassContent.signature().toList()
assertEquals(3, signatures.size, "Expected 2 signatures: class signature, constructor and property")

val property = signatures[2]
property.match(
"var ", A("property"), ":", A("Int"), Span(),
ignoreSpanWithTokenStyle = true
)
}

writerPlugin.writer.renderedContent("root/test/-java-class/index.html").let { kotlinClassContent ->
val signatures = kotlinClassContent.signature().toList()
assertEquals(2, signatures.size, "Expected 2 signatures: class signature and property")

val property = signatures[1]
property.match(
"open var ", A("property"), ":", A("Int"), Span(),
ignoreSpanWithTokenStyle = true
)
}
}
}
}
}
Expand Up @@ -4,6 +4,7 @@ import org.jetbrains.dokka.DokkaConfiguration
import org.jetbrains.dokka.base.testApi.testRunner.BaseAbstractTest
import org.jetbrains.dokka.links.DRI
import org.jetbrains.dokka.model.InheritedMember
import org.jetbrains.dokka.model.IsVar
import org.jetbrains.dokka.model.KotlinVisibility
import org.junit.jupiter.api.Test
import kotlin.test.assertEquals
Expand Down Expand Up @@ -51,6 +52,8 @@ class DescriptorSuperPropertiesTest : BaseAbstractTest() {

val getterInheritedFrom = property.getter?.extra?.get(InheritedMember)?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), getterInheritedFrom)

assertNull(property.extra[IsVar])
}
}
}
Expand Down Expand Up @@ -129,6 +132,8 @@ class DescriptorSuperPropertiesTest : BaseAbstractTest() {
assertEquals("setA", setter.name)
val setterInheritedFrom = setter.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), setterInheritedFrom)

assertNotNull(property.extra[IsVar])
}
}
}
Expand Down Expand Up @@ -162,6 +167,8 @@ class DescriptorSuperPropertiesTest : BaseAbstractTest() {
val setter = boolProperty.setter
assertNotNull(setter)
assertEquals("setBool", setter.name)

assertNotNull(boolProperty.extra[IsVar])
}
}
}
Expand Down Expand Up @@ -211,6 +218,8 @@ class DescriptorSuperPropertiesTest : BaseAbstractTest() {

val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom)

assertNotNull(property.extra[IsVar])
}
}
}
Expand Down Expand Up @@ -267,6 +276,8 @@ class DescriptorSuperPropertiesTest : BaseAbstractTest() {

val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom)

assertNotNull(property.extra[IsVar])
}
}
}
Expand Down Expand Up @@ -315,6 +326,33 @@ class DescriptorSuperPropertiesTest : BaseAbstractTest() {

val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom)

assertNotNull(property.extra[IsVar])
}
}
}

@Test
fun `should mark final property inherited from java as val`() {
testInline(
"""
|/src/test/A.java
|package test;
|public class A {
| public final int a = 1;
|}
|
|/src/test/B.kt
|package test
|class B : A {}
""".trimIndent(),
commonTestConfiguration
) {
documentablesMergingStage = { module ->
val kotlinProperties = module.packages.single().classlikes.single { it.name == "B" }.properties
val property = kotlinProperties.single { it.name == "a" }

assertNull(property.extra[IsVar])
}
}
}
Expand Down
29 changes: 23 additions & 6 deletions plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt
@@ -1,10 +1,10 @@
package superFields

import org.jetbrains.dokka.DokkaConfiguration
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.IsVar
import org.jetbrains.dokka.model.isJvmField
import org.junit.jupiter.api.Assertions.assertNotNull
import org.junit.jupiter.api.Assertions.assertNull
Expand Down Expand Up @@ -58,6 +58,7 @@ class PsiSuperFieldsTest : BaseAbstractTest() {
|package test
|open class A {
| var a: Int = 1
| val b: Int = 2
|}
|
|/src/test/B.java
Expand All @@ -68,13 +69,25 @@ class PsiSuperFieldsTest : BaseAbstractTest() {
) {
documentablesMergingStage = { module ->
val inheritorProperties = module.packages.single().classlikes.single { it.name == "B" }.properties
val property = inheritorProperties.single { it.name == "a" }
inheritorProperties.single { it.name == "a" }.let { mutableProperty ->
assertNotNull(mutableProperty.getter)
assertNotNull(mutableProperty.setter)

assertNotNull(property.getter)
assertNotNull(property.setter)
val inheritedFrom = mutableProperty.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom)

val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom)
assertNotNull(mutableProperty.extra[IsVar])
}

inheritorProperties.single { it.name == "b" }.let { immutableProperty ->
assertNotNull(immutableProperty.getter)
assertNull(immutableProperty.setter)

val inheritedFrom = immutableProperty.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom)

assertNull(immutableProperty.extra[IsVar])
}
}
}
}
Expand Down Expand Up @@ -107,6 +120,8 @@ class PsiSuperFieldsTest : BaseAbstractTest() {

val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom)

assertNotNull(property.extra[IsVar])
}
}
}
Expand Down Expand Up @@ -151,6 +166,8 @@ class PsiSuperFieldsTest : BaseAbstractTest() {

val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single()
assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom)

assertNotNull(property.extra[IsVar])
}
}
}
Expand Down