Preserve MustImplementMethodsVisitor method type ancestors (#329)

This PR addresses an issue I found in [NullAway issue
#103](uber/NullAway#103), where compile-time
errors were caused by missing methods and types. The missing methods
were caused by the following:

**1) Methods not preserved**

Foo extends (abstract) Bar extends [Some JDK interface]

If Bar contains an abstract method also present in the JDK interface,
those implementations in Foo were removed while the abstract definition
was maintained in Bar, causing compile-time errors. A check was added to
ensure that all super abstract methods did not contain anything that
needed to be preserved (i.e. when in Foo, instead of just checking Bar,
we check Bar and the JDK interface).

**2. Classes not preserved**

If a non-target class needs its method implementations to be preserved,
and some of its return/parameter types are solvable but were untracked
so far by `usedTypeElements`, their ancestors were not preserved. For
example, if a method visited by `MustImplementMethodsVisitor` is of type
`PeekingIterator<E>` which extends `Iterator<E>`, the `extends
Iterator<E>` portion is kept in the original definition but its import
statement was removed, leading to compile-time errors. The new approach
to this is, if a type element was not previously in `usedTypeElements`,
all of its ancestors (interfaces and super classes) would also be marked
for preservation.

A test case can be found in `Issue103Test`.

Thanks!

src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java

@@ -10,18 +10,21 @@
 import com.github.javaparser.ast.expr.SuperExpr;
 import com.github.javaparser.ast.stmt.BlockStmt;
 import com.github.javaparser.ast.type.ClassOrInterfaceType;
+import com.github.javaparser.ast.type.ReferenceType;
 import com.github.javaparser.ast.visitor.Visitable;
 import com.github.javaparser.resolution.UnsolvedSymbolException;
 import com.github.javaparser.resolution.declarations.ResolvedMethodDeclaration;
 import com.github.javaparser.resolution.declarations.ResolvedParameterDeclaration;
 import com.github.javaparser.resolution.types.ResolvedReferenceType;
+import com.github.javaparser.resolution.types.ResolvedType;
 import com.github.javaparser.resolution.types.parametrization.ResolvedTypeParametersMap;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.regex.Pattern;
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 /**
@@ -30,7 +33,6 @@
  * run after the list of used classes is finalized.
  */
 public class MustImplementMethodsVisitor extends SpeciminStateVisitor {
-
   /**
    * Constructs a new SolveMethodOverridingVisitor with the provided sets of target methods, used
    * members, and used classes.
@@ -63,15 +65,24 @@ public Visitable visit(MethodDeclaration method, Void p) {
     // implementing required methods from interfaces in the target code,
     // but unfortunately I think it's the best that we can do here. (@Override
     // is technically optional, but it is widely used.)
+    // However, if the current method is in a target type and in an interface/abstract class,
+    // we don't need to preserve the method since there are no children that require its definition
     if (isPreservedAndAbstract(overridden)
-        || (overridden == null && overridesAnInterfaceMethod(method))) {
+        || (overridden == null
+            && overridesAnInterfaceMethod(method)
+            && !isParentTargetAndInterfaceOrAbstract(method))) {
       ResolvedMethodDeclaration resolvedMethod = method.resolve();
-      Set<String> returnAndParamTypes = new HashSet<>();
+      Map<String, ResolvedType> returnAndParamAndThrowTypes = new HashMap<>();
       try {
-        returnAndParamTypes.add(resolvedMethod.getReturnType().describe());
+        returnAndParamAndThrowTypes.put(
+            resolvedMethod.getReturnType().describe(), resolvedMethod.getReturnType());
         for (int i = 0; i < resolvedMethod.getNumberOfParams(); ++i) {
           ResolvedParameterDeclaration param = resolvedMethod.getParam(i);
-          returnAndParamTypes.add(param.describeType());
+          returnAndParamAndThrowTypes.put(param.describeType(), param.getType());
+        }
+        for (ReferenceType thrownException : method.getThrownExceptions()) {
+          ResolvedType resolvedException = thrownException.resolve();
+          returnAndParamAndThrowTypes.put(resolvedException.describe(), resolvedException);
         }
       } catch (UnsolvedSymbolException | UnsupportedOperationException e) {
         // In this case, don't keep the method (it won't compile anyway,
@@ -80,7 +91,8 @@ public Visitable visit(MethodDeclaration method, Void p) {
         return super.visit(method, p);
       }
       usedMembers.add(resolvedMethod.getQualifiedSignature());
-      for (String type : returnAndParamTypes) {
+      for (String type : returnAndParamAndThrowTypes.keySet()) {
+        String originalType = type;
         type = type.trim();
         if (type.contains("<")) {
           // remove generics, if present, since this type will be used in
@@ -91,7 +103,16 @@ public Visitable visit(MethodDeclaration method, Void p) {
         if (type.contains("[]")) {
           type = type.replace("[]", "");
         }
+
+        boolean previouslyIncluded = usedTypeElements.contains(type);
+
         usedTypeElements.add(type);
+
+        ResolvedType resolvedType = returnAndParamAndThrowTypes.get(originalType);
+
+        if (!previouslyIncluded && resolvedType != null && resolvedType.isReferenceType()) {
+          addAllResolvableAncestors(resolvedType.asReferenceType());
+        }
       }
     }
     return super.visit(method, p);
@@ -116,6 +137,37 @@ private boolean isPreservedAndAbstract(@Nullable ResolvedMethodDeclaration metho
     return usedMembers.contains(methodSignature);
   }
 
+  /**
+   * Returns true iff the parent is an abstract class/interface and if it is a targeted type.
+   *
+   * @param node the Node to check
+   * @return true iff the parent is a target abstract class/interface
+   */
+  private boolean isParentTargetAndInterfaceOrAbstract(Node node) {
+    Node parent = JavaParserUtil.getEnclosingClassLike(node);
+
+    if (parent instanceof ClassOrInterfaceDeclaration
+        && (((ClassOrInterfaceDeclaration) parent).isInterface()
+            || ((ClassOrInterfaceDeclaration) parent).isAbstract())) {
+      String enclosingClassName =
+          ((ClassOrInterfaceDeclaration) parent).getFullyQualifiedName().orElse(null);
+
+      if (enclosingClassName != null) {
+        for (String targetMethod : targetMethods) {
+          if (targetMethod.startsWith(enclosingClassName)) {
+            return true;
+          }
+        }
+        for (String targetField : targetFields) {
+          if (targetField.startsWith(enclosingClassName)) {
+            return true;
+          }
+        }
+      }
+    }
+    return false;
+  }
+
   /**
    * Returns true iff the given method declaration is overriding a preserved unimplemented method in
    * an implemented interface. This is an expensive check that searches the implemented interfaces.
@@ -138,29 +190,58 @@ private boolean overridesAnInterfaceMethod(MethodDeclaration method) {
 
     if (typeElt instanceof EnumDeclaration) {
       EnumDeclaration asEnum = (EnumDeclaration) typeElt;
-      return overridesAnInterfaceMethodImpl(asEnum.getImplementedTypes(), signature);
+      Set<ResolvedReferenceType> parents =
+          convertToResolvedReferenceTypes(asEnum.getImplementedTypes());
+
+      return overridesAnInterfaceMethodImpl(parents, signature);
     } else if (typeElt instanceof ClassOrInterfaceDeclaration) {
       ClassOrInterfaceDeclaration asClass = (ClassOrInterfaceDeclaration) typeElt;
-      return overridesAnInterfaceMethodImpl(asClass.getImplementedTypes(), signature);
+
+      // Get directly implemented interfaces as well as types implemented through parent classes
+      Set<ResolvedReferenceType> parents =
+          convertToResolvedReferenceTypes(asClass.getImplementedTypes());
+      parents.addAll(convertToResolvedReferenceTypes(asClass.getExtendedTypes()));
+
+      return overridesAnInterfaceMethodImpl(parents, signature);
     } else {
       throw new RuntimeException(
           "unexpected enclosing structure " + typeElt + " for method " + method);
     }
   }
 
+  /**
+   * Adds all resolvable ancestors (interfaces, superclasses) to the usedTypeElements set. It is
+   * intended to be used when the type is not already present in usedTypeElements, but there is no
+   * harm in calling this elsewhere.
+   *
+   * @param resolvedType the reference type to add its ancestors
+   */
+  private void addAllResolvableAncestors(ResolvedReferenceType resolvedType) {
+    // If this method is called, this type is not used anywhere else except in this location
+    // Therefore, its inherited types (if solvable) should be preserved since it was
+    // not able to be preserved elsewhere.
+    for (ResolvedReferenceType implementation :
+        getAllImplementations(new HashSet<>(resolvedType.getAllAncestors()))) {
+      usedTypeElements.add(implementation.getQualifiedName());
+    }
+  }
+
   /**
    * Helper method for overridesAnInterfaceMethod, to allow the same code to be shared in the enum
    * and class cases.
    *
-   * @param implementedTypes the types of the implemented interfaces
+   * @param implementedTypes the types of the implemented interfaces/classes
    * @param signature the signature we're looking for
    * @return see {@link #overridesAnInterfaceMethod(MethodDeclaration)}
    */
   private boolean overridesAnInterfaceMethodImpl(
-      NodeList<ClassOrInterfaceType> implementedTypes, String signature) {
-
+      Set<ResolvedReferenceType> implementedTypes, String signature) {
+    // Classes may exist in this collection; their primary purpose is to exclude a method
+    // if a concrete method declaration exists
     Collection<ResolvedReferenceType> allImplementedTypes = getAllImplementations(implementedTypes);
 
+    boolean result = false;
+
     for (ResolvedReferenceType resolvedInterface : allImplementedTypes) {
       // This boolean is important to distinguish between the case of
       // an interface that's in the input/output (and therefore could change)
@@ -184,6 +265,9 @@ private boolean overridesAnInterfaceMethodImpl(
       for (String name : typeParametersMap.getNames()) {
         String interfaceViewpointName = name.substring(name.lastIndexOf('.') + 1);
         String localViewpointName = typeParametersMap.getValueBySignature(name).get().describe();
+        // Escape localViewpointName in case it contains special regex characters like []
+        // if the type is an array, for example
+        localViewpointName = Pattern.quote(localViewpointName);
         targetSignature = targetSignature.replaceAll(localViewpointName, interfaceViewpointName);
       }
       // Type parameters in the types are erased (as they would be by javac when doing method
@@ -197,43 +281,73 @@ private boolean overridesAnInterfaceMethodImpl(
 
       for (ResolvedMethodDeclaration methodInInterface :
           resolvedInterface.getAllMethodsVisibleToInheritors()) {
-        if (methodInInterface.isAbstract()
-            && erase(methodInInterface.getSignature()).equals(targetSignature)) {
-          // once we've found the correct method, we return to whether we
-          // control it or not. If we don't, it must be preserved. If we do, then we only
-          // preserve it if the PrunerVisitor won't remove it.
-          return !inOutput || usedMembers.contains(methodInInterface.getQualifiedSignature());
+        try {
+          if (erase(methodInInterface.getSignature()).equals(targetSignature)) {
+            if (methodInInterface.isAbstract()) {
+              // once we've found the correct method, we return to whether we
+              // control it or not. If we don't, it must be preserved. If we do, then we only
+              // preserve it if the PrunerVisitor won't remove it.
+              if (!inOutput || usedMembers.contains(methodInInterface.getQualifiedSignature())) {
+                // Do not immediately return; if two ancestors, unincluded interfaces are present,
+                // one with a method declaration and one without, we need to return false even if
+                // this may be true (depends on which method is traversed first)
+                result = true;
+                continue;
+              }
+            } else if (!inOutput) {
+              // If we can't control the method, and there's a definition provided, we can safely
+              // remove all overridden versions
+              return false;
+            }
+          }
+        } catch (UnsolvedSymbolException ex) {
+          // since we are going through all ancestor interfaces/abstract classes, we should
+          // expect that some method signature cannot be resolved. if this is the case, then
+          // it's definitely not the method we're looking for.
+          continue;
         }
       }
     }
     // if we don't find an overridden method in any of the implemented interfaces, return false.
-    return false;
+    // however, if this method only implements abstract methods we can't control, then return true.
+    return result;
   }
 
   /**
-   * Gets all interface implementations of a List of ClassOrInterfaceTypes, including those of
-   * ancestors. This method is intended to be used for interface implementations only (i.e. pass in
-   * ClassOrInterfaceDeclaration.getImplementedTypes()).
+   * Helper method to convert ClassOrInterfaceTypes to ResolvedReferenceTypes
    *
-   * @param types A List of interface types to find all parent interfaces
-   * @return A Collection of ResolvedReferenceTypes containing all parent interfaces
+   * @param types A List of interface/class types to convert
+   * @return A set of ResolvedReferenceTypes representing the resolved input types
    */
-  private static Collection<ResolvedReferenceType> getAllImplementations(
+  private static Set<ResolvedReferenceType> convertToResolvedReferenceTypes(
       List<ClassOrInterfaceType> types) {
-    Set<ResolvedReferenceType> toTraverse = new HashSet<>();
+    Set<ResolvedReferenceType> resolvedTypes = new HashSet<>();
 
     for (ClassOrInterfaceType type : types) {
       try {
         ResolvedReferenceType resolved =
             JavaParserUtil.classOrInterfaceTypeToResolvedReferenceType(type);
-        toTraverse.add(resolved);
+        resolvedTypes.add(resolved);
       } catch (UnsolvedSymbolException | UnsupportedOperationException ex) {
         // In this case, we're implementing an interface that we don't control
         // or that will not be preserved.
         continue;
       }
     }
 
+    return resolvedTypes;
+  }
+
+  /**
+   * Gets all interface implementations of a List of ClassOrInterfaceTypes, including those of
+   * ancestors. This method is intended to be used for interface / class implementations only (i.e.
+   * pass in ClassOrInterfaceDeclaration.getImplementedTypes() or getExtendedTypes()).
+   *
+   * @param toTraverse A List of resolved reference types to find all ancestors
+   * @return A Collection of ResolvedReferenceTypes containing all ancestors
+   */
+  private static Collection<ResolvedReferenceType> getAllImplementations(
+      Set<ResolvedReferenceType> toTraverse) {
     Map<String, ResolvedReferenceType> qualifiedNameToType = new HashMap<>();
     while (!toTraverse.isEmpty()) {
       Set<ResolvedReferenceType> newToTraverse = new HashSet<>();

src/main/java/org/checkerframework/specimin/SpeciminRunner.java

@@ -516,7 +516,7 @@ private static SpeciminStateVisitor processAnnotationTypes(
     AnnotationParameterTypesVisitor annotationParameterTypesVisitor =
         new AnnotationParameterTypesVisitor(last);
 
-    Set<String> relatedClass = new HashSet<>(parsedTargetFiles.keySet());
+    Set<String> classesToParse = new HashSet<>();
     Set<CompilationUnit> compilationUnitsToSolveAnnotations =
         new HashSet<>(parsedTargetFiles.values());
 
@@ -527,34 +527,59 @@ private static SpeciminStateVisitor processAnnotationTypes(
 
       // add all files related to the target annotations
       for (String annoFullName : annotationParameterTypesVisitor.getClassesToAdd()) {
+        if (annotationParameterTypesVisitor.getUsedTypeElements().contains(annoFullName)) {
+          continue;
+        }
         String directoryOfFile = annoFullName.replace(".", "/") + ".java";
         File thisFile = new File(root + directoryOfFile);
         // classes from JDK are automatically on the classpath, so UnsolvedSymbolVisitor will not
         // create synthetic files for them
         if (thisFile.exists()) {
-          relatedClass.add(directoryOfFile);
+          classesToParse.add(directoryOfFile);
+        } else {
+          // The given class may be an inner class, so we should find its encapsulating class
+          // Assuming following Java conventions, we will find the first instance of .{capital}
+          // and trim off subsequent .*s.
+          int dot = annoFullName.indexOf('.');
+          while (dot != -1) {
+            if (Character.isUpperCase(annoFullName.charAt(dot + 1))) {
+              dot = annoFullName.indexOf('.', dot + 1);
+              break;
+            }
+            dot = annoFullName.indexOf('.', dot + 1);
+          }
+
+          if (dot != -1) {
+            directoryOfFile = annoFullName.substring(0, dot).replace(".", "/") + ".java";
+            thisFile = new File(root + directoryOfFile);
+            // This inner class was just added, so we should re-parse the file
+            if (thisFile.exists()) {
+              classesToParse.add(directoryOfFile);
+            }
+          }
         }
       }
 
       compilationUnitsToSolveAnnotations.clear();
 
-      for (String directory : relatedClass) {
-        // directories already in parsedTargetFiles are original files in the root directory, we are
-        // not supposed to update them.
-        if (!parsedTargetFiles.containsKey(directory)) {
-          try {
-            // We need to continue solving annotations and parameters in newly added annotation
-            // files
+      for (String directory : classesToParse) {
+        // We need to continue solving annotations and parameters in newly added annotation files
+        try {
+          // directories already in parsedTargetFiles are original files in the root directory, we
+          // are not supposed to update them.
+          if (!parsedTargetFiles.containsKey(directory)) {
             CompilationUnit parsed = parseJavaFile(root, directory);
             parsedTargetFiles.put(directory, parsed);
-            compilationUnitsToSolveAnnotations.add(parsed);
-          } catch (ParseProblemException e) {
-            // TODO: Figure out why the CI is crashing.
-            continue;
           }
+          compilationUnitsToSolveAnnotations.add(parsedTargetFiles.get(directory));
+        } catch (ParseProblemException e) {
+          // TODO: Figure out why the CI is crashing.
+          continue;
         }
       }
 
+      classesToParse.clear();
+
       annotationParameterTypesVisitor
           .getUsedTypeElements()
           .addAll(annotationParameterTypesVisitor.getClassesToAdd());

src/main/java/org/checkerframework/specimin/SpeciminStateVisitor.java

@@ -249,9 +249,10 @@ protected void maintainDataStructuresPostSuper(TypeDeclaration<?> decl) {
   }
 
   /**
-   * Determines if the given Node is a target/used method or class.
+   * Determines if the given Node is a target/used member or class.
    *
    * @param node The node to check
+   * @return true iff the given Node is a target/used member or type.
    */
   protected boolean isTargetOrUsed(Node node) {
     String qualifiedName;