Create a binary JDK and stubs format#1853
Open
wmdietl wants to merge 117 commits into
Open
Conversation
Problem: The binary stub format serialized annotations as JavaParser strings, requiring full JavaParser round-trips at read time to reconstruct AnnotationMirrors. This was slow and allocated heavily during type-checking. Fix: Switch to a structural binary format (version 3) that stores annotation element values directly as typed records (primitives, strings, class literals, enum constants, nested annotations, arrays, name literals). Add an AnnotationPool in the writer to deduplicate identical annotations. Add caches in AnnotationFileElementTypes for annotation mirrors, resolved class types, and constant values to avoid repeated lookups. Rewrite BinaryStubReader to resolve structural values directly without JavaParser. Files changed: - framework/stub/BinaryStubWriter.java: structural writer + AnnotationPool - framework/stub/BinaryStubData.java: new value types + annotation pool - framework/stub/BinaryStubReader.java: structural reader with caches - framework/stub/AnnotationFileElementTypes.java: updated caches
…on position BinaryStubWriter stored all annotations from method, constructor, and field declaration position in declAnnos, regardless of their @target. Annotations that are TYPE_USE-only (e.g. @positive, @IntRange, @nonnegative) were therefore loaded by BinaryStubReader and placed into annotationFileAnnos.declAnnos. The framework then applied them as declaration annotations, producing @IntRangeFromPositive on the type via aliasing. The same annotation also ended up on the type directly via the reader's declAnnos-to-type copy, resulting in two annotations from the same Value checker hierarchy and a type.invalid.conflicting.annos error (ValueIgnoreRangeOverflowTest failure). The fix has three parts: 1. framework/build.gradle: add checker-qual to stubifierImplementation so Class.forName can resolve annotation classes at stub-generation time. 2. BinaryStubWriter: add hasTypeUse and isTypeUseOnly helpers that inspect @target via reflection. In processMethod, processConstructor, and processField, route annotations by their @target: - TYPE_USE-only (@positive, @IntRange, ...): typeAnnos/returnTypeAnnos only (empty path = applies to base type). - Declaration-only (@pure, @Native, ...): declAnnos only. - Dual-purpose (@MustCallAlias, @LengthOf, ...): both, matching the behavior of the text-based AnnotationFileParser which calls both recordDeclAnnotation and annotate for all declaration-position annotations. TypeAnno gains a no-path constructor that stores Collections.emptyList() instead of allocating a new ArrayList for the common empty-path case. 3. BinaryStubReader: remove the now-unnecessary code that applied declAnnos to the field/method return type (the writer now routes annotations correctly). Change applyTypeAnnos to use replaceAnnotation instead of addAnnotation, matching the text parser, so that when multiple annotations from the same hierarchy appear at the same type path the later one replaces the earlier rather than stacking. Verified: ValueTest, ValueIgnoreRangeOverflowTest, MustCallTest, and the full framework test suite pass.
…Annos Add imports for ArrayList, Collections, Element, ElementKind, ArrayType and replace all inline javax.lang.model.* and java.util.* qualified names with the imported simple names. Fix paramDeclAnnos handling to use replaceAnnotation instead of addAnnotation, consistent with field and method return type handling. This prevents a latent hierarchy-conflict (type.invalid.conflicting.annos) for dual-purpose (TYPE_USE + declaration) annotations on parameters. Pre-size the resolvedList ArrayList in addValueToBuilder with rawList.size() to avoid resizing.
Five improvements to the annotated-JDK binary stub loading path: 1. Cache @FromStubFile per factory. BinaryStubReader.applyTypeAnnos was calling AnnotationBuilder.fromClass on every type annotation applied. Store the mirror once in AnnotationFileElementTypes (built in the constructor) and read it directly. 2. Move annoCache, resolvedClassTypesCache, resolvedConstantsCache from per-factory AnnotationFileElementTypes fields to the per-compilation BinaryStubDataCache. A nullness compile drives four GATF instances; the old layout rebuilt these caches four times per compilation. The caches are now shared for the whole compilation run. 3. Cache getBinaryStubDataCache() in a local field. The method previously cast getProcessingEnvironment() to JavacProcessingEnvironment and fetched the compilation context on every call. It is now populated once on first access and returned directly thereafter. 4. Record outerNameIndex on ClassRecord. BinaryStubWriter stores the fully-qualified name of the outermost enclosing class alongside each class record. The reader uses this to build the inner-class map in a single O(n) pass over the class pool, replacing a string-scan heuristic that checked every '.' prefix against the class map. Binary format version bumped to 1 (first release of this format). VERSION constant added to BinaryStubData so reader and writer share the same symbolic name. 5. processedBinaryClasses: LinkedHashSet -> HashSet. Insertion order is not used; this avoids the linked-list overhead per entry. Also: add missing javadoc to MethodRecord, FieldRecord, ClassRecord default constructors; add @param tags to dispatchSetValue; add private constructor to BinaryStubReader utility class.
…most component The text-based stub parser's annotateAsArray calls annotateInnermostComponent- Type, which applies declaration-position TYPE_USE annotations to the innermost component of the array type, not to the array reference. For example, for @nullable T[] (where @nullable is TYPE_USE-only), the text parser applies @nullable to T, not to T[]. The binary writer was using an empty type path (= array reference level) for such annotations. CollectionToArrayHeuristics only sets the component nullness, leaving an outer-level @nullable on T[] that caused return.type.incompatible in NullnessAssumeInitializedTest (and mismatches for Arrays.copyOf). Fix: add arrayElementPath(Type) helper that builds one ARRAY path step per array dimension. processMethod and processField now use this path when routing declaration-position TYPE_USE-only annotations to returnTypeAnnos/typeAnnos, matching the text parser's annotateInnermostComponentType behavior. Note: pure declaration annotations (non-TYPE_USE) are unaffected — they go into declAnnos and correctly bind to the whole declared entity.
BinaryStubData: add MAGIC (0xCF4A444B = CF+JDK), VERSION, and FILENAME constants. Use MAGIC in the reader instead of the raw literal. Update cross-reference javadocs between BinaryStubData and BinaryStubWriter. BinaryStubWriter: use MAGIC and the new OUTPUT_FILENAME constant. Add 22 missing JavaParser imports that were previously written as inline qualified names. Add arrayElementPath helper (fixes regression: TYPE_USE-only annotations before array return types were stored at the array-reference level instead of the element level, matching AnnotationFileParser's annotateInnermostComponentType behaviour). JavaStubifier: use BinaryStubWriter.OUTPUT_FILENAME instead of the literal. AnnotationFileElementTypes: extract ANNOTATED_JDK_PATH constant and use BinaryStubData.FILENAME. Remove unused jdkJarfile parameter from prepJdkFromJar. Make fromStubFileAnno final. Improve javadoc on remainingJdkStubFilesJar. docs/developer/performance-notes.md: add PR #TODO entry for the binary stub format. Measured on allNullnessTests (warm daemon, 4 reps each): −7 s wall clock (master 1m51s → branch 1m44–45s, −6%) and −18% TLAB allocation events (99 823 → 81 807), with String −27% and byte[] −23% from eliminating JavaParser UTF-8 buffers per compilation.
Binary stub load failures (file not found or unreadable) now issue a Diagnostic.Kind.NOTE via checker.message, matching the existing pattern used throughout the class for other fallback/informational messages.
BinaryStubReader: - createAnnotationMirrorNoCache: catch(Throwable) -> catch(Exception) so OutOfMemoryError and StackOverflowError are not swallowed. - resolveSingleValue NameLiteralValue branch: return null instead of throwing RuntimeException when the constant cannot be resolved, matching AnnotationFileParser's silent-skip behavior. - Class javadoc: document that classes absent from the binary stub (due to hasComplexAnnos) fall through to the text parser. BinaryStubWriter: - Class javadoc: explain why the whole class must be omitted when hasComplexAnnos fires, not just the type-parameter annotations. Losing a bound like <K extends @nonnull Object> would silently drop a nullness constraint that the text parser enforces; the whole-class skip is intentional correctness protection, not just a limitation.
…Annos/ajavaTypes Remove the hasComplexAnnos fallback in BinaryStubWriter that excluded JDK classes with annotated type parameters or bounds (List, Collection, Hashtable, Comparable, Objects, Class, ...) from the binary stubs. Add TypeParamRecord to BinaryStubData/BinaryStubWriter/BinaryStubReader to store and apply type-parameter bound annotations. Every JDK class is now served from the binary path. BinaryStubData / BinaryStubWriter: - Add TypeParamRecord with int[] typeVarAnnos and TypeAnno[][] boundAnnos. - Add typeParams field to ClassRecord and MethodRecord. - Add writeTypeParams / readTypeParams helpers. - BinaryStubWriter.processClass/processMethod/processConstructor: extract and write type params via extractTypeParams. - Split parameter annotations: TYPE_USE-only go to paramAnnos (with arrayElementPath or [ARRAY] for varargs), declaration-only to paramDeclAnnos. BinaryStubReader: - applyClassTypeParams: annotate AnnotatedTypeVariables keyed by TypeParameterElement and store full AnnotatedDeclaredType keyed by TypeElement. - applyMethodTypeParams: annotate type variables in place and store in atypes. - propagateClassTypeParamBounds / copyBoundsFromAtypes: propagate class type-param bound annotations into method parameter type variables. - Apply class-level declAnnos (e.g. @Interned on Class<T>) as primary annotations on atypes[typeElt] so getTypeDeclarationBounds works correctly. - Change declAnnos storage from putIfAbsent to per-annotation add-if-absent so binary annotations like @invoke survive alongside @FromStubFile from user stubs. AnnotationFileElementTypes: - Add isStubTypes flag (set true for stubTypes, false for ajavaTypes/currentFile). - Guard binary loading in maybeParseEnclosingJdkClass with isStubTypes so the binary is never loaded into ajavaTypes; prevents mergeAnnotationFileAnnosIntoType from re-applying stale binary annotations over user-stub overrides. AnnotatedTypeFactory: - Call stubTypes.setIsStubTypes(true) after construction. Performance (allNullnessTests, warm daemon): - Wall-clock: master 1m 51s -> branch 1m 44-45s (-7 s, -6%) - TLAB events: -18%, String allocs: -27%, byte[] allocs: -23%
EisopIssue270.class and Issue1402EnumName.class were accidentally committed alongside their .java sources in an earlier commit on this branch. Gradle regenerates them at test time; they don't belong in version control. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tch text-parser semantics The binary stub format previously covered only classes and interfaces, so nested enums silently fell through no fallback at all (their enclosing class was handled by the binary path, so the text parser never ran for them either) -- losing annotations such as @pure on Character.UnicodeScript.of. Separately, several writer behaviors diverged from AnnotationFileParser: private declarations were written (the text parser skips them via skipNode), annotation-name resolution did not fall back to the unconditionally-seeded java.lang package or to asterisk imports, and multi-bound type parameters only ever considered the first bound. BinaryStubWriter now also processes EnumDeclaration (enum constants become field records) and AnnotationDeclaration (members become zero-parameter method records); record declarations remain the one unsupported construct. Name resolution, private-declaration skipping, and the single-annotated-bound-at-any-index rule for type parameters now match AnnotationFileParser exactly. @CFComment is no longer written to the binary pool (it is documentation for humans with no effect on checking). BinaryStubData gains matching read support and the BIN_SUFFIX constant used to name a stub file's binary sibling. Also collapses the near-duplicate processMethod/processConstructor and MethodSignaturePrinter bodies into shared helpers, and the repeated "read/write a length-prefixed int[]/TypeAnno[]" idiom (11 call sites in the reader, 8 in the writer) into named helper pairs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…che per JVM; add differential verification Three correctness bugs in the binary stub application layer, found by tracing the shipped annotated-jdk.bin.gz back through BinaryStubData: (1) hasAnyTypeAnnos ignored a method's own type-parameter annotations, so a method whose only annotation is on a type parameter (exactly one case in the JDK: StackWalker.walk's <T extends @nullable Object>) was silently skipped; (2) type-parameter handling only ever applied the first declared bound and always treated the type-variable annotation as a lower-bound annotation, rather than matching AnnotationFileParser.annotateTypeParameters (no-bound case: primary annotation; single annotated bound at any index: apply it; multiple annotated bounds: skip, matching the text parser's own unimplemented intersection handling); (3) stub methods declared on a class that only inherits them ("fake overrides", 9 in the JDK, e.g. @nullable on UncheckedDocletException.getCause()) were dropped entirely -- BinaryStubReader now mirrors AnnotationFileParser.processFakeOverride. Also: BinaryStubData is now cached per JVM (it holds no javac objects, only strings and primitives, so the ~330 KB gunzip+parse is paid once per JVM instead of once per compilation); the checker.jar entry enumeration used to build the text-parser fallback map is now built once per compilation and shared across a nullness compile's four factories instead of once per factory; declaration annotations are now filtered by @target applicability and merged by name (matching recordDeclAnnotation) at every site instead of overwritten; the dead @FromStubFile type-annotation write is removed (the text parser never marks JDK-stub elements, and the mirror was never a supported qualifier, so it was pure per-annotation dead work). Given how much of this format re-implements AnnotationFileParser's semantics by hand, a silent divergence is the main risk. This adds BinaryStubDiffChecker (activated by the -AbinaryStubDiffCheck option, never active in a normal checker run): it loads every class of a binary stub through both the binary reader and the text parser into separate scratch containers and reports any disagreement as an error, plus a record-to-storage presence oracle that would have caught the StackWalker.walk gap above mechanically. See its class Javadoc for the exact comparison scope (some positions, like implicit-wildcard bounds, are derived state computed differently by the two loaders and are deliberately excluded). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… time Only the annotated JDK's stub text was pre-parsed to the binary format; the ~40 built-in checker .astub files (jdk.astub, per-checker files like log4j.astub) were still text-parsed by JavaParser at every checker startup. A JFR trace of a small compile attributed roughly 36% of its inclusive time to this text parsing. BinaryStubFileGenerator walks a module's src/main/java and src/main/resources for .astub files and writes each one's binary form to a sibling <name>.astub.bin.gz resource, using the same BinaryStubWriter as the annotated JDK. A file the generator cannot represent (currently: one containing a record declaration) is left without a binary sibling and keeps text parsing -- skipping is always safe, it only forgoes the speedup for that file. The generateBinaryStubFiles task is registered once, in the top-level build.gradle's subprojects block, for every subproject that actually contains .astub files; framework exposes the stubifier tool (writer + generator + their runtime dependencies) through a new consumable 'stubifierTool' configuration, since resolving another project's source-set classpath directly from a shared subprojects block is incompatible with the configuration cache. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wires up BinaryStubDiffChecker (added in a prior commit) as a JUnit test: -AbinaryStubDiffCheck reports each disagreement between the binary and text stub-loading paths as an error, so a CheckerFrameworkPerDirectoryTest run that expects zero diagnostics fails on any regression. The Nullness Checker is the host checker because it exercises four annotated type factories and the largest set of JDK annotations. The placeholder source file only exists to give the per-directory harness a compilation to run; the real verification work happens at checker initialization. Currently green: 1450 top-level JDK classes and every built-in stub file loaded by the nullness compile, zero mismatches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-notes Adds a CHANGELOG bullet for the built-in-stub binary format (mirroring the existing annotated-JDK bullet), a note that the text stub files in checker.jar are expected to be dropped once the binary format has proven itself, and extends the performance-notes.md entry for this PR with: the fixes and semantics-parity work found by the differential checker, the per-JVM BinaryStubData cache and shared jar-entry scan, the built-in-stub generator, and the wall-clock/allocation A/B numbers at each stage (small-compile allocation -64%/wall -37% vs. master; allNullnessTests wall -16%). Also records the follow-up JFR trace showing a flat startup profile and lists the next options in priority order: lazy built-in-stub application (with the cross-file precedence hazard against option stubs that deliberately override the JDK), then dropping the text stubs from checker.jar, then widening the differential check to every factory, then measuring whether JavaExpressionParseUtil's expression-string parsing is worth removing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…terned/@FullyQualifiedName
checkInterning and checkNullness (running the Checker Framework on its
own source) both failed. checkNullness crashed outright:
buildSigIndex's "catch (BugInCF e) { throw e; }" was added under the
assumption that BugInCF always signals a real internal bug and should
propagate -- but TypesUtils.simpleTypeName throws exactly BugInCF for
an unresolvable ERROR-kind parameter type (e.g. a JDK-internal
sun.util.locale.provider.LocaleResources not exported to the
annotation processor's module), which is precisely the case the
surrounding catch was meant to skip. Since BugInCF extends
RuntimeException, the plain "catch (RuntimeException e)" already
covers it; the special-cased rethrow just undid the fix. Removed it.
checkInterning failed on a null-vs-non-null identity check written as
"text != binary" on two non-@Interned AnnotatedTypeMirror-typed
parameters; rewritten as a boolean comparison, which needs no
qualifier. Applied the Interning Checker's two safe suggested
simplifications elsewhere in the same file (Class objects and
AnnotationUtils.annotationName results are canonical, so == replaces
.equals()). checkSignature separately failed on six call sites passing
plain Strings decoded from the binary stub's string pool where
Elements.getTypeElement/AnnotationBuilder expect @FullyQualifiedName;
each is annotated with a narrow @SuppressWarnings, matching the
existing convention in AnnotationFileParser for stub-derived class
name strings that are known-good at runtime but not statically
checkable.
./gradlew typecheck (all self-check tasks: checkFormatter,
checkInterning, checkOptional, checkPurity, checkResourceLeak,
checkSignature, checkNullness, checkCompilerMessages) now passes, as
do :checker:test and :framework:test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BinaryStubDiffChecker.compareAtm (fixed in a prior commit) wrote "if (a
== null || b == null) { if (a != b) ... }" to detect "exactly one of
a, b is null" -- but knowing only the disjunction doesn't make a
non-null operand provably interned, so the Interning Checker correctly
rejected the identity comparison. That was only caught by running the
Interning Checker on the framework's own source (./gradlew
checkInterning); nothing in the ordinary test suite exercised this
pattern.
Add NullXorComparison.java to checker/tests/interning/, so InterningTest
(part of the normal :checker:test run) now covers both the anti-pattern
(expects the not.interned error) and the fix (comparing nullness itself
via two boolean equalities, which needs no qualifier). Verified the
test is meaningful by temporarily removing the expected-error comment
and confirming InterningTest then fails.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The interning regression test added in the previous commit covers the
not.interned diagnostic that checkInterning also reported, but not the
actual crash: buildSigIndex rethrowing BugInCF when TypesUtils.simpleTypeName
hits an ERROR-kind parameter type. That crash's real-world trigger
(sun.util.locale.provider.LocaleResources, an internal JDK type not
exported to the annotation processor's module) turns out not to be
reproducible through the ordinary per-directory test harness: it
depends on the specific classpath/module configuration of the Gradle
self-check tasks, not on anything a checker/tests/*.java file can
force -- confirmed by reproducing the crash live (temporarily
reverting the fix) and trying, without success, to trigger it via
checker/bin/javac with the self-check task's own classpath, both for a
minimal Locale-referencing file and for the whole checker module's
source set.
Instead, add a portable, JDK-version-independent unit test:
javac's error recovery still produces a real ExecutableElement for a
method whose parameter type does not exist, with that parameter
attributed as TypeKind.ERROR -- exactly the condition that crashed.
AnnotationFileElementTypesTest compiles such a method in memory and
calls buildSigIndex (widened from private to package-private for
this), asserting it skips the bad method instead of throwing and still
indexes the class's other methods.
Verified this test is meaningful by temporarily reintroducing the old
"catch (BugInCF e) { throw e; }" and confirming the test then fails
with the exact same crash.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a compressed binary stub format for the annotated JDK and built-in .astub resources, plus the corresponding build-time generators and runtime loader, to eliminate JavaParser-based text stub parsing overhead during checker startup.
Changes:
- Add build-time writers/generators to emit
annotated-jdk.bin.gzand*.astub.bin.gzresources. - Add runtime binary stub loading + application path (with a diff checker to validate parity vs text parsing).
- Update build/test/docs to generate, ship, and validate the new binary format.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/src/test/java/org/checkerframework/framework/stub/AnnotationFileElementTypesTest.java | Adds regression tests for skipping ERROR-kind signatures in executable indexing. |
| framework/src/stubifier/java/org/checkerframework/framework/stubifier/JavaStubifier.java | Writes annotated-jdk.bin.gz after minimizing sources (now also feeds BinaryStubWriter). |
| framework/src/stubifier/java/org/checkerframework/framework/stubifier/BinaryStubWriter.java | Implements binary stub serialization from JavaParser AST. |
| framework/src/stubifier/java/org/checkerframework/framework/stubifier/BinaryStubFileGenerator.java | Generates sibling .astub.bin.gz resources for built-in stub files at build time. |
| framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java | Threads an isStubTypes flag into AnnotationFileElementTypes construction. |
| framework/src/main/java/org/checkerframework/framework/stub/BinaryStubReader.java | Implements binary stub application to framework AnnotationFileAnnotations. |
| framework/src/main/java/org/checkerframework/framework/stub/BinaryStubDiffChecker.java | Adds differential checking between text vs binary stub loading (test-only option). |
| framework/src/main/java/org/checkerframework/framework/stub/BinaryStubData.java | Defines and parses the on-disk binary stub format into an immutable in-memory model. |
| framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java | Integrates binary stub loading for JDK and built-in stubs; adds caches and diff-check hook. |
| framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java | Registers -AbinaryStubDiffCheck option. |
| framework/build.gradle | Exposes a consumable stubifierTool configuration for cross-project binary stub generation. |
| docs/developer/performance-notes.md | Documents the binary stub design/perf improvements. |
| docs/CHANGELOG.md | Notes performance improvements and new binary stub distribution in release notes. |
| checker/tests/nullness-binarystubdiff/BinaryStubDiffPlaceholder.java | Adds a placeholder compilation unit for the diff-check test directory. |
| checker/tests/interning/NullXorComparison.java | Regression test for a prior diff-checker bug involving != on potentially-null refs. |
| checker/src/test/java/org/checkerframework/checker/test/junit/NullnessBinaryStubDiffTest.java | JUnit harness that runs the diff checker via -AbinaryStubDiffCheck. |
| build.gradle | Adds per-subproject generateBinaryStubFiles task wiring to produce .astub.bin.gz resources. |
Comments suppressed due to low confidence (1)
framework/src/stubifier/java/org/checkerframework/framework/stubifier/JavaStubifier.java:73
binaryStubWriteris a static final instance, butmainprocesses multiple directories. This causes stub data from earlier directories to leak into later outputs (each directory's annotated-jdk.bin.gz will include records from all previously-processed directories). Reinitialize the writer per directory (or make it local) before parsing.
/** The writer used to generate the compressed binary stub file. */
private static final BinaryStubWriter binaryStubWriter = new BinaryStubWriter();
/**
* Process each file in the given directory; see class documentation for details.
*
* @param dir directory to process
*/
private static void process(String dir) {
Path root = dirnameToPath(dir);
MinimizerCallback mc = new MinimizerCallback();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| private TypeAnno readTypeAnno(DataInputStream dataIn) throws IOException { | ||
| int annoIndex = dataIn.readInt(); | ||
| int pathLength = dataIn.readByte(); |
| handles first-encounter FQN annotations and populates both simple-name and FQN entries for future | ||
| hits. | ||
|
|
||
| - **PR #TODO** — *Binary pre-parsed JDK stub format.* |
Comment on lines
+609
to
+610
| - **`@FromStubFile` cached per factory.** `AnnotationFileElementTypes` builds the | ||
| `@FromStubFile` mirror once in its constructor. |
Every short-typed count/length field in the binary format (element count, field count, method count, param count, annotation-array length, decl-annotation-index count, type-annotation count, type-parameter count, bound count) was read with readShort(), which returns a signed value: a legitimate count of 32768 or more (JVM class files themselves allow up to 65535 fields/methods, u2) would read back negative, either silently dropping data (a for loop bounded by a negative count runs zero times) or crashing with NegativeArraySizeException. These are all transient local variables that size an array or loop, never stored as object fields, so switching to readUnsignedShort() (returning int, matching the count's actual domain) has no memory cost. path_length (JVMS 4.7.20.1, a u1) had the same bug for the same reason and is fixed the same way: readUnsignedByte(), since it's also just a transient local sizing the path array. kind and argIndex (also JVMS u1 fields, TypePathStep's per-step path kind and type-argument index) are different: unlike the counts above, one TypePathStep instance is retained per path step of every type annotation in the annotated JDK, so widening these fields to int would quadruple that memory cost. kind never exceeds 3 (four defined values), so it stays a plain signed byte with no unsigned handling needed at all. argIndex is kept as a compact signed byte too, matching JVMS's 1-byte type_argument_index; a value of 128 or greater is therefore storable only as a negative byte, so BinaryStubReader.resolvePath now reinterprets it via Byte.toUnsignedInt at the one point where it is actually widened to int (as an index into a type-argument list), instead of at read time -- reading via readByte() vs. (byte) readUnsignedByte() would store the identical bit pattern either way, since a byte's signedness is a property of how it is later widened, not of how it was read. Verified against the differential check (BinaryStubDiffChecker / NullnessBinaryStubDiffTest, still 0 mismatches across the JDK and all 42 built-in stub files) and the full :framework:test / :checker:test / :javacutil:test / :dataflow:test suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nnotations applyClassRecord's class-level-annotation block fetched an existing atypes[typeElt] entry (e.g. one already established by a user-supplied -Astubs override) and mutated it in place with the JDK's own class annotations. For java.util.Optional, whose annotated-JDK declaration carries a class-level @nonnull specifically so that "a stub file that overrides this class" (per its own javadoc) can permit @nullable Optional, this silently reintroduced @nonnull even after a user stub blanked the class out, breaking the jtreg regression test checker/jtreg/stubs/stub-over-jdk/Issue321.java. The fix builds the JDK's annotations onto a fresh type and only stores it if nothing already exists for that key, matching AnnotationFileParser.putMerge's JDK_STUB case. Auditing every other site with the same "fetch existing entry, mutate it, or unconditionally skip" shape turned up two related gaps, each compared directly against its text-parser equivalent: - mergeDeclAnnos (declaration annotations like @covariant or @EnsuresNonNull) always skipped an annotation if one of the same name already existed, matching putOrAddToDeclAnnos's JDK_STUB behavior unconditionally. But for two built-in stub files that both annotate the same element, the later file should replace the earlier one's annotation (parseStubFiles's javadoc: "the qualifier in the last stub file is applied"), not lose to it. This is not hypothetical: nullness's own junit-assertions.astub and permit-nullness-assertion-exception.astub both declare org.junit.jupiter.api.Assertions.assertNotNull(Object) with @EnsuresNonNull("#1") -- harmless today only because the content happens to match. - The same asymmetry existed for type-use annotations on fields, methods, method parameters, and class/method type-parameter bounds (applyFieldRecords, applyMethodRecords, applyClassTypeParams, applyMethodTypeParams): each backed off entirely whenever any entry already existed, rather than merging when the new content is itself from a built-in stub file (as opposed to the lazily-loaded JDK). All of these now use the same discriminator already available at every call site (fromStubFileAnno == null for the lazily-loaded JDK, non-null for an eagerly-applied built-in stub file) to decide whether to store fresh, merge via AnnotatedTypeFactory.replaceAnnotations, or leave an existing entry untouched -- mirroring AnnotationFileParser.putMerge/putOrAddToDeclAnnos exactly. Verified against the differential check (still 0 mismatches across 1450 classes and both of nullness's auto-loaded built-in stubs), the full checker/jtreg (88 passing) and framework/jtreg (4 passing) suites, and the full :framework:test/:checker:test/:javacutil:test/ :dataflow:test suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Filled in the PR #TODO placeholder for the binary pre-parsed JDK stub format now that it is pushed as PR #1853. Corrected two claims that no longer matched the code: the @FromStubFile mirror is built lazily on first use (getFromStubFileAnno), not in AnnotationFileElementTypes's constructor (per a Copilot review comment on the PR); and the differential-test class count/message format was stale (1441 -> ~1450, missing the "without text stub source" detail the message has carried since 7517c76). Also added a "Correctness follow-up" entry documenting the two fixes made after the initial performance work landed: the signed/unsigned byte-handling audit (BinaryStubData/BinaryStubWriter/BinaryStubReader) and the stub-precedence bug where a JDK class-level annotation (Optional's @nonnull) could override a user's stub override, plus the same asymmetry found and fixed in mergeDeclAnnos and the field/method/ type-parameter application sites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
JavaParser exposes annotations written immediately before a varargs ellipsis (e.g. "Foo @x ... args", which annotates the array type itself) through a dedicated Parameter.getVarArgsAnnotations() list, distinct from getAnnotations() (the parameter's own declaration annotations, which apply to the array's component type for varargs) and from the component type's own annotations. BinaryStubWriter's parameter loop read the first two but never called getVarArgsAnnotations() at all, so any such annotation was silently dropped from the binary form of the annotated JDK. This broke java.util.Class.getMethod, whose "Class<?>@nullable ... parameterTypes" declaration relies on exactly this position to permit a null varargs array: downstream Daikon CI (which compiles against this branch's checker.jar) failed with "cls.getMethod(name, (Class<?>[]) null)" rejected as argument.type.incompatible, since the array defaulted to @nonnull with the annotation missing. Confirmed via git worktree bisection that this predates the current session's other fixes -- it is not a regression from PR #1853's precedence work, but present since the binary format was extended to cover parameter types; it does not reproduce against master (pure text parsing), only against this branch's binary reader. Fixed by extracting getVarArgsAnnotations() for vararg parameters and recording each as a TypeAnno with an empty path (applying directly to the parameter's array type, matching AnnotationFileParser. processParameters's annotate(paramType, param.getVarArgsAnnotations(), param)). processCallable is shared by both processMethod and processConstructor, so the fix covers constructors too. Added checker/jtreg/nullness/VarargsArrayAnnotation.java as a regression test. It must run against the real annotated JDK's binary form (not a user-supplied stub, which is always text-parsed and so would not exercise this writer bug, and not the JUnit-based nullness test corpus, which does not use the annotated JDK at all), so it is a jtreg test exercising Class.getMethod directly, mirroring the existing Issue321-style tests in checker/jtreg/stubs/stub-over-jdk. Verified the test fails (2 errors instead of 1) when the fix is reverted, and passes with it applied. While investigating why NullnessBinaryStubDiffTest's differential check (0 mismatches) never caught this: java.lang.Class is one of the many common JDK types resolved as a side effect of comparing an earlier class in the same run, which removes it from the diff checker's one-shot remainingJdkStubFiles map before its own turn in the comparison loop, landing it silently in "classes without text stub source" instead of being compared. This is a pre-existing architectural gap in the differential checker's coverage, not something this commit attempts to fix. Verified: the new jtreg test, the full checker/jtreg suite (89 passing, +1 for this test) and framework/jtreg suite (4 passing), the differential test (still 0 mismatches), and the full :framework:test/:checker:test/:javacutil:test/:dataflow:test suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ookup Two independent gaps made the differential check far less thorough than it appeared, both traced while investigating why it reported 0 mismatches for a real bug (a dropped varargs-array annotation on Class.getMethod, fixed in the previous commit). 1. It ran once per compilation, under whichever stub-types factory's prepJdkStubs() got there first (BinaryStubDataCache.diffCheckDone). For a compound checker like Nullness, that factory can be a sub-checker -- e.g. KeyFor -- that does not support @Nullable/ @nonnull at all. AnnotatedTypeMirror.addAnnotation silently drops an annotation the current atypeFactory does not support, on both the text and binary sides equally, so comparing under only that one factory made the whole check blind to every qualifier it does not support: exactly how a missing @nullable on Class.getMethod's varargs array went unreported. prepJdkStubs (and so this method) already runs exactly once per factory on its own, so removing the single-run gate is enough to compare under all four of Nullness's factories, each against the qualifiers it actually supports. 2. parseJdkSourceInto (the differential check's only caller) looked up a class's text-stub source in remainingJdkStubFiles/ remainingJdkStubFilesJar, the same maps maybeParseEnclosingJdkClass consumes (removes entries from) as a side effect of ordinary lazy resolution -- including resolution triggered by the diff check's own comparison of an earlier class in the same run. A sufficiently common type (java.lang.Class among them) can already be resolved, and hence missing from these maps, before the outer loop ever reaches it as its own top-level entry, silently moving it from "checked" to "without text stub source". Changed the lookup to the BinaryStubDataCache's jdkStubPathsByClass/jdkJarEntriesByClass snapshots instead: populated once by the initial directory walk/jar scan and never mutated afterward, so every class with a text stub source is compared regardless of what else has already been resolved. On this JDK checkout, this took the comparison from 1450 classes (316 "without text stub source") to 1652 (114), a jump consistent with dozens of commonly-referenced JDK types previously skipped this way. Verified: differential check now runs 4 times (once per Nullness factory) with 0 mismatches; full checker/jtreg (89 passing) and framework/jtreg (4 passing); full :framework:test/:checker:test/ :javacutil:test/:dataflow:test suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
With the differential check now comparing under every factory (previous commit), NullnessBinaryStubDiffTest immediately found 40 real mismatches, all the same shape: an annotation explicitly written on the lower bound of a "? super X" wildcard (e.g. Optional.filter's "Predicate<? super @nonnull T>") was present on the text side and missing on the binary side, across java.util.Optional, Map, HashMap, TreeMap, ConcurrentHashMap, ConcurrentSkipListMap, and ImmutableCollections. Root cause was two compounding bugs: 1. BinaryStubReader.resolvePath's WILDCARD_BOUND case picked the bound with "awt.getExtendsBound() != null ? extends : super". CF's AnnotatedWildcardType always synthesizes both an extends and a super bound (defaulting whichever was not written in source), so getExtendsBound() is never null and this always selected the extends bound, even for an explicit "? super X" wildcard whose extends bound is an implicit "Object" default. 2. Once that was fixed to use an explicit marker instead, it exposed a second bug beneath it: BinaryStubWriter.TypeAnno#write only ever serialized a path step's argIndex byte for TYPE_ARGUMENT (kind 3); for WILDCARD_BOUND (kind 2) the byte was never written to the stream at all, matching JVMS's own assumption that type_argument_index is unused for that kind (a real wildcard has only one structurally possible bound, unlike CF's AnnotatedWildcardType). BinaryStubData's reader had the matching gap on the read side. So even after writing an extends/super marker into the in-memory TypePathStep, it was silently dropped at serialization and always read back as 0. Fixed by writing and reading argIndex for WILDCARD_BOUND too (0 = extends bound, 1 = super bound, repurposing the byte JVMS itself leaves unused for this kind), and using it in resolvePath instead of the never-null check. Verified: differential check back to 0 mismatches (now checked under all four Nullness factories per the previous commit); full checker/jtreg (89 passing) and framework/jtreg (4 passing); full :framework:test/:checker:test/:javacutil:test/:dataflow:test suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
checkerAnnotationNames restricts declaration-annotation comparison to org.checkerframework.* names, but the javadoc gave no rationale for excluding everything else. Tried widening the filter to compare all declaration annotations (except the already-documented CFComment carve-out) and ran NullnessBinaryStubDiffTest: it surfaced 7 mismatches on the JDK 21 annotated JDK, all text-parser name-resolution quirks (e.g. @Retention/@target on java.lang.Override/Deprecated/ SuppressWarnings resolved by the binary writer but not the text parser; a @DefinedBy reachable only via a static import; JDK-internal meta-annotations like jdk.internal.javac.PreviewFeature), not binary-reader bugs. Reverted the widening and instead documented the observed mismatches and the rationale in the javadoc, and noted org.jspecify as a known gap since the JDK minimizer already preserves org.jspecify content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
Three user-visible fixes on this branch had no CHANGELOG entry: the fakeOverriddenMethod generic-parameter matching fix (0360e55), the unbounded-wildcard NPE fix (a8d180d), and the permit-nullness-assertion-exception.astub missing import fix (f5a8b37). Add one bullet each, matching the style of the existing org-eclipse.astub typo bullet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
fullyQualify(String, CompilationUnit) resolved a simple class name only against explicit imports, java.lang, and asterisk imports. An unimported class in the stub file's own package -- e.g. @anno(Foo.class) where Foo is declared elsewhere in the same package -- was left as the bare name "Foo". BinaryStubReader#resolveSingleValue resolves a class-literal member with a single Elements.getTypeElement(fqName) call and no fallback, so the misqualified name silently dropped the annotation element. Add a package-prefix fallback (packageName + "." + name), mirroring the text parser's AnnotationFileParser.findTypeOfName, which tries the same fallback. The fallback is a Class.forName probe on the stubifier classpath, cached the same way annotationInPackage caches its own probes (including negative results), via a new classInPackage helper shared with two later cleanups in this area. Add a writer test (classLiteralInCurrentPackageIsFullyQualified) covering the case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
fullyQualify(Type, CompilationUnit) qualified a ClassOrInterfaceType only when !cit.getScope().isPresent(), so a scoped class literal (e.g. Map.Entry.class with "import java.util.Map;" in scope) was written as the unqualified "Map.Entry". BinaryStubReader#resolveSingleValue's single Elements.getTypeElement(fqName) call cannot resolve that, silently dropping the class-literal annotation element. Qualify the outermost scope through the same fullyQualify(String, ...) tables used for the unscoped case, then re-attach the nested simple names unchanged (Map.Entry -> java.util.Map.Entry). Class-literal types are always raw (JLS 15.8.2), so no scope level here carries type arguments to preserve. Add a writer test (scopedClassLiteralIsFullyQualified) covering the case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
…obes
fullyQualify(String, CompilationUnit) re-scanned cu.getImports() for a
name ending in "." + name, checking !imp.isAsterisk() but not
isStatic(). simpleToFqn (populated in initImportTables) already holds
every non-static, non-asterisk import, so for a regular import this
loop was unreachable dead code. For a static non-asterisk import (e.g.
"import static java.lang.Integer.MAX_VALUE;") it was live but wrong: a
class-literal simple name colliding with the imported member's simple
name (e.g. a class named "MAX_VALUE") would match on the ".MAX_VALUE"
suffix and return the constant's declaring class as if it were the
class's own package -- a class literal misqualified as a constant's
enclosing type. Delete the loop; simpleToFqn already covers everything
it could legitimately match.
Checked for interaction with T3's staticImportedConstants (added by an
earlier commit in this queue): the deleted loop only read
cu.getImports() directly and never touched the staticImportedConstants
map, which is populated separately in initImportTables and consumed
only by writeValue's NameExpr case. The two are independent; deleting
this loop does not affect static-imported-constant resolution.
Also delete the hard-coded 11-name java.lang class list: the
Class.forName("java.lang." + name) probe immediately below it resolves
every name on the list anyway, so the list only saved one exception
dispatch for eleven of the many java.lang classes actually in use.
Route both the java.lang probe and the asterisk-import loop's probe
through the new classInPackage helper (added for T20(a)), which caches
positive and negative results the same way annotationInPackage does,
so a name that resolves against neither the current package nor
java.lang.* nor any asterisk-imported package no longer pays for a
freshly-thrown-and-discarded ClassNotFoundException on every
occurrence.
No behavior change: verified via the binary-vs-text diff harness
(NullnessBinaryStubDiffTest), 0 mismatches across 1652 top-level
classes and all built-in stub files, both before and after this
change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
annotationTargets treated every dotless (unqualified) annotation name the same way: silently return NO_TARGET, routing the annotation as a declaration annotation with no diagnostic. The javadoc justified this by claiming neither BinaryStubReader nor the text parser can resolve such a name either. That is true only when the compilation unit has no asterisk import at all (e.g. the annotated JDK's own @SuppressFBWarnings and @ConstructorProperties, used by same-package visibility with no import - genuinely unresolvable on both sides). It is false when an asterisk import exists: fullyQualifyAnnotationName's Class.forName probe of that import only proves the package is absent from the stubifier's own narrow build classpath, not that a checker's Elements would also fail to resolve it on whatever classpath the checker's invocation supplies. That case silently produced a binary form BinaryStubReader can never resolve either, dropping an annotation the text parser could have handled correctly - with no diagnostic anywhere. Fail the stubification instead when a dotless name coincides with a present asterisk import, mirroring the existing NOT_LOADABLE handling for a dotted-but-unloadable name: BinaryStubFileGenerator already turns such a failure into a skipped (text-parsed) stub file, and JavaStubifier already fails the whole build for the annotated JDK, per commit 5886516. Keep the silent NO_TARGET fallback only when no asterisk import could be responsible. Verified against the real annotated JDK (copyAndMinimizeAnnotatedJdkFiles) and the built-in stub corpus (generateBinaryStubFiles for :framework and :checker) that this does not change the outcome of any stub shipped today. Rewrote the javadoc of annotationTargets and fullyQualifyAnnotationName to state the real distinction instead of the former blanket claim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
fakeOverriddenMethod's parameter comparison (sameTypes) unconditionally accepted any stub parameter when the javac candidate's parameter is a type variable (the leniency introduced to fix generic-parameter matching for methods like TreeMap.computeIfPresent, which a stub can only spell differently than javac's viewpoint-adapted form). But "<T> void f(T)" and "void f(String)" legally coexist as overloads (erasures f(Object) and f(String)), so if f(T) was visited first, a stub fake override f(String) bound to f(T) and its annotations landed on the wrong overload. Latent in the annotated-JDK corpus, but reproduced with a stub file: the fake override's @nullable return was reported at calls to the type-variable overload instead of the String overload. Match in two passes: first look for a candidate whose every parameter matches exactly (a type-variable parameter then only matches a stub parameter with the same type-variable name); only if none matches, fall back to the previous type-variable-lenient comparison. The fallback preserves the generic-parameter fix: a stub that spells a type-variable parameter differently than the JDK declaration still matches once no exact candidate exists. Add a NullnessStubfileTest input where a supertype declares both <T> f(T) and f(String) (in that order) and a stub fake-overrides f(String) on a subclass; the @nullable return must land on the String overload only. Verified the test fails without the fix (the error moves to the type-variable overload's call site). Verified with :framework:test, NullnessBinaryStubDiffTest (0 mismatches on all built-in stubs), NullnessTest, and NullnessStubfileTest. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
findFieldInType resolved a simple-name constant reference in an annotation value with a hand-rolled recursion that exhausted the whole superclass subtree before looking at any interface. The text parser's AnnotationFileParser.findFieldElement instead iterates ElementUtils.getAllFieldsIn, whose getSuperTypes traversal interleaves a type's directly-implemented interfaces with its superclass chain. For an ambiguous simple name declared in both an interface and a deeper superclass, the two paths could resolve to different fields, in exactly the code whose goal is matching the text parser. Replace the recursion with the same getAllFieldsIn iteration the text parser uses, keeping the FIELD-kind filter (getAllFieldsIn also returns ENUM_CONSTANT elements, which callers resolve through a separate enum-constant lookup). Add a regression test pinning the interface- before-deeper-superclass order for an ambiguous name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
BinaryStubReader's dispatchSetValue funnels 9 of its 13 value types through typed setValue overloads (Boolean, Character, Double, Float, Integer, Long, Short, String, AnnotationMirror) that were themselves pure delegations to the private setValue(CharSequence, Object). Adding a new supported value type required updating both the dispatch switch and a new typed overload, and a missed branch silently dropped the value. Make the generic setValue(CharSequence, Object) overload public so callers that already hold an Object of one of the supported types can call it directly instead of going through a typed overload. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
…hSetValue dispatchSetValue had 13 instanceof branches, 9 of which (Boolean, Character, Double, Float, Integer, Long, Short, String, AnnotationMirror) delegated to typed AnnotationBuilder overloads that are themselves pure delegations to the generic setValue(CharSequence, Object). A new supported value type had to be added in two places, and a missed branch silently dropped the value. Collapse those 9 branches into a single call to the now-public generic setValue, which performs the same checkSubtype validation. Keep as explicit branches the cases with real logic on the AnnotationBuilder side: TypeMirror (erase/box special-casing), VariableElement (exact enum enclosing-element match, bypasses checkSubtype), and the Byte -> Short workaround (routing a raw Byte through the generic path would store the wrong box type). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
The generated .astub.bin.gz resources were wired via a plain-directory
srcDir, a manual processResources dependsOn, and a configureEach hook
matching on the sourcesJar task's name. Any other task that packages
main resources got no dependency and could race or ship without the
binary stubs, and the name-matching hook silently did nothing if a
plugin registered the sources jar under a different name.
Wire sourceSets.main.resources.srcDir(tasks.named('generateBinaryStubFiles'))
instead: Gradle tracks the task as the producer of that source directory
and automatically adds the dependency to every consumer of main
resources, so the manual dependsOn and the name-matching hook are no
longer needed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
…ource dir" This reverts commit f282153. The task-provider srcDir idiom does not register the producer dependency in this build: with the reverted commit, processResources goes UP-TO-DATE without ever scheduling generateBinaryStubFiles, and framework.jar and checker.jar silently ship without any of the 42 .astub.bin.gz resources (checked after ./gradlew clean assemble sourcesJar). The same holds for srcDir(files(dir).builtBy(taskProvider)), with and without the configuration cache, on Gradle 9.6.1. The explicit processResources dependsOn plus the sourcesJar configureEach hook are load-bearing on this Gradle version; keep them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
JavaParser's Parameter.getType() for a varargs parameter is the ELEMENT type (it excludes the implicit array level), but BinaryStubReader applies the recorded parameter type-annotation paths against the full array parameter type. processCallable extracted the declared type's embedded annotations with no prefix, so every such path was off by one leading ARRAY step: - "List<@nullable String>... args": the recorded path [TYPE_ARGUMENT 0] hits a TYPE_ARGUMENT step on an AnnotatedArrayType in the reader's resolvePath, which returns null, and the annotation is silently dropped. - "java.lang.@nullable String... args": the empty path lands @nullable on the array type itself instead of the element type. The text parser anchors the declared type at the array's component type (AnnotationFileParser.processParameters), so both cases diverged from it. The other two vararg annotation sources in processCallable already compensate: decl-position annotations prepend ARRAY plus arrayElementPath, and getVarArgsAnnotations() correctly uses the empty path for the array type itself. Seed the extraction path with one ARRAY step for a varargs parameter, and add a comment table mapping each source to its anchor and prefix. No wire-format change: the format already encodes arbitrary paths; only the paths the writer emits change. The annotated JDK and the built-in stub files contain no annotation embedded in a vararg's declared type today (grepped for generic-argument, fully-qualified-element, and explicit-array-level patterns before "..."), so no end-to-end regression test is possible against the shipped stubs; checker/tests/nullness/ VarargsArrayAnnotation.java is therefore unchanged. Two writer round-trip tests pin the recorded paths for both scenarios. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
…DK application The lazy binary-JDK application path in maybeParseEnclosingJdkClass ran with parsingCount == 0, so isParsingAnnotationFile() stayed false while a class's records were only partially applied. Since processedBinaryClasses.add happens before applyClassRecord, and inner classes are applied sequentially after the outer class, a reentrant getAnnotatedType(overridden) call from BinaryStubReader.applyFakeOverride (reached while applying an override on the outer or an already-applied inner class) that targets a not-yet-applied sibling inner class would short-circuit in maybeParseEnclosingJdkClass (its outermost class was already registered) and freeze a type without its stub annotations into AnnotatedTypeFactory's elementCache/cacheDeclAnnos permanently, since no clearParsePhaseCache ran on this path. This was the only unbracketed application path; the built-in binary stubs (parseStubFiles) and both text-parsing paths (parseJdkStubFile/parseJdkJarEntry) already pair ++/--parsingCount with a clearParsePhaseCache when the count returns to 0. Wrap the whole outer-plus-inner-class application in maybeParseEnclosingJdkClass in the same ++parsingCount/--parsingCount bracket, outside applyBinaryClassRecord (which stays a plain, state-managing-free helper, as its javadoc already anticipated). Use a counter rather than a boolean because this method can itself be reentered through applyFakeOverride, so only the outermost call should observe parsingCount == 0 and clear GenericAnnotatedTypeFactory's parsePhasePrimaryDefaultsCache; that clear is required once bracketing is in place; the cache is populated only while isParsingAnnotationFile() is true, so leaving it unbracketed would leak incomplete parse-phase defaults into checking. The clear is cheap (a small IdentityHashMap), so no attempt is made to avoid it for classes without inner classes. No changes to the text-parser paths. Deliberately no new test: a direct reentrancy fixture (outer class whose inner class A has a fake override depending on sibling inner class B applied later) was assessed as impractical to construct reliably from real JDK stub data; the diff harness and the JUnit suites cover this path today with 0 mismatches. Verified: :framework:test, NullnessBinaryStubDiffTest (0 mismatches), NullnessTest all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
be2c1cc's omission of all-empty MethodRecords from the annotated JDK justified itself in part by "applyFakeOverride returns immediately on empty returnTypeAnnos". Per a maintainer decision, a fake override -- a stub declaration for a method the class only inherits -- must reset the member's type at that subtype even when it carries no annotations at all (BinaryStubReader#applyFakeOverride will be changed to store a fakeOverrides entry unconditionally in the next commit, matching AnnotationFileParser.processFakeOverride's existing text-side behavior). An all-empty fake-override record is therefore not a no-op, and omitting one silently drops that reset before the reader ever sees it. This writer has no javac Elements and cannot resolve whether a given method record will be found among a class's own real members at read time -- that depends on the JDK version actually being compiled against, which can differ from the JDK version the annotated JDK's own source tree reflects (a later JDK sometimes "pulls down" an interface default method into a concrete override, so the same method is a real override on one JDK version and a fake override on another). Two purely syntactic, conservative proxies stand in: MethodRecord#hasOverrideAnnotation (an explicit @OverRide, covering a superclass override) and a new whole-run interfaceMethodSigs set (every signature declared by any processed interface, covering an override of an interface's own method). The latter is needed because grepping ../jdk/src found a real, in-corpus example that carries neither: java.util.TreeMap.NavigableSubMap declares putIfAbsent, merge, computeIfAbsent, compute, and computeIfPresent -- overriding java.util.Map's default methods -- with no @OverRide and no annotations at all. Built-in .astub files were also grepped (BinaryStubFileGenerator never sets omitUnannotatedMembers, so they are unaffected regardless); nothing comparable was found there. Because interfaceMethodSigs is only complete once every compilation unit in the run has been processed, and a class can be processed before an interface it implements (NavigableSubMap is declared in TreeMap.java, which can be visited before Map.java), the omission decision can no longer be made eagerly in addMethodRecord. Method records are now always kept during processing and filtered in one pass at the start of writeTo, once the interface index is complete. Verified: :framework:test (BinaryStubWriterTest, including a new case for an unannotated @OverRide method), and :framework:generateBinaryStubFiles regenerates annotated-jdk.bin.gz successfully (248,858 bytes, up from 237,283 with the previous omission and still well under the 320,674 bytes with no omission at all); a small ad hoc check confirms NavigableSubMap's five methods above are now present in the regenerated file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
applyFakeOverride returned immediately when a fake-override method record carried no return-type annotations, storing nothing in fakeOverrides. AnnotationFileParser.processFakeOverride, the text parser's equivalent, stores a fakeOverrides entry unconditionally for every matched fake-override declaration -- there is no analogous early return. Per a maintainer decision, this asymmetry is a bug, not a deliberate difference: a fake override, even fully unannotated, must reset the method's type at that subtype to a fresh getAnnotatedType(overridden), overruling whatever was computed before, matching the text side. The previous commit (BinaryStubWriter: keep unannotated fake-override records) fixed the writer side of the same bug: without it, a fully-unannotated fake override was already dropped before ever reaching this method. Remove the early return so every fake-override record is stored, mirroring the text parser exactly; applyTypeAnnos and the rest of the method already handle an empty returnTypeAnnos array as a no-op, so no other change is needed. The atypeFactory.getAnnotatedType(overridden) call in this method can reenter AnnotationFileElementTypes#maybeParseEnclosingJdkClass for a sibling not-yet-applied inner class; this reentrancy is bracketed by parsingCount in commit c0ba0bb, so this call remains safe. Verified: :framework:test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
…tations The record oracle in verifyRecordApplied only checked the fakeOverrides store for records with returnTypeAnnos, and the whole method-record loop skipped records without hasTypeInfo before ever reaching the fake-override branch. Both gates reflected the binary reader's old behavior of skipping fake overrides without return-type annotations, which the previous commit removed; had they stayed, the new unannotated-fake-override records (kept by the writer two commits ago) would have remained permanently unverified by this harness. Remove both gates: every non-constructor method record that matches no declared method but does match an inherited one must now have produced a fakeOverrides entry, annotated or not. The atypes presence check keeps its hasTypeInfo condition, moved inline into the matched-method branch. Also rewrite the compareClass comment that described text-only fakeOverrides keys as a benign, deliberate binary-path skip; the invariant now is that both paths store an entry for every matched fake-override record. Verified: :framework:test, NullnessBinaryStubDiffTest (0 mismatches under the stricter oracle), and NullnessTest all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
…sspath The .astub.bin.gz resources are produced by the generateBinaryStubFiles task and reach the resources output only through explicit dependsOn hooks on processResources and sourcesJar; the idiomatic srcDir(taskProvider) wiring does not register the producer dependency on this Gradle version (see the revert in commit 278a3e3). If that wiring rots, the binaries silently vanish from the classpath and BinaryStubReader silently falls back to text parsing with no diagnostic. Add one JUnit test per subproject that asserts a known built-in binary stub (the framework's and the nullness checker's jdk11.astub.bin.gz) is present on the classpath, using Class.getResource with the same sibling-resource path AnnotationFileElementTypes.parseOneStubFile uses, so a wiring regression fails the test suite loudly. Verified that removing the resource from build output makes each test fail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
…racketing applyClassRecord must run only while isParsingAnnotationFile() is true: AnnotatedTypeFactory's elementCache/cacheDeclAnnos writes are suppressed during parsing, so applying a class record outside that bracket risks freezing a type computed from a partially-applied record into those caches permanently (the bug commit c0ba0bb fixed for the lazy binary-JDK path). All three current call sites are bracketed today: the lazy path directly (maybeParseEnclosingJdkClass, c0ba0bb), the built-in binary stub path via parseStubFiles, and the diff-checker path transitively via parseStubFiles -> prepJdkStubs -> BinaryStubDiffChecker. A future refactor could silently drop one of these brackets without being noticed until stub annotations mysteriously go missing. Add a one-line assertion at the top of applyClassRecord so a future regression fails loudly (under -ea, which framework/checker tests run with) instead of corrupting caches silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpsEToWDrZU7QDQ5i98s26
Every reference to org.checkerframework.framework.stubifier.BinaryStubWriter was spelled out fully-qualified, in both code and Javadoc, making the constant declarations and their comments harder to read than necessary. Import the class and use its simple name throughout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
framework.jar must work standalone, but the stubifier source set's classes (BinaryStubWriter, JavaStubifier, ...) are not bundled into it -- they ship only inside checker.jar's shadow jar. BinaryStubData already depended on BinaryStubWriter safely, because it only reads compile-time constants that javac inlines, leaving no runtime reference. That invariant was easy to break silently by future edits. Add a warning at the import in BinaryStubData.java and next to the `implementation sourceSets.stubifier.output` dependency in framework/build.gradle, spelling out what is safe (compile-time constants, or code exclusively run via checker.jar) and what is not (a real method call or non-constant field read from framework.jar-shipped code into a stubifier class). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
annotatedJdkHome was hardcoded to '../../jdk', a sibling directory that an external checker's own build may also want to use for its own, different annotated JDK fork when composed via --include-build. Allow -PannotatedJdkHome=/abs/path to redirect this project's own task at a different checkout; the default is unchanged for every existing caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n JDK clone This script deleted the eisop/jdk clone at '../jdk' and replaced it with JSpecify's own annotated JDK fork before running the composite build. framework:copyAndMinimizeAnnotatedJdkFiles always runs when building :checker-framework:checker (it is unconditional), so it then tried to process JSpecify's JDK fork with this project's own narrow stubifier classpath (stubparser + checker-qual, no JSpecify), and BinaryStubWriter fails loudly on any annotation it cannot resolve: "cannot load annotation org.jspecify.annotations.Nullable ...". jspecify-reference-checker already has its own independent mechanism (copyJSpecifyJDK + includeJSpecifyJDK) to build a complete binary annotated JDK for its own JSpecify fork, using its own classpath (which already includes org.jspecify). The two only collided because both conventionally expect a JDK checkout at the same sibling '../jdk' path. Leave '../jdk' as this project's own eisop/jdk clone, and clone JSpecify's fork to a separate '../jdk-jspecify' directory instead, passed to jspecify-reference-checker's build via -PjspecifyJdkHome (requires a matching override in jspecify-reference-checker's own build.gradle, prepared as a separate patch against that repo). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Added in 276cdd2 ("Support annotated JDKs that only use JSpecify annotations") for when this task's own annotatedJdkHome pointed directly at a JSpecify-flavored JDK checkout. Now that jspecify-reference-checker keeps its own separate JDK checkout (previous commit) and recognizes its own annotations independently via its own copyJSpecifyJDK task, this project's own task has no reason to special-case a specific external checker's annotations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JavaStubifier and BinaryStubWriter were only reachable through
shadowJar's minimize() by accident: RemoveAnnotationsForInference (an
unrelated dev tool in framework main) happens to call
JavaStubifier.dirnameToPath, which keeps both classes referenced.
BinaryStubFileGenerator has no such incidental reference and was
silently dropped. External checkers building their own binary
annotated JDK or .astub files (e.g. jspecify-reference-checker's
includeJSpecifyJDK, via checker.jar) depend on all three being present.
"exclude(project(':framework'))" in minimize() does not protect them:
the stubifier sourceSet's classes reach :checker: only as an anonymous
file-collection dependency riding along inside :framework's own
"implementation sourceSets.stubifier.output", which Gradle does not
give a resolvable-dependency identity that minimize()'s exclude() can
match against.
Add a dedicated stubifierToolForShadow configuration that resolves
framework's stubifierTool configuration as a real, separately
identified project dependency (kept out of "implementation"/"api" to
avoid conflicting with the existing "api project(':framework')"
dependency's capability), merge it into shadowJar, and exclude it from
minimize(). Verified via `jar tf` that checker.jar now retains
JavaStubifier, BinaryStubWriter, and BinaryStubFileGenerator, with no
duplicate-entry warnings from Shadow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First developed with Kiro and then further refined with Claude.
Then two rounds of reviews with Antigravity, with Claude fixing those issues.