From be2ee8bfc82e87e3d1eaea5f84b5801b54853e89 Mon Sep 17 00:00:00 2001 From: Ignat Beresnev Date: Thu, 16 Jun 2022 20:16:57 +0200 Subject: [PATCH] Fix incorrectly labeling java properties as val/var Fixes #2539 --- core/api/core.api | 7 ++ .../kotlin/model/documentableProperties.kt | 10 +++ .../signatures/KotlinSignatureProvider.kt | 6 +- ...faultDescriptorToDocumentableTranslator.kt | 17 ++++- .../psi/DefaultPsiToDocumentableTranslator.kt | 8 ++- ...FunctionalTypeConstructorsSignatureTest.kt | 4 +- .../InheritedAccessorsSignatureTest.kt | 4 +- .../test/kotlin/signatures/SignatureTest.kt | 44 ++++++++++++ .../DescriptorSuperPropertiesTest.kt | 13 ++++ .../kotlin/superFields/PsiSuperFieldsTest.kt | 8 ++- ...tDescriptorToDocumentableTranslatorTest.kt | 29 ++++++++ .../DefaultPsiToDocumentableTranslatorTest.kt | 67 ++++++++++++++++++- 12 files changed, 207 insertions(+), 10 deletions(-) diff --git a/core/api/core.api b/core/api/core.api index 3efc0da506..4e1ed7c181 100644 --- a/core/api/core.api +++ b/core/api/core.api @@ -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; diff --git a/core/src/main/kotlin/model/documentableProperties.kt b/core/src/main/kotlin/model/documentableProperties.kt index 87f40bd6ce..9fe8aa1de9 100644 --- a/core/src/main/kotlin/model/documentableProperties.kt +++ b/core/src/main/kotlin/model/documentableProperties.kt @@ -39,6 +39,16 @@ object ObviousMember : ExtraProperty, ExtraProperty.Key = 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, ExtraProperty.Key { + override val key: ExtraProperty.Key = this +} + data class CheckedExceptions(val exceptions: SourceSetDependent>) : ExtraProperty, ExtraProperty.Key { companion object : ExtraProperty.Key { override fun mergeStrategyFor(left: CheckedExceptions, right: CheckedExceptions) = diff --git a/plugins/base/src/main/kotlin/signatures/KotlinSignatureProvider.kt b/plugins/base/src/main/kotlin/signatures/KotlinSignatureProvider.kt index 02da3f24fb..e66d828ea3 100644 --- a/plugins/base/src/main/kotlin/signatures/KotlinSignatureProvider.kt +++ b/plugins/base/src/main/kotlin/signatures/KotlinSignatureProvider.kt @@ -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) { @@ -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") diff --git a/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt b/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt index 38992ba009..debb6397ce 100644 --- a/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt +++ b/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt @@ -470,6 +470,18 @@ private class DokkaDescriptorVisitor( return coroutineScope { val generics = async { descriptor.typeParameters.parallelMap { it.toVariantTypeParameter() } } + val getter = getDescriptorGetter() ?: getImplicitAccessorGetter() + val setter = getDescriptorSetter() ?: getImplicitAccessorSetter() + + val isVar = if (descriptor is JavaPropertyDescriptor) { + // java properties are `var` by default from descriptor's point of view, + // as there's no binding of fields to getters/setter. + // java property should be var if it has no accessors at all or has a setter + (getter == null && setter == null) || (getter != null && setter != null) + } else { + descriptor.isVar + } + DProperty( dri = dri, name = descriptor.name.asString(), @@ -477,8 +489,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(), @@ -495,6 +507,7 @@ private class DokkaDescriptorVisitor( .toAnnotations(), descriptor.getDefaultValue()?.let { DefaultValue(it.toSourceSetDependent()) }, inheritedFrom?.let { InheritedMember(it.toSourceSetDependent()) }, + takeIf { isVar }?.let { IsVar }, ) ) ) diff --git a/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt b/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt index bef86144fc..e2914e41b9 100644 --- a/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt +++ b/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt @@ -620,6 +620,11 @@ class DefaultPsiToDocumentableTranslator( private fun parseField(psi: PsiField, getter: DFunction?, setter: DFunction?, inheritedFrom: DRI? = null): DProperty { val dri = DRI.from(psi) + + // java property without accessors should be var + // setter should be non null when inheriting kotlin vars + val isVar = (getter == null && setter == null) || (getter != null && setter != null) + return DProperty( dri = dri, name = psi.name, @@ -645,7 +650,8 @@ class DefaultPsiToDocumentableTranslator( PropertyContainer.withAll( inheritedFrom?.let { inheritedFrom -> InheritedMember(inheritedFrom.toSourceSetDependent()) }, it.toSourceSetDependent().toAdditionalModifiers(), - annotations.toSourceSetDependent().toAnnotations() + annotations.toSourceSetDependent().toAnnotations(), + takeIf { isVar }?.let { IsVar } ) } ) diff --git a/plugins/base/src/test/kotlin/signatures/FunctionalTypeConstructorsSignatureTest.kt b/plugins/base/src/test/kotlin/signatures/FunctionalTypeConstructorsSignatureTest.kt index 66d8496776..374f2a6a5a 100644 --- a/plugins/base/src/test/kotlin/signatures/FunctionalTypeConstructorsSignatureTest.kt +++ b/plugins/base/src/test/kotlin/signatures/FunctionalTypeConstructorsSignatureTest.kt @@ -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 ) } @@ -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 ) } diff --git a/plugins/base/src/test/kotlin/signatures/InheritedAccessorsSignatureTest.kt b/plugins/base/src/test/kotlin/signatures/InheritedAccessorsSignatureTest.kt index d3e0366680..76e02c34c2 100644 --- a/plugins/base/src/test/kotlin/signatures/InheritedAccessorsSignatureTest.kt +++ b/plugins/base/src/test/kotlin/signatures/InheritedAccessorsSignatureTest.kt @@ -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 ) } @@ -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 ) } diff --git a/plugins/base/src/test/kotlin/signatures/SignatureTest.kt b/plugins/base/src/test/kotlin/signatures/SignatureTest.kt index b021fae147..79089967f4 100644 --- a/plugins/base/src/test/kotlin/signatures/SignatureTest.kt +++ b/plugins/base/src/test/kotlin/signatures/SignatureTest.kt @@ -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 + ) + } + } + } + } } diff --git a/plugins/base/src/test/kotlin/superFields/DescriptorSuperPropertiesTest.kt b/plugins/base/src/test/kotlin/superFields/DescriptorSuperPropertiesTest.kt index 06ced8c9dd..167ae13425 100644 --- a/plugins/base/src/test/kotlin/superFields/DescriptorSuperPropertiesTest.kt +++ b/plugins/base/src/test/kotlin/superFields/DescriptorSuperPropertiesTest.kt @@ -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 @@ -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]) } } } @@ -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]) } } } @@ -162,6 +167,8 @@ class DescriptorSuperPropertiesTest : BaseAbstractTest() { val setter = boolProperty.setter assertNotNull(setter) assertEquals("setBool", setter.name) + + assertNotNull(boolProperty.extra[IsVar]) } } } @@ -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]) } } } @@ -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]) } } } @@ -315,6 +326,8 @@ class DescriptorSuperPropertiesTest : BaseAbstractTest() { val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single() assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom) + + assertNotNull(property.extra[IsVar]) } } } diff --git a/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt b/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt index 8dd74ef261..a4498a1de3 100644 --- a/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt +++ b/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 @@ -75,6 +75,8 @@ class PsiSuperFieldsTest : BaseAbstractTest() { val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single() assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom) + + assertNotNull(property.extra[IsVar]) } } } @@ -107,6 +109,8 @@ class PsiSuperFieldsTest : BaseAbstractTest() { val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single() assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom) + + assertNotNull(property.extra[IsVar]) } } } @@ -151,6 +155,8 @@ class PsiSuperFieldsTest : BaseAbstractTest() { val inheritedFrom = property.extra[InheritedMember]?.inheritedFrom?.values?.single() assertEquals(DRI(packageName = "test", classNames = "A"), inheritedFrom) + + assertNotNull(property.extra[IsVar]) } } } diff --git a/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt b/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt index a9466f2992..e463e2ec5a 100644 --- a/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt +++ b/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt @@ -9,6 +9,7 @@ import org.jetbrains.dokka.model.doc.P import org.jetbrains.dokka.model.doc.Text import org.junit.Assert import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNull import org.junit.jupiter.api.Disabled import org.junit.jupiter.api.Test import kotlin.test.assertNotNull @@ -735,6 +736,34 @@ class DefaultDescriptorToDocumentableTranslatorTest : BaseAbstractTest() { } } } + + @Test + fun `should correctly add IsVar extra for properties`() { + testInline( + """ + |/src/main/kotlin/A.kt + |package test + |class A { + | public var mutable: Int = 0 + | public val immutable: Int = 0 + |} + """.trimIndent(), + configuration + ) { + documentablesMergingStage = { module -> + val testClass = module.packages.single().classlikes.single { it.name == "A" } + assertEquals(2, testClass.properties.size) + + val mutable = testClass.properties[0] + assertEquals("mutable", mutable.name) + assertNotNull(mutable.extra[IsVar]) + + val immutable = testClass.properties[1] + assertEquals("immutable", immutable.name) + assertNull(immutable.extra[IsVar]) + } + } + } } private sealed class TestSuite { diff --git a/plugins/base/src/test/kotlin/translators/DefaultPsiToDocumentableTranslatorTest.kt b/plugins/base/src/test/kotlin/translators/DefaultPsiToDocumentableTranslatorTest.kt index 25d6b22e4b..857863f6af 100644 --- a/plugins/base/src/test/kotlin/translators/DefaultPsiToDocumentableTranslatorTest.kt +++ b/plugins/base/src/test/kotlin/translators/DefaultPsiToDocumentableTranslatorTest.kt @@ -7,7 +7,6 @@ import org.jetbrains.dokka.links.DRI import org.jetbrains.dokka.model.* import org.jetbrains.dokka.model.doc.Text import org.jetbrains.dokka.plugability.DokkaPlugin -import org.junit.jupiter.api.Assertions import org.junit.jupiter.api.Assertions.* import org.junit.jupiter.api.Test import utils.assertNotNull @@ -330,4 +329,70 @@ class DefaultPsiToDocumentableTranslatorTest : BaseAbstractTest() { } } } + + @Test + fun `should add IsVar extra for field with getter and setter`() { + testInline( + """ + |/src/main/java/test/A.java + |package test; + |public class A { + | private int a = 1; + | public int getA() { return a; } + | public void setA(int a) { this.a = a; } + |} + """.trimIndent(), + configuration + ) { + documentablesMergingStage = { module -> + val testedClass = module.packages.single().classlikes.single { it.name == "A" } + + val property = testedClass.properties.single { it.name == "a" } + assertNotNull(property.extra[IsVar]) + } + } + } + + @Test + fun `should not add IsVar extra if field does not have a setter`() { + testInline( + """ + |/src/main/java/test/A.java + |package test; + |public class A { + | private int a = 1; + | public int getA() { return a; } + |} + """.trimIndent(), + configuration + ) { + documentablesMergingStage = { module -> + val testedClass = module.packages.single().classlikes.single { it.name == "A" } + + val property = testedClass.properties.single { it.name == "a" } + assertNull(property.extra[IsVar]) + } + } + } + + @Test + fun `should add IsVar for java field without any accessors`() { + testInline( + """ + |/src/main/java/test/A.java + |package test; + |public class A { + | private int a = 1; + |} + """.trimIndent(), + configuration + ) { + documentablesMergingStage = { module -> + val testedClass = module.packages.single().classlikes.single { it.name == "A" } + + val property = testedClass.properties.single { it.name == "a" } + assertNotNull(property.extra[IsVar]) + } + } + } }