Check that return types are assignable instead of exact.
We now check that the new return type is assignable to the old
return type instead of just comparing the types as strings when
checking the consistency of two APIs.
Fixes:
https://android-review.googlesource.com/#/c/58843/
Change-Id: I6a18a76e5296d58c05f1bb59341cdaa820f0c540
diff --git a/src/com/google/doclava/ClassInfo.java b/src/com/google/doclava/ClassInfo.java
index 68bf9e6..fb98376 100644
--- a/src/com/google/doclava/ClassInfo.java
+++ b/src/com/google/doclava/ClassInfo.java
@@ -553,7 +553,15 @@
}
public void addAnnotationElement(MethodInfo method) {
- mAnnotationElements.add(method);
+ mAnnotationElements.add(method);
+ }
+
+ // Called by PackageInfo when a ClassInfo is added to a package.
+ // This is needed because ApiCheck uses PackageInfo.addClass
+ // rather than using setContainingPackage to dispatch to the
+ // appropriate method. TODO: move ApiCheck away from addClass.
+ void setPackage(PackageInfo pkg) {
+ mContainingPackage = pkg;
}
public void setContainingPackage(PackageInfo pkg) {
@@ -1487,34 +1495,41 @@
* Returns true if {@code cl} implements the interface {@code iface} either by either being that
* interface, implementing that interface or extending a type that implements the interface.
*/
- private boolean implementsInterface(ClassInfo cl, String iface) {
- if (cl.qualifiedName().equals(iface)) {
+ public boolean implementsInterface(String iface) {
+ if (qualifiedName().equals(iface)) {
return true;
}
- for (ClassInfo clImplements : cl.interfaces()) {
- if (implementsInterface(clImplements, iface)) {
+ for (ClassInfo clImplements : interfaces()) {
+ if (clImplements.implementsInterface(iface)) {
return true;
}
}
- if (cl.mSuperclass != null && implementsInterface(cl.mSuperclass, iface)) {
+ if (mSuperclass != null && mSuperclass.implementsInterface(iface)) {
return true;
}
return false;
}
/**
- * Returns true if {@code cl} extends the class {@code ext}.
+ * Returns true if {@code this} extends the class {@code ext}.
*/
- private boolean extendsClass(ClassInfo cl, String ext) {
- if (cl.qualifiedName().equals(ext)) {
+ public boolean extendsClass(String cl) {
+ if (qualifiedName().equals(cl)) {
return true;
}
- if (cl.mSuperclass != null && extendsClass(cl.mSuperclass, ext)) {
+ if (mSuperclass != null && mSuperclass.extendsClass(cl)) {
return true;
}
return false;
}
+ /**
+ * Returns true if {@code this} is assignable to cl
+ */
+ public boolean isAssignableTo(String cl) {
+ return implementsInterface(cl) || extendsClass(cl);
+ }
+
public void addInterface(ClassInfo iface) {
mRealInterfaces.add(iface);
}
@@ -1595,13 +1610,13 @@
consistent = false;
}
for (ClassInfo iface : mRealInterfaces) {
- if (!implementsInterface(cl, iface.mQualifiedName)) {
+ if (!cl.implementsInterface(iface.mQualifiedName)) {
Errors.error(Errors.REMOVED_INTERFACE, cl.position(), "Class " + qualifiedName()
+ " no longer implements " + iface);
}
}
for (ClassInfo iface : cl.mRealInterfaces) {
- if (!implementsInterface(this, iface.mQualifiedName)) {
+ if (!implementsInterface(iface.mQualifiedName)) {
Errors.error(Errors.ADDED_INTERFACE, cl.position(), "Added interface " + iface
+ " to class " + qualifiedName());
consistent = false;
@@ -1732,7 +1747,7 @@
}
if (superclassName() != null) { // java.lang.Object can't have a superclass.
- if (!extendsClass(cl, superclassName())) {
+ if (!cl.extendsClass(superclassName())) {
consistent = false;
Errors.error(Errors.CHANGED_SUPERCLASS, cl.position(), "Class " + qualifiedName()
+ " superclass changed from " + superclassName() + " to " + cl.superclassName());
diff --git a/src/com/google/doclava/MethodInfo.java b/src/com/google/doclava/MethodInfo.java
index db5e0cf..301b935 100644
--- a/src/com/google/doclava/MethodInfo.java
+++ b/src/com/google/doclava/MethodInfo.java
@@ -18,6 +18,7 @@
import com.google.clearsilver.jsilver.data.Data;
import com.google.doclava.apicheck.AbstractMethodInfo;
+import com.google.doclava.apicheck.ApiInfo;
import java.util.*;
@@ -708,13 +709,25 @@
}
return false;
}
-
+
public boolean isConsistent(MethodInfo mInfo) {
boolean consistent = true;
if (this.mReturnType != mInfo.mReturnType && !this.mReturnType.equals(mInfo.mReturnType)) {
- consistent = false;
- Errors.error(Errors.CHANGED_TYPE, mInfo.position(), "Method " + mInfo.qualifiedName()
- + " has changed return type from " + mReturnType + " to " + mInfo.mReturnType);
+ if (!mReturnType.isPrimitive() && !mInfo.mReturnType.isPrimitive()) {
+ // Check to see if our class extends the old class.
+ ApiInfo infoApi = mInfo.containingClass().containingPackage().containingApi();
+ ClassInfo infoReturnClass = infoApi.findClass(mInfo.mReturnType.qualifiedTypeName());
+ // Find the classes.
+ consistent = infoReturnClass != null &&
+ infoReturnClass.isAssignableTo(mReturnType.qualifiedTypeName());
+ } else {
+ consistent = false;
+ }
+
+ if (!consistent) {
+ Errors.error(Errors.CHANGED_TYPE, mInfo.position(), "Method " + mInfo.qualifiedName()
+ + " has changed return type from " + mReturnType + " to " + mInfo.mReturnType);
+ }
}
if (mIsAbstract != mInfo.mIsAbstract) {
diff --git a/src/com/google/doclava/PackageInfo.java b/src/com/google/doclava/PackageInfo.java
index 0c437c1..65a9639 100644
--- a/src/com/google/doclava/PackageInfo.java
+++ b/src/com/google/doclava/PackageInfo.java
@@ -16,6 +16,7 @@
package com.google.doclava;
+import com.google.doclava.apicheck.ApiInfo;
import com.google.clearsilver.jsilver.data.Data;
import com.sun.javadoc.*;
@@ -175,6 +176,14 @@
return mErrors;
}
+ public ApiInfo containingApi() {
+ return mContainingApi;
+ }
+
+ public void setContainingApi(ApiInfo api) {
+ mContainingApi = api;
+ }
+
// in hashed containers, treat the name as the key
@Override
public int hashCode() {
@@ -183,6 +192,7 @@
private String mName;
private PackageDoc mPackage;
+ private ApiInfo mContainingApi;
private ClassInfo[] mInterfaces;
private ClassInfo[] mOrdinaryClasses;
private ClassInfo[] mEnums;
@@ -225,6 +235,7 @@
}
public void addInterface(ClassInfo cls) {
+ cls.setPackage(this);
mInterfacesMap.put(cls.name(), cls);
}
@@ -237,6 +248,7 @@
}
public void addOrdinaryClass(ClassInfo cls) {
+ cls.setPackage(this);
mOrdinaryClassesMap.put(cls.name(), cls);
}
@@ -245,6 +257,7 @@
}
public void addEnum(ClassInfo cls) {
+ cls.setPackage(this);
this.mEnumsMap.put(cls.name(), cls);
}
@@ -259,8 +272,9 @@
// TODO: Leftovers from ApiCheck that should be better merged.
private HashMap<String, ClassInfo> mClasses = new HashMap<String, ClassInfo>();
- public void addClass(ClassInfo cl) {
- mClasses.put(cl.name(), cl);
+ public void addClass(ClassInfo cls) {
+ cls.setPackage(this);
+ mClasses.put(cls.name(), cls);
}
public HashMap<String, ClassInfo> allClasses() {
diff --git a/src/com/google/doclava/apicheck/ApiInfo.java b/src/com/google/doclava/apicheck/ApiInfo.java
index 758942a..11d6f8f 100644
--- a/src/com/google/doclava/apicheck/ApiInfo.java
+++ b/src/com/google/doclava/apicheck/ApiInfo.java
@@ -92,6 +92,7 @@
protected void addPackage(PackageInfo pInfo) {
// track the set of organized packages in the API
+ pInfo.setContainingApi(this);
mPackages.put(pInfo.name(), pInfo);
// accumulate a direct map of all the classes in the API
diff --git a/test/api/changed-assignable-return-1.xml b/test/api/changed-assignable-return-1.xml
new file mode 100644
index 0000000..8166bbd
--- /dev/null
+++ b/test/api/changed-assignable-return-1.xml
@@ -0,0 +1,9 @@
+<api>
+<package name="test" >
+<class name="A" extends="java.lang.Object" abstract="false" static="false" final="false" deprecated="not deprecated" visibility="public" />
+<class name="B" extends="test.A" abstract="false" static="false" final="false" deprecated="not deprecated" visibility="public">
+<method name="me" return="test.A" abstract="false" native="false" synchronized="false" static="false" final="false" deprecated="not deprecated" visibility="public" />
+</class>
+</package>
+</api>
+
diff --git a/test/api/changed-assignable-return-2.xml b/test/api/changed-assignable-return-2.xml
new file mode 100644
index 0000000..7d3e271
--- /dev/null
+++ b/test/api/changed-assignable-return-2.xml
@@ -0,0 +1,9 @@
+<api>
+<package name="test" >
+<class name="A" extends="java.lang.Object" abstract="false" static="false" final="false" deprecated="not deprecated" visibility="public" />
+<class name="B" extends="test.A" abstract="false" static="false" final="false" deprecated="not deprecated" visibility="public">
+<method name="me" return="test.B" abstract="false" native="false" synchronized="false" static="false" final="false" deprecated="not deprecated" visibility="public" />
+</class>
+</package>
+</api>
+
diff --git a/test/doclava/ApiCheckTest.java b/test/doclava/ApiCheckTest.java
index b58a831..c3393a1 100644
--- a/test/doclava/ApiCheckTest.java
+++ b/test/doclava/ApiCheckTest.java
@@ -102,6 +102,13 @@
assertEquals(1, report.errors().size());
assertEquals(Errors.CHANGED_SUPERCLASS, report.errors().iterator().next().error());
}
+
+ public void testChangedAssignableReturn() {
+ String[] args = { "test/api/changed-assignable-return-1.xml", "test/api/changed-assignable-return-2.xml" };
+ ApiCheck apiCheck = new ApiCheck();
+ Report report = apiCheck.checkApi(args);
+ assertEquals(0, report.errors().size());
+ }
public void testInsertedSuper() {
String[] args = { "test/api/inserted-super-1.xml", "test/api/inserted-super-2.xml" };