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

7 files changed, 87 insertions, 19 deletions

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 class ClassInfo extends DocInfo implements ContainerInfo, Comparable, Sco
   }
 
   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 @@ public class ClassInfo extends DocInfo implements ContainerInfo, Comparable, Sco
    * 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 @@ public class ClassInfo extends DocInfo implements ContainerInfo, Comparable, Sco
       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 @@ public class ClassInfo extends DocInfo implements ContainerInfo, Comparable, Sco
     }
 
     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 @@ package com.google.doclava;
 
 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 @@ public class MethodInfo extends MemberInfo implements AbstractMethodInfo, Resolv
     }
     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 @@ public class PackageInfo extends DocInfo implements ContainerInfo {
     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 @@ public class PackageInfo extends DocInfo implements ContainerInfo {
 
   private String mName;
   private PackageDoc mPackage;
+  private ApiInfo mContainingApi;
   private ClassInfo[] mInterfaces;
   private ClassInfo[] mOrdinaryClasses;
   private ClassInfo[] mEnums;
@@ -225,6 +235,7 @@ public class PackageInfo extends DocInfo implements ContainerInfo {
   }
 
   public void addInterface(ClassInfo cls) {
+      cls.setPackage(this);
       mInterfacesMap.put(cls.name(), cls);
   }
 
@@ -237,6 +248,7 @@ public class PackageInfo extends DocInfo implements ContainerInfo {
   }
 
   public void addOrdinaryClass(ClassInfo cls) {
+      cls.setPackage(this);
       mOrdinaryClassesMap.put(cls.name(), cls);
   }
 
@@ -245,6 +257,7 @@ public class PackageInfo extends DocInfo implements ContainerInfo {
   }
 
   public void addEnum(ClassInfo cls) {
+      cls.setPackage(this);
       this.mEnumsMap.put(cls.name(), cls);
   }
 
@@ -259,8 +272,9 @@ public class PackageInfo extends DocInfo implements ContainerInfo {
   // 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 @@ public class ApiInfo {
 
   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 @@ public class ApiCheckTest extends TestCase {
     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" };