diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index d03203bb..cfc0a59e 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -10,11 +10,13 @@ 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; @@ -22,6 +24,7 @@ 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 returnAndParamTypes = new HashSet<>(); + Map 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 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 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 implementedTypes, String signature) { - + Set implementedTypes, String signature) { + // Classes may exist in this collection; their primary purpose is to exclude a method + // if a concrete method declaration exists Collection 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,36 +281,53 @@ 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 getAllImplementations( + private static Set convertToResolvedReferenceTypes( List types) { - Set toTraverse = new HashSet<>(); + Set 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. @@ -234,6 +335,19 @@ private static Collection getAllImplementations( } } + 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 getAllImplementations( + Set toTraverse) { Map qualifiedNameToType = new HashMap<>(); while (!toTraverse.isEmpty()) { Set newToTraverse = new HashSet<>(); diff --git a/src/main/java/org/checkerframework/specimin/SpeciminRunner.java b/src/main/java/org/checkerframework/specimin/SpeciminRunner.java index aa4b67bb..13ea6dd0 100644 --- a/src/main/java/org/checkerframework/specimin/SpeciminRunner.java +++ b/src/main/java/org/checkerframework/specimin/SpeciminRunner.java @@ -516,7 +516,7 @@ private static SpeciminStateVisitor processAnnotationTypes( AnnotationParameterTypesVisitor annotationParameterTypesVisitor = new AnnotationParameterTypesVisitor(last); - Set relatedClass = new HashSet<>(parsedTargetFiles.keySet()); + Set classesToParse = new HashSet<>(); Set 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()); diff --git a/src/main/java/org/checkerframework/specimin/SpeciminStateVisitor.java b/src/main/java/org/checkerframework/specimin/SpeciminStateVisitor.java index f87b8748..3a1dc284 100644 --- a/src/main/java/org/checkerframework/specimin/SpeciminStateVisitor.java +++ b/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; diff --git a/src/test/java/org/checkerframework/specimin/Issue103Test.java b/src/test/java/org/checkerframework/specimin/Issue103Test.java new file mode 100644 index 00000000..d0be425f --- /dev/null +++ b/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()"}); + } +} diff --git a/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java b/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java new file mode 100644 index 00000000..6b1c162c --- /dev/null +++ b/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java @@ -0,0 +1,102 @@ +package com.example; + +import java.util.AbstractCollection; + +public abstract class AbstractLinkedDeque extends AbstractCollection implements LinkedDeque { + + public int size() { + throw new Error(); + } + + public E peek() { + throw new Error(); + } + + public E peekFirst() { + throw new Error(); + } + + public E peekLast() { + throw new Error(); + } + + public E getFirst() { + throw new Error(); + } + + public E getLast() { + throw new Error(); + } + + public E element() { + throw new Error(); + } + + public boolean offer(E e) { + throw new Error(); + } + + public boolean offerFirst(E e) { + throw new Error(); + } + + public boolean offerLast(E e) { + throw new Error(); + } + + public void addFirst(E e) { + throw new Error(); + } + + public void addLast(E e) { + throw new Error(); + } + + public E poll() { + throw new Error(); + } + + public E pollFirst() { + throw new Error(); + } + + public E pollLast() { + throw new Error(); + } + + public E remove() { + throw new Error(); + } + + public E removeFirst() { + throw new Error(); + } + + public boolean removeFirstOccurrence(Object o) { + throw new Error(); + } + + public E removeLast() { + throw new Error(); + } + + public boolean removeLastOccurrence(Object o) { + throw new Error(); + } + + public void push(E e) { + throw new Error(); + } + + public E pop() { + throw new Error(); + } + + public PeekingIterator iterator() { + throw new Error(); + } + + public PeekingIterator descendingIterator() { + throw new Error(); + } +} diff --git a/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java b/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java new file mode 100644 index 00000000..3c351b1d --- /dev/null +++ b/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java @@ -0,0 +1,6 @@ +package com.example; + +import java.util.Deque; + +public final class AccessOrderDeque extends AbstractLinkedDeque { +} diff --git a/src/test/resources/issue103/expected/com/example/LinkedDeque.java b/src/test/resources/issue103/expected/com/example/LinkedDeque.java new file mode 100644 index 00000000..f1296ad4 --- /dev/null +++ b/src/test/resources/issue103/expected/com/example/LinkedDeque.java @@ -0,0 +1,10 @@ +package com.example; + +import java.util.Deque; +import java.util.Iterator; + +public interface LinkedDeque extends Deque { + + interface PeekingIterator extends Iterator { + } +} diff --git a/src/test/resources/issue103/expected/com/example/Simple.java b/src/test/resources/issue103/expected/com/example/Simple.java new file mode 100644 index 00000000..a0ca71be --- /dev/null +++ b/src/test/resources/issue103/expected/com/example/Simple.java @@ -0,0 +1,12 @@ +package com.example; + +public class Simple { + + AccessOrderDeque bar() { + throw new Error(); + } + + void foo() { + AccessOrderDeque x = bar(); + } +} diff --git a/src/test/resources/issue103/input/com/example/AbstractLinkedDeque.java b/src/test/resources/issue103/input/com/example/AbstractLinkedDeque.java new file mode 100644 index 00000000..4a4ee29b --- /dev/null +++ b/src/test/resources/issue103/input/com/example/AbstractLinkedDeque.java @@ -0,0 +1,121 @@ +package com.example; + +import java.util.AbstractCollection; +import java.util.Collection; + +public abstract class AbstractLinkedDeque extends AbstractCollection implements LinkedDeque { + + public boolean isEmpty() { + throw new Error(); + } + + public int size() { + throw new Error(); + } + + public void clear() { + throw new Error(); + } + + public abstract boolean contains(Object o); + + public E peek() { + throw new Error(); + } + + public E peekFirst() { + throw new Error(); + } + + public E peekLast() { + throw new Error(); + } + + public E getFirst() { + throw new Error(); + } + + public E getLast() { + throw new Error(); + } + + public E element() { + throw new Error(); + } + + public boolean offer(E e) { + throw new Error(); + } + + public boolean offerFirst(E e) { + throw new Error(); + } + + public boolean offerLast(E e) { + throw new Error(); + } + + public boolean add(E e) { + throw new Error(); + } + + public void addFirst(E e) { + throw new Error(); + } + + public void addLast(E e) { + throw new Error(); + } + + public E poll() { + throw new Error(); + } + + public E pollFirst() { + throw new Error(); + } + + public E pollLast() { + throw new Error(); + } + + public E remove() { + throw new Error(); + } + + public E removeFirst() { + throw new Error(); + } + + public boolean removeFirstOccurrence(Object o) { + throw new Error(); + } + + public E removeLast() { + throw new Error(); + } + + public boolean removeLastOccurrence(Object o) { + throw new Error(); + } + + public boolean removeAll(Collection c) { + throw new Error(); + } + + public void push(E e) { + throw new Error(); + } + + public E pop() { + throw new Error(); + } + + public PeekingIterator iterator() { + throw new Error(); + } + + public PeekingIterator descendingIterator() { + throw new Error(); + } +} diff --git a/src/test/resources/issue103/input/com/example/AccessOrderDeque.java b/src/test/resources/issue103/input/com/example/AccessOrderDeque.java new file mode 100644 index 00000000..608fb30a --- /dev/null +++ b/src/test/resources/issue103/input/com/example/AccessOrderDeque.java @@ -0,0 +1,30 @@ +package com.example; + +import java.util.Deque; + +public final class AccessOrderDeque extends AbstractLinkedDeque { + + public boolean contains(Object o) { + throw new Error(); + } + + public boolean remove(Object o) { + throw new Error(); + } + + public E getPrevious(E e) { + throw new Error(); + } + + public void setPrevious(E e, E prev) { + throw new Error(); + } + + public E getNext(E e) { + throw new Error(); + } + + public void setNext(E e, E next) { + throw new Error(); + } +} diff --git a/src/test/resources/issue103/input/com/example/LinkedDeque.java b/src/test/resources/issue103/input/com/example/LinkedDeque.java new file mode 100644 index 00000000..f1296ad4 --- /dev/null +++ b/src/test/resources/issue103/input/com/example/LinkedDeque.java @@ -0,0 +1,10 @@ +package com.example; + +import java.util.Deque; +import java.util.Iterator; + +public interface LinkedDeque extends Deque { + + interface PeekingIterator extends Iterator { + } +} diff --git a/src/test/resources/issue103/input/com/example/Simple.java b/src/test/resources/issue103/input/com/example/Simple.java new file mode 100644 index 00000000..24c7cd09 --- /dev/null +++ b/src/test/resources/issue103/input/com/example/Simple.java @@ -0,0 +1,12 @@ +package com.example; + +public class Simple { + AccessOrderDeque bar() { + throw new Error(); + } + + // Target method + void foo() { + AccessOrderDeque x = bar(); + } +}