Handle properties without backing fields
This CL fixes a bug KSP abstraction where it would consider any property
as a field even when the property does not have a backing field.
Now instead, we properly exclude them from fields list.
We also had another small bug where we didn't create fields for
companion properties. KAPT does so now we do as well. For that, I've
changed the KSDeclaration isStatic helper to check for companion
objects. It is necessary as `isStatic` is used for overrides, asMemberOf
etc. (asMemberOf won't work between a class and its companion).
We've hit a bug in KSP where backing field value is wrong for static
companion fields in the classpath so that test path is excluded for now:
https://github.com/google/ksp/issues/491
Also fixed the declared fields implementation in KSP to properly return
based on backing field.
Bug: 177660733
Test: XTypeElementTest
Change-Id: I978c285e5b720bd8cbabb4df563a85153cbb5918
diff --git a/room/room-compiler-processing/build.gradle b/room/room-compiler-processing/build.gradle
index 5a91131..f1c4286 100644
--- a/room/room-compiler-processing/build.gradle
+++ b/room/room-compiler-processing/build.gradle
@@ -57,8 +57,7 @@
}
tasks.withType(Test).configureEach {
- // TODO: re-enable once b/177660733 is fixed.
- it.systemProperty("androidx.room.compiler.processing.strict", "false")
+ it.systemProperty("androidx.room.compiler.processing.strict", "true")
}
androidx {
diff --git a/room/room-compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KSDeclarationExt.kt b/room/room-compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KSDeclarationExt.kt
index 2b59a34..7d79231 100644
--- a/room/room-compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KSDeclarationExt.kt
+++ b/room/room-compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KSDeclarationExt.kt
@@ -62,6 +62,9 @@
internal fun KSDeclaration.isStatic(): Boolean {
return modifiers.contains(Modifier.JAVA_STATIC) || hasJvmStaticAnnotation() ||
+ // declarations in the companion object move into the enclosing class as statics.
+ // https://kotlinlang.org/docs/java-to-kotlin-interop.html#static-fields
+ this.findEnclosingAncestorClassDeclaration()?.isCompanionObject == true ||
when (this) {
is KSPropertyAccessor -> this.receiver.findEnclosingAncestorClassDeclaration() == null
is KSPropertyDeclaration -> this.findEnclosingAncestorClassDeclaration() == null
diff --git a/room/room-compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KspTypeElement.kt b/room/room-compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KspTypeElement.kt
index 9084059..3f3e58a 100644
--- a/room/room-compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KspTypeElement.kt
+++ b/room/room-compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KspTypeElement.kt
@@ -110,8 +110,12 @@
containing = this
)
}.let {
- // only order instance fields, we don't care about the order of companion fields.
- KspClassFileUtility.orderFields(declaration, it.toList())
+ // only order instance properties with backing fields, we don't care about the order
+ // of companion properties or properties without backing fields.
+ val (withBackingField, withoutBackingField) = it.partition {
+ it.declaration.hasBackingField
+ }
+ KspClassFileUtility.orderFields(declaration, withBackingField) + withoutBackingField
}
val companionProperties = declaration
@@ -130,6 +134,12 @@
declaredProperties + companionProperties
}
+ private val _declaredFields by lazy {
+ _declaredProperties.filter {
+ it.declaration.hasBackingField
+ }
+ }
+
private val syntheticGetterSetterMethods: List<XMethodElement> by lazy {
_declaredProperties.flatMap { field ->
when {
@@ -153,6 +163,17 @@
// them out.
it.modifiers.contains(Modifier.PRIVATE)
}
+ .filter {
+ if (field.isStatic()) {
+ // static fields are the properties that are coming from the
+ // companion. Whether we'll generate method for it or not depends on
+ // the JVMStatic annotation
+ it.hasJvmStaticAnnotation() ||
+ field.declaration.hasJvmStaticAnnotation()
+ } else {
+ true
+ }
+ }
.map { accessor ->
KspSyntheticPropertyMethodElement.create(
env = env,
@@ -205,14 +226,6 @@
return !isInterface() && !declaration.isOpen()
}
- private val _declaredFields by lazy {
- if (declaration.classKind == ClassKind.INTERFACE) {
- _declaredProperties.filter { it.isStatic() }
- } else {
- _declaredProperties.filter { !it.isAbstract() }
- }
- }
-
override fun getDeclaredFields(): List<XFieldElement> {
return _declaredFields
}
@@ -232,9 +245,11 @@
.filterNot { it.isConstructor() }
val companionMethods = declaration.findCompanionObject()
?.getDeclaredFunctions()
- ?.asSequence()
+ ?.filterNot {
+ it.isConstructor()
+ }
?.filter {
- it.isStatic()
+ it.hasJvmStaticAnnotation()
} ?: emptySequence()
val declaredMethods = (instanceMethods + companionMethods)
.filterNot {
diff --git a/room/room-compiler-processing/src/test/java/androidx/room/compiler/processing/InternalModifierTest.kt b/room/room-compiler-processing/src/test/java/androidx/room/compiler/processing/InternalModifierTest.kt
index 900a9b3..f5098cd 100644
--- a/room/room-compiler-processing/src/test/java/androidx/room/compiler/processing/InternalModifierTest.kt
+++ b/room/room-compiler-processing/src/test/java/androidx/room/compiler/processing/InternalModifierTest.kt
@@ -20,7 +20,7 @@
import androidx.room.compiler.processing.util.Source
import androidx.room.compiler.processing.util.runKaptTest
import androidx.room.compiler.processing.util.runKspTest
-import com.google.common.truth.Truth.assertThat
+import com.google.common.truth.Truth.assertWithMessage
import org.junit.Test
class InternalModifierTest {
@@ -38,24 +38,23 @@
internal class InternalClass(val value: String)
inline class InlineClass(val value:String)
abstract class Subject {
- var normalProp: String = TODO()
- var inlineProp: InlineClass = TODO()
+ var normalProp: String = ""
+ var inlineProp: InlineClass = InlineClass("")
internal abstract var internalAbstractProp: String
- internal var internalProp: String = TODO()
- internal var internalInlineProp: InlineClass = TODO()
- private var internalTypeProp : InternalClass = TODO()
+ internal var internalProp: String = ""
+ internal var internalInlineProp: InlineClass = InlineClass("")
+ private var internalTypeProp : InternalClass = InternalClass("")
@get:JvmName("explicitGetterName")
@set:JvmName("explicitSetterName")
- var jvmNameProp:String
+ var jvmNameProp:String = ""
fun normalFun() {}
@JvmName("explicitJvmName")
fun hasJvmName() {}
fun inlineReceivingFun(value: InlineClass) {}
- fun inlineReturningFun(): InlineClass = TODO()
+ fun inlineReturningFun(): InlineClass = InlineClass("")
internal fun internalInlineReceivingFun(value: InlineClass) {}
- internal fun internalInlineReturningFun(): InlineClass = TODO()
+ internal fun internalInlineReturningFun(): InlineClass = InlineClass("")
inline fun inlineFun() {
- TODO()
}
}
""".trimIndent()
@@ -101,7 +100,7 @@
) { invocation ->
kspResult = traverse(invocation.processingEnv)
}
-
- assertThat(kspResult).isEqualTo(kaptResult)
+ assertWithMessage("$kspResult\n--\n$kaptResult")
+ .that(kspResult).isEqualTo(kaptResult)
}
}
\ No newline at end of file
diff --git a/room/room-compiler-processing/src/test/java/androidx/room/compiler/processing/XTypeElementTest.kt b/room/room-compiler-processing/src/test/java/androidx/room/compiler/processing/XTypeElementTest.kt
index ac777c9..65055f6 100644
--- a/room/room-compiler-processing/src/test/java/androidx/room/compiler/processing/XTypeElementTest.kt
+++ b/room/room-compiler-processing/src/test/java/androidx/room/compiler/processing/XTypeElementTest.kt
@@ -22,7 +22,6 @@
import androidx.room.compiler.processing.util.getAllFieldNames
import androidx.room.compiler.processing.util.getField
import androidx.room.compiler.processing.util.getMethod
-import androidx.room.compiler.processing.util.runKspTest
import androidx.room.compiler.processing.util.runProcessorTest
import com.google.common.truth.Truth.assertThat
import com.google.common.truth.Truth.assertWithMessage
@@ -397,6 +396,112 @@
}
@Test
+ fun fieldsMethodsWithoutBacking() {
+ fun buildSrc(pkg: String) = Source.kotlin(
+ "Foo.kt",
+ """
+ package $pkg;
+ class Subject {
+ val realField: String = ""
+ get() = field
+ val noBackingVal: String
+ get() = ""
+ var noBackingVar: String
+ get() = ""
+ set(value) {}
+
+ companion object {
+ @JvmStatic
+ val staticRealField: String = ""
+ get() = field
+ @JvmStatic
+ val staticNoBackingVal: String
+ get() = ""
+ @JvmStatic
+ var staticNoBackingVar: String
+ get() = ""
+ set(value) {}
+ }
+ }
+ """.trimIndent()
+ )
+ val lib = compileFiles(listOf(buildSrc("lib")))
+ runProcessorTest(
+ sources = listOf(buildSrc("main")),
+ classpath = lib
+ ) { invocation ->
+ listOf("lib", "main").forEach { pkg ->
+ val subject = invocation.processingEnv.requireTypeElement("$pkg.Subject")
+ val declaredFields = subject.getDeclaredFields().map { it.name } -
+ listOf("Companion") // skip Companion, KAPT generates it
+ val expectedFields = if (invocation.isKsp && pkg == "lib") {
+ // TODO https://github.com/google/ksp/issues/491
+ // KSP returns false for companions in compiled code
+ listOf("realField")
+ } else {
+ listOf("realField", "staticRealField")
+ }
+ assertWithMessage(subject.qualifiedName)
+ .that(declaredFields)
+ .containsExactlyElementsIn(expectedFields)
+ val allFields = subject.getAllFieldsIncludingPrivateSupers().map { it.name } -
+ listOf("Companion") // skip Companion, KAPT generates it
+ assertWithMessage(subject.qualifiedName)
+ .that(allFields.toList())
+ .containsExactlyElementsIn(expectedFields)
+ val methodNames = subject.getDeclaredMethods().map { it.name }
+ assertWithMessage(subject.qualifiedName)
+ .that(methodNames)
+ .containsAtLeast("getNoBackingVal", "getNoBackingVar", "setNoBackingVar")
+ assertWithMessage(subject.qualifiedName)
+ .that(methodNames)
+ .doesNotContain("setNoBackingVal")
+ }
+ }
+ }
+
+ @Test
+ fun abstractFields() {
+ fun buildSource(pkg: String) = Source.kotlin(
+ "Foo.kt",
+ """
+ package $pkg;
+ abstract class Subject {
+ val value: String = ""
+ abstract val abstractValue: String
+ companion object {
+ var realCompanion: String = ""
+ @JvmStatic
+ var jvmStatic: String = ""
+ }
+ }
+ """.trimIndent()
+ )
+
+ val lib = compileFiles(listOf(buildSource("lib")))
+ runProcessorTest(
+ sources = listOf(buildSource("main")),
+ classpath = lib
+ ) { invocation ->
+ listOf("lib", "main").forEach { pkg ->
+ val subject = invocation.processingEnv.requireTypeElement("$pkg.Subject")
+ val declaredFields = subject.getDeclaredFields().map { it.name } -
+ listOf("Companion")
+ val expectedFields = if (invocation.isKsp && pkg == "lib") {
+ // TODO https://github.com/google/ksp/issues/491
+ // KSP returns false for companions in compiled code
+ listOf("value")
+ } else {
+ listOf("value", "realCompanion", "jvmStatic")
+ }
+ assertWithMessage(subject.qualifiedName)
+ .that(declaredFields)
+ .containsExactlyElementsIn(expectedFields)
+ }
+ }
+ }
+
+ @Test
fun fieldsInInterfaces() {
val src = Source.kotlin(
"Foo.kt",
@@ -677,7 +782,7 @@
}
@Test
- fun gettersSetters_companion() {
+ fun companion() {
val src = Source.kotlin(
"Foo.kt",
"""
@@ -688,30 +793,80 @@
@JvmStatic
val immutableStatic: String = "bar"
val companionProp: Int = 3
+ @get:JvmStatic
+ var companionProp_getterJvmStatic:Int =3
+ @set:JvmStatic
+ var companionProp_setterJvmStatic:Int =3
+
+ fun companionMethod() {
+ }
+
+ @JvmStatic
+ fun companionMethodWithJvmStatic() {}
}
}
class SubClass : CompanionSubject()
""".trimIndent()
)
- // KAPT is a bit aggressive in adding fields, specifically, it adds companionProp and
- // Companion as static fields which are not really fields from room's perspective.
- runKspTest(sources = listOf(src)) { invocation ->
+ runProcessorTest(sources = listOf(src)) { invocation ->
+ val objectMethodNames = invocation.processingEnv.requireTypeElement(
+ Any::class
+ ).getAllMethods().names()
val subject = invocation.processingEnv.requireTypeElement("CompanionSubject")
- assertThat(subject.getAllFieldNames()).containsExactly(
- "mutableStatic", "immutableStatic"
+ assertThat(subject.getAllFieldNames() - "Companion").containsExactly(
+ "mutableStatic", "immutableStatic", "companionProp",
+ "companionProp_getterJvmStatic", "companionProp_setterJvmStatic"
)
- assertThat(subject.getDeclaredMethods().names()).containsExactly(
- "getMutableStatic", "setMutableStatic", "getImmutableStatic"
+ val expectedMethodNames = listOf(
+ "getMutableStatic", "setMutableStatic", "getImmutableStatic",
+ "getCompanionProp_getterJvmStatic", "setCompanionProp_setterJvmStatic",
+ "companionMethodWithJvmStatic"
)
- assertThat(subject.getAllMethods().names()).containsExactly(
- "getMutableStatic", "setMutableStatic", "getImmutableStatic"
+ assertThat(
+ subject.getDeclaredMethods().names()
+ ).containsExactlyElementsIn(
+ expectedMethodNames
)
- assertThat(subject.getAllNonPrivateInstanceMethods().names()).isEmpty()
+ assertThat(
+ subject.getAllMethods().names() - objectMethodNames
+ ).containsExactlyElementsIn(
+ expectedMethodNames
+ )
+ assertThat(
+ subject.getAllNonPrivateInstanceMethods().names() - objectMethodNames
+ ).isEmpty()
val subClass = invocation.processingEnv.requireTypeElement("SubClass")
assertThat(subClass.getDeclaredMethods()).isEmpty()
- assertThat(subClass.getAllMethods().names()).containsExactly(
- "getMutableStatic", "setMutableStatic", "getImmutableStatic"
+ assertThat(
+ subClass.getAllMethods().names() - objectMethodNames
+ ).containsExactlyElementsIn(
+ expectedMethodNames
)
+
+ // make sure everything coming from companion is marked as static
+ subject.getDeclaredFields().forEach {
+ assertWithMessage(it.name).that(it.isStatic()).isTrue()
+ }
+ subject.getDeclaredMethods().forEach {
+ assertWithMessage(it.name).that(it.isStatic()).isTrue()
+ }
+
+ // make sure asMemberOf works fine for statics
+ val subClassType = subClass.type
+ subject.getDeclaredFields().forEach {
+ try {
+ it.asMemberOf(subClassType)
+ } catch (th: Throwable) {
+ throw AssertionError("Couldn't run asMemberOf for ${it.name}")
+ }
+ }
+ subject.getDeclaredMethods().forEach {
+ try {
+ it.asMemberOf(subClassType)
+ } catch (th: Throwable) {
+ throw AssertionError("Couldn't run asMemberOf for ${it.name}")
+ }
+ }
}
}
