diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f028f037af6b..4e77865bcf81 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -148,6 +148,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:** diff --git a/framework/src/stubifier/java/org/checkerframework/framework/stubifier/BinaryStubWriter.java b/framework/src/stubifier/java/org/checkerframework/framework/stubifier/BinaryStubWriter.java index cb96fbeae319..5527f226efe3 100644 --- a/framework/src/stubifier/java/org/checkerframework/framework/stubifier/BinaryStubWriter.java +++ b/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. + * + *

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. + * + *

Trade-off: 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 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 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))) { diff --git a/framework/src/stubifier/java/org/checkerframework/framework/stubifier/JavaStubifier.java b/framework/src/stubifier/java/org/checkerframework/framework/stubifier/JavaStubifier.java index 242d21886ec3..cb84eda46680 100644 --- a/framework/src/stubifier/java/org/checkerframework/framework/stubifier/JavaStubifier.java +++ b/framework/src/stubifier/java/org/checkerframework/framework/stubifier/JavaStubifier.java @@ -44,6 +44,12 @@ *

  • all initializer blocks, *
  • attributes to the {@code Deprecated} annotation (to be Java 8 compatible). * + * + *

    Usage: {@code JavaStubifier [--skipUnloadableAnnotations] [ ...]}. 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,18 +63,50 @@ 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}. + * + *

    Trade-off: 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] [ ...]%n", + SKIP_UNLOADABLE_ANNOTATIONS_FLAG); System.exit(1); } for (String arg : args) { - process(arg); + if (!arg.equals(SKIP_UNLOADABLE_ANNOTATIONS_FLAG)) { + process(arg, skipUnloadableAnnotations); + } } } @@ -76,8 +114,10 @@ public static void main(String[] args) { * 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; diff --git a/framework/src/test/java/org/checkerframework/framework/stubifier/JavaStubifierTest.java b/framework/src/test/java/org/checkerframework/framework/stubifier/JavaStubifierTest.java index 7696e10a93ef..f0e85545c320 100644 --- a/framework/src/test/java/org/checkerframework/framework/stubifier/JavaStubifierTest.java +++ b/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,95 @@ public void recordOnlyFileIsKept() throws IOException { 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(); + // The three-argument PrintStream(OutputStream, boolean, Charset) constructor is not + // available on Java 8, which this project's compileTestJava must still target; the + // String-charset-name overload is the Java-8-compatible equivalent, and is explicit + // about the charset (unlike the default-charset constructor), so no behavior is lost. + @SuppressWarnings("JdkObsolete") + PrintStream captureStream = + new PrintStream(capturedErr, true, StandardCharsets.UTF_8.name()); + System.setErr(captureStream); + try { + JavaStubifier.main( + new String[] { + JavaStubifier.SKIP_UNLOADABLE_ANNOTATIONS_FLAG, dir.toString() + }); + } finally { + System.setErr(originalErr); + } + // Same Java-8-compatibility reasoning as above: ByteArrayOutputStream.toString(Charset) + // is not available on Java 8. + @SuppressWarnings("JdkObsolete") + String warnings = capturedErr.toString(StandardCharsets.UTF_8.name()); + 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); + } + } }