Skip to content

Commit

Permalink
Fix ClassCastException in XType.getTypeElement().className.
Browse files Browse the repository at this point in the history
This CL fixes a bug where calling `XType.getTypeElement().className`
results in a `ClassCastException` when called on an array type in
KSP. The underlying issue is that `XType.getTypeElement()` gets
tricked into thinking there's an actual `TypeElement` for an array
type since it has a declaration, `Array` which it thinks is the
appropriate `TypeElement`, which it's not.

This leads to two issues.

  1. Since an array type isn't actually represented as a `ClassName`,
     we get a `ClassCastException` when trying to cast it.
  2. It leads to inconsistencies when compiling in Javac versus KSP.

For the same consistency reason as #2, I've also changed the
implementation for primitive types to return null type element in
KSP since this matches the Javac behavior where there is no valid
ClassName for a primitive type.

Test: XTypeTest
Change-Id: I7e5d8856115d99e666e487c52cc5d9f538149cae
  • Loading branch information
bcorso committed Aug 9, 2022
1 parent 2df05b0 commit 092f2de
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 54 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package androidx.room.compiler.processing.ksp
import androidx.room.compiler.processing.XEquality
import androidx.room.compiler.processing.XNullability
import androidx.room.compiler.processing.XType
import androidx.room.compiler.processing.isArray
import androidx.room.compiler.processing.tryBox
import androidx.room.compiler.processing.tryUnbox
import com.google.devtools.ksp.symbol.KSClassDeclaration
Expand Down Expand Up @@ -84,9 +85,18 @@ internal abstract class KspType(
}

override val typeElement by lazy {
// for primitive types, we could technically return null from here as they are not backed
// by a type element in javac but in Kotlin we have types for them, hence returning them
// is better.
// Array types don't have an associated type element (only the componentType does), so
// return null.
if (isArray()) {
return@lazy null
}

// If the typeName is primitive, return null for consistency since primitives normally imply
// that there isn't an associated type element.
if (typeName.isPrimitive) {
return@lazy null
}

val declaration = ksType.declaration as? KSClassDeclaration
declaration?.let {
env.wrapClassDeclaration(it)
Expand Down
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.ksp.ERROR_TYPE_NAME
import androidx.room.compiler.processing.util.Source
import androidx.room.compiler.processing.util.XTestInvocation
import androidx.room.compiler.processing.util.className
import androidx.room.compiler.processing.util.getDeclaredMethodByJvmName
import androidx.room.compiler.processing.util.getField
Expand Down Expand Up @@ -830,6 +831,76 @@ class XTypeTest {
}
}

@Test
fun arrayTypes() {
// Used to test both java and kotlin sources.
fun XTestInvocation.checkArrayTypesTest() {
val fooElement = processingEnv.requireTypeElement("foo.bar.Foo")
val barElement = processingEnv.requireTypeElement("foo.bar.Bar")
val barType = fooElement.getField("bar").type
val barArrayType = fooElement.getField("barArray").type

assertThat(barType.typeElement).isEqualTo(barElement)
assertThat(barType.isArray()).isFalse()
assertThat(barArrayType.typeElement).isNull()
assertThat(barArrayType.isArray()).isTrue()
}

runProcessorTest(listOf(Source.java(
"foo.bar.Foo",
"""
package foo.bar;
class Foo {
Bar bar;
Bar[] barArray;
}
class Bar {}
""".trimIndent()
))) { it.checkArrayTypesTest() }

runProcessorTest(listOf(Source.kotlin(
"foo.bar.Foo.kt",
"""
package foo.bar;
class Foo {
val bar: Bar = TODO()
val barArray: Array<Bar> = TODO()
}
class Bar
""".trimIndent()
))) { it.checkArrayTypesTest() }
}

@Test
fun primitiveTypes() {
fun XTestInvocation.checkPrimitiveType() {
val fooElement = processingEnv.requireTypeElement("foo.bar.Foo")
val primitiveType = fooElement.getField("i").type
assertThat(primitiveType.typeName).isEqualTo(TypeName.INT)
assertThat(primitiveType.typeElement).isNull()
}

runProcessorTest(listOf(Source.java(
"foo.bar.Foo",
"""
package foo.bar;
class Foo {
int i;
}
""".trimIndent()
))) { it.checkPrimitiveType() }

runProcessorTest(listOf(Source.kotlin(
"foo.bar.Foo.kt",
"""
package foo.bar
class Foo {
val i: Int = TODO()
}
""".trimIndent()
))) { it.checkPrimitiveType() }
}

/**
* Dumps the typename with its bounds in a given depth.
* This makes tests more readable.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1155,19 +1155,9 @@ class DatabaseProcessorTest {
baseContext = invocation.context,
element = element
).process()
assertThat(result.daoMethods).hasSize(
// for KSP, it will still show as a method, just bad return type
if (invocation.isKsp) 1 else 0
)
assertThat(result.daoMethods).hasSize(0)
invocation.assertCompilationResult {
hasErrorContaining(
if (invocation.isKsp) {
// no primitives in KSP hence we'll get another error
ProcessorErrors.DAO_MUST_BE_ANNOTATED_WITH_DAO
} else {
ProcessorErrors.DATABASE_INVALID_DAO_METHOD_RETURN_TYPE
}
)
hasErrorContaining(ProcessorErrors.DATABASE_INVALID_DAO_METHOD_RETURN_TYPE)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -582,20 +582,11 @@ abstract class DeleteOrUpdateShortcutMethodProcessorTest<out T : DeleteOrUpdateS
""",
) { _, invocation ->
invocation.assertCompilationResult {
if (invocation.isKsp) {
hasErrorContaining(
ProcessorErrors.noColumnsInPartialEntity(
"java.lang.Long"
)
)
} else {
// javac has a different error for primitives.
hasErrorContaining(
ProcessorErrors.shortcutMethodArgumentMustBeAClass(
TypeName.LONG
)
hasErrorContaining(
ProcessorErrors.shortcutMethodArgumentMustBeAClass(
TypeName.LONG
)
}
)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -330,16 +330,8 @@ class PojoProcessorTest {
int embeddedPrimitive;
"""
) { _, invocation ->
if (invocation.isKsp) {
// there are no primitives in KSP so this won't work. Instead, it will fail
// because we cannot find a constructor for `int`
invocation.assertCompilationResult {
hasErrorContaining(MISSING_POJO_CONSTRUCTOR)
}
} else {
invocation.assertCompilationResult {
hasErrorContaining(ProcessorErrors.EMBEDDED_TYPES_MUST_BE_A_CLASS_OR_INTERFACE)
}
invocation.assertCompilationResult {
hasErrorContaining(ProcessorErrors.EMBEDDED_TYPES_MUST_BE_A_CLASS_OR_INTERFACE)
}
}
}
Expand Down Expand Up @@ -481,22 +473,8 @@ class PojoProcessorTest {
public long user;
"""
) { _, invocation ->
if (invocation.isKsp) {
// in KSP, there are no primitives so `long` (kotlin.Long) will still look like a
// class but then we'll fail because it doesn't hvae a `uid` column
invocation.assertCompilationResult {
hasErrorContaining(
relationCannotFindEntityField(
entityName = "java.lang.Long",
columnName = "uid",
availableColumns = emptyList()
)
)
}
} else {
invocation.assertCompilationResult {
hasErrorContaining(ProcessorErrors.RELATION_TYPE_MUST_BE_A_CLASS_OR_INTERFACE)
}
invocation.assertCompilationResult {
hasErrorContaining(ProcessorErrors.RELATION_TYPE_MUST_BE_A_CLASS_OR_INTERFACE)
}
}
}
Expand Down

0 comments on commit 092f2de

Please sign in to comment.