Remove unused ElementTypes in @Target (#355)

Removes unused `ElementType`s in `@Target` annotations. The current approach is to only remove `ElementType`s, not add them, so we don't accidentally make a non `TYPE_USE` annotation into a `TYPE_USE` annotation, for example. I also addressed two small bugs I encountered: - When all the wildcard import are JDK imports, synthetic classes should be generated in the current package instead of the first wildcard import package - Don't throw an exception when an annotation is applied on a package declaration Instead of creating a new test case for this, I just modified one of the existing ones (`SyntheticAnnotationTargetTest`), since it's purpose already seemed to fit this PR. Thanks!
njit-jerse · Aug 26, 2024 · 20a1a97 · 20a1a97
1 parent 4b16216
commit 20a1a97
Show file tree

Hide file tree

Showing 38 changed files with 405 additions and 50 deletions.
diff --git a/src/main/java/org/checkerframework/specimin/AnnotationParameterTypesVisitor.java b/src/main/java/org/checkerframework/specimin/AnnotationParameterTypesVisitor.java
@@ -3,6 +3,7 @@
 import com.github.javaparser.ast.ImportDeclaration;
 import com.github.javaparser.ast.Node;
 import com.github.javaparser.ast.NodeList;
+import com.github.javaparser.ast.PackageDeclaration;
 import com.github.javaparser.ast.body.AnnotationMemberDeclaration;
 import com.github.javaparser.ast.expr.AnnotationExpr;
 import com.github.javaparser.ast.expr.ArrayInitializerExpr;
@@ -112,6 +113,11 @@ public Visitable visit(AnnotationMemberDeclaration decl, Void p) {
 
   @Override
   public Visitable visit(MarkerAnnotationExpr anno, Void p) {
+    // Annotations on packages cause an exception in findClosestParentMemberOrClassLike
+    if (anno.hasParentNode() && anno.getParentNode().get() instanceof PackageDeclaration) {
+      return super.visit(anno, p);
+    }
+
     Node parent = JavaParserUtil.findClosestParentMemberOrClassLike(anno);
 
     if (isTargetOrUsed(parent)) {
@@ -122,6 +128,11 @@ public Visitable visit(MarkerAnnotationExpr anno, Void p) {
 
   @Override
   public Visitable visit(SingleMemberAnnotationExpr anno, Void p) {
+    // Annotations on packages cause an exception in findClosestParentMemberOrClassLike
+    if (anno.hasParentNode() && anno.getParentNode().get() instanceof PackageDeclaration) {
+      return super.visit(anno, p);
+    }
+
     Node parent = JavaParserUtil.findClosestParentMemberOrClassLike(anno);
 
     if (isTargetOrUsed(parent)) {
@@ -132,6 +143,11 @@ public Visitable visit(SingleMemberAnnotationExpr anno, Void p) {
 
   @Override
   public Visitable visit(NormalAnnotationExpr anno, Void p) {
+    // Annotations on packages cause an exception in findClosestParentMemberOrClassLike
+    if (anno.hasParentNode() && anno.getParentNode().get() instanceof PackageDeclaration) {
+      return super.visit(anno, p);
+    }
+
     Node parent = JavaParserUtil.findClosestParentMemberOrClassLike(anno);
 
     if (isTargetOrUsed(parent)) {

diff --git a/src/main/java/org/checkerframework/specimin/AnnotationTargetRemoverVisitor.java b/src/main/java/org/checkerframework/specimin/AnnotationTargetRemoverVisitor.java
@@ -0,0 +1,254 @@
+package org.checkerframework.specimin;
+
+import com.github.javaparser.StaticJavaParser;
+import com.github.javaparser.ast.Node;
+import com.github.javaparser.ast.PackageDeclaration;
+import com.github.javaparser.ast.body.AnnotationDeclaration;
+import com.github.javaparser.ast.body.AnnotationMemberDeclaration;
+import com.github.javaparser.ast.body.ConstructorDeclaration;
+import com.github.javaparser.ast.body.EnumConstantDeclaration;
+import com.github.javaparser.ast.body.FieldDeclaration;
+import com.github.javaparser.ast.body.MethodDeclaration;
+import com.github.javaparser.ast.body.Parameter;
+import com.github.javaparser.ast.body.RecordDeclaration;
+import com.github.javaparser.ast.body.TypeDeclaration;
+import com.github.javaparser.ast.expr.AnnotationExpr;
+import com.github.javaparser.ast.expr.ArrayInitializerExpr;
+import com.github.javaparser.ast.expr.Expression;
+import com.github.javaparser.ast.expr.FieldAccessExpr;
+import com.github.javaparser.ast.expr.MarkerAnnotationExpr;
+import com.github.javaparser.ast.expr.NameExpr;
+import com.github.javaparser.ast.expr.NormalAnnotationExpr;
+import com.github.javaparser.ast.expr.SingleMemberAnnotationExpr;
+import com.github.javaparser.ast.expr.VariableDeclarationExpr;
+import com.github.javaparser.ast.type.TypeParameter;
+import com.github.javaparser.ast.visitor.ModifierVisitor;
+import com.github.javaparser.ast.visitor.Visitable;
+import java.lang.annotation.ElementType;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Removes all unnecessary ElementType values from each annotation declaration, based on their
+ * usages. Run this visitor after PrunerVisitor to ensure all unnecessary ElementTypes are removed.
+ */
+public class AnnotationTargetRemoverVisitor extends ModifierVisitor<Void> {
+  /** A Map of fully qualified annotation names to its ElementTypes. */
+  private final Map<String, Set<ElementType>> annotationToElementTypes = new HashMap<>();
+
+  /** A Map of fully qualified annotation names to their declarations. */
+  private final Map<String, AnnotationDeclaration> annotationToDeclaration = new HashMap<>();
+
+  /** Constant for the package of {@code @Target} ({@code java.lang.annotation}) */
+  private static final String TARGET_PACKAGE = "java.lang.annotation";
+
+  /** Constant for {@code @Target}'s name (does not include the {@code @}) */
+  private static final String TARGET_NAME = "Target";
+
+  /** Constant for {@code @Target}'s fully qualified name */
+  private static final String FULLY_QUALIFIED_TARGET = TARGET_PACKAGE + "." + TARGET_NAME;
+
+  /**
+   * Updates all annotation declaration {@code @Target} values to match their usages. Only removes
+   * {@code ElementType}s, doesn't add them. Call this method once after visiting all files.
+   */
+  public void removeExtraAnnotationTargets() {
+    for (Map.Entry<String, AnnotationDeclaration> pair : annotationToDeclaration.entrySet()) {
+      AnnotationExpr targetAnnotation =
+          pair.getValue().getAnnotationByName(TARGET_NAME).orElse(null);
+
+      if (targetAnnotation == null) {
+        // This is most likely an existing definition. If there is no @Target annotation,
+        // we shouldn't add it
+        continue;
+      }
+
+      boolean useFullyQualified = targetAnnotation.getNameAsString().equals(FULLY_QUALIFIED_TARGET);
+
+      // Only handle java.lang.annotation.Target
+      if (!targetAnnotation.resolve().getQualifiedName().equals(FULLY_QUALIFIED_TARGET)) {
+        continue;
+      }
+
+      Set<ElementType> actualElementTypes = annotationToElementTypes.get(pair.getKey());
+
+      if (actualElementTypes == null) {
+        // No usages of the annotation itself (see Issue272Test)
+        actualElementTypes = new HashSet<>();
+      }
+
+      Set<ElementType> elementTypes = new HashSet<>();
+      Set<ElementType> staticallyImportedElementTypes = new HashSet<>();
+      Expression memberValue;
+
+      // @Target(ElementType.___)
+      // @Target({ElementType.___, ElementType.___})
+      if (targetAnnotation.isSingleMemberAnnotationExpr()) {
+        SingleMemberAnnotationExpr asSingleMember = targetAnnotation.asSingleMemberAnnotationExpr();
+        memberValue = asSingleMember.getMemberValue();
+      }
+      // @Target(value = ElementType.___)
+      // @Target(value = {ElementType.___, ElementType.___})
+      else if (targetAnnotation.isNormalAnnotationExpr()) {
+        NormalAnnotationExpr asNormal = targetAnnotation.asNormalAnnotationExpr();
+        memberValue = asNormal.getPairs().get(0).getValue();
+      } else {
+        throw new RuntimeException("@Target annotation must contain an ElementType");
+      }
+
+      // If there's only one ElementType, we can't remove anything
+      // We should only be removing ElementTypes, not adding: we do not want to
+      // convert a non TYPE_USE annotation to a TYPE_USE annotation, for example
+      if (memberValue.isFieldAccessExpr() || memberValue.isNameExpr()) {
+        continue;
+      } else if (memberValue.isArrayInitializerExpr()) {
+        ArrayInitializerExpr arrayExpr = memberValue.asArrayInitializerExpr();
+        if (arrayExpr.getValues().size() <= 1) {
+          continue;
+        }
+        for (Expression value : arrayExpr.getValues()) {
+          if (value.isFieldAccessExpr()) {
+            FieldAccessExpr fieldAccessExpr = value.asFieldAccessExpr();
+            ElementType elementType = ElementType.valueOf(fieldAccessExpr.getNameAsString());
+            if (actualElementTypes.contains(elementType)) {
+              elementTypes.add(elementType);
+            }
+          } else if (value.isNameExpr()) {
+            // In case of static imports
+            NameExpr nameExpr = value.asNameExpr();
+            ElementType elementType = ElementType.valueOf(nameExpr.getNameAsString());
+            if (actualElementTypes.contains(elementType)) {
+              elementTypes.add(elementType);
+              staticallyImportedElementTypes.add(elementType);
+            }
+          }
+        }
+      }
+
+      StringBuilder newAnnotation = new StringBuilder();
+      newAnnotation.append("@");
+
+      if (useFullyQualified) {
+        newAnnotation.append(TARGET_PACKAGE);
+        newAnnotation.append(".");
+      }
+
+      newAnnotation.append(TARGET_NAME);
+      newAnnotation.append("(");
+
+      newAnnotation.append('{');
+
+      List<ElementType> sortedElementTypes = new ArrayList<>(elementTypes);
+      Collections.sort(sortedElementTypes, (a, b) -> a.name().compareTo(b.name()));
+
+      for (int i = 0; i < sortedElementTypes.size(); i++) {
+        ElementType elementType = sortedElementTypes.get(i);
+        if (!staticallyImportedElementTypes.contains(elementType)) {
+          if (useFullyQualified) {
+            newAnnotation.append(TARGET_PACKAGE);
+            newAnnotation.append(".");
+          }
+          newAnnotation.append("ElementType.");
+        }
+        newAnnotation.append(elementType.name());
+
+        if (i < sortedElementTypes.size() - 1) {
+          newAnnotation.append(", ");
+        }
+      }
+
+      newAnnotation.append("})");
+      AnnotationExpr trimmed = StaticJavaParser.parseAnnotation(newAnnotation.toString());
+
+      targetAnnotation.remove();
+
+      pair.getValue().addAnnotation(trimmed);
+    }
+  }
+
+  @Override
+  public Visitable visit(AnnotationDeclaration decl, Void p) {
+    annotationToDeclaration.put(decl.getFullyQualifiedName().get(), decl);
+    return super.visit(decl, p);
+  }
+
+  @Override
+  public Visitable visit(MarkerAnnotationExpr anno, Void p) {
+    updateAnnotationElementTypes(anno);
+    return super.visit(anno, p);
+  }
+
+  @Override
+  public Visitable visit(NormalAnnotationExpr anno, Void p) {
+    updateAnnotationElementTypes(anno);
+    return super.visit(anno, p);
+  }
+
+  @Override
+  public Visitable visit(SingleMemberAnnotationExpr anno, Void p) {
+    updateAnnotationElementTypes(anno);
+    return super.visit(anno, p);
+  }
+
+  /**
+   * Helper method to update the ElementTypes for an annotation.
+   *
+   * @param anno The annotation to update element types for
+   */
+  private void updateAnnotationElementTypes(AnnotationExpr anno) {
+    Node parent = anno.getParentNode().orElse(null);
+
+    if (parent == null) {
+      return;
+    }
+
+    Set<ElementType> elementTypes = annotationToElementTypes.get(anno.resolve().getQualifiedName());
+    if (elementTypes == null) {
+      elementTypes = new HashSet<>();
+      annotationToElementTypes.put(anno.resolve().getQualifiedName(), elementTypes);
+    }
+
+    if (parent instanceof AnnotationDeclaration) {
+      elementTypes.add(ElementType.ANNOTATION_TYPE);
+      elementTypes.add(ElementType.TYPE);
+    } else if (parent instanceof ConstructorDeclaration) {
+      elementTypes.add(ElementType.CONSTRUCTOR);
+    } else if (parent instanceof FieldDeclaration || parent instanceof EnumConstantDeclaration) {
+      elementTypes.add(ElementType.FIELD);
+    } else if (parent instanceof VariableDeclarationExpr) {
+      elementTypes.add(ElementType.LOCAL_VARIABLE);
+    } else if (parent instanceof MethodDeclaration) {
+      elementTypes.add(ElementType.METHOD);
+
+      if (((MethodDeclaration) parent).getType().isVoidType()) {
+        // If it's void we don't need to add TYPE_USE
+        return;
+      }
+    } else if (parent instanceof AnnotationMemberDeclaration) {
+      elementTypes.add(ElementType.METHOD);
+    } else if (parent instanceof PackageDeclaration) {
+      elementTypes.add(ElementType.PACKAGE);
+      return;
+    } else if (parent instanceof Parameter) {
+      if (parent.getParentNode().isPresent()
+          && parent.getParentNode().get() instanceof RecordDeclaration) {
+        elementTypes.add(ElementType.RECORD_COMPONENT);
+      } else {
+        elementTypes.add(ElementType.PARAMETER);
+      }
+    } else if (parent instanceof TypeDeclaration) {
+      // TypeDeclaration is the parent class for class, interface, annotation, record declarations
+      // https://www.javadoc.io/doc/com.github.javaparser/javaparser-core/latest/com/github/javaparser/ast/body/TypeDeclaration.html
+      elementTypes.add(ElementType.TYPE);
+    } else if (parent instanceof TypeParameter) {
+      elementTypes.add(ElementType.TYPE_PARAMETER);
+    }
+
+    elementTypes.add(ElementType.TYPE_USE);
+  }
+}
diff --git a/src/main/java/org/checkerframework/specimin/SpeciminRunner.java b/src/main/java/org/checkerframework/specimin/SpeciminRunner.java
@@ -512,6 +512,7 @@ private static void performMinimizationImpl(
       cu.accept(methodPruner, null);
     }
 
+    pruneAnnotationDeclarationTargets(parsedTargetFiles);
     removeUnusedImports(parsedTargetFiles);
 
     // cache to avoid called Files.createDirectories repeatedly with the same arguments
@@ -661,6 +662,17 @@ private static SpeciminStateVisitor processAnnotationTypes(
     return annotationParameterTypesVisitor;
   }
 
+  /** Runs AnnotationTargetRemoverVisitor on the target files. Call after PrunerVisitor. */
+  private static void pruneAnnotationDeclarationTargets(
+      Map<String, CompilationUnit> parsedTargetFiles) {
+    AnnotationTargetRemoverVisitor targetPruner = new AnnotationTargetRemoverVisitor();
+    for (CompilationUnit cu : parsedTargetFiles.values()) {
+      cu.accept(targetPruner, null);
+    }
+
+    targetPruner.removeExtraAnnotationTargets();
+  }
+
   /**
    * Removes all unused imports in each output file through {@code UnusedImportRemoverVisitor}.
    *

diff --git a/src/main/java/org/checkerframework/specimin/UnsolvedClassOrInterface.java b/src/main/java/org/checkerframework/specimin/UnsolvedClassOrInterface.java
@@ -476,6 +476,7 @@ public String toString() {
               + "\tjava.lang.annotation.ElementType.LOCAL_VARIABLE, \n"
               + "\tjava.lang.annotation.ElementType.ANNOTATION_TYPE,\n"
               + "\tjava.lang.annotation.ElementType.PACKAGE,\n"
+              + "\tjava.lang.annotation.ElementType.TYPE_PARAMETER,\n"
               + "\tjava.lang.annotation.ElementType.TYPE_USE \n"
               + "})");
     }

diff --git a/src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java b/src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java
@@ -2885,10 +2885,15 @@ public String getPackageFromClassName(String className) {
           return wildcardPkg;
         }
       }
-      // If none do, then default to the first wildcard import.
+      // If none do, then default to the first wildcard import that is not a JDK package.
       // TODO: log a warning about this once we have a logger
-      String wildcardPkg = wildcardImports.get(0);
-      return wildcardPkg;
+      for (String wildcardPkg : wildcardImports) {
+        if (!JavaLangUtils.inJdkPackage(wildcardPkg)) {
+          return wildcardPkg;
+        }
+      }
+      // If we're here, all wildcard imports are jdk imports; use current package instead
+      return currentPackage;
     }
   }
 

diff --git a/src/test/java/org/checkerframework/specimin/SyntheticAnnotationTargetTest.java b/src/test/java/org/checkerframework/specimin/SyntheticAnnotationTargetTest.java
@@ -4,8 +4,8 @@
 import org.junit.Test;
 
 /**
- * This test checks if synthetic annotations used in different locations will compile based
- * on @Target.
+ * This test checks if synthetic annotations used in different locations will contain the proper
+ * ElementTypes in their {@code @Target} annotations
  */
 public class SyntheticAnnotationTargetTest {
   @Test

diff --git a/src/test/resources/annoingenerictarget/expected/com/example/Simple.java b/src/test/resources/annoingenerictarget/expected/com/example/Simple.java
@@ -5,6 +5,7 @@
 import org.checkerframework.checker.initialization.qual.Initialized;
 
 class Simple {
+
     @Initialized
     @NonNull
     @UnknownKeyFor

diff --git a/...ngenerictarget/expected/org/checkerframework/checker/initialization/qual/Initialized.java b/...ngenerictarget/expected/org/checkerframework/checker/initialization/qual/Initialized.java
@@ -1,5 +1,5 @@
 package org.checkerframework.checker.initialization.qual;
 
-@java.lang.annotation.Target({ java.lang.annotation.ElementType.TYPE, java.lang.annotation.ElementType.FIELD, java.lang.annotation.ElementType.METHOD, java.lang.annotation.ElementType.PARAMETER, java.lang.annotation.ElementType.CONSTRUCTOR, java.lang.annotation.ElementType.LOCAL_VARIABLE, java.lang.annotation.ElementType.ANNOTATION_TYPE, java.lang.annotation.ElementType.PACKAGE, java.lang.annotation.ElementType.TYPE_USE })
+@java.lang.annotation.Target({ java.lang.annotation.ElementType.METHOD, java.lang.annotation.ElementType.PARAMETER, java.lang.annotation.ElementType.TYPE_USE })
 public @interface Initialized {
 }
diff --git a/...annoingenerictarget/expected/org/checkerframework/checker/nullness/qual/KeyForBottom.java b/...annoingenerictarget/expected/org/checkerframework/checker/nullness/qual/KeyForBottom.java
@@ -1,5 +1,5 @@
 package org.checkerframework.checker.nullness.qual;
 
-@java.lang.annotation.Target({ java.lang.annotation.ElementType.TYPE, java.lang.annotation.ElementType.FIELD, java.lang.annotation.ElementType.METHOD, java.lang.annotation.ElementType.PARAMETER, java.lang.annotation.ElementType.CONSTRUCTOR, java.lang.annotation.ElementType.LOCAL_VARIABLE, java.lang.annotation.ElementType.ANNOTATION_TYPE, java.lang.annotation.ElementType.PACKAGE, java.lang.annotation.ElementType.TYPE_USE })
+@java.lang.annotation.Target({ java.lang.annotation.ElementType.TYPE_USE })
 public @interface KeyForBottom {
 }
diff --git a/...rces/annoingenerictarget/expected/org/checkerframework/checker/nullness/qual/NonNull.java b/...rces/annoingenerictarget/expected/org/checkerframework/checker/nullness/qual/NonNull.java
@@ -1,5 +1,5 @@
 package org.checkerframework.checker.nullness.qual;
 
-@java.lang.annotation.Target({ java.lang.annotation.ElementType.TYPE, java.lang.annotation.ElementType.FIELD, java.lang.annotation.ElementType.METHOD, java.lang.annotation.ElementType.PARAMETER, java.lang.annotation.ElementType.CONSTRUCTOR, java.lang.annotation.ElementType.LOCAL_VARIABLE, java.lang.annotation.ElementType.ANNOTATION_TYPE, java.lang.annotation.ElementType.PACKAGE, java.lang.annotation.ElementType.TYPE_USE })
+@java.lang.annotation.Target({ java.lang.annotation.ElementType.METHOD, java.lang.annotation.ElementType.PARAMETER, java.lang.annotation.ElementType.TYPE_USE })
 public @interface NonNull {
 }
diff --git a/...ces/annoingenerictarget/expected/org/checkerframework/checker/nullness/qual/Nullable.java b/...ces/annoingenerictarget/expected/org/checkerframework/checker/nullness/qual/Nullable.java
@@ -1,5 +1,5 @@
 package org.checkerframework.checker.nullness.qual;
 
-@java.lang.annotation.Target({ java.lang.annotation.ElementType.TYPE, java.lang.annotation.ElementType.FIELD, java.lang.annotation.ElementType.METHOD, java.lang.annotation.ElementType.PARAMETER, java.lang.annotation.ElementType.CONSTRUCTOR, java.lang.annotation.ElementType.LOCAL_VARIABLE, java.lang.annotation.ElementType.ANNOTATION_TYPE, java.lang.annotation.ElementType.PACKAGE, java.lang.annotation.ElementType.TYPE_USE })
+@java.lang.annotation.Target({ java.lang.annotation.ElementType.TYPE_USE })
 public @interface Nullable {
 }