Use TypeItem for TypeParameterBounds am: c0bc120695 am: 1757065fdd am: ba55d4f244
Original change: https://android-review.googlesource.com/c/platform/tools/metalava/+/1999454
Change-Id: Id70d9bd3bba1c2a86c054a4109187ad08e7ad209
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: