Add test for @SuppressWarnings and @AnnotatedFor interaction and refine SourceChecker logic - #1699
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes warning suppression behavior in SourceChecker so an enclosing @SuppressWarnings still applies when an inner declaration is marked @AnnotatedFor. It also adds regression tests in both framework-level subtyping tests and nullness conservative-default tests.
Changes:
- Removed the redundant early return in
SourceChecker.shouldSuppressWarnings(Element, String)so suppression lookup continues through enclosing elements. - Added a subtyping regression test covering class-level
@SuppressWarningswith a method-level@AnnotatedFor. - Added a nullness regression test covering the same interaction for nullness checking.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
framework/tests/conservative-defaults/annotatedfor/AnnotatedForTest.java |
Adds a regression case proving outer @SuppressWarnings("subtyping") overrides inner @AnnotatedFor("subtyping"). |
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java |
Removes the early exit that prevented suppression lookup from reaching enclosing scopes. |
checker/tests/nulless-conservative-defaults/annotatedfornullness/AnnotatedForNullness.java |
Adds a nullness regression case for the same suppression/@AnnotatedFor interaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thisisalexandercook
left a comment
thisisalexandercook
left a comment
There was a problem hiding this comment.
Great catch! It may be worth noting that #1331 also removes this check as part of its broader refactor, but this bug is independent and makes sense as its own PR.
Co-authored-by: Alex Cook <43047600+thisisalexandercook@users.noreply.github.com>
Thanks! Yes, that's the point of having this PR. |
wmdietl
left a comment
wmdietl
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
Also, please go through the javadoc and manual and see where this behavior is described.
wmdietl
left a comment
wmdietl
left a comment
There was a problem hiding this comment.
Some high-level clarifications that I need to understand what this is trying to do.
|
@aosen-xiong Please review what I did with Antigravity. |
Thanks! Your changes make the Element and TreePath overloads consistent. |
|
@wmdietl pin for review. |
Three conflicts, all from eisop#1699, which refined the same @AnnotatedFor and @SuppressWarnings logic this branch refactors. SourceChecker: kept master's post-eisop#1699 control flow -- the foundAnnotatedFor accumulation and hasSuppressWarningsAnnotationForErrorKey -- and applied only this branch's change on top: rename isAnnotatedForThisCheckerOrUpstreamChecker to isElementAnnotatedForThisCheckerOrUpstreamChecker and make it abstract, implemented in BaseTypeChecker. This branch had returned false as soon as an @AnnotatedFor element was found; eisop#1699 deliberately keeps scanning so that a @SuppressWarnings on an enclosing declaration still wins, which is what its new ElementSuppressionTest pins. The redundant packageElement call this branch deleted is kept, since it sits inside that logic. AnnotatedTypeFactory: both sides added the same method under different names. Took master's isParsingAnnotationFile() and its Javadoc, and updated this branch's caller in QualifierDefaults. QualifierDefaults: the relocated isElementAnnotatedForThisChecker stays removed, and this branch's annotation-file-parsing guard is kept, without the TODO comment that master deleted.
Fix
@SuppressWarningson an enclosing element being ignored when an inner element carries@AnnotatedFor.The early return was redundant: the loop's natural termination already produces false when no matching
@SuppressWarningsis found. Removing it lets the walk continue past @AnnotatedFor-annotated elements to enclosing scopes.