Skip to content

RemoveUnusedParams #560

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

iddeepak
Copy link
Contributor

What’s changed?

I’ve added a new recipe class, RemoveUnusedParams, that automatically removes parameters from Java methods when they’re declared but never referenced in the method body. It still respects:

  • @OverRide methods (both explicit and those detected across the codebase)
  • Native methods
  • Parameters with their own annotations (e.g. @Deprecated)
  • Constructors, varargs, interfaces, and other edge cases

Alongside the main recipe, I’ve expanded the unit tests to cover:

  • Basic removal of unused parameters
  • Preservation of used, overridden, and annotated parameters
  • Constructors with unused arguments
  • Varargs methods
  • Native declarations
  • Interface methods (no change expected)

What’s your motivation?

Over time, code tends to accumulate unused parameters—leftovers from refactoring, changing requirements, or evolving APIs. They add noise, make signatures harder to read, and can confuse future maintainers. This recipe:

  1. Keeps signatures lean, showing exactly what each method actually needs
  2. Automates cleanup, reducing manual PR churn
  3. Helps enforce consistency across a large codebase

Anything in particular you'd like reviewers to focus on?

  • Override detection: Does the scan/visitor logic reliably catch all overridden methods?
  • Edge-case coverage: Any real-world patterns (e.g. multi-parameter annotations, complex varargs, inner-class overrides) that might slip through?
  • Performance: The recipe now uses a single JavaIsoVisitor pass—does it still scale on large projects?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

  • Keeping the original “shadow-stack” approach to detect name shadowing more precisely—but in practice it added complexity without significant benefit.
  • Using a dedicated MethodMatcher or type-based override detection; this felt like overkill for a simple annotation check.

Any additional context

  • This work is driven by feedback on Issue #559.
  • All existing tests pass, and I’ve verified the new cases locally with mvn test.
  • I’ve also run the IntelliJ auto-formatter to keep the style consistent.

Checklist

  • I’ve added unit tests to cover both positive and negative cases
  • I’ve read and applied the recipe conventions and best practices
  • I’ve used the IntelliJ IDEA auto-formatter on affected files

@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite May 16, 2025
Copy link
Contributor

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

well done!

try polish.

@Pankraz76
Copy link
Contributor

This one done:

Do we cover generic assignment as well? Might be dedicated recipe, to give options.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 660-659
    • lines 705-721
    • lines 1254-1273
    • lines 1306-1305
    • lines 2337-2336
    • lines 2352-2364
    • lines 2801-2800
    • lines 2820-2831

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 660-659
    • lines 705-721
    • lines 1254-1273
    • lines 1306-1305
    • lines 2337-2336
    • lines 2352-2364
    • lines 2801-2800
    • lines 2820-2831

@timtebeek timtebeek self-requested a review May 16, 2025 09:26
@iddeepak
Copy link
Contributor Author

This one done:

Do we cover generic assignment as well? Might be dedicated recipe, to give options.

RemoveUnusedParams only handles unused parameters. Unused assignments would need their own recipe

@iddeepak iddeepak requested a review from Pankraz76 May 16, 2025 16:26
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 328-345
    • lines 660-659
    • lines 705-721
    • lines 1254-1273
    • lines 1306-1305
    • lines 2251-2250
    • lines 2337-2336
    • lines 2352-2364
    • lines 2801-2800
    • lines 2820-2831

…s.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

try avoid feature envy (& DRY) giving single point of truth (SPOT)

Copy link
Contributor

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

new test case - cover constructor as well.

iddeepak and others added 2 commits May 17, 2025 22:21
Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments

Updated review comments
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 328-345
    • lines 660-659
    • lines 705-721
    • lines 1254-1273
    • lines 1306-1305
    • lines 2251-2250
    • lines 2337-2336
    • lines 2352-2364
    • lines 2801-2800
    • lines 2820-2831

iddeepak and others added 2 commits May 17, 2025 22:34
…s.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…s.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@iddeepak iddeepak requested a review from Pankraz76 May 17, 2025 17:14
Copy link
Contributor

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

last polish.

Done very well, could already merge.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 328-345
    • lines 660-659
    • lines 705-721
    • lines 1254-1273
    • lines 1306-1305
    • lines 2251-2250
    • lines 2337-2336
    • lines 2352-2364
    • lines 2801-2800
    • lines 2820-2831

Copy link
Contributor

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

uncle bob would be proud.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 328-345
    • lines 660-659
    • lines 705-721
    • lines 1254-1273
    • lines 1306-1305
    • lines 2251-2250
    • lines 2337-2336
    • lines 2352-2364
    • lines 2801-2800
    • lines 2820-2831

@Pankraz76
Copy link
Contributor

@iddeepak
Copy link
Contributor Author

this could be follow up:

@Pankraz76 Thanks! I’ve opened Issue #564 to track adding the UnusedAssignment recipe. Please feel free to drop any additional examples or feedback over there!

@timtebeek
Copy link
Contributor

Thanks both! I'm not likely to get to a review this week with travel ahead, but perhaps @MBoegers can have a look while I'm out.

@timtebeek timtebeek moved this from In Progress to Ready to Review in OpenRewrite May 19, 2025
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

A quick first round of review; great that you worked out you need a scanning recipe already; My comments are mostly to make better use of utilities and conventions we have in the project.

Comment on lines 91 to 98
private J.MethodDeclaration collectOverrideSignature(final J.MethodDeclaration m, final Accumulator acc) {
m.getLeadingAnnotations().stream()
.map(J.Annotation::getSimpleName)
.filter(OVERRIDE_ANNOTATION::equals)
.findAny()
.ifPresent(annotationName -> acc.add(buildSignature(m)));
return m;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than look for an override annotation, we have a method called isOverride on the type that should be used here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

Comment on lines 169 to 206
private boolean isShadowed(final String name, final Deque<Set<String>> shadowStack) {
return shadowStack.stream().anyMatch(scope -> scope.contains(name));
}

private boolean isDeclaredAsParameter(final String name, final J.MethodDeclaration m) {
return m.getParameters().stream()
.filter(p -> p instanceof J.VariableDeclarations)
.flatMap(p -> ((J.VariableDeclarations) p).getVariables().stream())
.anyMatch(v -> v.getSimpleName().equals(name));
}

private List<Statement> filterUnusedParameters(final J.MethodDeclaration m, final Set<String> usedParams) {
return m.getParameters().stream()
.flatMap(param -> param instanceof J.VariableDeclarations ?
filterDeclaration((J.VariableDeclarations) param, usedParams)
: Stream.of(param))
.collect(Collectors.toList());
}

private Stream<Statement> filterDeclaration(final J.VariableDeclarations decl, final Set<String> usedParams) {
return Optional.of(decl)
.filter(d -> !d.getLeadingAnnotations().isEmpty())
.map(d -> Stream.<Statement>of(d))
.orElseGet(() -> pruneByUsage(decl, usedParams));
}

private Stream<Statement> pruneByUsage(final J.VariableDeclarations decl, final Set<String> usedParams) {
return Optional.of(collectUsedParameters(decl, usedParams))
.filter(kept -> !kept.isEmpty())
.map(kept -> Stream.<Statement>of(decl.withVariables(kept)))
.orElseGet(Stream::empty);
}

private List<J.VariableDeclarations.NamedVariable> collectUsedParameters(final J.VariableDeclarations decl, final Set<String> usedParams) {
return decl.getVariables().stream()
.filter(v -> usedParams.contains(v.getSimpleName()))
.collect(Collectors.toList());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In general we try to avoid the streams API for as much as possible, especially when applied to every LST element of a particular type, as it leads to poor performance at scale with a lot of object allocations. Keep in kind that we're targeting Java 8 still, so any performance improvements in newer Java versions do not automatically apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, I replaced those streams with plain loops

Copy link
Contributor

Choose a reason for hiding this comment

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

might consider rule for this, with checkstyle custom imports. This is never ending story other way.

Sorry forgot about stream limitation. Its random impl. detail, at least functional method scope can be obtained.

This case could be new a migration recipe as well.

In both ways: giving lambda from list and backwards like seen here.

@github-project-automation github-project-automation bot moved this from Ready to Review to In Progress in OpenRewrite May 19, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 328-345
    • lines 660-659
    • lines 705-721
    • lines 1254-1273
    • lines 1306-1305
    • lines 2251-2250
    • lines 2337-2336
    • lines 2352-2364
    • lines 2801-2800
    • lines 2820-2831

@iddeepak iddeepak requested a review from timtebeek May 19, 2025 19:05
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 328-345
    • lines 660-659
    • lines 705-721
    • lines 1254-1273
    • lines 1306-1305
    • lines 2251-2250
    • lines 2337-2336
    • lines 2352-2364
    • lines 2801-2800
    • lines 2820-2831

Comment on lines +28 to +29
import java.time.Duration;
import java.util.ArrayDeque;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import java.time.Duration;
import java.util.ArrayDeque;
import java.util.ArrayDeque;

Comment on lines +108 to +114
return m.getBody() != null
&& m.getMethodType() != null
&& !m.hasModifier(J.Modifier.Type.Native)
&& m.getLeadingAnnotations().isEmpty()
&& !acc.contains(buildSignature(m));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return m.getBody() != null
&& m.getMethodType() != null
&& !m.hasModifier(J.Modifier.Type.Native)
&& m.getLeadingAnnotations().isEmpty()
&& !acc.contains(buildSignature(m));
}
return m.getBody() != null &&
m.getMethodType() != null &&
!m.hasModifier(J.Modifier.Type.Native) &&
m.getLeadingAnnotations().isEmpty() &&
!acc.contains(buildSignature(m));
return m.getSimpleName() + "#" +
m.getMethodType().getParameterTypes().stream()

Comment on lines +116 to +119
return m.getSimpleName() + "#"
+ m.getMethodType().getParameterTypes().stream()
.map(Object::toString)
.collect(Collectors.joining(","));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use MethodMatcher.methodPattern here instead of composing this yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants