Preserve MustImplementMethodsVisitor method type ancestors

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,7 @@ private boolean overridesAnInterfaceMethodImpl(
       for (String name : typeParametersMap.getNames()) {
         String interfaceViewpointName = name.substring(name.lastIndexOf('.') + 1);
         String localViewpointName = typeParametersMap.getValueBySignature(name).get().describe();
+        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 +279,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

@@ -229,9 +229,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;

src/test/java/org/checkerframework/specimin/Issue103Test.java

@@ -0,0 +1,20 @@
+package org.checkerframework.specimin;
+
+import java.io.IOException;
+import org.junit.Test;
+
+/**
+ * This test checks if Specimin will preserve ancestors of types added in
+ * MustImplementMethodsVisitor. It also checks to see if methods whose direct override is abstract,
+ * but original definition is JDK are preserved. Test code derived from a modified, minimized
+ * version of NullAway issue 103.
+ */
+public class Issue103Test {
+  @Test
+  public void runTest() throws IOException {
+    SpeciminTestExecutor.runTestWithoutJarPaths(
+        "issue103",
+        new String[] {"com/example/Simple.java"},
+        new String[] {"com.example.Simple#foo()"});
+  }
+}