From 56b410cc9decd741889ba58aa30e786c0cac2f58 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 23 Jul 2024 15:21:43 -0400 Subject: [PATCH 01/17] MustImplementMethodsVisitor should include return types --- .../specimin/MustImplementMethodsVisitor.java | 160 +++++++++++++++++- 1 file changed, 156 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 1fca919c..64f1f298 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -1,5 +1,6 @@ package org.checkerframework.specimin; +import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.Node; import com.github.javaparser.ast.NodeList; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; @@ -8,14 +9,19 @@ import com.github.javaparser.ast.body.Parameter; import com.github.javaparser.ast.expr.MethodCallExpr; import com.github.javaparser.ast.expr.SuperExpr; +import com.github.javaparser.ast.nodeTypes.NodeWithMembers; import com.github.javaparser.ast.stmt.BlockStmt; import com.github.javaparser.ast.type.ClassOrInterfaceType; import com.github.javaparser.ast.visitor.Visitable; +import com.github.javaparser.resolution.MethodUsage; 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.parametrization.ResolvedTypeParametersMap; +import java.util.AbstractMap; +import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.Set; import org.checkerframework.checker.nullness.qual.Nullable; @@ -26,6 +32,12 @@ * run after the list of used classes is finalized. */ public class MustImplementMethodsVisitor extends SpeciminStateVisitor { + /** + * Keeps unused class declarations (with their corresponding parents for re-insertion) in a + * HashMap to re-visit if needed. + */ + private final Map> + qualifiedNameToUnusedClassDeclarationMap = new HashMap<>(); /** * Constructs a new SolveMethodOverridingVisitor with the provided sets of target methods, used @@ -40,9 +52,21 @@ public MustImplementMethodsVisitor(SpeciminStateVisitor previousVisitor) { @Override @SuppressWarnings("nullness:return") // ok to return null, because this is a void visitor public Visitable visit(ClassOrInterfaceDeclaration type, Void p) { - if (type.getFullyQualifiedName().isPresent() - && usedTypeElements.contains(type.getFullyQualifiedName().get())) { - return super.visit(type, p); + if (type.getFullyQualifiedName().isPresent()) { + String fullyQualifiedName = type.getFullyQualifiedName().get(); + if (usedTypeElements.contains(fullyQualifiedName)) { + return super.visit(type, p); + } else { + // in this case, we do not currently need this type element + // however, if we end up needing it again later in visit(MethodDeclaration), we + // should keep them so we can re-visit the type so it is not removed. + // We also need to store the parent node in case we need to re-insert the node to + // prevent an IllegalStateException + Node parent = type.getParentNode().orElseThrow(); + qualifiedNameToUnusedClassDeclarationMap.put( + fullyQualifiedName, new AbstractMap.SimpleEntry<>(parent, type)); + return null; + } } else { // the effect of not calling super here is that only used classes // will actually be visited by this class @@ -59,7 +83,7 @@ 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.) - if (isPreservedAndAbstract(overridden) + if (ancestorMethodPreservedAndAbstract(method) || (overridden == null && overridesAnInterfaceMethod(method))) { ResolvedMethodDeclaration resolvedMethod = method.resolve(); Set returnAndParamTypes = new HashSet<>(); @@ -87,12 +111,87 @@ public Visitable visit(MethodDeclaration method, Void p) { if (type.contains("[]")) { type = type.replace("[]", ""); } + usedTypeElements.add(type); + + if (qualifiedNameToUnusedClassDeclarationMap.containsKey(type)) { + Map.Entry entry = + qualifiedNameToUnusedClassDeclarationMap.get(type); + + Node parent = entry.getKey(); + ClassOrInterfaceDeclaration decl = entry.getValue(); + // Re-add to the compilation unit to prevent IllegalStateException + if (!parent.getChildNodes().contains(decl)) { + if (parent instanceof CompilationUnit) { + ((CompilationUnit) parent).addType(decl); + } else if (parent instanceof NodeWithMembers) { + ((NodeWithMembers) parent).addMember(decl); + } + } + + // Since we are here, 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(decl.getImplementedTypes())) { + usedTypeElements.add(implementation.getQualifiedName()); + } + + for (ResolvedReferenceType implementation : + getAllImplementations(decl.getExtendedTypes())) { + usedTypeElements.add(implementation.getQualifiedName()); + } + + // Revisit the given class declaration so it is not removed + super.visit(decl, p); + + qualifiedNameToUnusedClassDeclarationMap.remove(type); + } } } return super.visit(method, p); } + /** + * Returns true iff any parent method is abstract and preserved. Use this method if it is unclear + * whether the direct super method will be preserved or not. + * + * @param method The method declaration to check + * @return true iff any parent method is abstract and preserved + */ + private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { + ResolvedMethodDeclaration overridden = getOverriddenMethodInSuperClass(method); + if (overridden == null) { + return false; + } + + if (isPreservedAndAbstract(overridden)) { + return true; + } + + for (ResolvedReferenceType implementation : + getAllImplementations(new HashSet<>(overridden.declaringType().getAncestors()))) { + for (MethodUsage potentialSuperMethod : implementation.getDeclaredMethods()) { + if (potentialSuperMethod.getDeclaration().isAbstract()) { + String methodSignature = potentialSuperMethod.getQualifiedSignature(); + if (!potentialSuperMethod.getQualifiedSignature().equals(methodSignature)) { + continue; + } + // These classes are beyond our control. It's better to retain the implementations of all + // abstract methods to ensure the code remains compilable. + if (JavaLangUtils.inJdkPackage(methodSignature)) { + return true; + } + if (usedMembers.contains(methodSignature)) { + return true; + } + } + } + } + + return false; + } + /** * Returns true if the given method is abstract. * @@ -212,6 +311,59 @@ && erase(methodInInterface.getSignature()).equals(targetSignature)) { return false; } + /** + * Helper method for getAllImplementations(Set) + * + * @param types A List of interface/class types to find all ancestors + * @return A Collection of ResolvedReferenceTypes containing all ancestors + */ + private static Collection getAllImplementations( + List types) { + Set toTraverse = new HashSet<>(); + + for (ClassOrInterfaceType type : types) { + try { + ResolvedReferenceType resolved = + JavaParserUtil.classOrInterfaceTypeToResolvedReferenceType(type); + toTraverse.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 getAllImplementations(toTraverse); + } + + /** + * 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<>(); + for (ResolvedReferenceType type : toTraverse) { + if (!qualifiedNameToType.containsKey(type.getQualifiedName())) { + qualifiedNameToType.put(type.getQualifiedName(), type); + for (ResolvedReferenceType implemented : type.getAllAncestors()) { + newToTraverse.add(implemented); + } + } + } + toTraverse.clear(); + toTraverse = newToTraverse; + } + + return qualifiedNameToType.values(); + } + /** * Erases type arguments from a method signature string. * From f7e8096f4991aba23e21a4ff789f0d3466432fc9 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 23 Jul 2024 16:52:13 -0400 Subject: [PATCH 02/17] branch change conflict changes --- .../specimin/MustImplementMethodsVisitor.java | 69 +++++++++++++------ 1 file changed, 47 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 64f1f298..37432629 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -18,11 +18,14 @@ 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.AbstractMap; 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 org.checkerframework.checker.nullness.qual.Nullable; @@ -86,12 +89,13 @@ public Visitable visit(MethodDeclaration method, Void p) { if (ancestorMethodPreservedAndAbstract(method) || (overridden == null && overridesAnInterfaceMethod(method))) { ResolvedMethodDeclaration resolvedMethod = method.resolve(); - Set returnAndParamTypes = new HashSet<>(); + Map returnAndParamTypes = new HashMap<>(); try { - returnAndParamTypes.add(resolvedMethod.getReturnType().describe()); + returnAndParamTypes.put( + resolvedMethod.getReturnType().describe(), resolvedMethod.getReturnType()); for (int i = 0; i < resolvedMethod.getNumberOfParams(); ++i) { ResolvedParameterDeclaration param = resolvedMethod.getParam(i); - returnAndParamTypes.add(param.describeType()); + returnAndParamTypes.put(param.describeType(), param.getType()); } } catch (UnsolvedSymbolException | UnsupportedOperationException e) { // In this case, don't keep the method (it won't compile anyway, @@ -100,7 +104,7 @@ public Visitable visit(MethodDeclaration method, Void p) { return super.visit(method, p); } usedMembers.add(resolvedMethod.getQualifiedSignature()); - for (String type : returnAndParamTypes) { + for (String type : returnAndParamTypes.keySet()) { type = type.trim(); if (type.contains("<")) { // remove generics, if present, since this type will be used in @@ -112,6 +116,8 @@ public Visitable visit(MethodDeclaration method, Void p) { type = type.replace("[]", ""); } + boolean previouslyIncluded = usedTypeElements.contains(type); + usedTypeElements.add(type); if (qualifiedNameToUnusedClassDeclarationMap.containsKey(type)) { @@ -129,24 +135,16 @@ public Visitable visit(MethodDeclaration method, Void p) { } } - // Since we are here, 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(decl.getImplementedTypes())) { - usedTypeElements.add(implementation.getQualifiedName()); - } - - for (ResolvedReferenceType implementation : - getAllImplementations(decl.getExtendedTypes())) { - usedTypeElements.add(implementation.getQualifiedName()); - } - // Revisit the given class declaration so it is not removed super.visit(decl, p); qualifiedNameToUnusedClassDeclarationMap.remove(type); } + + ResolvedType resolvedType = returnAndParamTypes.get(type); + if (!previouslyIncluded && resolvedType != null && resolvedType.isReferenceType()) { + addAllResolvableAncestors(resolvedType.asReferenceType()); + } } } return super.visit(method, p); @@ -161,20 +159,30 @@ public Visitable visit(MethodDeclaration method, Void p) { */ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { ResolvedMethodDeclaration overridden = getOverriddenMethodInSuperClass(method); - if (overridden == null) { - return false; - } if (isPreservedAndAbstract(overridden)) { return true; } + ResolvedMethodDeclaration resolvedMethod; + try { + resolvedMethod = method.resolve(); + } catch (UnsolvedSymbolException ex) { + return false; + } + + String currentMethodSignature = resolvedMethod.getQualifiedSignature(); + String currentMethodName = + currentMethodSignature.substring(currentMethodSignature.lastIndexOf('.') + 1); + for (ResolvedReferenceType implementation : - getAllImplementations(new HashSet<>(overridden.declaringType().getAncestors()))) { + getAllImplementations(new HashSet<>(resolvedMethod.declaringType().getAncestors()))) { for (MethodUsage potentialSuperMethod : implementation.getDeclaredMethods()) { if (potentialSuperMethod.getDeclaration().isAbstract()) { String methodSignature = potentialSuperMethod.getQualifiedSignature(); - if (!potentialSuperMethod.getQualifiedSignature().equals(methodSignature)) { + String potentialSuperMethodName = + methodSignature.substring(methodSignature.lastIndexOf('.') + 1); + if (!currentMethodName.equals(potentialSuperMethodName)) { continue; } // These classes are beyond our control. It's better to retain the implementations of all @@ -243,6 +251,23 @@ private boolean overridesAnInterfaceMethod(MethodDeclaration 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. From eb84c4e142fd5065f7d71fe2518e8596ca9bfb70 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 23 Jul 2024 19:32:34 -0400 Subject: [PATCH 03/17] fix bugs + add test cases --- .../specimin/MustImplementMethodsVisitor.java | 71 ++-------- .../specimin/Issue103Test.java | 19 +++ .../com/example/AbstractLinkedDeque.java | 121 ++++++++++++++++++ .../com/example/AccessOrderDeque.java | 14 ++ .../expected/com/example/LinkedDeque.java | 10 ++ .../issue103/expected/com/example/Simple.java | 12 ++ .../com/example/AbstractLinkedDeque.java | 121 ++++++++++++++++++ .../input/com/example/AccessOrderDeque.java | 30 +++++ .../input/com/example/LinkedDeque.java | 10 ++ .../issue103/input/com/example/Simple.java | 12 ++ 10 files changed, 363 insertions(+), 57 deletions(-) create mode 100644 src/test/java/org/checkerframework/specimin/Issue103Test.java create mode 100644 src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java create mode 100644 src/test/resources/issue103/expected/com/example/AccessOrderDeque.java create mode 100644 src/test/resources/issue103/expected/com/example/LinkedDeque.java create mode 100644 src/test/resources/issue103/expected/com/example/Simple.java create mode 100644 src/test/resources/issue103/input/com/example/AbstractLinkedDeque.java create mode 100644 src/test/resources/issue103/input/com/example/AccessOrderDeque.java create mode 100644 src/test/resources/issue103/input/com/example/LinkedDeque.java create mode 100644 src/test/resources/issue103/input/com/example/Simple.java diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 2f0cc461..19d51e77 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -1,6 +1,14 @@ package org.checkerframework.specimin; -import com.github.javaparser.ast.CompilationUnit; +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 org.checkerframework.checker.nullness.qual.Nullable; + import com.github.javaparser.ast.Node; import com.github.javaparser.ast.NodeList; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; @@ -9,7 +17,6 @@ import com.github.javaparser.ast.body.Parameter; import com.github.javaparser.ast.expr.MethodCallExpr; import com.github.javaparser.ast.expr.SuperExpr; -import com.github.javaparser.ast.nodeTypes.NodeWithMembers; import com.github.javaparser.ast.stmt.BlockStmt; import com.github.javaparser.ast.type.ClassOrInterfaceType; import com.github.javaparser.ast.visitor.Visitable; @@ -20,18 +27,6 @@ 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.AbstractMap; -import java.util.Collection; -import java.util.HashMap; -import java.util.Collection; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.List; -import java.util.Map; -import java.util.Set; -import org.checkerframework.checker.nullness.qual.Nullable; /** * If a used class includes methods that must be implemented (because it extends an abstract class @@ -39,13 +34,6 @@ * run after the list of used classes is finalized. */ public class MustImplementMethodsVisitor extends SpeciminStateVisitor { - /** - * Keeps unused class declarations (with their corresponding parents for re-insertion) in a - * HashMap to re-visit if needed. - */ - private final Map> - qualifiedNameToUnusedClassDeclarationMap = new HashMap<>(); - /** * Constructs a new SolveMethodOverridingVisitor with the provided sets of target methods, used * members, and used classes. @@ -59,21 +47,9 @@ public MustImplementMethodsVisitor(SpeciminStateVisitor previousVisitor) { @Override @SuppressWarnings("nullness:return") // ok to return null, because this is a void visitor public Visitable visit(ClassOrInterfaceDeclaration type, Void p) { - if (type.getFullyQualifiedName().isPresent()) { - String fullyQualifiedName = type.getFullyQualifiedName().get(); - if (usedTypeElements.contains(fullyQualifiedName)) { - return super.visit(type, p); - } else { - // in this case, we do not currently need this type element - // however, if we end up needing it again later in visit(MethodDeclaration), we - // should keep them so we can re-visit the type so it is not removed. - // We also need to store the parent node in case we need to re-insert the node to - // prevent an IllegalStateException - Node parent = type.getParentNode().orElseThrow(); - qualifiedNameToUnusedClassDeclarationMap.put( - fullyQualifiedName, new AbstractMap.SimpleEntry<>(parent, type)); - return null; - } + if (type.getFullyQualifiedName().isPresent() + && usedTypeElements.contains(type.getFullyQualifiedName().get())) { + return super.visit(type, p); } else { // the effect of not calling super here is that only used classes // will actually be visited by this class @@ -109,6 +85,7 @@ public Visitable visit(MethodDeclaration method, Void p) { } usedMembers.add(resolvedMethod.getQualifiedSignature()); for (String type : returnAndParamTypes.keySet()) { + String originalType = type; type = type.trim(); if (type.contains("<")) { // remove generics, if present, since this type will be used in @@ -124,28 +101,8 @@ public Visitable visit(MethodDeclaration method, Void p) { usedTypeElements.add(type); - if (qualifiedNameToUnusedClassDeclarationMap.containsKey(type)) { - Map.Entry entry = - qualifiedNameToUnusedClassDeclarationMap.get(type); - - Node parent = entry.getKey(); - ClassOrInterfaceDeclaration decl = entry.getValue(); - // Re-add to the compilation unit to prevent IllegalStateException - if (!parent.getChildNodes().contains(decl)) { - if (parent instanceof CompilationUnit) { - ((CompilationUnit) parent).addType(decl); - } else if (parent instanceof NodeWithMembers) { - ((NodeWithMembers) parent).addMember(decl); - } - } - - // Revisit the given class declaration so it is not removed - super.visit(decl, p); - - qualifiedNameToUnusedClassDeclarationMap.remove(type); - } + ResolvedType resolvedType = returnAndParamTypes.get(originalType); - ResolvedType resolvedType = returnAndParamTypes.get(type); if (!previouslyIncluded && resolvedType != null && resolvedType.isReferenceType()) { addAllResolvableAncestors(resolvedType.asReferenceType()); } 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..9dc069e4 --- /dev/null +++ b/src/test/java/org/checkerframework/specimin/Issue103Test.java @@ -0,0 +1,19 @@ +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..4a4ee29b --- /dev/null +++ b/src/test/resources/issue103/expected/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/expected/com/example/AccessOrderDeque.java b/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java new file mode 100644 index 00000000..8099dfe8 --- /dev/null +++ b/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java @@ -0,0 +1,14 @@ +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(); + } +} 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(); + } +} From e9404d64e3c2d3a0d4bc996b3a18ff73440a94fe Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 23 Jul 2024 19:45:04 -0400 Subject: [PATCH 04/17] spotless --- .../specimin/MustImplementMethodsVisitor.java | 16 +++++++--------- .../checkerframework/specimin/Issue103Test.java | 11 ++++++----- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 19d51e77..6ac92164 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -1,14 +1,5 @@ package org.checkerframework.specimin; -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 org.checkerframework.checker.nullness.qual.Nullable; - import com.github.javaparser.ast.Node; import com.github.javaparser.ast.NodeList; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; @@ -27,6 +18,13 @@ 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 org.checkerframework.checker.nullness.qual.Nullable; /** * If a used class includes methods that must be implemented (because it extends an abstract class diff --git a/src/test/java/org/checkerframework/specimin/Issue103Test.java b/src/test/java/org/checkerframework/specimin/Issue103Test.java index 9dc069e4..d0be425f 100644 --- a/src/test/java/org/checkerframework/specimin/Issue103Test.java +++ b/src/test/java/org/checkerframework/specimin/Issue103Test.java @@ -1,13 +1,14 @@ 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. -*/ +/** + * 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 { From c2782cc0f06524bfa473650cf9b8ba8e4427fb20 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 23 Jul 2024 20:13:06 -0400 Subject: [PATCH 05/17] Fix UnsolvedSymbolException --- .../specimin/MustImplementMethodsVisitor.java | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 6ac92164..d88317a5 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -123,6 +123,11 @@ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { return true; } + if (overridden != null) { + return false; + } + // The parent method is abstract, we should continue upwards + ResolvedMethodDeclaration resolvedMethod; try { resolvedMethod = method.resolve(); @@ -136,23 +141,29 @@ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { for (ResolvedReferenceType implementation : getAllImplementations(new HashSet<>(resolvedMethod.declaringType().getAncestors()))) { - for (MethodUsage potentialSuperMethod : implementation.getDeclaredMethods()) { - if (potentialSuperMethod.getDeclaration().isAbstract()) { + try { + for (MethodUsage potentialSuperMethod : implementation.getDeclaredMethods()) { String methodSignature = potentialSuperMethod.getQualifiedSignature(); String potentialSuperMethodName = methodSignature.substring(methodSignature.lastIndexOf('.') + 1); if (!currentMethodName.equals(potentialSuperMethodName)) { continue; } - // These classes are beyond our control. It's better to retain the implementations of all - // abstract methods to ensure the code remains compilable. - if (JavaLangUtils.inJdkPackage(methodSignature)) { - return true; - } - if (usedMembers.contains(methodSignature)) { - return true; + if (potentialSuperMethod.getDeclaration().isAbstract()) { + // These classes are beyond our control. It's better to retain the implementations of + // all + // abstract methods to ensure the code remains compilable. + if (JavaLangUtils.inJdkPackage(methodSignature)) { + return true; + } + if (usedMembers.contains(methodSignature)) { + return true; + } } } + } catch (UnsolvedSymbolException ex) { + // At least one of the methods can't be solved so we will ignore this type. + continue; } } From 77cb8449978c78571daf927409f9db115a0b991b Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 23 Jul 2024 20:20:03 -0400 Subject: [PATCH 06/17] Adjust test case to match recent changes --- .../com/example/AbstractLinkedDeque.java | 17 ----------------- .../expected/com/example/AccessOrderDeque.java | 4 ---- 2 files changed, 21 deletions(-) diff --git a/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java b/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java index 4a4ee29b..8729a199 100644 --- a/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java +++ b/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java @@ -1,22 +1,13 @@ 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() { @@ -55,10 +46,6 @@ public boolean offerLast(E e) { throw new Error(); } - public boolean add(E e) { - throw new Error(); - } - public void addFirst(E e) { throw new Error(); } @@ -99,10 +86,6 @@ public boolean removeLastOccurrence(Object o) { throw new Error(); } - public boolean removeAll(Collection c) { - throw new Error(); - } - public void push(E e) { 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 index 8099dfe8..62391b42 100644 --- a/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java +++ b/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java @@ -7,8 +7,4 @@ public final class AccessOrderDeque extends AbstractLinkedDeque { public boolean contains(Object o) { throw new Error(); } - - public boolean remove(Object o) { - throw new Error(); - } } From 3d7579e35317d6600933d90021850f165be2810e Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 23 Jul 2024 20:22:38 -0400 Subject: [PATCH 07/17] `getQualifiedSignature()` can also throw UnsolvedSymbolException --- .../checkerframework/specimin/MustImplementMethodsVisitor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index d88317a5..3eb5008b 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -129,13 +129,14 @@ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { // The parent method is abstract, we should continue upwards ResolvedMethodDeclaration resolvedMethod; + String currentMethodSignature; try { resolvedMethod = method.resolve(); + currentMethodSignature = resolvedMethod.getQualifiedSignature(); } catch (UnsolvedSymbolException ex) { return false; } - String currentMethodSignature = resolvedMethod.getQualifiedSignature(); String currentMethodName = currentMethodSignature.substring(currentMethodSignature.lastIndexOf('.') + 1); From fa721f74f78092ba9acccce008ae909389013e16 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 23 Jul 2024 22:14:38 -0400 Subject: [PATCH 08/17] Method name code was wrong --- .../specimin/MustImplementMethodsVisitor.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 3eb5008b..cd4e5c8f 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -138,22 +138,22 @@ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { } String currentMethodName = - currentMethodSignature.substring(currentMethodSignature.lastIndexOf('.') + 1); - + currentMethodSignature.substring( + currentMethodSignature.lastIndexOf('.', currentMethodSignature.indexOf('(')) + 1); for (ResolvedReferenceType implementation : getAllImplementations(new HashSet<>(resolvedMethod.declaringType().getAncestors()))) { try { for (MethodUsage potentialSuperMethod : implementation.getDeclaredMethods()) { String methodSignature = potentialSuperMethod.getQualifiedSignature(); String potentialSuperMethodName = - methodSignature.substring(methodSignature.lastIndexOf('.') + 1); + methodSignature.substring( + methodSignature.lastIndexOf('.', methodSignature.indexOf('(')) + 1); if (!currentMethodName.equals(potentialSuperMethodName)) { continue; } if (potentialSuperMethod.getDeclaration().isAbstract()) { // These classes are beyond our control. It's better to retain the implementations of - // all - // abstract methods to ensure the code remains compilable. + // all abstract methods to ensure the code remains compilable. if (JavaLangUtils.inJdkPackage(methodSignature)) { return true; } From 9d0eda2a7bbeb45aace93f3800746d1915f7a55e Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Wed, 24 Jul 2024 10:09:36 -0400 Subject: [PATCH 09/17] Handle interface/abstract methods a different way --- .../specimin/MustImplementMethodsVisitor.java | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index cd4e5c8f..c7517b91 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -13,6 +13,7 @@ import com.github.javaparser.ast.visitor.Visitable; import com.github.javaparser.resolution.MethodUsage; import com.github.javaparser.resolution.UnsolvedSymbolException; +import com.github.javaparser.resolution.declarations.ResolvedClassDeclaration; import com.github.javaparser.resolution.declarations.ResolvedMethodDeclaration; import com.github.javaparser.resolution.declarations.ResolvedParameterDeclaration; import com.github.javaparser.resolution.types.ResolvedReferenceType; @@ -137,6 +138,25 @@ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { return false; } + boolean isInterfaceOrAbstract = resolvedMethod.declaringType().isInterface(); + + if (!isInterfaceOrAbstract && resolvedMethod.declaringType().isClass()) { + ResolvedClassDeclaration classDecl = resolvedMethod.declaringType().asClass(); + // Check to see if an abstract method exists. If it does, then this class is abstract. + // getAllMethods() includes inherited methods as well; those should be overridden already + // if the class is abstract. + try { + for (MethodUsage methodUsage : classDecl.getAllMethods()) { + if (methodUsage.getDeclaration().isAbstract()) { + isInterfaceOrAbstract = true; + break; + } + } + } catch (UnsolvedSymbolException ex) { + isInterfaceOrAbstract = false; + } + } + String currentMethodName = currentMethodSignature.substring( currentMethodSignature.lastIndexOf('.', currentMethodSignature.indexOf('(')) + 1); @@ -154,10 +174,12 @@ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { if (potentialSuperMethod.getDeclaration().isAbstract()) { // These classes are beyond our control. It's better to retain the implementations of // all abstract methods to ensure the code remains compilable. - if (JavaLangUtils.inJdkPackage(methodSignature)) { + if (usedMembers.contains(methodSignature)) { return true; } - if (usedMembers.contains(methodSignature)) { + // If the member is not used (inherit from JDK), and we're in an interface or + // abstract class, no need to preserve it since original JDK definition will persist + if (JavaLangUtils.inJdkPackage(methodSignature) && !isInterfaceOrAbstract) { return true; } } From b4203b8463003c4fe0a9fe6f63de9793e1fdba50 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Wed, 24 Jul 2024 10:14:32 -0400 Subject: [PATCH 10/17] Add thrown exceptions --- .../specimin/MustImplementMethodsVisitor.java | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index c7517b91..d6e70f65 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -1,5 +1,14 @@ package org.checkerframework.specimin; +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 org.checkerframework.checker.nullness.qual.Nullable; + import com.github.javaparser.ast.Node; import com.github.javaparser.ast.NodeList; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; @@ -10,6 +19,7 @@ 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.MethodUsage; import com.github.javaparser.resolution.UnsolvedSymbolException; @@ -19,13 +29,6 @@ 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 org.checkerframework.checker.nullness.qual.Nullable; /** * If a used class includes methods that must be implemented (because it extends an abstract class @@ -68,13 +71,17 @@ public Visitable visit(MethodDeclaration method, Void p) { if (ancestorMethodPreservedAndAbstract(method) || (overridden == null && overridesAnInterfaceMethod(method))) { ResolvedMethodDeclaration resolvedMethod = method.resolve(); - Map returnAndParamTypes = new HashMap<>(); + Map returnAndParamAndThrowTypes = new HashMap<>(); try { - returnAndParamTypes.put( + returnAndParamAndThrowTypes.put( resolvedMethod.getReturnType().describe(), resolvedMethod.getReturnType()); for (int i = 0; i < resolvedMethod.getNumberOfParams(); ++i) { ResolvedParameterDeclaration param = resolvedMethod.getParam(i); - returnAndParamTypes.put(param.describeType(), param.getType()); + 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, @@ -83,7 +90,7 @@ public Visitable visit(MethodDeclaration method, Void p) { return super.visit(method, p); } usedMembers.add(resolvedMethod.getQualifiedSignature()); - for (String type : returnAndParamTypes.keySet()) { + for (String type : returnAndParamAndThrowTypes.keySet()) { String originalType = type; type = type.trim(); if (type.contains("<")) { @@ -100,7 +107,7 @@ public Visitable visit(MethodDeclaration method, Void p) { usedTypeElements.add(type); - ResolvedType resolvedType = returnAndParamTypes.get(originalType); + ResolvedType resolvedType = returnAndParamAndThrowTypes.get(originalType); if (!previouslyIncluded && resolvedType != null && resolvedType.isReferenceType()) { addAllResolvableAncestors(resolvedType.asReferenceType()); From 81387079bdf0590f81c67ad8ba0a354163b7bc85 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Wed, 24 Jul 2024 10:23:29 -0400 Subject: [PATCH 11/17] changes should only apply to abstract methods --- .../specimin/MustImplementMethodsVisitor.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index d6e70f65..66e46ec9 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -184,9 +184,11 @@ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { if (usedMembers.contains(methodSignature)) { return true; } - // If the member is not used (inherit from JDK), and we're in an interface or - // abstract class, no need to preserve it since original JDK definition will persist - if (JavaLangUtils.inJdkPackage(methodSignature) && !isInterfaceOrAbstract) { + // If the abstract member is not used, and we're in an interface or abstract class, + // there is no need to preserve it since the original JDK abstract definition will + // be inherited + if (JavaLangUtils.inJdkPackage(methodSignature) + && (!resolvedMethod.isAbstract() || !isInterfaceOrAbstract)) { return true; } } From 791b4af000ac596bc9d013f8ac21100005070f06 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Wed, 24 Jul 2024 10:27:20 -0400 Subject: [PATCH 12/17] reflect latest changes for test cases --- .../issue103/expected/com/example/AbstractLinkedDeque.java | 2 -- .../issue103/expected/com/example/AccessOrderDeque.java | 4 ---- 2 files changed, 6 deletions(-) diff --git a/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java b/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java index 8729a199..6b1c162c 100644 --- a/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java +++ b/src/test/resources/issue103/expected/com/example/AbstractLinkedDeque.java @@ -8,8 +8,6 @@ public int size() { throw new Error(); } - public abstract boolean contains(Object o); - public E peek() { 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 index 62391b42..3c351b1d 100644 --- a/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java +++ b/src/test/resources/issue103/expected/com/example/AccessOrderDeque.java @@ -3,8 +3,4 @@ import java.util.Deque; public final class AccessOrderDeque extends AbstractLinkedDeque { - - public boolean contains(Object o) { - throw new Error(); - } } From af9d177e98ab74c6a9eefeffe35480870333d08b Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Wed, 24 Jul 2024 18:19:34 -0400 Subject: [PATCH 13/17] Only handle abstract classes in ancestorMethodPreservedAndAbstract --- .../specimin/MustImplementMethodsVisitor.java | 73 +++++++++++-------- 1 file changed, 41 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 66e46ec9..67e4d010 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -1,14 +1,5 @@ package org.checkerframework.specimin; -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 org.checkerframework.checker.nullness.qual.Nullable; - import com.github.javaparser.ast.Node; import com.github.javaparser.ast.NodeList; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; @@ -26,9 +17,17 @@ import com.github.javaparser.resolution.declarations.ResolvedClassDeclaration; import com.github.javaparser.resolution.declarations.ResolvedMethodDeclaration; import com.github.javaparser.resolution.declarations.ResolvedParameterDeclaration; +import com.github.javaparser.resolution.declarations.ResolvedTypeDeclaration; 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 org.checkerframework.checker.nullness.qual.Nullable; /** * If a used class includes methods that must be implemented (because it extends an abstract class @@ -169,33 +168,43 @@ private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { currentMethodSignature.lastIndexOf('.', currentMethodSignature.indexOf('(')) + 1); for (ResolvedReferenceType implementation : getAllImplementations(new HashSet<>(resolvedMethod.declaringType().getAncestors()))) { - try { - for (MethodUsage potentialSuperMethod : implementation.getDeclaredMethods()) { - String methodSignature = potentialSuperMethod.getQualifiedSignature(); - String potentialSuperMethodName = - methodSignature.substring( - methodSignature.lastIndexOf('.', methodSignature.indexOf('(')) + 1); - if (!currentMethodName.equals(potentialSuperMethodName)) { - continue; - } - if (potentialSuperMethod.getDeclaration().isAbstract()) { - // These classes are beyond our control. It's better to retain the implementations of - // all abstract methods to ensure the code remains compilable. - if (usedMembers.contains(methodSignature)) { - return true; + ResolvedTypeDeclaration implementationDeclaration = + implementation.getTypeDeclaration().orElse(null); + // Only process classes here because interfaces are dealt with in overridesAnInterfaceMethod + if (implementationDeclaration != null && !implementationDeclaration.isInterface()) { + try { + for (MethodUsage potentialSuperMethod : implementation.getDeclaredMethods()) { + String methodSignature = potentialSuperMethod.getQualifiedSignature(); + String potentialSuperMethodName = + methodSignature.substring( + methodSignature.lastIndexOf('.', methodSignature.indexOf('(')) + 1); + if (!currentMethodName.equals(potentialSuperMethodName)) { + continue; } - // If the abstract member is not used, and we're in an interface or abstract class, - // there is no need to preserve it since the original JDK abstract definition will - // be inherited - if (JavaLangUtils.inJdkPackage(methodSignature) - && (!resolvedMethod.isAbstract() || !isInterfaceOrAbstract)) { - return true; + if (potentialSuperMethod.getDeclaration().isAbstract()) { + // These classes are beyond our control. It's better to retain the implementations of + // all abstract methods to ensure the code remains compilable. + if (usedMembers.contains(methodSignature)) { + return true; + } + // If the abstract member is not used, and we're in an interface or abstract class, + // there is no need to preserve it since the original JDK abstract definition will + // be inherited + if (JavaLangUtils.inJdkPackage(methodSignature) + && (!resolvedMethod.isAbstract() || !isInterfaceOrAbstract)) { + return true; + } + } else if (usedMembers.contains(methodSignature)) { + // If any ancestors define the body of a method, and that ancestor definition + // is already included, we don't need to preserve any overrides since the method + // already has a defined body. + return false; } } + } catch (UnsolvedSymbolException ex) { + // At least one of the methods can't be solved so we will ignore this type. + continue; } - } catch (UnsolvedSymbolException ex) { - // At least one of the methods can't be solved so we will ignore this type. - continue; } } From e8e78358b31e9085995edbe1e0496fbf9473c54a Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Wed, 24 Jul 2024 18:25:07 -0400 Subject: [PATCH 14/17] fix no `@return` warning --- .../org/checkerframework/specimin/SpeciminStateVisitor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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; From 4c73f0ef630b003ab432bb7a8cbcec72ab6eb7f6 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Wed, 24 Jul 2024 23:12:54 -0400 Subject: [PATCH 15/17] fix integration test bugs? --- .../specimin/MustImplementMethodsVisitor.java | 162 +++++------------- .../specimin/SpeciminRunner.java | 51 ++++-- 2 files changed, 82 insertions(+), 131 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 67e4d010..71b88e91 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -12,12 +12,9 @@ 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.MethodUsage; import com.github.javaparser.resolution.UnsolvedSymbolException; -import com.github.javaparser.resolution.declarations.ResolvedClassDeclaration; import com.github.javaparser.resolution.declarations.ResolvedMethodDeclaration; import com.github.javaparser.resolution.declarations.ResolvedParameterDeclaration; -import com.github.javaparser.resolution.declarations.ResolvedTypeDeclaration; import com.github.javaparser.resolution.types.ResolvedReferenceType; import com.github.javaparser.resolution.types.ResolvedType; import com.github.javaparser.resolution.types.parametrization.ResolvedTypeParametersMap; @@ -67,7 +64,7 @@ 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.) - if (ancestorMethodPreservedAndAbstract(method) + if (isPreservedAndAbstract(overridden) || (overridden == null && overridesAnInterfaceMethod(method))) { ResolvedMethodDeclaration resolvedMethod = method.resolve(); Map returnAndParamAndThrowTypes = new HashMap<>(); @@ -116,101 +113,6 @@ public Visitable visit(MethodDeclaration method, Void p) { return super.visit(method, p); } - /** - * Returns true iff any parent method is abstract and preserved. Use this method if it is unclear - * whether the direct super method will be preserved or not. - * - * @param method The method declaration to check - * @return true iff any parent method is abstract and preserved - */ - private boolean ancestorMethodPreservedAndAbstract(MethodDeclaration method) { - ResolvedMethodDeclaration overridden = getOverriddenMethodInSuperClass(method); - - if (isPreservedAndAbstract(overridden)) { - return true; - } - - if (overridden != null) { - return false; - } - // The parent method is abstract, we should continue upwards - - ResolvedMethodDeclaration resolvedMethod; - String currentMethodSignature; - try { - resolvedMethod = method.resolve(); - currentMethodSignature = resolvedMethod.getQualifiedSignature(); - } catch (UnsolvedSymbolException ex) { - return false; - } - - boolean isInterfaceOrAbstract = resolvedMethod.declaringType().isInterface(); - - if (!isInterfaceOrAbstract && resolvedMethod.declaringType().isClass()) { - ResolvedClassDeclaration classDecl = resolvedMethod.declaringType().asClass(); - // Check to see if an abstract method exists. If it does, then this class is abstract. - // getAllMethods() includes inherited methods as well; those should be overridden already - // if the class is abstract. - try { - for (MethodUsage methodUsage : classDecl.getAllMethods()) { - if (methodUsage.getDeclaration().isAbstract()) { - isInterfaceOrAbstract = true; - break; - } - } - } catch (UnsolvedSymbolException ex) { - isInterfaceOrAbstract = false; - } - } - - String currentMethodName = - currentMethodSignature.substring( - currentMethodSignature.lastIndexOf('.', currentMethodSignature.indexOf('(')) + 1); - for (ResolvedReferenceType implementation : - getAllImplementations(new HashSet<>(resolvedMethod.declaringType().getAncestors()))) { - ResolvedTypeDeclaration implementationDeclaration = - implementation.getTypeDeclaration().orElse(null); - // Only process classes here because interfaces are dealt with in overridesAnInterfaceMethod - if (implementationDeclaration != null && !implementationDeclaration.isInterface()) { - try { - for (MethodUsage potentialSuperMethod : implementation.getDeclaredMethods()) { - String methodSignature = potentialSuperMethod.getQualifiedSignature(); - String potentialSuperMethodName = - methodSignature.substring( - methodSignature.lastIndexOf('.', methodSignature.indexOf('(')) + 1); - if (!currentMethodName.equals(potentialSuperMethodName)) { - continue; - } - if (potentialSuperMethod.getDeclaration().isAbstract()) { - // These classes are beyond our control. It's better to retain the implementations of - // all abstract methods to ensure the code remains compilable. - if (usedMembers.contains(methodSignature)) { - return true; - } - // If the abstract member is not used, and we're in an interface or abstract class, - // there is no need to preserve it since the original JDK abstract definition will - // be inherited - if (JavaLangUtils.inJdkPackage(methodSignature) - && (!resolvedMethod.isAbstract() || !isInterfaceOrAbstract)) { - return true; - } - } else if (usedMembers.contains(methodSignature)) { - // If any ancestors define the body of a method, and that ancestor definition - // is already included, we don't need to preserve any overrides since the method - // already has a defined body. - return false; - } - } - } catch (UnsolvedSymbolException ex) { - // At least one of the methods can't be solved so we will ignore this type. - continue; - } - } - } - - return false; - } - /** * Returns true if the given method is abstract. * @@ -252,10 +154,19 @@ 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); @@ -283,15 +194,18 @@ private void addAllResolvableAncestors(ResolvedReferenceType resolvedType) { * 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) @@ -328,34 +242,46 @@ 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()); + 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; + } } } } // 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; } /** - * Helper method for getAllImplementations(Set) + * Helper method to convert ClassOrInterfaceTypes to ResolvedReferenceTypes * - * @param types A List of interface/class types to find all ancestors - * @return A Collection of ResolvedReferenceTypes containing all ancestors + * @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. @@ -363,7 +289,7 @@ private static Collection getAllImplementations( } } - return getAllImplementations(toTraverse); + return resolvedTypes; } /** 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()); From d7f65adb3f723bb5021574123ca498411f62090a Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Thu, 25 Jul 2024 12:27:29 -0400 Subject: [PATCH 16/17] fix errors --- .../specimin/MustImplementMethodsVisitor.java | 76 +++++++++++++++---- 1 file changed, 60 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 71b88e91..4698df36 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -24,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; /** @@ -64,8 +65,12 @@ 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(); Map returnAndParamAndThrowTypes = new HashMap<>(); try { @@ -132,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. @@ -229,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 @@ -242,23 +279,30 @@ private boolean overridesAnInterfaceMethodImpl( for (ResolvedMethodDeclaration methodInInterface : resolvedInterface.getAllMethodsVisibleToInheritors()) { - 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; + 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; } - } 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; } } } From a49c0920827a45afd6459e8889289aa882a08e29 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Thu, 25 Jul 2024 13:29:22 -0400 Subject: [PATCH 17/17] add comment --- .../checkerframework/specimin/MustImplementMethodsVisitor.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index 4698df36..cfc0a59e 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -265,6 +265,8 @@ 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); }