feat: Add `ReplaceObsoletesStep` #2530

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.

Sign up for GitHub

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

Jump to bottom

Closed

Pankraz76 wants to merge 1 commit into diffplug:main from Pankraz76:ReplaceObsoletesStep

Contributor

Pankraz76 commented Jun 25, 2025 •

edited

Loading

feat: Add ReplaceObsoletesStep

This PR introduces a new step to remove common boilerplate code, such as:

Redundant declarations reimplementing Java defaults
Unnecessary explicit language defaults
Other similar redundancies

Motivation:
Discovered during the review of:

Call getProperty can be simplified for line.separator #2524
- This comment highlighting the need for such cleanup.

Benefits:

Reduces noise in codebases
Eliminates unnecessary maintenance of obvious defaults
Follows DRY and CoC principles

Pankraz76 force-pushed the ReplaceObsoletesStep branch from 3c0e723 to ee24a54 Compare

June 25, 2025 10:51

Pankraz76 marked this pull request as ready for review

June 25, 2025 10:51

Contributor Author

Pankraz76 commented Jun 25, 2025

kindly request review @iddeepak.

Pankraz76 force-pushed the ReplaceObsoletesStep branch 5 times, most recently from be0e0a5 to 5b74aa0 Compare

June 25, 2025 11:19

Contributor

iddeepak commented Jun 25, 2025 •

edited

Loading

Add here
plugin-gradle here
plugin-maven here

Added

ReplaceObsoletesStep removes redundant default initializations (#2530)

iddeepak reviewed

View reviewed changes

plugin-maven/src/test/java/com/diffplug/spotless/maven/java/ReplaceObsoletesStepTest.java

+              	}
+              	@Test
+              	@Disabled("feature envy: (edge case/hard to implement) having a dedicated method")

Contributor

iddeepak Jun 25, 2025

Thanks for adding this. Since it's currently disabled, maybe a follow-up issue should be created ?

Contributor Author

Pankraz76 commented Jun 25, 2025

thx for feedback.

Pankraz76 force-pushed the ReplaceObsoletesStep branch from 5b74aa0 to fcc724d Compare

June 25, 2025 13:17

Pankraz76 requested a review from iddeepak

June 25, 2025 13:18


          feat: Add ReplaceObsoletesStep

a5d9e0b

Pankraz76 force-pushed the ReplaceObsoletesStep branch from fcc724d to a5d9e0b Compare

June 25, 2025 13:19

github-advanced-security bot found potential problems

View reviewed changes

lib/src/main/java/com/diffplug/spotless/generic/ReplaceObsoletesStep.java

+              		String processed = compile(
+              				"(enum\\s+\\w+\\s*\\{[^}]*?)(public\\s+static\\s+)(\\w+\\s*,\\s*|\\w+\\s*;)",
+              				MULTILINE)
+              				.matcher(input)

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'enum 0{' and with many repetitions of 'enum 0{'.

lib/src/main/java/com/diffplug/spotless/generic/ReplaceObsoletesStep.java

+              		processed = compile(
+              				"(enum\\s+\\w+\\s*\\{[^}]*?)(public\\s+static\\s+)(\\w+\\s+\\w+\\s*\\([^)]*\\)\\s*(?:throws\\s+[\\w.]+(?:\\s*,\\s*[\\w.]+)*\\s*)?\\{)",
+              				MULTILINE)
+              				.matcher(processed)

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'enum 0{' and with many repetitions of 'enum 0{'.
This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'enum 0{public static 0 0(' and with many repetitions of 'public static 0 0('.

lib/src/main/java/com/diffplug/spotless/generic/ReplaceObsoletesStep.java

+              		processed = compile(
+              				"(interface\\s+\\w+\\s*\\{[^}]*?)(public\\s+static\\s+)(final\\s+)(\\w+\\s+\\w+\\s*=)",
+              				MULTILINE)
+              				.matcher(processed)

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'interface 0{' and with many repetitions of 'interface 0{'.

lib/src/main/java/com/diffplug/spotless/generic/ReplaceObsoletesStep.java

+              		processed = compile(
+              				"(interface\\s+\\w+\\s*\\{[^}]*?)(public\\s+static\\s+)(\\w+\\s+\\w+\\s*\\([^)]*\\)\\s*(?:throws\\s+[\\w.]+(?:\\s*,\\s*[\\w.]+)*\\s*)?;)",
+              				MULTILINE)
+              				.matcher(processed)

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'interface 0{' and with many repetitions of 'interface 0{'.
This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'interface 0{public static 0 0(' and with many repetitions of 'public static 0 0('.

lib/src/main/java/com/diffplug/spotless/generic/ReplaceObsoletesStep.java

+              		processed = compile(
+              				"(interface\\s+\\w+\\s*\\{[^}]*?)(public\\s+static\\s+)(class\\s+\\w+\\s*\\{)",
+              				MULTILINE)
+              				.matcher(processed)

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'interface 0{' and with many repetitions of 'interface 0{'.

lib/src/main/java/com/diffplug/spotless/generic/ReplaceObsoletesStep.java

+              		return compile(
+              				"(interface\\s+\\w+\\s*\\{[^}]*?)(?:public\\s+)?abstract\\s+(\\w+\\s+\\w+\\s*\\([^)]*\\)\\s*(?:throws\\s+[\\w.]+(?:\\s*,\\s*[\\w.]+)*\\s*)?;)",
+              				MULTILINE)
+              				.matcher(input)

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'interface 0{' and with many repetitions of 'interface 0{'.
This

regular expression

that depends on a

user-provided value

may run slow on strings starting with 'interface 0{abstract 0 0(' and with many repetitions of 'abstract 0 0('.

iddeepak approved these changes

View reviewed changes

Contributor

iddeepak commented Jun 25, 2025

thx for feedback.

Thank you!

Member

nedtwigg commented Jun 25, 2025

Spotless is primarily a switchboard for existing formatters. Sometimes we merge in a step where the implementation is inside our repository, but that tends to cause problems with backward compatibility - Spotless has pretty strict backwards-compatibility. If the step implementation lives in a separate jar, that gives the step a lot more flexibility to change, and Spotless users can choose to pin an older version of the step's behavior but still get new Gradle/Maven support from Spotless if they want.

In my view, this step has some great cleanups (redundant static for example), and some that I disagree with (I prefer to always initialize fields, I think Kotlin designed this better than Java, I would prefer a rule that does the opposite of this one).

I'm very happy to merge support for this step whether I agree with it or not, but I think it should live in a separate project similar to say google-java-format. I would think about the README.md for this project - what is the philosophy, what users will want it, which don't, etc.

If that's too much work and you just want to use if for your own projects, you can always put it in buildSrc and just call addStep :)

nedtwigg closed this

Contributor Author

Pankraz76 commented Jun 26, 2025

Thanks for the insight. I partially agree.

If this step is generic, it should apply to Google, Palantir, and any other Java formatter. The same goes for wildcard imports—this is a general feature we want, regardless of the formatter in use.

But what if one project uses Palantir and wants to apply this recipe, even though it lives under the Google formatter? How would we handle that?

Assuming this is as generic as the wildcard feature, I don’t see why it should be treated differently.

Contributor Author

Pankraz76 commented Jun 26, 2025

What about splitting the process?
We could first address static abstract redundancies in a redundantDeclaration step, and then extract the initialization logic into a redundantInitialization step.

This way, users can choose which aspect they want to focus on.
The assumption is that the first step would avoid any obvious overhead, as already marked by the IDE.

This was referenced Jun 27, 2025

feat: Add RemoveUnusedDeclarations google/google-java-format#1263

Closed

feat: Add RemoveUnusedDeclarations palantir/palantir-java-format#1342

Open

Pankraz76 mentioned this pull request

feat: Add RemoveUnusedDeclarations #2547

Draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet