Fix incorrectly labeling java properties as val/var (#2540)

Fixes #2539

10 files changed, 256 insertions, 14 deletions

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])
+            }
+        }
+    }
 }