Skip to content

[POC] rewrite-maven-plugin: Introduce OpenRewrite by Moderne #2322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

pom.xml Outdated
<!-- https://github.com/apache/maven/pull/2317 -->
<!-- <recipe>org.openrewrite.staticanalysis.RemoveUnusedPrivateFields</recipe> -->
<!-- https://github.com/apache/maven/pull/2310 -->
<recipe>org.openrewrite.staticanalysis.RemoveUnusedPrivateMethods</recipe>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@Pankraz76 Pankraz76 force-pushed the editorconfig-maven-plugin-rewrite branch 2 times, most recently from 9101253 to d70cd35 Compare May 11, 2025 20:57
pom.xml Outdated
<execution>
<id>rewrite-maven-plugin</id>
<goals>
<goal>run</goal>
Copy link
Contributor Author

@Pankraz76 Pankraz76 May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we need new goal @timtebeek failOnDryRunDetection - then we complete request @gnodet.

which fails the build if a rule cannot be validated

Suggested change
<goal>run</goal>
<goal>failOnDryRunDetection</goal>

@Pankraz76 Pankraz76 marked this pull request as ready for review May 11, 2025 21:04
pom.xml Outdated
<!-- <recipe>org.openrewrite.java.testing.junit5.JUnit5BestPractices</recipe> -->
<!-- <recipe>org.openrewrite.staticanalysis.CodeCleanup</recipe> -->
<!-- <recipe>org.openrewrite.staticanalysis.CommonStaticAnalysis</recipe> -->
<!-- <recipe>org.openrewrite.staticanalysis.RemoveUnusedLocalVariables</recipe> -->
Copy link
Contributor Author

@Pankraz76 Pankraz76 May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pankraz76 Pankraz76 changed the title [POC] rewrite-maven-plugin: Remove unused private methods [POC] rewrite-maven-plugin: Introduce OpenRewrite by Moderne (RemoveUnusedPrivateMethods) May 12, 2025
@Pankraz76 Pankraz76 force-pushed the editorconfig-maven-plugin-rewrite branch from d70cd35 to 26152a3 Compare May 12, 2025 07:56
pom.xml Outdated
<exportDatatables>true</exportDatatables>
<failOnDryRunResults>true</failOnDryRunResults>
<exclusions>
<!-- wait for suppression: RemoveUnusedPrivateMethods -->
Copy link
Contributor Author

@Pankraz76 Pankraz76 May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need suppression to be exact not broad. @timtebeek

LOC and class option

Copy link
Contributor Author

@Pankraz76 Pankraz76 May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its not working either:

[WARNING] The recipe produced 17 warning(s). Please report this to the recipe author.
[WARNING] These recipes would make changes to src/mdo/java/WrapperProperties.java:
[WARNING] org.openrewrite.staticanalysis.RemoveUnusedPrivateMethods
[WARNING] These recipes would make changes to src/mdo/java/WrapperList.java:
[WARNING] org.openrewrite.staticanalysis.RemoveUnusedPrivateMethods

<plugin>
<groupId>org.openrewrite.maven</groupId>
<artifactId>rewrite-maven-plugin</artifactId>
</plugin>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its only active on clean install when here, why so?

@Pankraz76 Pankraz76 force-pushed the editorconfig-maven-plugin-rewrite branch from 3ee2ff6 to 32a08dd Compare May 12, 2025 08:36
@timtebeek
Copy link
Contributor

timtebeek commented May 12, 2025

As reported on the other PR as well I wouldn't fail the CI if we can post fix suggestions instead.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't want to introduce additional plugins into the build.

  2. Immutability is a good idea when:
    A. The API clearly indicates that promise.
    B. You're not changing existing public API.

Neither of those is true here

@Pankraz76
Copy link
Contributor Author

2. You're not changing existing public API.

yes,

  • I don't want to introduce additional plugins into the build.

spot is limited to format only, while rewrite kicks in where others sign off.

@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 12, 2025

  • I don't want to introduce additional plugins into the build.

no body want but only computer can Command & Conquer. As we out of nature not being consistent.

comparing clean code vs runtime its an easy tradeoff as motivation is just the same like spot and check.

OpenRewrite is ATM king and the one to rule them all.

@gnodet
Copy link
Contributor

gnodet commented May 12, 2025

As an alternative, could open rewrite be setup to comment on PRs ?

@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 12, 2025

As an alternative, could open rewrite be setup to comment on PRs ?

of course, this is aimed happy path for good integration and acceptance.

Its easy to apply then, but will spam and take away, as friction we aim to automate will still happen.
Mostly to dedicated dev and pr which is fine.

But this violates fail early fail often. therefore we need failOnDryRunResults local failure and online mode. If local build is skipped then still it will be applied.

This will save community resources shifting from online to prior local run giving dev chance to learn and gain early feedback.

As happy path is clean install verify will trigger failOnDryRunResults, tempting dev to run local fixup same like with spot, but way more mighty.

openrewrite/rewrite-static-analysis#544 (comment)

image

@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 12, 2025

  • I don't want to introduce additional plugins into the build.

Format is limited to cosmetics. Check is just file parser too, therefore very limited.

Ideally, we would superset what Checkstyle relies on using:

It's interesting to see that each tool has its niche:

  • PMD often passes
  • SpotBugs really taps in if something is sus

Checkstyle PMD Reference

Trading just a few seconds for higher code standards is generally an effort taken.

Current real world problem: Missing auto-fix effectively preventing integration.

This is where Rewrite kicks in. So we need Rewrite as enabler.


Ideally org.openrewrite.staticanalysis.CodeCleanup would sooner or later:

  • Fix all violations covered by other tools
  • Make them kind of obsolete in comparison
  • Yet keep them very valuable each on their own

Even when having the option, running them would make sense to really make sure. Once (and for all) fixed by Rewrite CodeCleanup, technical debt is considered non-existent anymore.

SpotBugs/PMD checks for more than 400 bug patterns.

While Rewrite aims to:

  • Cover all existing tools
  • Automate them (its dedicated super power)

Trading CPU time for maintenance and bugs avoidance seems justiciable.

[INFO] --- pmd:3.23.0:check (default) @ maven-api-di ---
[INFO] PMD version: 7.0.0
[INFO] PMD Failure: org.apache.maven.api.di.Inject:25 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.CONSTRUCTOR'.
[INFO] PMD Failure: org.apache.maven.api.di.Inject:26 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.FIELD'.
[INFO] PMD Failure: org.apache.maven.api.di.Inject:27 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.METHOD'.
[INFO] PMD Failure: org.apache.maven.api.di.Inject:28 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.MojoExecutionScoped:25 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.METHOD'.
[INFO] PMD Failure: org.apache.maven.api.di.MojoExecutionScoped:26 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.TYPE'.
[INFO] PMD Failure: org.apache.maven.api.di.MojoExecutionScoped:27 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.Named:24 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.Priority:25 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.METHOD'.
[INFO] PMD Failure: org.apache.maven.api.di.Priority:26 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.TYPE'.
[INFO] PMD Failure: org.apache.maven.api.di.Priority:27 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.Provides:25 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.METHOD'.
[INFO] PMD Failure: org.apache.maven.api.di.Provides:26 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.Qualifier:25 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.ANNOTATION_TYPE'.
[INFO] PMD Failure: org.apache.maven.api.di.Qualifier:26 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.Scope:25 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.ANNOTATION_TYPE'.
[INFO] PMD Failure: org.apache.maven.api.di.Scope:26 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.SessionScoped:25 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.METHOD'.
[INFO] PMD Failure: org.apache.maven.api.di.SessionScoped:26 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.TYPE'.
[INFO] PMD Failure: org.apache.maven.api.di.SessionScoped:27 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.Singleton:24 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.
[INFO] PMD Failure: org.apache.maven.api.di.Typed:25 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.FIELD'.
[INFO] PMD Failure: org.apache.maven.api.di.Typed:26 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.METHOD'.
[INFO] PMD Failure: org.apache.maven.api.di.Typed:27 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.ElementType.TYPE'.
[INFO] PMD Failure: org.apache.maven.api.di.Typed:28 Rule:UnnecessaryImport Priority:4 Unused static import 'java.lang.annotation.RetentionPolicy.RUNTIME'.

@Pankraz76
Copy link
Contributor Author

alternative

yes. If integrated it should live in cloud to save ci time. On other hands it needs fix on local setup so lets think about this. Rewrite will be fast, once applied.

pom.xml Outdated
<configuration>
<activeRecipes>
<recipe>org.openrewrite.java.format.AutoFormat</recipe>
<recipe>org.openrewrite.java.recipes.JavaRecipeBestPractices</recipe>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<recipe>org.openrewrite.java.recipes.JavaRecipeBestPractices</recipe>
<recipe>org.openrewrite.staticanalysis.ReplaceCollectToListWithToList </recipe>

@Pankraz76 Pankraz76 force-pushed the editorconfig-maven-plugin-rewrite branch from 0f8c205 to d99935a Compare May 15, 2025 20:15
@Pankraz76 Pankraz76 requested a review from gnodet May 15, 2025 20:15
@@ -39,8 +39,8 @@

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileWriter;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security´s everybody favourite.
This might be a good example to integrate and then build up from there avoiding further friction. Giving more freedom and attention to review.

[WARNING] The recipe produced 16 warning(s). Please report this to the recipe author.
[WARNING] Changes have been made to its/core-it-suite/src/test/resources/mng-0612/plugin/src/main/java/org/apache/maven/its/it0125/DependenciesMojo.java by:
[WARNING]     org.openrewrite.java.security.JavaSecurityBestPractices
[WARNING]         org.openrewrite.staticanalysis.BufferedWriterCreationRecipes
[WARNING]             org.openrewrite.staticanalysis.BufferedWriterCreationRecipes$BufferedWriterFromNewFileWriterWithFileArgumentRecipe
[WARNING] Changes have been made to compat/maven-model-builder/src/test/java/org/apache/maven/model/building/FileModelSourceTest.java by:
[WARNING]     org.openrewrite.java.security.JavaSecurityBestPractices
[WARNING]         org.openrewrite.java.security.SecureTempFileCreation
[WARNING] Changes have been made to impl/maven-core/src/test/java/org/apache/maven/graph/ProjectSelectorTest.java by:
[WARNING]     org.openrewrite.java.security.JavaSecurityBestPractices
[WARNING]         org.openrewrite.java.security.SecureTempFileCreation
[WARNING] Changes have been made to compat/maven-compat/src/main/java/org/apache/maven/repository/legacy/DefaultWagonManager.java by:
[WARNING]     org.openrewrite.java.security.JavaSecurityBestPractices
[WARNING]         org.openrewrite.java.security.SecureTempFileCreation
[WARNING] Changes have been made to compat/maven-compat/src/test/java/org/apache/maven/artifact/testutils/TestFileManager.java by:
[WARNING]     org.openrewrite.java.security.JavaSecurityBestPractices
[WARNING]         org.openrewrite.java.security.SecureTempFileCreation
[WARNING] Please review and commit the results.
[WARNING] Estimate time saved: 9m

@gnodet
Copy link
Contributor

gnodet commented May 16, 2025

@Pankraz76 if you want to discuss such changes, you should do so on the dev mailing list rather than raising PRs. Such important changes should be discussed openly with the whole community.

Also, you should define 2 profiles, triggered by a property. Take a look at how spotless is set up.

@gnodet gnodet marked this pull request as draft May 16, 2025 05:49
pom.xml Outdated
<!-- <recipe>org.openrewrite.staticanalysis.RemoveUnusedPrivateMethods</recipe> -->
<!-- <recipe>org.openrewrite.staticanalysis.UnnecessaryThrows</recipe> -->
<!-- <recipe>org.openrewrite.java.ShortenFullyQualifiedTypeReferences</recipe> -->
<recipe>org.openrewrite.java.RemoveUnusedImports</recipe>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one is unneeded, it's covered by spotless already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, nice yes. Then remove.

@Pankraz76
Copy link
Contributor Author

PMD or rewrite, might even do both.

Spot seems ending up being short:

@Pankraz76 Pankraz76 force-pushed the editorconfig-maven-plugin-rewrite branch from d99935a to 606471e Compare May 16, 2025 08:06
@Pankraz76
Copy link
Contributor Author

@Pankraz76 if you want to discuss such changes, you should do so on the dev mailing list rather than raising PRs. Such important changes should be discussed openly with the whole community.

Also, you should define 2 profiles, triggered by a property. Take a look at how spotless is set up.

yes, of course. This is just demo to fullfill request.

As he have clear leckage this should actually be in every body´s favor.

@Pankraz76
Copy link
Contributor Author

openly

just writing email from my acc to [email protected] is enough right?

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be aware and take care about the license of openwrite and single rules.

https://docs.openrewrite.org/reference/latest-versions-of-every-openrewrite-module

@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 17, 2025

@Pankraz76 Pankraz76 closed this May 17, 2025
@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 17, 2025

then checkstyle-stack using PMD/spotbugs seems only option - long live the king:

Might use rewrite to fix legacy, by using old version. This should be legal as in past was open source. With new licence this wont be done of course.

Is this any good @timtebeek ?

@timtebeek
Copy link
Contributor

I've spoken at length with the author linked above at Code Remix Summit last week, helped him understand and analyze the changes, and got him set up to continue his work. Despite this he came out with a deeply flawed analysis and false statements, which unfortunately are being taken at face value.

Figured I'd provide some more context about the changes made from our perspective:

You're all welcome to use and contribute to OpenRewrite for as much as you'd like; we see no reason to be hesitant at all, but understand opinions may differ. Either way I'll continue to support the Apache Maven project and users for as much as I can, as I have when talking to Apache Maven contributors over the years, especially with an eye towards changes in the upcoming v4.

@elharo
Copy link
Contributor

elharo commented May 18, 2025 via email

@Pankraz76
Copy link
Contributor Author

@gastaldi I think we talked about using rewrite. FYI: consider license. What do you say about it, as quarkus leverages rewrite?

Please enlighten us about Quarkus perspective on this. Thanks.

@gastaldi
Copy link
Member

Please enlighten us about Quarkus perspective on this. Thanks.

@gsmet may know more about this subject

@timtebeek
Copy link
Contributor

Please enlighten us about Quarkus perspective on this. Thanks.

@gsmet may know more about this subject

We coordinated at the time; Quarkus only uses the Apache Licensed core of OpenRewrite, including recipes moved for that specific purpose.

For Apache Maven there would similarly still be a large suite of recipes that continue to be Apache Licensed, like for instance the Apache Maven best practices, which we'll expand to cover more of the Maven 4 migration nice to haves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants