Add test for @SuppressWarnings and @AnnotatedFor interaction and refine SourceChecker logic#1699
Add test for @SuppressWarnings and @AnnotatedFor interaction and refine SourceChecker logic#1699aosen-xiong wants to merge 18 commits into
@SuppressWarnings and @AnnotatedFor interaction and refine SourceChecker logic#1699Conversation
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.
| user-written annotation, \emph{and} the checker issues no warnings. | ||
| The command-line argument \code{-AonlyAnnotatedFor} can be used to suppress errors and warnings outside of the scope of an \<@AnnotatedFor> annotation, | ||
| but does not change the default qualifiers for source code (See Section~\ref{aonlyannotatedfor}). | ||
| Warnings in code with a relevant \<@AnnotatedFor> annotation can still be suppressed by |
There was a problem hiding this comment.
This sentence comes out of the blue and the connection to what's in the paragraph before is unclear. What is "still" referring to?
Would it make more sense to first keep the existing text and then say the new part? Is the "still" referring to passing -AonlyAnnotatedFor? Is that the important point?
There was a problem hiding this comment.
I updated the document.
| You can suppress all errors and warnings for code outside of a corresponding | ||
| \code{@AnnotatedFor} by applying this command-line option. | ||
| Note that the \code{@AnnotatedFor} annotation must include the checker's name to | ||
| enable warnings from that checker, except for warnings suppressed by another |
There was a problem hiding this comment.
Are the "Note" and "except" part really connected? Maybe make the "except" part a separate sentence that highlights that explicit SuppressWarnings suppress warnings in AnnotatedFor code and in UnannotatedFor code (if the extra flag is not there).
There was a problem hiding this comment.
Thanks, I updated the document.
| * otherwise | ||
| */ | ||
| public boolean shouldSuppressWarnings(TreePath path, String errKey) { | ||
| if (shouldSuppress(getSuppressWarningsStringsFromOption(), errKey)) { |
There was a problem hiding this comment.
The shouldSuppressWarnings(Tree tree, String errKey) performs this check (with a helpful comment) and another check, before calling this method.
Adding this check here now performs this check twice.
Should both checks from the Tree version be moved to the TreePath version? Are there other places that call this method that should not have both checks?
There was a problem hiding this comment.
Okay, I moved them to the treepath version. I checked the call sites and did not see any that had the checks.
| * @param errKey the error key the checker is emitting | ||
| * @return true if {@code elt} has an corresponding {@code @SuppressWarnings} annotation | ||
| */ | ||
| private boolean shouldSuppressWarningsOnElement(Element elt, String errKey) { |
There was a problem hiding this comment.
There already is a shouldSuppressWarnings(Element elt, String errKey) overload. What is the relationship between these two methods and why do we need both?
There was a problem hiding this comment.
Renamed the helper to hasSuppressWarningsAnnotationForErrorKey to make the relationship clearer. The shouldSuppressWarnings(Element, String) walks the enclosing element chain and applies global -AsuppressWarnings. This helper checks only one element’s direct @SuppressWarnings annotation, which the TreePath overload needs, while it separately tracks the nearest relevant @AnnotatedFor scope.
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.