Skip to content

Commit

Permalink
Implement findPrimaryConstructor in KspTypeElement
Browse files Browse the repository at this point in the history 
This CL implements findPrimaryConstructor in KspTypeElement.

I've also found a bug in KotlinMetadata implementation where
we would crash if it does not have a primary contructor.

I've also added tests for Java sources which required adding
a new synthetic constructor element for Java classes which
are not abstract but has no explicit constructor.
google/ksp#98

Also discovered another bug w/ KSP java interop where it could
return the same function declaration twice if it is coming from
java, added a workaround for it.

google/ksp#99

With this change, we can enable more of XProcessingEnv tests to
include KSP. I've enabled all except for the primitive types test.

Bug: 160322705
Test: KspTypeElementTest#constructors, XProcessingEnvTest
Change-Id: I2def5651fb3d7d693e7256646168d12622eb6928
  • Loading branch information
@yigit
yigit committed Oct 7, 2020
1 parent 7df02bc commit b45f9cc
Show file tree
Hide file tree
Showing 5 changed files with 231 additions and 24 deletions.
4 changes: 2 additions & 2 deletions ...ing/src/main/java/androidx/room/compiler/processing/javac/kotlin/KotlinMetadataElement.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ internal class KotlinMetadataElement(
private val ExecutableElement.descriptor: String
get() = descriptor()

fun findPrimaryConstructorSignature() = constructorList.first {
fun findPrimaryConstructorSignature() = constructorList.firstOrNull {
it.isPrimary()
}.descriptor
}?.descriptor

fun isObject(): Boolean = classMetadata.isObject()

Expand Down
62 changes: 50 additions & 12 deletions ...compiler-processing/src/main/java/androidx/room/compiler/processing/ksp/KspTypeElement.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import androidx.room.compiler.processing.XMethodElement
import androidx.room.compiler.processing.XType
import androidx.room.compiler.processing.XTypeElement
import androidx.room.compiler.processing.ksp.KspAnnotated.UseSiteFilter.Companion.NO_USE_SITE
import androidx.room.compiler.processing.ksp.synthetic.KspSyntheticConstructorForJava
import androidx.room.compiler.processing.ksp.synthetic.KspSyntheticPropertyMethodElement
import com.squareup.javapoet.ClassName
import org.jetbrains.kotlin.ksp.getAllSuperTypes
Expand All @@ -32,8 +33,10 @@ import org.jetbrains.kotlin.ksp.getDeclaredProperties
import org.jetbrains.kotlin.ksp.isOpen
import org.jetbrains.kotlin.ksp.symbol.ClassKind
import org.jetbrains.kotlin.ksp.symbol.KSClassDeclaration
import org.jetbrains.kotlin.ksp.symbol.KSFunctionDeclaration
import org.jetbrains.kotlin.ksp.symbol.KSPropertyDeclaration
import org.jetbrains.kotlin.ksp.symbol.Modifier
import org.jetbrains.kotlin.ksp.symbol.Origin

internal class KspTypeElement(
env: KspProcessingEnv,
Expand Down Expand Up @@ -181,7 +184,18 @@ internal class KspTypeElement(
}

override fun findPrimaryConstructor(): XConstructorElement? {
TODO("Not yet implemented")
val primary = if (declaration.declaredConstructors().isEmpty() && !isInterface()) {
declaration.primaryConstructor
} else {
declaration.getNonSyntheticPrimaryConstructor()
}
return primary?.let {
KspConstructorElement(
env = env,
containing = this,
declaration = it
)
}
}

private val _declaredMethods by lazy {
Expand Down Expand Up @@ -212,25 +226,33 @@ internal class KspTypeElement(
}

override fun getConstructors(): List<XConstructorElement> {
val constructors = declaration.getDeclaredFunctions()
.filter {
it.simpleName.asString() == name
}.toMutableList()
declaration.primaryConstructor?.let { primary ->
// workaround for https://github.com/android/kotlin/issues/136
// TODO remove once that bug is fixed
if (primary.simpleName.asString() != "<init>") {
constructors.add(primary)
}
val constructors = declaration.declaredConstructors().toMutableList()
val primary = if (constructors.isEmpty() && !isInterface()) {
declaration.primaryConstructor
} else {
declaration.getNonSyntheticPrimaryConstructor()
}
primary?.let(constructors::add)

return constructors.map {
val elements: List<XConstructorElement> = constructors.map {
KspConstructorElement(
env = env,
containing = this,
declaration = it
)
}
return if (elements.isEmpty() &&
declaration.origin == Origin.JAVA &&
!isInterface()) {
// workaround for https://github.com/google/ksp/issues/98
// TODO remove if KSP support this
listOf(KspSyntheticConstructorForJava(
env = env,
origin = this
))
} else {
elements
}
}

override fun getSuperInterfaceElements(): List<XTypeElement> {
Expand All @@ -243,4 +265,20 @@ internal class KspTypeElement(
env.wrapClassDeclaration(it)
}
}

private fun KSClassDeclaration.getNonSyntheticPrimaryConstructor(): KSFunctionDeclaration? {
val primary = declaration.primaryConstructor
// workaround for https://github.com/android/kotlin/issues/136
// TODO remove once that bug is fixed
return if (primary?.simpleName?.asString() != "<init>") {
primary
} else {
null
}
}

private fun KSClassDeclaration.declaredConstructors() = this.getDeclaredFunctions()
.filter {
it.simpleName == this.simpleName
}.distinct() // workaround for: https://github.com/google/ksp/issues/99
}
68 changes: 68 additions & 0 deletions ...in/java/androidx/room/compiler/processing/ksp/synthetic/KspSyntheticConstructorForJava.kt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* Copyright 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package androidx.room.compiler.processing.ksp.synthetic

import androidx.room.compiler.processing.XAnnotated
import androidx.room.compiler.processing.XConstructorElement
import androidx.room.compiler.processing.XExecutableParameterElement
import androidx.room.compiler.processing.XTypeElement
import androidx.room.compiler.processing.ksp.KspAnnotated
import androidx.room.compiler.processing.ksp.KspProcessingEnv
import androidx.room.compiler.processing.ksp.KspTypeElement

/**
* KSP does not create any constructor for java classes that do not have explicit constructor so
* we synthesize one.
*
* see: https://github.com/google/ksp/issues/98
*/
internal class KspSyntheticConstructorForJava(
val env: KspProcessingEnv,
val origin: KspTypeElement
) : XConstructorElement,
XAnnotated by KspAnnotated.create(
env = env,
delegate = null, // there is no way to annotate this synthetic in kotlin
filter = KspAnnotated.UseSiteFilter.NO_USE_SITE
) {

override val enclosingTypeElement: XTypeElement
get() = origin

override val parameters: List<XExecutableParameterElement>
get() = emptyList()

override fun isVarArgs() = false

override fun isPublic() = origin.isPublic()

override fun isProtected() = origin.isProtected()

override fun isAbstract() = false

override fun isPrivate() = origin.isPrivate()

override fun isStatic() = false

override fun isTransient() = false

override fun isFinal() = origin.isFinal()

override fun kindName(): String {
return "synthetic java constructor"
}
}
10 changes: 6 additions & 4 deletions ...compiler-processing/src/test/java/androidx/room/compiler/processing/XProcessingEnvTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package androidx.room.compiler.processing

import androidx.room.compiler.processing.util.Source
import androidx.room.compiler.processing.util.runProcessorTest
import androidx.room.compiler.processing.util.runProcessorTestIncludingKsp
import com.google.common.truth.Truth.assertThat
import com.squareup.javapoet.ClassName
import com.squareup.javapoet.TypeName
Expand All @@ -29,7 +30,7 @@ import org.junit.runners.JUnit4
class XProcessingEnvTest {
@Test
fun getElement() {
runProcessorTest(
runProcessorTestIncludingKsp(
listOf(
Source.java(
"foo.bar.Baz", """
Expand Down Expand Up @@ -91,7 +92,7 @@ class XProcessingEnvTest {

@Test
fun basic() {
runProcessorTest(
runProcessorTestIncludingKsp(
listOf(
Source.java(
"foo.bar.Baz", """
Expand All @@ -111,11 +112,12 @@ class XProcessingEnvTest {
assertThat(element.name).isEqualTo("Baz")
assertThat(element.asDeclaredType().typeName)
.isEqualTo(ClassName.get("foo.bar", "Baz"))
assertThat(element.findPrimaryConstructor()).isNull()
assertThat(element.getConstructors()).hasSize(1)
assertThat(element.getDeclaredMethods()).hasSize(2)
assertThat(element.kindName()).isEqualTo("class")
assertThat(element.isInterface()).isFalse()
assertThat(element.superType?.typeName).isEqualTo(TypeName.OBJECT)
assertThat(element.superType?.typeName).isEqualTo(it.types.objectOrAny)
}
}

Expand Down Expand Up @@ -148,7 +150,7 @@ class XProcessingEnvTest {
}
}
""".trimIndent())
runProcessorTest(sources = listOf(src)) {
runProcessorTestIncludingKsp(sources = listOf(src)) {
it.processingEnv.requireTypeElement("foo.bar.Outer.Inner").let {
val className = it.className
assertThat(className.packageName()).isEqualTo("foo.bar")
Expand Down
111 changes: 105 additions & 6 deletions ...iler-processing/src/test/java/androidx/room/compiler/processing/ksp/KspTypeElementTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ 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.runProcessorTestIncludingKsp
import com.google.common.truth.Truth
import com.google.common.truth.Truth.assertThat
import com.squareup.javapoet.ClassName
import com.squareup.javapoet.ParameterizedTypeName
Expand Down Expand Up @@ -460,6 +462,7 @@ class KspTypeElementTest {
val src = Source.kotlin(
"Foo.kt", """
interface MyInterface
class NoExplicitConstructor
open class Base(x:Int)
open class ExplicitConstructor {
constructor(x:Int)
Expand All @@ -472,23 +475,50 @@ class KspTypeElementTest {
constructor(list:List<String>): this()
constructor(list:List<String>, x:Int): this()
}
abstract class AbstractNoExplicit
abstract class AbstractExplicit(x:Int)
""".trimIndent()
)
runKspTest(sources = listOf(src), succeed = true) { invocation ->
val constructorCounts = listOf(
"MyInterface", "Base", "ExplicitConstructor", "BaseWithSecondary", "Sub",
"SubWith3Constructors"
).map {
runProcessorTestIncludingKsp(sources = listOf(src)) { invocation ->
val subjects = listOf(
"MyInterface", "NoExplicitConstructor", "Base", "ExplicitConstructor",
"BaseWithSecondary", "Sub", "SubWith3Constructors",
"AbstractNoExplicit", "AbstractExplicit"
)
val constructorCounts = subjects.map {
it to invocation.processingEnv.requireTypeElement(it).getConstructors().size
}
assertThat(constructorCounts)
.containsExactly(
"MyInterface" to 0,
"NoExplicitConstructor" to 1,
"Base" to 1,
"ExplicitConstructor" to 1,
"BaseWithSecondary" to 2,
"Sub" to 1,
"SubWith3Constructors" to 3
"SubWith3Constructors" to 3,
"AbstractNoExplicit" to 1,
"AbstractExplicit" to 1
)

val primaryConstructorParameterNames = subjects.map {
it to invocation.processingEnv.requireTypeElement(it)
.findPrimaryConstructor()
?.parameters?.map {
it.name
}
}
assertThat(primaryConstructorParameterNames)
.containsExactly(
"MyInterface" to null,
"NoExplicitConstructor" to emptyList<String>(),
"Base" to listOf("x"),
"ExplicitConstructor" to null,
"BaseWithSecondary" to listOf("x"),
"Sub" to listOf("x"),
"SubWith3Constructors" to emptyList<String>(),
"AbstractNoExplicit" to emptyList<String>(),
"AbstractExplicit" to listOf("x")
)
}
}
Expand All @@ -509,6 +539,75 @@ class KspTypeElementTest {
}
}

@Test
fun constructors_java() {
val src = Source.java(
"Source", """
import java.util.List;
interface MyInterface {}
class NoExplicitConstructor{}
class Base {
Base(int x){}
}
class ExplicitConstructor {
ExplicitConstructor(int x){}
}
class BaseWithSecondary {
BaseWithSecondary(int x){}
BaseWithSecondary(String y){}
}
class Sub extends Base {
Sub(int x) {
super(x);
}
}
class SubWith3Constructors extends BaseWithSecondary {
SubWith3Constructors() {
super(3);
}
SubWith3Constructors(List<String> list) {
super(3);
}
SubWith3Constructors(List<String> list, int x) {
super(3);
}
}
abstract class AbstractNoExplicit {}
abstract class AbstractExplicit {
AbstractExplicit(int x) {}
}
""".trimIndent()
)
runProcessorTestIncludingKsp(sources = listOf(src)) { invocation ->
val subjects = listOf(
"MyInterface", "NoExplicitConstructor", "Base", "ExplicitConstructor",
"BaseWithSecondary", "Sub", "SubWith3Constructors",
"AbstractNoExplicit", "AbstractExplicit"
)
val constructorCounts = subjects.map {
it to invocation.processingEnv.requireTypeElement(it).getConstructors().size
}
assertThat(constructorCounts)
.containsExactly(
"MyInterface" to 0,
"NoExplicitConstructor" to 1,
"Base" to 1,
"ExplicitConstructor" to 1,
"BaseWithSecondary" to 2,
"Sub" to 1,
"SubWith3Constructors" to 3,
"AbstractNoExplicit" to 1,
"AbstractExplicit" to 1
)

subjects.forEach {
Truth.assertWithMessage(it)
.that(invocation.processingEnv.requireTypeElement(it).findPrimaryConstructor())
.isNull()
}
}
}

private fun List<XMethodElement>.names() = map {
it.name
}
Expand Down

0 comments on commit b45f9cc

Please sign in to comment.