apache
Improve a PMD usage
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 ....
what about automating this and using errorprone.refasterrules? They offer quite the same, even fixing the violations unlike PMD, making the integration smooth.
- https://github.com/pmd/pmd/pull/5956
- https://github.com/checkstyle/checkstyle/pull/17490
- https://github.com/apache/maven/issues/11010
- https://github.com/apache/maven-parent/issues/491
- https://github.com/apache/kafka/pull/20219
- https://github.com/diffplug/spotless/pull/2576
is the ConstantNaming rule something we like to integrate?
- https://github.com/diffplug/spotless/pull/2687
- https://github.com/diffplug/spotless/pull/2664
- https://github.com/junit-team/junit-framework/pull/5006
is the ConstantNaming rule something we like to integrate?
We already check that one with checkstyle, so no need to duplicate.
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...
UnusedLocalVariableare 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
lockis not used and not named conventionally asunusedto 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.
UnusedLocalVariableare plain wrong imhoconvention 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
lockis not used and not named conventionally asunusedto 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 ?
and calling it
unusedwould 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.
and calling it
unusedwould be wrong imho.Yes, also allowed to be called
ignoredthen.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 ?
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
- https://github.com/apache/maven/pull/11165
- https://github.com/apache/maven/pull/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:
- https://github.com/apache/maven/pull/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:
- https://github.com/diffplug/spotless/pull/2663
Would suggest some simple recipe to start with:
- https://github.com/keycloak/keycloak/pull/43457
