Fix "unneeded.suppression" warning#7747
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSourceChecker drops exported suppression constants and rewrites unneeded-suppression handling: warnUnneededSuppressions now takes checkerPrefixes and inspects Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java (1)
2825-2835:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompare against
messageKey, not the suppression token.At Line 2825 and Line 2834, the check now uses
currentSuppressWarningsInEffect. In these branches, that value is a prefix (or"all"), so the condition is effectively always true and can incorrectly suppress theunneeded.suppressionwarning.🔧 Proposed fix
- boolean result = !currentSuppressWarningsInEffect.equals("unneeded.suppression"); + boolean result = !messageKey.equals("unneeded.suppression"); return result; ... - boolean result = !currentSuppressWarningsInEffect.equals("unneeded.suppression"); + boolean result = !messageKey.equals("unneeded.suppression"); return result;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java` around lines 2825 - 2835, The code is comparing the suppression token (currentSuppressWarningsInEffect) against "unneeded.suppression" and therefore always succeeds for prefixes/"all"; change those checks to compare the actual message key instead. In the SourceChecker logic where you currently compute boolean result = !currentSuppressWarningsInEffect.equals("unneeded.suppression") (both occurrences guarded by requirePrefixInWarningSuppressions and SUPPRESS_ALL_MESSAGE_KEY), replace the comparison to use messageKey (e.g., boolean result = !messageKey.equals("unneeded.suppression")) so suppression decisions check the real warning message key rather than the suppression token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java`:
- Around line 2825-2835: The code is comparing the suppression token
(currentSuppressWarningsInEffect) against "unneeded.suppression" and therefore
always succeeds for prefixes/"all"; change those checks to compare the actual
message key instead. In the SourceChecker logic where you currently compute
boolean result = !currentSuppressWarningsInEffect.equals("unneeded.suppression")
(both occurrences guarded by requirePrefixInWarningSuppressions and
SUPPRESS_ALL_MESSAGE_KEY), replace the comparison to use messageKey (e.g.,
boolean result = !messageKey.equals("unneeded.suppression")) so suppression
decisions check the real warning message key rather than the suppression token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b702b2a8-b4cf-4e96-bfac-5bafd2a70cd5
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java (3)
2472-2480: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove the dead
allErrorKeysplumbing from the warning pass.
warnUnneededSuppressions(...)no longer consultsallErrorKeys, but the caller still rebuilds that set for every compilation unit. That extra scan is now pure overhead in the hot path this PR is optimizing.♻️ Proposed cleanup
- Set<String> errorKeys = new HashSet<>(messagesProperties.stringPropertyNames()); for (SourceChecker subChecker : subcheckers) { allElementsWithSuppressedWarnings.addAll(subChecker.elementsWithSuppressedWarnings); subChecker.elementsWithSuppressedWarnings.clear(); prefixes.addAll(subChecker.getSuppressWarningsPrefixes()); - errorKeys.addAll(subChecker.messagesProperties.stringPropertyNames()); subChecker.getVisitor().treesWithSuppressWarnings.clear(); } - warnUnneededSuppressions(allElementsWithSuppressedWarnings, prefixes, errorKeys); + warnUnneededSuppressions(allElementsWithSuppressedWarnings, prefixes); @@ - * `@param` allErrorKeys all error keys that can be issued by this checker */ protected void warnUnneededSuppressions( - Set<Element> elementsSuppress, Set<String> checkerPrefixes, Set<String> allErrorKeys) { + Set<Element> elementsSuppress, Set<String> checkerPrefixes) {Also applies to: 2492-2496
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java` around lines 2472 - 2480, The code builds a local Set<String> errorKeys from messagesProperties.stringPropertyNames() and merges subcheckers' messagesProperties into it only to pass to warnUnneededSuppressions, but warnUnneededSuppressions no longer uses that parameter; remove this dead "allErrorKeys" plumbing by deleting the creation and incremental population of errorKeys (the HashSet based on messagesProperties.stringPropertyNames() and the errorKeys.addAll(...) calls inside the subcheckers loop) and simply call warnUnneededSuppressions(allElementsWithSuppressedWarnings, prefixes) (or the current two-arg form); also remove the analogous redundant code in the other similar block that merges subcheckers (the block using elementsWithSuppressedWarnings, getSuppressWarningsPrefixes(), messagesProperties, and getVisitor().treesWithSuppressWarnings).
2822-2835:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck the emitted
messageKey, not the suppression token.These branches are supposed to block prefix-only suppressions from hiding the
unneeded.suppressionmeta-warning, butcurrentSuppressWarningsInEffectis the prefix (checkername/all), so the guard is always true. As written,@SuppressWarnings("checkername")and@SuppressWarnings("all")still suppress the meta-warning, which violates the documented contract.🐛 Proposed fix
if (prefixes.contains(currentSuppressWarningsInEffect)) { // The value in the `@SuppressWarnings` is exactly a prefix. // Suppress the warning unless its message key is "unneeded.suppression". - boolean result = !currentSuppressWarningsInEffect.equals("unneeded.suppression"); - return result; + return !messageKey.equals("unneeded.suppression"); } else if (requirePrefixInWarningSuppressions) { @@ } else if (currentSuppressWarningsInEffect.equals(SUPPRESS_ALL_MESSAGE_KEY)) { // Prefixes aren't required and the SuppressWarnings string is "all". // Suppress the warning unless its message key is "unneeded.suppression". - boolean result = !currentSuppressWarningsInEffect.equals("unneeded.suppression"); - return result; + return !messageKey.equals("unneeded.suppression"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java` around lines 2822 - 2835, The code is comparing the suppression token (currentSuppressWarningsInEffect / SUPPRESS_ALL_MESSAGE_KEY) to "unneeded.suppression" instead of checking the emitted diagnostic's message key, so prefix-only suppressions still hide the meta-warning; update the three places that compute boolean result to check the diagnostic message key instead (e.g., use messageKey.equals("unneeded.suppression") or message.getKey() if messageKey isn't in scope) and return !messageKey.equals("unneeded.suppression") (or !message.getKey().equals(...)) so that only a suppression that targets the actual message key suppresses the meta-warning; keep the existing branches (prefixes.contains(...), requirePrefixInWarningSuppressions, SUPPRESS_ALL_MESSAGE_KEY) but change the equality check to the messageKey.
2518-2520:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire an exact
:unneeded.suppressionsuffix here.
regionMatchesonly checks the prefix, so strings likechecker:unneeded.suppression.fooare treated as exempt and never reported, even thoughshouldSuppress(...)will not let them suppress the emitted meta-warning. Compare the suffix exactly instead of prefix-matching it.🐛 Proposed fix
if (colonPos != -1) { String warningPrefix = suppressWarningsString.substring(0, colonPos); + String suppressedKey = suppressWarningsString.substring(colonPos + 1); if (checkerPrefixes.contains(warningPrefix) - && !suppressWarningsString.regionMatches( - colonPos, ":unneeded.suppression", 0, ":unneeded.suppression".length())) { + && !suppressedKey.equals("unneeded.suppression")) { reportUnneededSuppression(tree, suppressWarningsString); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java` around lines 2518 - 2520, The current check uses regionMatches which only matches a prefix at colonPos, allowing strings like "checker:unneeded.suppression.foo" to slip through; change it to require an exact suffix match by comparing the substring starting at colonPos to the literal ":unneeded.suppression" (e.g., use suppressWarningsString.substring(colonPos).equals(":unneeded.suppression") or ensure colonPos + literal.length() == suppressWarningsString.length() after regionMatches). Update the conditional that references checkerPrefixes, warningPrefix, suppressWarningsString, and colonPos so it only treats the string as exempt when the substring equals the exact literal (this keeps behavior consistent with shouldSuppress(...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java`:
- Around line 2472-2480: The code builds a local Set<String> errorKeys from
messagesProperties.stringPropertyNames() and merges subcheckers'
messagesProperties into it only to pass to warnUnneededSuppressions, but
warnUnneededSuppressions no longer uses that parameter; remove this dead
"allErrorKeys" plumbing by deleting the creation and incremental population of
errorKeys (the HashSet based on messagesProperties.stringPropertyNames() and the
errorKeys.addAll(...) calls inside the subcheckers loop) and simply call
warnUnneededSuppressions(allElementsWithSuppressedWarnings, prefixes) (or the
current two-arg form); also remove the analogous redundant code in the other
similar block that merges subcheckers (the block using
elementsWithSuppressedWarnings, getSuppressWarningsPrefixes(),
messagesProperties, and getVisitor().treesWithSuppressWarnings).
- Around line 2822-2835: The code is comparing the suppression token
(currentSuppressWarningsInEffect / SUPPRESS_ALL_MESSAGE_KEY) to
"unneeded.suppression" instead of checking the emitted diagnostic's message key,
so prefix-only suppressions still hide the meta-warning; update the three places
that compute boolean result to check the diagnostic message key instead (e.g.,
use messageKey.equals("unneeded.suppression") or message.getKey() if messageKey
isn't in scope) and return !messageKey.equals("unneeded.suppression") (or
!message.getKey().equals(...)) so that only a suppression that targets the
actual message key suppresses the meta-warning; keep the existing branches
(prefixes.contains(...), requirePrefixInWarningSuppressions,
SUPPRESS_ALL_MESSAGE_KEY) but change the equality check to the messageKey.
- Around line 2518-2520: The current check uses regionMatches which only matches
a prefix at colonPos, allowing strings like "checker:unneeded.suppression.foo"
to slip through; change it to require an exact suffix match by comparing the
substring starting at colonPos to the literal ":unneeded.suppression" (e.g., use
suppressWarningsString.substring(colonPos).equals(":unneeded.suppression") or
ensure colonPos + literal.length() == suppressWarningsString.length() after
regionMatches). Update the conditional that references checkerPrefixes,
warningPrefix, suppressWarningsString, and colonPos so it only treats the string
as exempt when the substring equals the exact literal (this keeps behavior
consistent with shouldSuppress(...)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a48038e8-7995-49a3-8fab-35436769854e
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
mernst
changed the title
mernst
commented
May 28, 2026
This was referenced May 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java`:
- Around line 2843-2847: The branch in SourceChecker that currently checks
currentSuppressWarningsPrefix.equals("allcheckers") ||
!prefixes.contains(currentSuppressWarningsPrefix) wrongly skips
"allcheckers:<key>" forms; update the logic used in the loop that evaluates
suppressWarningsString() so that if currentSuppressWarningsPrefix
startsWith("allcheckers") (or matches the exact "allcheckers" token with a colon
suffix) you treat it as a valid global prefix rather than skipping it — i.e.,
detect the "allcheckers:" variant from currentSuppressWarningsPrefix and allow
it to be handled like other supported prefixes (so that
warnUnneededSuppressions(...) will recognize and not report those suppressions
as unused).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25d65127-1aba-4e9e-a64b-fc205535ebbe
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Contributor
|
Actionable comments posted: 0 |
Contributor
|
Actionable comments posted: 0 |
smillst
approved these changes
May 29, 2026
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
2 participants
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.
No description provided.