Improve a PMD usage

[slawekjaranowski]

New feature, improvement proposal

Currently:

• we have only site report with PMD
• we use default rules

To have a benefit of it:

• we should define a rules which interes us
• we should add it do standard build to brake / warn early

Or simply drop a report if we don't want to work with it ....

[slawekjaranowski]

#254

[Pankraz76]

what about automating this and using errorprone.refasterrules? They offer quite the same, even fixing the violations unlike PMD, making the integration smooth.

• Fix #5953: [java] PoC: Add rewrite support for errorprone.refasterrules pmd/pmd#5956
• Issue #17487: Add rewrite support for errorprone.refasterrules checkstyle/checkstyle#17490
• Proposal: Add rewrite support for errorprone.refasterrules maven#11010
• Proposal: Add rewrite support for errorprone.refasterrules #491
• PoC: Add rewrite support for errorprone.refasterrules kafka#20219
• PoC: Add rewrite support for errorprone.refasterrules diffplug/spotless#2576

[Pankraz76]

is the ConstantNaming rule something we like to integrate?

• [prone] Add ConstantNaming diffplug/spotless#2687
• [prone] Add error-prone.picnic.tech featuring RedundantStringConversion diffplug/spotless#2664
• Add error-prone.picnic.tech featuring RedundantStringConversion junit-team/junit-framework#5006

[gnodet]

is the ConstantNaming rule something we like to integrate?

• Add ConstantNaming diffplug/spotless#2687
• Add RedundantStringConversion diffplug/spotless#2664
• Add error-prone.picnic.tech featuring RedundantStringConversion junit-team/junit-framework#5006

We already check that one with checkstyle, so no need to duplicate.

[gnodet]

Looking at https://maven.apache.org/ref/3.9.11/maven-core/pmd.html, I can see that UnusedFormalParameter and UnusedLocalVariable are plain wrong imho. We end up with tons of warnings which we don't care about. I'm not sure any one in these should actually be fixed.

I think we should avoid any tool telling us that we could rewrite the same thing in a different way and should focus on tools (or rules) that raise potential errors and bugs. I think PMD has categories, so we should just enable the ones that makes sense.

But even the ReturnFromFinallyBlock in the above report is slightly wrong, as it reports a return from a catch clause, not a finally clause. It also seems that UnnecessaryFullyQualifiedName cannot deal with import conflicts, so it's useless.

Not sure it actually worth it...

[Pankraz76]

UnusedLocalVariable are plain wrong imho

convention tells them to be named accordingly like unused or ignored just like error-prone does.

• https://maven.apache.org/ref/3.9.11/maven-core/xref/org/apache/maven/lifecycle/internal/MojoExecutor.html#L313
  • lock is not used and not named conventionally as unused to avoid questions, so the analysis seems to be correct.
• https://errorprone.info/bugpattern/UnusedVariable

False positives on fields and parameters can be suppressed by prefixing the variable name with unused, e.g.:

checkstyle

check has no type awareness so RedundantStringConversion for sure violating.

[gnodet]

UnusedLocalVariable are plain wrong imho

convention tells them to be named accordingly like unused or ignored just like error-prone does.

• https://maven.apache.org/ref/3.9.11/maven-core/xref/org/apache/maven/lifecycle/internal/MojoExecutor.html#L313

  • lock is not used and not named conventionally as unused to avoid questions, so the analysis seems to be correct.

• https://errorprone.info/bugpattern/UnusedVariable

In the first case, if the tool was able to see that this should not apply on override methods, that could eventually be useful, but having to rename variables to unused to please the tool is wrong imho.

On the lock, that variable is used, we need it because it's an auto-closable and calling it unused would be wrong imho. That rule predates the enhanced try/catch block and I think it should be avoided now.

False positives on fields and parameters can be suppressed by prefixing the variable name with unused, e.g.:

checkstyle

check has no type awareness so RedundantStringConversion for sure violating.

I'm not saying we should not try to improve / fix our code (when there's actually something to improve). But maybe PMD is too old and there are better alternatives. What about SpotBugs ?

[Pankraz76]

and calling it unused would be wrong imho.

Yes, also allowed to be called ignored then.

In this case — actually, also in general, imho — that might be a better approach because it’s always intentional when done so.

I’m not saying we shouldn’t try to improve or fix our code (when there’s actually something to improve). But maybe PMD is too old and there are better alternatives. What about SpotBugs?

Old but gold, just like this tool.

Both have their reasons to be used. SpotBugs, imho, is even “golder,” but it’s working fine — that’s the point. Coming from PMD, I can see the Prone community is much more active due to Google and the Picnic fork.

Check is all all about code quality, of course, using all of them right now including Rewrite. There’s only one tool missing, which is Spotless — then Checkstyle is having every possible tool I even know of ^^.

I can confirm PMD sometimes passes, but then SpotBugs really finds something. It always feels like there’s something really wrong, as it has to pass PMD’s error rules — which are good defaults for a reason, just like in Prone’s case.

Long story short, I’d suggest going with Prone for finding some stuff. It’s not doing any harm like @elharo would see the rewriting or called patching in this context. Its just analysis, like we’re used to.

[gnodet]

and calling it unused would be wrong imho.

Yes, also allowed to be called ignored then.

In this case — actually, also in general, imho — that might be a better approach because it’s always intentional when done so.

I’m not saying we shouldn’t try to improve or fix our code (when there’s actually something to improve). But maybe PMD is too old and there are better alternatives. What about SpotBugs?

Old but gold, just like this tool.

Both have their reasons to be used. SpotBugs, imho, is even “golder,” but it’s working fine — that’s the point. Coming from PMD, I can see the Prone community is much more active due to Google and the Picnic fork.

Check is all all about code quality, of course, using all of them right now including Rewrite. There’s only one tool missing, which is Spotless — then Checkstyle is having every possible tool I even know of ^^.

I can confirm PMD sometimes passes, but then SpotBugs really finds something. It always feels like there’s something really wrong, as it has to pass PMD’s error rules — which are good defaults for a reason, just like in Prone’s case.

Long story short, I’d suggest going with Prone for finding some stuff. It’s not doing any harm like @elharo would see the rewriting or called patching in this context. Its just analysis, like we’re used to.

Have you tried error prone on the maven codebase ? do you have a report that you can share ?

[Pankraz76]

No, but I will do so. Also, as the tool covers the same topic, I can say we have the need for detecting unused methods and ensuring override annotations — which can actually be done with PMD as well.

• https://errorprone.info/bugpattern/UnusedMethod
• Remove unused private methods maven#11165
• [S1161] chore: Add missing @Override to overriding and implementing methods maven#2446

Also, Error Prone is the door opener for using Refaster rules. Having done this already with another tool applying the same concept, we can see there is (or was) a need to have this rule in place:

• Apply StreamRulesRecipes maven#11159

It’s like every other codebase — even when there are no issues, it’s still worth adding them to help maintain compliance and avoid potential problems in the future.

Just like we’ve added many rewrite rules to ensure they pass in the future with Spotless:

• [openrewrite] RecipeNullabilityBestPractices diffplug/spotless#2663

Would suggest some simple recipe to start with:

• Add ConstantNaming keycloak/keycloak#43457