MINOR: Add RemoveUnusedPrivateMethods #21165 #21168 #21149

[Pankraz76]

MINOR: Add RemoveUnusedPrivateMethods #21165 #21168 #21149

• https://docs.openrewrite.org/recipes/staticanalysis/removeunusedprivatemethods

According to:

• refine get counter performance #21149

[Pankraz76]

kindly asking, if this is any good?

@AndrewJSchofield

[AndrewJSchofield]

I do not have any experience of this. My view is that a PR which makes changes to the build should contain just those changes and you would need to convince a committer familiar with the build process that it was a worthwhile change.

[Pankraz76]

Yes, that's also a simple way to merge: activating the recipes after the merge.

Still, it's hard for me to believe that this will be merged without knowing upfront what it's doing and what the benefits are. This is the same task all the SCA tools have, so there are many ways to integrate. Of course, a small and simple approach is in my favor as well.

[Pankraz76]

also this could be handled next as also encountered in:

• MINOR: Add SanityCheck #21165 [S3599]  #21168

@mjsax

[mjsax]

No idea what "Moderne" is. Do we want to depend on it? \cc @ijuma @chia7712

In general, I am a big supported to add checks and fail the build if we detect issue. but "automated code transformations" sounds not desirable to me personally

[Pankraz76]

Actually, it's not fully automated—it just fails the build when it detects problematic code, which is exactly what everyone wants to see.

Just like Error Prone offers patching, Rewrite does too. That's a good thing, because it saves time on trivial fixes that the tool can handle automatically.

Still, it's up to each developer to apply and verify the changes themselves. It's simply an optional tool (run), not a requirement.

As long as the code is complain in the end, Rewrite is satisfied. How you get there is up to each individual.

sounds not desirable to me personally

Also im wondering about this argument in the presents of spotless. This tool is all about automating stuff and patching. So its actually nothing new, just covers the most issues as well, but can got beyond as well, making it some consideration.

[Pankraz76]

adjusted to RemoveUnusedPrivateMethods. Here the value should be very simple to comprehend and rely on. Fortunately there is only 1 finding making it the ideal intro.

[squah-confluent]

#21168's checks already exist as checkstyle's AvoidDoubleBraceInitialization and spotbugs' UPM_UNCALLED_PRIVATE_METHOD. It looks like your goal here is to introduce openrewrite as a whole rather than add any particular check.

The project already runs checkstyle for linting and spotbugs for static analysis. I do not think it makes sense to add a second linting and static analysis pass because of the overlap in functionality. If the proposal is to replace checkstyle and spotbugs with openrewrite, we should discuss that.

[chia7712]

If the proposal is to replace checkstyle and spotbugs with openrewrite, we should discuss that.

Spot on. +1.

[Pankraz76]

If the proposal is to replace checkstyle and spotbugs with openrewrite, we should discuss that.

No, that's not the goal. All the tools have their individual benefit. Even on one and the same issue like this, having a layered stack of Checkstyle, Spotless, SpotBugs, PMD, and Error Prone, Rewrite still finds some methods that all other tools don't. (That's actually the Checkstyle stack, so this is a real-world example.)

So it's about having redundancy in critical infrastructure.

BTW: Thats something Germany seems not been capable of, like recently seen.
Maybe this could be valuable example and lesson for others, to do better.

Also, each tool has its dedicated features, like Java migration as an example:

• Fix skipMavenParsing to Enable Java 21 Migration checkstyle/checkstyle#18531

It's not about talking good or bad about any other tool. It's just about getting the job done on the unused method stuff, then considering that—beyond recipes—each offers individual benefit again.

So the discussion should be simple in this regard, as it's "only" a new tool solving an already applied convention.

[Pankraz76]

We can learn from Spotless that it seems acceptable to have this tool, as they asked to put an accidentally deleted rule back in.

• [rewrite] apply JavaLangAPIs #2804 diffplug/spotless#2810

From Checkstyle, we can learn that redundancy alone may not be enough — over time, surprising issues can arise, as usually expected.
For instance, a check might turn out not to be active in reality. Therefore, it’s better to have stronger safeguards in gatekeeping rather than weaker ones, to gather diverse perspectives and feedback.

---

Relying on a single point of failure — putting all your eggs in one basket — goes against common system design principles.

• Error Prone Plugin Not Applied and Build Passes Despite Errors #18470 checkstyle/checkstyle#18479
  • Pull #18589: Fix error-prone CI config #17988 #17985 #18479 checkstyle/checkstyle#18589
• PMD Tool Fails Locally but CI Doesn't Detect It #18479 checkstyle/checkstyle#18470
  • Pull #18530: Fix PMD violations #18470 checkstyle/checkstyle#18530