Use TypeItem for TypeParameterBounds
This CL changes TypeParameterItem's bounds type to TypeItem from
ClassItem. Technically, type bounds are not classes, they are "type
references" which seems to be mostly represented as types in metalava.
Due to downstream branches, I had to use a new name (typeBounds) and
deprecated the old bounds() function.
I also noticed that ApiLint was doing a primitive bounds check relying
on these being classes. I wrote an alternative implementation when PSI
is present to use type assignability instead. For text based types, it
is harder to implement assignability so I left the code there as
fallback for text based types.
This change also found a false negative in Compatibility check tests
where it was asserting an error due to a type parameter name change even
though the types are still equal.
CompatibilityChecker still uses class to check for bounds right now.
I've not changed it since one of the TypeItem's there is text based,
hence implementing extends is tricky there :(
Finally, TextTypeParameterItems's bounds method was dropping type
arguments, which broke platform build (it was OK when these were classes
since classes don't have arguments, only parameter types). I tried to
properly fix it but it was getting too complicated so I rather patched
the bounds method to include the type arguments. Added a test to
validate.
Bug: 221963837
Test: PsiTypeItemAssignabilityTest, CompatibilityCheckTest
Change-Id: I28e9e53da9ef5c3d038baeaaf2cdcb8a26f6ab9c
diff --git a/src/main/java/com/android/tools/metalava/ApiLint.kt b/src/main/java/com/android/tools/metalava/ApiLint.kt
index 7586d57..624945d 100644
--- a/src/main/java/com/android/tools/metalava/ApiLint.kt
+++ b/src/main/java/com/android/tools/metalava/ApiLint.kt
@@ -154,6 +154,7 @@
import com.android.tools.metalava.model.SetMinSdkVersion
import com.android.tools.metalava.model.TypeItem
import com.android.tools.metalava.model.psi.PsiMethodItem
+import com.android.tools.metalava.model.psi.PsiTypeItem
import com.android.tools.metalava.model.visitors.ApiVisitor
import com.intellij.psi.JavaRecursiveElementVisitor
import com.intellij.psi.PsiClassObjectAccessExpression
@@ -1039,7 +1040,7 @@
// Maps each setter to a list of potential getters that would satisfy it.
val expectedGetters = mutableListOf<Pair<Item, Set<String>>>()
var builtType: TypeItem? = null
- val clsType = cls.toType().toTypeString()
+ val clsType = cls.toType()
for (method in methods) {
val name = method.name()
@@ -1052,17 +1053,30 @@
"Getter should be on the built object, not the builder: ${method.describe()}"
)
} else if (name.startsWith("set") || name.startsWith("add") || name.startsWith("clear")) {
- val returnType = method.returnType()?.toTypeString() ?: ""
- val returnTypeBounds = method.returnType()?.asTypeParameter(context = method)?.bounds()?.map {
- it.toType().toTypeString()
- } ?: listOf()
-
- if (returnType != clsType && !returnTypeBounds.contains(clsType)) {
- report(
- SETTER_RETURNS_THIS, method,
- "Methods must return the builder object (return type $clsType instead of $returnType): ${method.describe()}"
- )
+ val returnType = method.returnType()
+ if (returnType != null) {
+ val returnsClassType = if (
+ returnType is PsiTypeItem && clsType is PsiTypeItem
+ ) {
+ clsType.isAssignableFromWithoutUnboxing(returnType)
+ } else {
+ // fallback to a limited text based check
+ val returnTypeBounds = returnType
+ .asTypeParameter(context = method)
+ ?.typeBounds()?.map {
+ it.toTypeString()
+ } ?: emptyList()
+ returnTypeBounds.contains(clsType.toTypeString()) || returnType == clsType
+ }
+ if (!returnsClassType) {
+ report(
+ SETTER_RETURNS_THIS, method,
+ "Methods must return the builder object (return type " +
+ "$clsType instead of $returnType): ${method.describe()}"
+ )
+ }
}
+
if (method.modifiers.isNullable()) {
report(
SETTER_RETURNS_THIS, method,
diff --git a/src/main/java/com/android/tools/metalava/CompatibilityCheck.kt b/src/main/java/com/android/tools/metalava/CompatibilityCheck.kt
index a6bdeda..119c545 100644
--- a/src/main/java/com/android/tools/metalava/CompatibilityCheck.kt
+++ b/src/main/java/com/android/tools/metalava/CompatibilityCheck.kt
@@ -368,10 +368,11 @@
compatible = false
}
} else if (oldTypeParameter == null && newTypeParameter != null) {
- val constraints = newTypeParameter.bounds()
+ val constraints = newTypeParameter.typeBounds()
for (constraint in constraints) {
val oldClass = oldReturnType.asClass()
- if (oldClass == null || !oldClass.extendsOrImplements(constraint.qualifiedName())) {
+ val newClass = constraint.asClass()
+ if (oldClass == null || newClass == null || !oldClass.extendsOrImplements(newClass.qualifiedName())) {
compatible = false
}
}
@@ -382,8 +383,8 @@
} else {
// If both return types are parameterized then the constraints must be
// exactly the same.
- val oldConstraints = oldTypeParameter?.bounds() ?: emptyList()
- val newConstraints = newTypeParameter?.bounds() ?: emptyList()
+ val oldConstraints = oldTypeParameter?.typeBounds() ?: emptyList()
+ val newConstraints = newTypeParameter?.typeBounds() ?: emptyList()
if (oldConstraints.size != newConstraints.size ||
newConstraints != oldConstraints
) {
@@ -599,13 +600,13 @@
private fun describeBounds(
type: TypeItem,
- constraints: List<ClassItem>
+ constraints: List<TypeItem>
): String {
return type.toSimpleType() +
if (constraints.isEmpty()) {
" (extends java.lang.Object)"
} else {
- " (extends ${constraints.joinToString(separator = " & ") { it.qualifiedName() }})"
+ " (extends ${constraints.joinToString(separator = " & ") { it.toTypeString() }})"
}
}
diff --git a/src/main/java/com/android/tools/metalava/model/TypeParameterItem.kt b/src/main/java/com/android/tools/metalava/model/TypeParameterItem.kt
index 5cb8524..036dd79 100644
--- a/src/main/java/com/android/tools/metalava/model/TypeParameterItem.kt
+++ b/src/main/java/com/android/tools/metalava/model/TypeParameterItem.kt
@@ -17,6 +17,14 @@
package com.android.tools.metalava.model
interface TypeParameterItem : ClassItem {
- fun bounds(): List<ClassItem>
+ @Deprecated(
+ message = "Please use typeBounds() instead.",
+ level = DeprecationLevel.ERROR,
+ replaceWith = ReplaceWith("typeBounds().mapNotNull { it.asClass() }")
+ )
+ fun bounds(): List<ClassItem> = typeBounds().mapNotNull {
+ it.asClass()
+ }
+ fun typeBounds(): List<TypeItem>
fun isReified(): Boolean
}
diff --git a/src/main/java/com/android/tools/metalava/model/psi/PsiTypeItem.kt b/src/main/java/com/android/tools/metalava/model/psi/PsiTypeItem.kt
index af49410..23086e3 100644
--- a/src/main/java/com/android/tools/metalava/model/psi/PsiTypeItem.kt
+++ b/src/main/java/com/android/tools/metalava/model/psi/PsiTypeItem.kt
@@ -342,6 +342,16 @@
toInnerAnnotatedString = toAnnotatedString
}
+ /**
+ * Returns `true` if `this` type can be assigned from `other` without unboxing the other.
+ */
+ fun isAssignableFromWithoutUnboxing(other: PsiTypeItem): Boolean {
+ if (this.primitive && !other.primitive) {
+ return false
+ }
+ return TypeConversionUtil.isAssignable(psiType, other.psiType)
+ }
+
companion object {
private fun getPrimitiveSignature(typeName: String): String? = when (typeName) {
"boolean" -> "Z"
diff --git a/src/main/java/com/android/tools/metalava/model/psi/PsiTypeParameterItem.kt b/src/main/java/com/android/tools/metalava/model/psi/PsiTypeParameterItem.kt
index 019eebe..2eb2229 100644
--- a/src/main/java/com/android/tools/metalava/model/psi/PsiTypeParameterItem.kt
+++ b/src/main/java/com/android/tools/metalava/model/psi/PsiTypeParameterItem.kt
@@ -16,7 +16,6 @@
package com.android.tools.metalava.model.psi
-import com.android.tools.metalava.model.ClassItem
import com.android.tools.metalava.model.TypeParameterItem
import com.android.tools.metalava.model.psi.ClassType.TYPE_PARAMETER
import com.intellij.psi.PsiTypeParameter
@@ -42,13 +41,13 @@
fromClassPath = false
),
TypeParameterItem {
- override fun bounds(): List<ClassItem> = bounds
+ override fun typeBounds(): List<PsiTypeItem> = bounds
override fun isReified(): Boolean {
return isReified(element as? PsiTypeParameter)
}
- private lateinit var bounds: List<ClassItem>
+ private lateinit var bounds: List<PsiTypeItem>
override fun finishInitialization() {
super.finishInitialization()
@@ -57,7 +56,7 @@
bounds = if (refs != null && refs.isNotEmpty()) {
// Omit java.lang.Object since PSI will turn "T extends Comparable" to "T extends Object & Comparable"
// and this just makes comparisons harder; *everything* extends Object.
- refs.mapNotNull { PsiTypeItem.create(codebase, it).asClass() }.filter { !it.isJavaLangObject() }
+ refs.mapNotNull { PsiTypeItem.create(codebase, it) }.filter { !it.isJavaLangObject() }
} else {
emptyList()
}
diff --git a/src/main/java/com/android/tools/metalava/model/text/TextTypeItem.kt b/src/main/java/com/android/tools/metalava/model/text/TextTypeItem.kt
index dd65021..0ba6abc 100644
--- a/src/main/java/com/android/tools/metalava/model/text/TextTypeItem.kt
+++ b/src/main/java/com/android/tools/metalava/model/text/TextTypeItem.kt
@@ -141,10 +141,10 @@
return dimensions
}
- private fun findTypeVariableBounds(typeParameterList: TypeParameterList, name: String): List<ClassItem> {
+ private fun findTypeVariableBounds(typeParameterList: TypeParameterList, name: String): List<TypeItem> {
for (p in typeParameterList.typeParameters()) {
if (p.simpleName() == name) {
- val bounds = p.bounds()
+ val bounds = p.typeBounds()
if (bounds.isNotEmpty()) {
return bounds
}
@@ -154,7 +154,7 @@
return emptyList()
}
- private fun findTypeVariableBounds(context: Item?, name: String): List<ClassItem> {
+ private fun findTypeVariableBounds(context: Item?, name: String): List<TypeItem> {
if (context is MethodItem) {
val bounds = findTypeVariableBounds(context.typeParameterList(), name)
if (bounds.isNotEmpty()) {
@@ -173,7 +173,7 @@
val typeParameter =
TextTypeParameterItem.create(codebase, context as? TypeParameterListOwner, toTypeString())
- if (context != null && typeParameter.bounds().isEmpty()) {
+ if (context != null && typeParameter.typeBounds().isEmpty()) {
val bounds = findTypeVariableBounds(context, typeParameter.simpleName())
if (bounds.isNotEmpty()) {
val filtered = bounds.filter { !it.isJavaLangObject() }
@@ -239,7 +239,7 @@
): String {
return if (erased) {
val raw = eraseTypeArguments(type)
- val concrete = substituteTypeParameters(raw, context)
+ val concrete = eraseTypeArguments(substituteTypeParameters(raw, context))
if (outerAnnotations && innerAnnotations) {
concrete
} else {
@@ -264,9 +264,9 @@
val v = s.substring(0, end)
val parameter = context.resolveParameter(v)
if (parameter != null) {
- val bounds = parameter.bounds()
+ val bounds = parameter.typeBounds()
if (bounds.isNotEmpty()) {
- return bounds.first().qualifiedName() + s.substring(end)
+ return bounds.first().toTypeString() + s.substring(end)
}
@Suppress("ConstantConditionIf")
if (ASSUME_TYPE_VARS_EXTEND_OBJECT) {
diff --git a/src/main/java/com/android/tools/metalava/model/text/TextTypeParameterItem.kt b/src/main/java/com/android/tools/metalava/model/text/TextTypeParameterItem.kt
index 17bd71c..11de657 100644
--- a/src/main/java/com/android/tools/metalava/model/text/TextTypeParameterItem.kt
+++ b/src/main/java/com/android/tools/metalava/model/text/TextTypeParameterItem.kt
@@ -16,8 +16,8 @@
package com.android.tools.metalava.model.text
-import com.android.tools.metalava.model.ClassItem
import com.android.tools.metalava.model.DefaultModifierList
+import com.android.tools.metalava.model.TypeItem
import com.android.tools.metalava.model.TypeParameterItem
import com.android.tools.metalava.model.TypeParameterListOwner
@@ -26,7 +26,7 @@
private val owner: TypeParameterListOwner?,
private val typeParameterString: String,
name: String,
- private var bounds: List<ClassItem>? = null
+ private var bounds: List<TypeItem>? = null
) :
TextClassItem(
codebase = codebase,
@@ -36,19 +36,14 @@
),
TypeParameterItem {
- override fun bounds(): List<ClassItem> {
+ override fun typeBounds(): List<TypeItem> {
if (bounds == null) {
val boundsString = bounds(typeParameterString, owner)
bounds = if (boundsString.isEmpty()) {
emptyList()
} else {
boundsString.mapNotNull {
- val clz = codebase.findClass(it)
- if (clz == null && it.contains(".")) {
- codebase.getOrCreateClass(it)
- } else {
- clz
- }
+ codebase.obtainTypeFromString(it)
}.filter {
!it.isJavaLangObject()
}
@@ -66,7 +61,7 @@
codebase: TextCodebase,
owner: TypeParameterListOwner?,
typeParameterString: String,
- bounds: List<ClassItem>? = null
+ bounds: List<TypeItem>? = null
): TextTypeParameterItem {
val length = typeParameterString.length
var nameEnd = length
@@ -96,29 +91,27 @@
?: return emptyList()
for (p in parameters) {
if (p.simpleName() == s) {
- return p.bounds().filter { !it.isJavaLangObject() }.map { it.qualifiedName() }
+ return p.typeBounds().filter { !it.isJavaLangObject() }.map { it.toTypeString() }
}
}
return emptyList()
}
val list = mutableListOf<String>()
- var balance = 0
+ var angleBracketBalance = 0
var start = index + "extends ".length
val length = s.length
for (i in start until length) {
val c = s[i]
- if (c == '&' && balance == 0) {
+ if (c == '&' && angleBracketBalance == 0) {
add(list, typeString, start, i)
start = i + 1
} else if (c == '<') {
- balance++
- if (balance == 1) {
- add(list, typeString, start, i)
- }
+ angleBracketBalance++
} else if (c == '>') {
- balance--
- if (balance == 0) {
+ angleBracketBalance--
+ if (angleBracketBalance == 0) {
+ add(list, typeString, start, i + 1)
start = i + 1
}
}
diff --git a/src/test/java/com/android/tools/metalava/CompatibilityCheckTest.kt b/src/test/java/com/android/tools/metalava/CompatibilityCheckTest.kt
index 678dea6..eccb26c 100644
--- a/src/test/java/com/android/tools/metalava/CompatibilityCheckTest.kt
+++ b/src/test/java/com/android/tools/metalava/CompatibilityCheckTest.kt
@@ -1328,7 +1328,6 @@
src/test/pkg/MyClass.java:7: error: Method test.pkg.MyClass.method3 has changed return type from java.util.List<Integer> to java.util.List<java.lang.Number> [ChangedType]
src/test/pkg/MyClass.java:8: error: Method test.pkg.MyClass.method4 has changed return type from String to String[] [ChangedType]
src/test/pkg/MyClass.java:9: error: Method test.pkg.MyClass.method5 has changed return type from String[] to String[][] [ChangedType]
- src/test/pkg/MyClass.java:10: error: Method test.pkg.MyClass.method6 has changed return type from T (extends java.lang.Object) to U (extends java.lang.Number) [ChangedType]
src/test/pkg/MyClass.java:11: error: Method test.pkg.MyClass.method7 has changed return type from T to Number [ChangedType]
src/test/pkg/MyClass.java:13: error: Method test.pkg.MyClass.method9 has changed return type from X (extends java.lang.Throwable) to U (extends java.lang.Number) [ChangedType]
""",
@@ -3850,6 +3849,32 @@
)
}
+ @Test
+ fun `unchanged self-referencing type parameter is compatible`() {
+ check(
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public abstract class Foo<T extends test.pkg.Foo<T>> {
+ method public static <T extends test.pkg.Foo<T>> T valueOf(Class<T>, String);
+ }
+ }
+ """,
+ sourceFiles = arrayOf(
+ java(
+ """
+ package test.pkg;
+ import android.annotation.NonNull;
+ public abstract class Foo<T extends Foo<T>> {
+ @NonNull
+ public static <T extends Foo<T>> T valueOf(@NonNull Class<T> fooType, @NonNull String name) {}
+ }
+ """
+ ),
+ nonNullSource
+ )
+ )
+ }
+
// TODO: Check method signatures changing incompatibly (look especially out for adding new overloaded
// methods and comparator getting confused!)
// ..equals on the method items should actually be very useful!
diff --git a/src/test/java/com/android/tools/metalava/model/PsiTypeItemAssignabilityTest.kt b/src/test/java/com/android/tools/metalava/model/PsiTypeItemAssignabilityTest.kt
new file mode 100644
index 0000000..08bad44
--- /dev/null
+++ b/src/test/java/com/android/tools/metalava/model/PsiTypeItemAssignabilityTest.kt
@@ -0,0 +1,159 @@
+/*
+ * Copyright (C) 2022 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 com.android.tools.metalava.model
+
+import com.android.tools.metalava.java
+import com.android.tools.metalava.kotlin
+import com.android.tools.metalava.model.psi.PsiTypeItem
+import com.android.tools.metalava.model.psi.testCodebase
+import com.google.common.truth.Truth.assertThat
+import org.junit.Rule
+import org.junit.Test
+import org.junit.rules.TemporaryFolder
+
+class PsiTypeItemAssignabilityTest {
+ @get:Rule
+ val tmpFolder = TemporaryFolder()
+
+ @Test
+ fun `Assignability in PSI`() {
+ val sourceFiles = arrayOf(
+ // pass in the same class structure in kotlin and java.
+ java(
+ """
+ package test.foo;
+ import java.util.*;
+ public class JavaSubject {
+ public Object obj;
+ public String string;
+ public int primitiveInt;
+ public Number number;
+ public Integer boxedInt;
+ public List<Integer> listOfInt;
+ public List<Number> listOfNumber;
+ public Map<Integer, String> mapOfIntToString;
+ public Map<Number, String> mapOfNumberToString;
+ }
+ """
+ ),
+ kotlin(
+ """
+ package test.foo;
+ class KotlinSubject {
+ @JvmField
+ var obj: Any? = null
+ @JvmField
+ var string: String? = null
+ @JvmField
+ var primitiveInt = 0
+ @JvmField
+ var number: Number? = null
+ @JvmField
+ var boxedInt: Int? = null
+ @JvmField
+ var listOfInt: MutableList<Int>? = null
+ @JvmField
+ var listOfNumber: MutableList<Number>? = null
+ @JvmField
+ var mapOfIntToString: MutableMap<Int, String>? = null
+ @JvmField
+ var mapOfNumberToString: MutableMap<Number, String>? = null
+ }
+ """
+ )
+ )
+
+ testCodebase(
+ sources = sourceFiles
+ ) { codebase ->
+ val javaSubject = codebase.findClass("test.foo.JavaSubject")
+ ?: error("Cannot find java subject")
+ val kotlinSubject = codebase.findClass("test.foo.KotlinSubject")
+ ?: error("Cannot find subject")
+ val testSubjects = listOf(javaSubject, kotlinSubject)
+ // helper method to check assignability between fields
+ fun String.isAssignableFromWithoutUnboxing(
+ otherField: String
+ ): Boolean {
+ val results = testSubjects.map { subject ->
+ val field1Type = checkNotNull(
+ subject.findField(this)?.type() as? PsiTypeItem
+ ) {
+ "cannot find $this in $subject"
+ }
+ val field2Type = checkNotNull(
+ subject.findField(otherField)?.type() as? PsiTypeItem
+ ) {
+ "cannot find $otherField in $subject"
+ }
+ field1Type.isAssignableFromWithoutUnboxing(field2Type)
+ }
+ check(results.toSet().size == 1) {
+ "isAssignable check for $this to $otherField returned inconsistent results " +
+ "between kotlin and java: $results"
+ }
+ return results.first()
+ }
+
+ assertThat(
+ "string".isAssignableFromWithoutUnboxing("string")
+ ).isTrue()
+ assertThat(
+ "obj".isAssignableFromWithoutUnboxing("string")
+ ).isTrue()
+ assertThat(
+ "string".isAssignableFromWithoutUnboxing("obj")
+ ).isFalse()
+ assertThat(
+ "primitiveInt".isAssignableFromWithoutUnboxing("number")
+ ).isFalse()
+ assertThat(
+ "number".isAssignableFromWithoutUnboxing("primitiveInt")
+ ).isTrue()
+ assertThat(
+ "boxedInt".isAssignableFromWithoutUnboxing("primitiveInt")
+ ).isTrue()
+ assertThat(
+ "primitiveInt".isAssignableFromWithoutUnboxing("boxedInt")
+ ).isFalse()
+ assertThat(
+ "number".isAssignableFromWithoutUnboxing("boxedInt")
+ ).isTrue()
+ assertThat(
+ "boxedInt".isAssignableFromWithoutUnboxing("number")
+ ).isFalse()
+ assertThat(
+ "listOfInt".isAssignableFromWithoutUnboxing("listOfInt")
+ ).isTrue()
+ assertThat(
+ "listOfInt".isAssignableFromWithoutUnboxing("listOfNumber")
+ ).isFalse()
+ assertThat(
+ "listOfNumber".isAssignableFromWithoutUnboxing("listOfInt")
+ ).isFalse()
+ assertThat(
+ "mapOfNumberToString".isAssignableFromWithoutUnboxing("mapOfNumberToString")
+ ).isTrue()
+ assertThat(
+ "mapOfNumberToString".isAssignableFromWithoutUnboxing("mapOfIntToString")
+ ).isFalse()
+ assertThat(
+ "mapOfIntToString".isAssignableFromWithoutUnboxing("mapOfNumberToString")
+ ).isFalse()
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/metalava/model/text/TextTypeParameterItemTest.kt b/src/test/java/com/android/tools/metalava/model/text/TextTypeParameterItemTest.kt
index 6b13ce0..ae2ebeb 100644
--- a/src/test/java/com/android/tools/metalava/model/text/TextTypeParameterItemTest.kt
+++ b/src/test/java/com/android/tools/metalava/model/text/TextTypeParameterItemTest.kt
@@ -28,9 +28,9 @@
assertThat(bounds("X").toString()).isEqualTo("[]")
assertThat(bounds("DEF extends T").toString()).isEqualTo("[T]")
assertThat(bounds("T extends java.lang.Comparable<? super T>").toString())
- .isEqualTo("[java.lang.Comparable]")
+ .isEqualTo("[java.lang.Comparable<? super T>]")
assertThat(bounds("T extends java.util.List<Number> & java.util.RandomAccess").toString())
- .isEqualTo("[java.util.List, java.util.RandomAccess]")
+ .isEqualTo("[java.util.List<Number>, java.util.RandomAccess]")
// When a type variable is on a member and the type variable is defined on the surrounding
// class, look up the bound on the class type parameter: