8344159: Add lint warnings for unnecessary warning suppression

[archiecobbs]

This PR adds a new compiler warning for @SuppressWarnings annotations that don't actually suppress any warnings.

Summary of code changes:

• Add new warning and associated lint category "suppression"
• Update LintMapper to keep track of which @SuppressWarnings suppressions have been validated ¹
• Update Log.warning() so it validates any current suppression of the warning's lint category in effect.
• Add a new validate parameter to Lint.isEnabled() and Lint.isSuppressed() that specifies whether to also validate any current suppression.
• Add Lint.isActive() to check whether a category is enabled or suppression of the category is being tracked - in other words, whether the warning calculation needs to be performed. Used for non-trivial warning calculations.
• Add -Xlint:-suppression flags to *.gmk build files so the build doesn't break

¹ The suppression of a lint category is "validated" as soon as it suppresses some warning in that category

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)

Issues

• JDK-8344159: Add lint warnings for unnecessary warning suppression (Enhancement - P4)
• JDK-6723459: Compiler should flag where warning suppression is not required (Enhancement - P3)

Reviewers

• Erik Joelsson (@erikj79 - Reviewer) 🔄 Re-review required (review applies to f2b75547)
• Magnus Ihse Bursie (@magicus - Reviewer) 🔄 Re-review required (review applies to e3283ba9)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25167/head:pull/25167
$ git checkout pull/25167

Update a local copy of the PR:
$ git checkout pull/25167
$ git pull https://git.openjdk.org/jdk.git pull/25167/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25167

View PR using the GUI difftool:
$ git pr show -t 25167

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25167.diff

Using Webrev

Link to Webrev Comment

[erikj79]

/label remove kulla

I have removed kulla-dev from the bot configuration so the stuck label is now benign.

[jddarcy]

Hi @archiecobbs , are there bugs filed to clean the causes of the warnings?

[archiecobbs]

Hi @archiecobbs , are there bugs filed to clean the causes of the warnings?

Hi @jddarcy,

At the beginning of this project as a preliminary step I filed a bunch of bugs+PR's to remove unnecessary @SuppressWarnings annotations. These were accepted, but they affected every area of the JDK and people kept adding new ones so it was impractical to keep up. The only way to make this cleanup operation "stick" is to fail the build whenever anyone violates it, and of course that requires that this PR be integrated first. If/when that happens I plan to do one final "mop up" round that should finally stick :)

Here is the list of earlier clean up PR's:

• 8346953: Remove unnecessary @SuppressWarnings annotations (client, #2) #22906
• 8343486: Remove unnecessary @SuppressWarnings annotations and -Xlint:-foo options #21859
• 8343484: Remove unnecessary @SuppressWarnings annotations (nio) #21858
• 8343483: Remove unnecessary @SuppressWarnings annotations (serviceability) #21857
• 8343482: Remove unnecessary @SuppressWarnings annotations (net) #21856
• 8343481: Remove unnecessary @SuppressWarnings annotations (kulla) #21855
• 8343480: Remove unnecessary @SuppressWarnings annotations (javadoc) #21854
• 8343479: Remove unnecessary @SuppressWarnings annotations (hotspot) #21853
• 8343478: Remove unnecessary @SuppressWarnings annotations (core-libs) #21852
• 8343477: Remove unnecessary @SuppressWarnings annotations (compiler) #21851
• 8343476: Remove unnecessary @SuppressWarnings annotations (client) #21850
• 8343467: Remove unnecessary @SuppressWarnings annotations (security) #21844

[jddarcy]

Hi @archiecobbs , are there bugs filed to clean the causes of the warnings?

Hi @jddarcy,

At the beginning of this project as a preliminary step I filed a bunch of bugs+PR's to remove unnecessary @SuppressWarnings annotations. These were accepted, but they affected every area of the JDK and people kept adding new ones so it was impractical to keep up. The only way to make this cleanup operation "stick" is to fail the build whenever anyone violates it, and of course that requires that this PR be integrated first. If/when that happens I plan to do one final "mop up" round that should finally stick :)

Here is the list of earlier clean up PR's:

* [8346953: Remove unnecessary @SuppressWarnings annotations (client, #2) #22906](https://github.com/openjdk/jdk/pull/22906)

* [8343486: Remove unnecessary @SuppressWarnings annotations and -Xlint:-foo options #21859](https://github.com/openjdk/jdk/pull/21859)

* [8343484: Remove unnecessary @SuppressWarnings annotations (nio) #21858](https://github.com/openjdk/jdk/pull/21858)

* [8343483: Remove unnecessary @SuppressWarnings annotations (serviceability) #21857](https://github.com/openjdk/jdk/pull/21857)

* [8343482: Remove unnecessary @SuppressWarnings annotations (net) #21856](https://github.com/openjdk/jdk/pull/21856)

* [8343481: Remove unnecessary @SuppressWarnings annotations (kulla) #21855](https://github.com/openjdk/jdk/pull/21855)

* [8343480: Remove unnecessary @SuppressWarnings annotations (javadoc) #21854](https://github.com/openjdk/jdk/pull/21854)

* [8343479: Remove unnecessary @SuppressWarnings annotations (hotspot) #21853](https://github.com/openjdk/jdk/pull/21853)

* [8343478: Remove unnecessary @SuppressWarnings annotations (core-libs) #21852](https://github.com/openjdk/jdk/pull/21852)

* [8343477: Remove unnecessary @SuppressWarnings annotations (compiler) #21851](https://github.com/openjdk/jdk/pull/21851)

* [8343476: Remove unnecessary @SuppressWarnings annotations (client) #21850](https://github.com/openjdk/jdk/pull/21850)

* [8343467: Remove unnecessary @SuppressWarnings annotations (security) #21844](https://github.com/openjdk/jdk/pull/21844)

Sounds good; I linked the umbrella issue for the earlier round of cleanup to this issue in JBS.

My interest is getting back to at least a subset of modules, including java.base, compiling under "-Xlint:all -Werror" and it sounds like that is the plan as follow-up work after this PR goes in; thanks.

[magicus]

Build changes look fine.

[archiecobbs]

Bumping up the reviewer count to ensure someone from the compiler group approves.

/reviewers 3

[openjdk]

@archiecobbs
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

[pavelrappo]

Hi Archie. I've just seen this PR. It looks nice.

While I've read the CSR, I haven't read any compiler-dev thread(s). At least not carefully. I also haven't seen the diff.

IIUC, this annotation can be applied to itself: @SuppressWarnings("suppression"). Has the scope of @SuppressWarnings always included the annotation itself, or it's something that you had to tweak in this PR?

[archiecobbs]

While I've read the CSR, I haven't read any compiler-dev thread(s). At least not carefully. I also haven't seen the diff.

FWIW there is a discussion thread (starting here), but it's been on/off since November 2024.

IIUC, this annotation can be applied to itself: @SupressWarnings("suppression"). Has the scope of @SupressWarnings always included the annotation itself, or it's something that you had to tweak in this PR?

Astute question :) In general, it's completely up to the particular warning. The scope can even extend before the annotation; this is the case with "dangling-doc-comments" (see #24600). But normally the "scope" of a declaration includes any annotations on that declaration, simply because the start position of the declaration includes them (the annotations are part of the declaration, not a separate prior thing).

Regarding exactly how "it's completely up to the particular warning": Since #26138, the DiagnosticPosition class now has a getLintPosition() property. This allows the caller to specify any arbitrary source code position at which to define which @SuppressWarnings annotations apply to the warning. Search for withLintPosition() to see a couple of uses of it.

[pavelrappo]

While I've read the CSR, I haven't read any compiler-dev thread(s). At least not carefully. I also haven't seen the diff.

FWIW there is a discussion thread (starting here), but it's been on/off since November 2024.

IIUC, this annotation can be applied to itself: @SupressWarnings("suppression"). Has the scope of @SupressWarnings always included the annotation itself, or it's something that you had to tweak in this PR?

Astute question :) In general, it's completely up to the particular warning. The scope can even extend before the annotation; this is the case with "dangling-doc-comments" (see #24600). But normally the "scope" of a declaration includes any annotations on that declaration, simply because the start position of the declaration includes them (the annotations are part of the declaration, not a separate prior thing).

Regarding exactly how "it's completely up to the particular warning": Since #26138, the DiagnosticPosition class now has a getLintPosition() property. This allows the caller to specify any arbitrary source code position at which to define which @SuppressWarnings annotations apply to the warning. Search for withLintPosition() to see a couple of uses of it.

The reason I ask is that I can probably see how it could be useful to emit a "suppression" warning from this:

@SuppressWarnings("suppression")
public class A { }

I'm not suggesting the PR should implement it, I'm just thinking aloud.

I realise that if it were to implement the annotation as non-self-referring, then there would need to be a way to suppress it too, however silly it may sound.

IIRC there's no way to apply a meta annotation on a call site. Even if @SuppressWarnings was a @Repeatable annotation, this would not have the desired effect:

@SuppressWarnings("suppression")
@SuppressWarnings("suppression")
public void foo(String bar) { ...

Another option is to go one scope higher and put an annotation there:

@SuppressWarnings("suppression")
public class FooBar { ...

    @SuppressWarnings("suppression")
    public void foo(String bar) { ...

The problem is that you may not have a higher scope. Which sounds like a rare, uninteresting corner case.

[archiecobbs]

The reason I ask is that I can probably see how it could be useful to emit a "suppression" warning from this:

It's a reasonable option to consider. I think the main worry is that it could complicate build situations where the same code was being compiled with -Werror under two different compiler versions. Suppose you had code like this:

method m() {
   // warning here
}

that generated a "foo" warning in JDK N but not JDK M. So you do this:

@SuppressWarnings("foo")
method m() {
  // warning here
}

OK, now you get a "suppression" warning in JDK M but not JDK N. So next you do this:

@SuppressWarnings({ "foo", "suppression" })
method m() {
  // warning here
}

Oops! Now you get a "suppression" warning in JDK N.

Then the only way out would be -Xlint:-suppression, which means the feature has caused a wild goose chase and is being somewhat self-defeating.

[pavelrappo]

Oops! Now you get a "suppression" warning in JDK N.

Thanks for this explanation. I can now see what you mean in the Different Compiler Versions section of the CSR. That's some quality reasoning and future-proofing.

I suppose, it would unnecessarily complicate the design if we stipulate that @SuppressWarnings("suppression") should only emit a warning if there are no @SuppressWarnings in any of the child scopes. Note, not just ineffective @SuppressWarningss, but none at all.

[archiecobbs]

Thanks for this explanation. I can now see what you mean in the Different Compiler Versions section of the CSR. That's some quality reasoning and future-proofing.

Credit goes to @jddarcy for pinpointing those possible traps.

I suppose, it would unnecessarily complicate the design if we stipulate that @SuppressWarnings("suppression") should only emit a warning if there are no @SuppressWarnings in any of the child scopes. Note, not just ineffective @SuppressWarningss, but none at all.

Here's an analogy that helps me justify the current design, fwiw. Have you ever been to New York or another city and walked by an otherwise completely blank wall that has "POST NO BILLS" signs stuck to it, and then thought about how ironic it is that posting bills is required in order to notify people not to post bills?

Sticking @SuppressWarnings("suppression") on a declaration is analogous. We're posting a notification not to do something, even though the notification itself risks doing it. But that's OK and we make a special exception for it.

If we didn't make such exceptions, then when you walked by such a wall you'd see a workman repeatedly posting and unposting the "POST NO BILLS" bills :)

[pavelrappo]

Thanks for this explanation. I can now see what you mean in the Different Compiler Versions section of the CSR. That's some quality reasoning and future-proofing.

Credit goes to @jddarcy for pinpointing those possible traps.

I suppose, it would unnecessarily complicate the design if we stipulate that @SuppressWarnings("suppression") should only emit a warning if there are no @SuppressWarnings in any of the child scopes. Note, not just ineffective @SuppressWarningss, but none at all.

Here's an analogy that helps me justify the current design, fwiw. Have you ever been to New York or another city and walked by an otherwise completely blank wall that has "POST NO BILLS" signs stuck to it, and then thought about how ironic it is that posting bills is required in order to notify people not to post bills?

Sticking @SuppressWarnings("suppression") on a declaration is analogous. We're posting a notification not to do something, even though the notification itself risks doing it. But that's OK and we make a special exception for it.

If we didn't make such exceptions, then when you walked by such a wall you'd see a workman repeatedly posting and unposting the "POST NO BILLS" bills :)

That analogy is amusing, but it either barely applies here, or I'm too tired to see how it does.

In my most recent message, I mused about this: what if @SuppressWarnings("suppression") were to emit a warning only if there's no @SuppressWarnings of any kind anywhere under it? This behaviour is minimally stricter than that of this PR, and is compatible with the M-N-compiler use case.

Basically, if @SuppressWarnings("suppression") was accidentally left with no suppressions under it, the compiler would warn you, so you could remove it. Because what's the reason for @SuppressWarnings("suppression") whose scope contains no suppressions?

Just to clarify, here's an example of what I mean:

@SuppressWarnings("suppression")
public void m() {
    /* a method that contains no @SuppressWarnings
       annotations whatsoever */
}

Here's an example of what I don't mean:

@SuppressWarnings("suppression")
public void m() {
    /* a method that only contains @SuppressWarnings
       annotations that are ineffective. That is,
       annotations, that if removed, would __not__
       break compilation with -Werror */
}

The idea here is to minimise the chance of @SuppressWarnings("suppression") becoming part of the furniture and hiding unneeded future suppressions. But maybe changing the behaviour as described above would introduce more complexity and future issues than it would catch potential bugs.

[archiecobbs]

Basically, if @SuppressWarnings("suppression") was accidentally left with no suppressions under it, the compiler would warn you, so you could remove it. Because what's the reason for @SuppressWarnings("suppression") whose scope contains no suppressions?

Sorry I didn't address this question before - but yes, this is a reasonable ida. It would basically be adding an exception to the exception. So, a bit of additional complexity but for a worthwhile cause. To stretch the analogy, it would be like making the observation: "If the area of the wall is zero, then you never need to post a "POST NO BILLS" bill on it".

I can think of one possible wrinkle though: warnings can actually be suppressed in the source code in two ways, by @SuppressWarnings and by @Deprecated (see Lint.suppressionsFrom(Symbol)). So you'd also have to look for @Deprecated; but this also brings up the unsettling possibility that in the future, some new annotation might have a similar side-effect, and then we'd be in trouble again. So there is an aspect of this idea that requires us to make certain predictions about the future, which of course is dangerous. I think it's safer to "just say no".

[pavelrappo]

I can think of one possible wrinkle though: warnings can actually be suppressed in the source code in two ways, by @SuppressWarnings and by @Deprecated (see Lint.suppressionsFrom(Symbol)). So you'd also have to look for @Deprecated; but this also brings up the unsettling possibility that in the future, some new annotation might have a similar side-effect, and then we'd be in trouble again. So there is an aspect of this idea that requires us to make certain predictions about the future, which of course is dangerous. I think it's safer to "just say no".

@Deprecated as well as a conceivable future tag whose secondary effect is to silence possible warnings are very different from @SuppressWarnings. While I don't see why we might need to consider those underneath @SuppressWarnings("suppression") to decide if it should emit a warning, it's okay to say that it's safer not to complicate things. Thanks.

[archiecobbs]

@erikj79, @magicus - Would you mind updating your review since the recent changes that addressed merge conflicts?

Thanks!

[erikj79]

Build changes still look good.

[bridgekeeper]

@archiecobbs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[archiecobbs]

/touch

[openjdk]

@archiecobbs The pull request is being re-evaluated and the inactivity timeout has been reset.