From 89fc2494b40624c03a74e01f2f4ec4941bb5314d Mon Sep 17 00:00:00 2001 From: Ignat Beresnev Date: Sun, 19 Jun 2022 17:30:05 +0200 Subject: Fix incorrectly labeling java properties as val/var (#2540) Fixes #2539 --- core/api/core.api | 7 ++ .../main/kotlin/model/documentableProperties.kt | 10 +++ .../kotlin/signatures/KotlinSignatureProvider.kt | 6 +- .../DefaultDescriptorToDocumentableTranslator.kt | 20 ++++- .../psi/DefaultPsiToDocumentableTranslator.kt | 9 ++- .../FunctionalTypeConstructorsSignatureTest.kt | 4 +- .../signatures/InheritedAccessorsSignatureTest.kt | 4 +- .../src/test/kotlin/signatures/SignatureTest.kt | 44 +++++++++++ .../superFields/DescriptorSuperPropertiesTest.kt | 38 ++++++++++ .../test/kotlin/superFields/PsiSuperFieldsTest.kt | 29 ++++++-- ...efaultDescriptorToDocumentableTranslatorTest.kt | 29 ++++++++ .../DefaultPsiToDocumentableTranslatorTest.kt | 87 ++++++++++++++++++++++ 12 files changed, 273 insertions(+), 14 deletions(-) diff --git a/core/api/core.api b/core/api/core.api index 3efc0da5..4e1ed7c1 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 87f40bd6..9fe8aa1d 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 02da3f24..24ed0765 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 38992ba0..c4a8253a 100644 --- a/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt +++ b/plugins/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt @@ -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, @@ -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(), @@ -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 = diff --git a/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt b/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt index bef86144..f64eb261 100644 --- a/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt +++ b/plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt @@ -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, @@ -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 } ) } ) diff --git a/plugins/base/src/test/kotlin/signatures/FunctionalTypeConstructorsSignatureTest.kt b/plugins/base/src/test/kotlin/signatures/FunctionalTypeConstructorsSignatureTest.kt index 66d84967..374f2a6a 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 d3e03666..76e02c34 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 b021fae1..79089967 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 06ced8c9..a189894c 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,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]) } } } diff --git a/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt b/plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt index 8dd74ef2..9c1265a6 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 @@ -58,6 +58,7 @@ class PsiSuperFieldsTest : BaseAbstractTest() { |package test |open class A { | var a: Int = 1 + | val b: Int = 2 |} | |/src/test/B.java @@ -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]) + } } } } @@ -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]) } } } @@ -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]) } } } diff --git a/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt b/plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt index a9466f29..e463e2ec 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 25d6b22e..1ac54ae2 100644 --- a/plugins/base/src/test/kotlin/translators/DefaultPsiToDocumentableTranslatorTest.kt +++ b/plugins/base/src/test/kotlin/translators/DefaultPsiToDocumentableTranslatorTest.kt @@ -330,4 +330,91 @@ 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 non-final 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]) + } + } + } + + @Test + fun `should not add IsVar for final java field`() { + testInline( + """ + |/src/main/java/test/A.java + |package test; + |public class A { + | public final int a = 2; + |} + """.trimIndent(), + configuration + ) { + documentablesMergingStage = { module -> + val testedClass = module.packages.single().classlikes.single { it.name == "A" } + + val publicFinal = testedClass.properties.single { it.name == "a" } + assertNull(publicFinal.extra[IsVar]) + } + } + } } -- cgit