JavaStubifier: improve unloadable-annotation failure DX

docs/CHANGELOG.md

@@ -139,6 +139,12 @@ Other improvements and bug fixes:
   recover Java type variables inferred by javac.
 - Fixed a latent aliasing bug in `AnnotatedTypeCopier` for executable types.
 - Fixed an `IndexOutOfBoundsException` for lambdas in varargs.
+- `JavaStubifier`'s "cannot load annotation" failure now names the source
+  file being processed, not just the annotation, making the offending file
+  easy to find in a large tree. Added a `--skipUnloadableAnnotations`
+  command-line flag that drops such an annotation from the binary stub
+  output (with a warning naming the annotation and file) instead of
+  aborting the run.
 
 **Closed issues:**
 

framework/src/stubifier/java/org/checkerframework/framework/stubifier/BinaryStubWriter.java

@@ -93,6 +93,10 @@
  * presence-only signature entries (see {@link ClassRecord#presenceOnlyMethodSigs}); that is correct
  * only for the annotated JDK. See that field's documentation. Equivalence is enforced by {@code
  * BinaryStubDiffChecker}; run {@code NullnessBinaryStubDiffTest} after changing this class.
+ *
+ * <p>A writer constructed with {@link #BinaryStubWriter(boolean, boolean)
+ * skipUnloadableAnnotations} drops, rather than fails on, an annotation whose {@code @Target}
+ * cannot be read; see that field's documentation for the trade-off this accepts.
  */
 public class BinaryStubWriter {
     /**
@@ -267,20 +271,52 @@ private static boolean isPlatformPackage(String name) {
      */
     private final boolean omitUnannotatedMembers;
 
+    /**
+     * Whether an annotation whose {@code @Target} cannot be read is dropped from the binary stub
+     * output -- with a warning to stderr naming the annotation and its source file -- instead of
+     * aborting the writer with the {@link #annotationTargets} failure. False by default, so the
+     * writer fails fast unless a caller opts in. Set from {@code JavaStubifier}'s {@code
+     * --skipUnloadableAnnotations} command-line flag; see that flag's Javadoc for the intended use.
+     *
+     * <p><b>Trade-off:</b> dropping such an annotation is not always safe. The annotation could be
+     * a real type qualifier that a checker's own (wider) classpath would have resolved; dropping it
+     * here silently removes it from the annotated JDK, and no checker run afterward has any way to
+     * tell that it is missing. This field exists so that trade-off is opt-in and always paired with
+     * a warning, rather than a hard-coded default -- it must never be set true except in response
+     * to an explicit user request to keep going past an unloadable annotation.
+     */
+    private final boolean skipUnloadableAnnotations;
+
     /** Creates a writer for a built-in stub file, which keeps every member record. */
     public BinaryStubWriter() {
-        this(false);
+        this(false, false);
     }
 
     /**
-     * Creates a writer.
+     * Creates a writer that fails on an unloadable annotation; see {@link
+     * #BinaryStubWriter(boolean, boolean)}.
      *
      * @param omitUnannotatedMembers whether to omit unannotated field records and demote
      *     unannotated method records to presence-only signature entries; see {@link
      *     #omitUnannotatedMembers}. Pass true only for the annotated JDK.
      */
     public BinaryStubWriter(boolean omitUnannotatedMembers) {
+        this(omitUnannotatedMembers, false);
+    }
+
+    /**
+     * Creates a writer.
+     *
+     * @param omitUnannotatedMembers whether to omit unannotated field records and demote
+     *     unannotated method records to presence-only signature entries; see {@link
+     *     #omitUnannotatedMembers}. Pass true only for the annotated JDK.
+     * @param skipUnloadableAnnotations whether to drop an annotation whose {@code @Target} cannot
+     *     be read, with a warning, instead of aborting the writer; see {@link
+     *     #skipUnloadableAnnotations} for the trade-off this accepts.
+     */
+    public BinaryStubWriter(boolean omitUnannotatedMembers, boolean skipUnloadableAnnotations) {
         this.omitUnannotatedMembers = omitUnannotatedMembers;
+        this.skipUnloadableAnnotations = skipUnloadableAnnotations;
     }
 
     /** Constant pool for strings to minimize binary size. */
@@ -927,6 +963,46 @@ private boolean isUnloadablePlatformAnnotation(String name) {
         return true;
     }
 
+    /**
+     * Annotation-and-file pairs already reported by {@link #dropUnloadableAnnotation}, so that an
+     * annotation used repeatedly in the same file (e.g. {@code @Test} on every method) produces one
+     * warning line rather than one per occurrence.
+     */
+    private final Set<String> reportedSkippedAnnotations = new HashSet<>();
+
+    /**
+     * If {@link #skipUnloadableAnnotations} is set, checks whether {@code anno}'s {@code @Target}
+     * can be read and, if not, prints a warning naming the annotation and its source file and
+     * reports that it must be dropped from routing. Otherwise -- including when {@link
+     * #skipUnloadableAnnotations} is false -- always returns false, leaving {@link
+     * #annotationTargets} to fail normally for a caller that goes on to route the annotation.
+     *
+     * @param anno the annotation expression to check
+     * @return true if {@code anno}'s {@code @Target} could not be read and {@link
+     *     #skipUnloadableAnnotations} requests dropping it instead of failing
+     */
+    private boolean dropUnloadableAnnotation(AnnotationExpr anno) {
+        if (!skipUnloadableAnnotations) {
+            return false;
+        }
+        try {
+            annotationTargets(anno);
+            return false;
+        } catch (IOException e) {
+            String location = sourceDescription(anno);
+            if (reportedSkippedAnnotations.add(anno.getNameAsString() + "@" + location)) {
+                System.err.println(
+                        "BinaryStubWriter: dropping @"
+                                + anno.getNameAsString()
+                                + " in "
+                                + location
+                                + " from the binary stub file: "
+                                + e.getMessage());
+            }
+            return true;
+        }
+    }
+
     /**
      * Returns the {@code @Target} element types of {@code anno}, for {@link #hasTypeUse} and {@link
      * #isTypeUseOnly} to route the annotation with.
@@ -1497,7 +1573,10 @@ private String fullyQualify(String name, CompilationUnit cu) {
      *     #isTypeUseOnly}; if false, all valid annotations are added to the provided list(s)
      *     unconditionally.
      * @param cu the compilation unit
-     * @throws IOException if writing to the annotation pool fails
+     * @throws IOException if writing to the annotation pool fails, or an annotation's
+     *     {@code @Target} cannot be read and neither {@link #isUnloadablePlatformAnnotation} nor
+     *     {@link #dropUnloadableAnnotation} (i.e. {@link #skipUnloadableAnnotations} is not set)
+     *     applies
      */
     private void routeAnnotations(
             List<AnnotationExpr> annotations,
@@ -1512,12 +1591,14 @@ private void routeAnnotations(
             if (idx == IGNORED) {
                 continue;
             }
-            if (filterByTarget && isUnloadablePlatformAnnotation(anno.getNameAsString())) {
+            if (filterByTarget
+                    && (isUnloadablePlatformAnnotation(anno.getNameAsString())
+                            || dropUnloadableAnnotation(anno))) {
                 // Routing needs the annotation's @Target, which cannot be read here; see
-                // isUnloadablePlatformAnnotation. Leave it out of both lists rather than fail.
-                // Only the routing path is affected: a caller that does not route by @Target (a
-                // class declaration's annotations, say) never needs to load the annotation at all,
-                // and still writes it.
+                // isUnloadablePlatformAnnotation and dropUnloadableAnnotation. Leave it out of both
+                // lists rather than fail. Only the routing path is affected: a caller that does not
+                // route by @Target (a class declaration's annotations, say) never needs to load the
+                // annotation at all, and still writes it.
                 continue;
             }
             if (typeAnnos != null && (!filterByTarget || hasTypeUse(anno))) {

framework/src/stubifier/java/org/checkerframework/framework/stubifier/JavaStubifier.java

@@ -44,6 +44,12 @@
  *   <li>all initializer blocks,
  *   <li>attributes to the {@code Deprecated} annotation (to be Java 8 compatible).
  * </ol>
+ *
+ * <p>Usage: {@code JavaStubifier [--skipUnloadableAnnotations] <directory> [<directory> ...]}. By
+ * default, an annotation whose {@code @Target} cannot be read (because its class is missing from
+ * the stubifier's own build classpath) aborts the run with a message naming the annotation and the
+ * source file being processed; see {@link #SKIP_UNLOADABLE_ANNOTATIONS_FLAG} for the opt-in flag
+ * that trades that safety for being able to finish the run anyway.
  */
 public class JavaStubifier {
     /**
@@ -57,27 +63,61 @@ public class JavaStubifier {
      */
     public static final LanguageLevel DEFAULT_LANGUAGE_LEVEL = LanguageLevel.JAVA_21;
 
+    /**
+     * Command-line flag that makes an unloadable annotation a dropped-and-warned event rather than
+     * a fatal error. When passed, {@link BinaryStubWriter} omits, from the binary stub output, any
+     * annotation whose {@code @Target} it cannot read -- printing one warning line to stderr naming
+     * the annotation and its source file -- and processing continues with the rest of the
+     * directory. Without the flag (the default), the same condition aborts the whole run; see
+     * {@link BinaryStubWriter#annotationTargets}.
+     *
+     * <p><b>Trade-off:</b> dropping such an annotation is not always safe. It could be a real type
+     * qualifier that a checker's own (wider) classpath would have resolved, in which case dropping
+     * it here silently removes it from the annotated JDK with no way for a later checker run to
+     * detect the omission. Pass this flag only when the unloadable annotation is known to be
+     * irrelevant to type-checking (e.g. a JUnit annotation on a stray test source file that ended
+     * up in the JDK source tree being stubified), not as a default way to push through classpath
+     * problems.
+     */
+    public static final String SKIP_UNLOADABLE_ANNOTATIONS_FLAG = "--skipUnloadableAnnotations";
+
     /**
      * Processes each provided command-line argument; see class documentation for details.
      *
-     * @param args command-line arguments: directories to process
+     * @param args command-line arguments: an optional {@link #SKIP_UNLOADABLE_ANNOTATIONS_FLAG},
+     *     followed by one or more directories to process
      */
     public static void main(String[] args) {
-        if (args.length < 1) {
-            System.err.println("Usage: provide one or more directory names to process");
+        boolean skipUnloadableAnnotations = false;
+        int dirCount = 0;
+        for (String arg : args) {
+            if (arg.equals(SKIP_UNLOADABLE_ANNOTATIONS_FLAG)) {
+                skipUnloadableAnnotations = true;
+            } else {
+                dirCount++;
+            }
+        }
+        if (dirCount < 1) {
+            System.err.printf(
+                    "Usage: JavaStubifier [%s] <directory> [<directory> ...]%n",
+                    SKIP_UNLOADABLE_ANNOTATIONS_FLAG);
             System.exit(1);
         }
         for (String arg : args) {
-            process(arg);
+            if (!arg.equals(SKIP_UNLOADABLE_ANNOTATIONS_FLAG)) {
+                process(arg, skipUnloadableAnnotations);
+            }
         }
     }
 
     /**
      * Process each file in the given directory; see class documentation for details.
      *
      * @param dir directory to process
+     * @param skipUnloadableAnnotations whether to drop, rather than fail on, an annotation whose
+     *     {@code @Target} cannot be read; see {@link #SKIP_UNLOADABLE_ANNOTATIONS_FLAG}
      */
-    private static void process(String dir) {
+    private static void process(String dir, boolean skipUnloadableAnnotations) {
         // Scoped to this call, not a shared/static field: main() may process several
         // directories, and a BinaryStubWriter accumulates classes/pool state across every
         // process(CompilationUnit) call with no reset, so reusing one across directories would
@@ -88,7 +128,7 @@ private static void process(String dir) {
         // effect and is not worth writing. BinaryStubFileGenerator, which produces the built-in
         // stub files' binaries, must not pass this.
         BinaryStubWriter binaryStubWriter =
-                new BinaryStubWriter(/* omitUnannotatedMembers= */ true);
+                new BinaryStubWriter(/* omitUnannotatedMembers= */ true, skipUnloadableAnnotations);
         Path root = dirnameToPath(dir);
         MinimizerCallback mc = new MinimizerCallback(binaryStubWriter);
         CollectionStrategy strategy = new ParserCollectionStrategy();
@@ -197,7 +237,19 @@ public Result process(
                     new File(absolutePath.toUri()).delete();
                     res = Result.DONT_SAVE;
                 } else {
-                    binaryStubWriter.process(cu);
+                    try {
+                        binaryStubWriter.process(cu);
+                    } catch (RuntimeException e) {
+                        // BinaryStubWriter's own failure messages (e.g. "cannot load annotation
+                        // ...") do not know which file was being processed -- it operates on a
+                        // CompilationUnit, and process(CompilationUnit) is also called directly by
+                        // tests with no file behind it at all. This layer is the first one that
+                        // does know the file for certain (it is a callback parameter), so it is
+                        // the simplest place to add that context to every failure that can escape
+                        // process(cu), not just the annotation-loading one.
+                        throw new RuntimeException(
+                                "Failed to process " + absolutePath + ": " + e.getMessage(), e);
+                    }
                 }
             }
             return res;

framework/src/test/java/org/checkerframework/framework/stubifier/JavaStubifierTest.java

@@ -4,10 +4,12 @@
 import org.junit.Assert;
 import org.junit.Test;
 
+import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.PrintStream;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -163,4 +165,85 @@
             deleteRecursively(dir);
         }
     }
+
+    /** Source text of a class whose field is annotated with an annotation not on the classpath. */
+    private static final String UNLOADABLE_ANNOTATION_SOURCE =
+            "import com.example.NotOnClasspath;\n"
+                    + "public class NeedsAnnotation {\n"
+                    + "  public @NotOnClasspath Object f;\n"
+                    + "}\n";
+
+    /**
+     * By default, an unloadable annotation aborts the whole run, and the failure names both the
+     * annotation and the source file that was being processed -- the file path used to be missing,
+     * which made finding the offending file among thousands require grepping the whole tree.
+     */
+    @Test
+    public void unloadableAnnotationFailsWithFilePathInMessage() throws IOException {
+        Path dir = Files.createTempDirectory("stubifier-test-unloadable");
+        try {
+            Path file = dir.resolve("NeedsAnnotation.java");
+            Files.write(file, UNLOADABLE_ANNOTATION_SOURCE.getBytes(StandardCharsets.UTF_8));
+
+            RuntimeException e =
+                    Assert.assertThrows(
+                            RuntimeException.class,
+                            () -> JavaStubifier.main(new String[] {dir.toString()}));
+            Assert.assertTrue(
+                    "the failure must name the unloadable annotation, but was: " + e.getMessage(),
+                    e.getMessage().contains("com.example.NotOnClasspath"));
+            Assert.assertTrue(
+                    "the failure must name the file being processed, but was: " + e.getMessage(),
+                    e.getMessage().contains(file.toString()));
+        } finally {
+            deleteRecursively(dir);
+        }
+    }
+
+    /**
+     * With {@link JavaStubifier#SKIP_UNLOADABLE_ANNOTATIONS_FLAG}, the same input succeeds instead:
+     * the unloadable annotation is dropped from the binary stub output (so the field ends up with
+     * no annotations, and the annotated-JDK writer then omits its record entirely, per {@link
+     * BinaryStubWriterTest#jdkWriterDemotesUnannotatedMethodRecords}'s analogous case for methods),
+     * and a warning naming the annotation and the file is printed to stderr.
+     */
+    @Test
+    public void skipUnloadableAnnotationsFlagOmitsAnnotationAndSucceeds() throws IOException {
+        Path dir = Files.createTempDirectory("stubifier-test-skip");
+        try {
+            Path file = dir.resolve("NeedsAnnotation.java");
+            Files.write(file, UNLOADABLE_ANNOTATION_SOURCE.getBytes(StandardCharsets.UTF_8));
+
+            PrintStream originalErr = System.err;
+            ByteArrayOutputStream capturedErr = new ByteArrayOutputStream();
+            System.setErr(new PrintStream(capturedErr, true, StandardCharsets.UTF_8));
+            try {
+                JavaStubifier.main(
+                        new String[] {
+                            JavaStubifier.SKIP_UNLOADABLE_ANNOTATIONS_FLAG, dir.toString()
+                        });
+            } finally {
+                System.setErr(originalErr);
+            }
+            String warnings = capturedErr.toString(StandardCharsets.UTF_8);
+            Assert.assertTrue(
+                    "the warning must name the dropped annotation, but was: " + warnings,
+                    warnings.contains("NotOnClasspath"));
+            Assert.assertTrue(
+                    "the warning must name the file it was dropped from, but was: " + warnings,
+                    warnings.contains(file.toString()));
+
+            BinaryStubData data = load(dir);
+            BinaryStubData.ClassRecord cr = data.classes.get("NeedsAnnotation");
+            Assert.assertNotNull("the class record must still be written", cr);
+            Assert.assertEquals(
+                    "the field must be omitted: its only annotation was dropped, leaving it"
+                            + " unannotated, and the annotated-JDK writer drops unannotated field"
+                            + " records",
+                    0,
+                    cr.fields.length);
+        } finally {
+            deleteRecursively(dir);
+        }
+    }
 }