First off, thanks! I've long wanted to tackle this, it's a massive win; as shown by the number of findings it has. Spcially as we remove FPs over time that used to be suppressed.

As for this…

We could have a ruleset in pmd-core with just this rule predefined (eg category/core/bestpractices.xml), but I don't think it's the right way, even though it would save me some typing...

I'm actually not against it… in my long list of "maybe interesting ideas" was having a Universal AST (UAST) in the form of interfaces, that languages could implement in their corresponding nodes; and some rules could be rewritten in terms of the UAST as language-agnostic rules (ie: EmptyControlStatement only needs to know what nodes are control statements, and the number of children they have). This obviously has some deep implications (ie: needing to be able to transverse the AST based on interfaces, which is unsupported in XPath rules nor on node visitors); but ideally this would allow us to define the rule once in an abstract ruleset (ie: category/uast/codestyle.xml under pmd-core with a bogus language such as uast), and then have each language, under the appropriate category, such as category/java/codestyle.xml, include a <rule ref="category/uast/codestyle.xml/EmptyCatchBlock" language="java">. This makes it 100% an implementation detail, users can reference it through category/java/codestyle.xml/EmptyCatchBlock without ever knowing this is backed by a rule living somewhere else…

Ensuring a single implementation allows us to make sure any fixes / improvements are applied across the board too.

I see no reason why we wouldn't want to take such an approach here to be honest…
We can (and should) still have per-language tests.
Changes to doc generation may be required obviously.

Thank you for the feedback.

Ensuring a single implementation allows us to make sure any fixes / improvements are applied across the board too.
I see no reason why we wouldn't want to take such an approach here to be honest…
We can (and should) still have per-language tests.
Changes to doc generation may be required obviously.

I think we are definitely going to share the implementation. But I'm not sure we should surface that to users, which is why one rule per language (which use the same rule class) is my preferred course for now. Actually for most language a default implementation that is in an internal ruleset would be enough though (with a reference as you show). I'm not sure it is possible though, as I don't think it is possible to override the language of a rule, and it is definitely not possible to define a rule that doesn't have a language for now.

One thing I want to do with this rule is also have a java-specific implementation that also covers more @SuppressWarnings things, like @SuppressWarnings("unchecked") for instance. Which is why I don't think we should surface "UAST" rules or rules that apply to any language as anything else than just several language-specific rules.

What about having a feature toggle for this? So, you run "pmd check --report-unused-suppressions" - and internally, we will add the new internal rule to the analysis execution (like this happens now in this PR). We could then later still change this to be a different kind of rule or any other element (report summarizer, whatever, multi-file analysis aware rule, #5254, ...). How we implemented this is then hidden and can be changed.

I think there are very good reasons for having this as a rule.

• For users, it is a check that adds violations to the ruleset. Those violations behave exactly like other violations (they may fail the build for instance). So for an external user, it looks like a rule. It would be surprising if it weren't a rule.

• If we want to add configuration properties, we cannot do that easily with a CLI switch. But that is directly possible with rule properties.

• Also a CLI switch needs to be surfaced in all other integrations (maven, gradle, ant, etc), whereas if it is just a part of your ruleset, the integration comes for free.

• There is overlap between this check and another rule that reports unused @SuppressWarnings in general. That's because the same @SuppressWarnings annotation is used to suppress PMD violations and other kinds of warnings (eg, unchecked compiler warnings). It would be IMO very useful to report those (eg @SuppressWarnings("unchecked") when there is no such warning), and that would of course be a regular rule. But since it reports the same constructs, and therefore needs to parse annotations in the same way, it makes sense to centralize the logic for both in a single rule. This will avoid duplicate violations for instance.

  The boundaries between an "unused pmd warning suppression" check, and an "unused compiler warning suppression" is made even more blurry by [java] Full @SuppressWarnings support #1900. Some PMD rules support a suppression string that is also used by compiler warnings, like fallthrough or unused. If we were to split the checks we would have to duplicate work, and possibly end up with a dysfunctional rule. For instance, if you have

   @SuppressWarnings("unused") void foo(int x) {}

  and you run PMD without UnusedFormalParameterRule, but with --report-unused-suppression. What is the "report unused suppression" check going to do? Should it report the annotation because there was no PMD warning? Obviously it should not, because there is in fact an unused parameter, so the annotation is also suppressing the compiler/IDE warning. A violation here would be a false positive. So there is no way around making the "report-unused-suppression" check aware of language-specific compiler/ide warnings. If you make the check language-specific, then to me, the following are direct consequences:

  • Since your check already somewhat needs to understand compiler warnings to avoid reporting falso positives, the check is also the best place to implement the "unused compiler warning suppression" rule. The logic is exactly the same.
  • The simplest implementation of the check is a rule. This is because, the simplest way to make a language-specific extension to PMD is a rule. It could be in the appropriate category for the language, with documentation about what it supports, how to configure it, etc. If it isn't a rule, we have to invent a new extension mechanism for --report-unused-suppression.

there is no defined execution order of the enabled rules, executing this rule alone doesn't work etc...

About this point, I think it isn't as bad as you think. It is appropriate for this check to not be suppressible, otherwise you end up with recursive suppression... where does it end. The only constraint on this rule is that all violations that may be suppressed are emitted before this rule's execution. This defines an easy execution order, which should be completely transparent to users anyway: all rules that may be suppressed have to executed before all rules that may not be suppressed. There is no defined execution order within either of those two categories (as of now). There may be other rules that don't support suppressions in the future, maybe for instance, rules that don't report violations, but metric values, if we want to implement that. If we do make "not suppressible" applicable to more rules, then it is likely we want a marker interface to tag rules for instance.

Thanks for the explanations!

I think there are very good reasons for having this as a rule.

• For users, it is a check that adds violations to the ruleset. Those violations behave exactly like other violations (they may fail the build for instance). So for an external user, it looks like a rule. It would be surprising if it weren't a rule.

Yes, I think you convinced me 😄 It makes sense to expose this as a rule/ruleviolations.

there is no defined execution order of the enabled rules, executing this rule alone doesn't work etc...

About this point, I think it isn't as bad as you think.

The only point here I have is: this is another special behavior for one special rule, that users need to be aware of - it differs from other rules. But true, it is logical and easy to understand, that this "report unnecessary suppression rule" needs to run at the end (after all other rules).

I'm ok to start with that. For now we don't need to expose this feature to other rules (for now "UnnecessaryPmdSuppressionRule" is the only rule with that special behavior). The generalization of this behavior would be part of #5254 .

FYI - I'm moving this PR to 7.14.0 now... and doing the release of 7.13.0.

Documentation Preview

Compared to main:
This changeset changes 0 violations,
introduces 109 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.

Regression Tester Report

(comment created at 2025-05-30 16:04:12+00:00 for d24775b)

I just wonder if the new violations are not mostly FPs... They are related to igniring with "unused". I had to build a check that tries to find any unused thing, but for instance I assumed that public methods cannot be reported as unused (although, that depends on your ide settings I guess), and so some public methods are reported here. Is this acceptable? Since violations of this rule cannot be suppressed we need to not have any FPs ^^

I just wonder if the new violations are not mostly FPs... They are related to igniring with "unused". I had to build a check that tries to find any unused thing, but for instance I assumed that public methods cannot be reported as unused (although, that depends on your ide settings I guess), and so some public methods are reported here. Is this acceptable? Since violations of this rule cannot be suppressed we need to not have any FPs ^^

I think, this now as good as it gets now. We have a lot violations about unused suppressions on private static inner classes - which look on first glance legit. I think, this all can be improved later on - after all: we explicitly say, this is experimental. And this rule is not enabled by default.