Share the @AnnotatedFor lookup between suppression and conservative defaults - #1331
Merged
Merged
Conversation
aosen-xiong
commented
Aug 26, 2025
wmdietl
requested changes
Sep 1, 2025
aosen-xiong
force-pushed
the
code-refactor-annotatedfor
branch
from
October 7, 2025 23:47
c10565f to
eb741f4
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the cache and method for determining if an element is annotated for a checker, moving it from QualifierDefaults to AnnotatedTypeFactory to enable code reuse. The method isElementAnnotatedForThisChecker is renamed to isElementAnnotatedForThisCheckerOrUpstreamChecker and made abstract in SourceChecker.
- Moved the
isElementAnnotatedForThisCheckermethod and its cache fromQualifierDefaultstoAnnotatedTypeFactory - Made
SourceChecker#isElementAnnotatedForThisCheckerOrUpstreamCheckeran abstract method - Implemented the abstract method in all subclasses (
BaseTypeChecker,AggregateChecker, and utility checkers)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| QualifierDefaults.java | Removed private isElementAnnotatedForThisChecker method and elementAnnotatedFors cache; updated calls to use the factory method |
| AnnotatedTypeFactory.java | Added public isElementAnnotatedForThisCheckerOrUpstreamChecker method with the cache implementation |
| SourceChecker.java | Changed method from private to protected abstract; removed implementation and imports; updated method calls |
| BaseTypeChecker.java | Implemented abstract method by delegating to the type factory |
| AggregateChecker.java | Implemented abstract method to return false (delegates to subcheckers) |
| SignaturePrinter.java | Implemented abstract method to throw BugInCF (not expected to be called) |
| JavaCodeStatistics.java | Implemented abstract method to throw BugInCF (not expected to be called) |
| AnnotationStatistics.java | Implemented abstract method to throw BugInCF (not expected to be called) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
@aosen-xiong Please go through the copilot suggestions and see what should be addressed. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
wmdietl
changed the title
@AnnotatedFor
wmdietl
reviewed
Aug 28, 2026
A stub file for a class that is being compiled is used only with -AmergeStubsWithSource. AnnotatedTypeFactory.fromElement applies that rule to type annotations, but getDeclAnnotations adds annotation-file declaration annotations unconditionally, so unifying the @AnnotatedFor lookup on getDeclAnnotation made a stub mark a source class as annotated without the flag. Read @AnnotatedFor off the element itself for an element declared in source code. This is a workaround for eisop#1987, which tracks the general problem in getDeclAnnotations. Add jtreg tests for both consequences of @AnnotatedFor, since master's two implementations disagreed and the disagreement cancelled out: the suppression test covers whether warnings inside the class are issued, and the conservative-defaults test covers what a caller sees. Both fail on master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wmdietl
reviewed
Aug 28, 2026
wmdietl
reviewed
Aug 28, 2026
…ompiled" This reverts commit b5c822b.
This reverts commit fb62ef4.
Whether an @AnnotatedFor written in a stub file applies to a class being compiled is the subject of eisop#1987 and eisop#1988, not of this refactoring. eisop#1988 adds its own test for it in checker/jtreg/stubs/issue1987. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aosen-xiong
changed the title
@AnnotatedFor@AnnotatedFor lookup between suppression and conservative defaults
aosen-xiong
added a commit
to aosen-xiong/checker-framework
that referenced
this pull request
Aug 29, 2026
Keep only the @NullMarked aliasing and its test. The @AnnotatedFor cache refactor that this branch also carried is eisop#1331; take master's version of every file it touched, which is also how the SourceChecker conflict is resolved. In docs/CHANGELOG.md, keep master's entries and this branch's @NullMarked bullet.
With -AmergeStubsWithSource the stub applies, so Lib carries
@AnnotatedFor("nullness") and is checked: `return null` from a method whose
return type defaults to @nonnull is an error.
The empty expected output recorded the two @AnnotatedFor lookups disagreeing.
Conservative defaults saw the stub, so Lib.get() was assumed to return
@nonnull, while warning suppression did not, so the null return went
unreported -- and Use accepted a value that is null at run time. Sharing one
lookup removes that.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aosen-xiong
commented
Aug 29, 2026
Collaborator
Author
|
Merging this PR enables us to merge #1304, which introduces |
wmdietl
approved these changes
Aug 29, 2026
wmdietl
left a comment
wmdietl
left a comment
Member
There was a problem hiding this comment.
Thanks, I think this is very nice now.
aosen-xiong
added a commit
to aosen-xiong/checker-framework
that referenced
this pull request
Aug 29, 2026
Conflict in QualifierDefaults: eisop#1331 removed isElementAnnotatedForThisChecker and its elementAnnotatedFors cache from QualifierDefaults, so this branch's changes to that method are obsolete. Take master's side; the @UnannotatedFor logic is ported to the shared lookup in a follow-up commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aosen-xiong
added a commit
to aosen-xiong/checker-framework
that referenced
this pull request
Aug 29, 2026
…or scope @UnannotatedFor was defined and given an AnnotatedTypeFactory helper, but its effect was implemented in QualifierDefaults.isElementAnnotatedForThisChecker, which eisop#1331 removed. Implement it in the lookup that replaced it, BaseTypeChecker.isElementAnnotatedForThisCheckerOrUpstreamChecker, so that it governs both conservative defaults and warning suppression. @AnnotatedFor is tested first, so @UnannotatedFor is purely subtractive: it stops the walk to the enclosing element, and never overrides an explicit @AnnotatedFor on the same element. A nested @AnnotatedFor takes effect again. Both shouldSuppressWarnings overloads accumulated the @AnnotatedFor answer over every enclosing declaration, and the TreePath overload additionally asked about the enclosing package. That defeated the exclusion: an @UnannotatedFor class in an @AnnotatedFor package was defaulted as unchecked code but still had its warnings reported. Ask isElementAnnotatedForThisCheckerOrUpstreamChecker once, about the innermost declaration; it already resolves enclosing scopes, so the per-level query was redundant work even before this change. Only one cache is needed. The element's cached boolean is the fully-resolved scope answer, including @UnannotatedFor exclusions. Also mirror doesUnannotatedForApplyToThisChecker on its @AnnotatedFor sibling, and correct the @UnannotatedFor javadoc, which claimed an effect on bytecode although the annotation has source retention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aosen-xiong
added a commit
to aosen-xiong/checker-framework
that referenced
this pull request
Aug 30, 2026
The branch was 310 commits behind, and master reworked the code it changes. Carry the optimistic defaults forward onto master's design: - eisop#1331 removed QualifierDefaults.isElementAnnotatedForThisChecker. The new applyOptimisticDefaults calls BaseTypeChecker.isElementAnnotatedForThisCheckerOrUpstreamChecker instead, as applyConservativeDefaults now does, and shares its cache. - applyDefaultsElement now applies a memoized, precedence-ordered default list from fusedDefaultsFor rather than looping per call. Its boolean parameter becomes a three-valued DefaultsMode, with a third empty-scope slot and a third identity cache; invalidateFusedDefaults clears all six. - applyOptimisticDefaults mirrors applyConservativeDefaults, including the fast path on the two flags and the isParsingAnnotationFile guard. Without the guard it reaches the checker before the visitor is installed and throws a NullPointerException; without the fast path it costs a stub-file and bytecode test on every defaulted type. - master's bytecode test is atypeFactory.isFromByteCode, not the three-part isElementFromByteCode/declarationFromElement/isFromStubFile check. Keep the existing method names. Renaming addUncheckedCodeDefault and addUncheckedStandardDefaults would break downstream checkers, and was why this branch disabled the JSpecify reference checker in CI; the optimistic variants are added beside them instead, and that CI script is restored. Fix the bytecode branch of applyOptimisticDefaults, which tested the source flag. Regenerate the four expected outputs in checker/jtreg/nullness/onlyannotatedfor: the added @compile directive shifts every line number by one. Co-Authored-By: Claude Opus 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.
Determining whether an element is in the scope of an
@AnnotatedForwas implemented twice:SourceChecker.isAnnotatedForThisCheckerOrUpstreamCheckerfor warning suppression andQualifierDefaults.isElementAnnotatedForThisChecker for conservative defaults. Only the latter was cached.Both now call
SourceChecker.isElementAnnotatedForThisCheckerOrUpstreamChecker(Element), which BaseTypeChecker implements with a cache. The default implementation returns false, which is correct for a checker that does not type-check.