Skip to content

Handle static method calls when removing unnecessary explicit type arguments. #547

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

Conversation

JohannisK
Copy link
Contributor

@JohannisK JohannisK commented May 8, 2025

What's changed?

Adde rules to the removing unnecessary explicit type arguments recipe to deal with static method calls.

What's your motivation?

The recipe currently removes explicit type arguments from method calls that need it.

@JohannisK JohannisK self-assigned this May 8, 2025
@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite May 8, 2025
@JohannisK JohannisK added bug Something isn't working recipe labels May 8, 2025
github-actions[bot]

This comment was marked as off-topic.

@@ -64,7 +80,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu
.count() > 1) {
return m;
}
enclosingType = enclosingMethod.getType();
enclosingType = m.getMethodType().getReturnType();
Copy link
Contributor Author

@JohannisK JohannisK May 8, 2025

Choose a reason for hiding this comment

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

This change might be to simplistic, but when the enclosing is a method invocation:
We're dealing with GenericClass.<String>someTypedBuilder(String.class), and enclosing is GenericClass.<String>someTypedBuilder(String.class).build()

<String> is already kept and returned earlier if the argument of the method does not do type inference (e.g. it does not take Class<T> arg. If it does do type inference, the method is its own enclosing type.
E.G.
When GenericClass.<String>someTypedBuilder(String.class) returns a type inferred instance GenericClassBuild<String>,
GenericClass.<String>someTypedBuilder(String.class).build() should be the same as

GenericClassBuild<String> gcb = GenericClass.someTypedBuilder(String.class);
gcb.build();

and so the explicit generic type can be removed?

Please double check my logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assigning the returnType to enclosingType is a bit of an ugly way of forcing the existing code to remove explicit generic type information.
Maybe I should rename enclosingType to inferedType?

if (enclosingType != null && TypeUtils.isOfType(enclosingType, m.getMethodType().getReturnType())) {
    m = m.withTypeParameters(null);
}

if the return type of the method invocation is the same as the inferred type, the explicit type arguments may be removed?

Copy link
Contributor

@jevanlingen jevanlingen May 9, 2025

Choose a reason for hiding this comment

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

Maybe I should rename enclosingType to inferedType?

Yeah do that, that's way easier to understand if you read this recipe for the first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, leaving this open for @timtebeek to have this context when he reviews.

@timtebeek timtebeek self-requested a review May 8, 2025 23:09
github-actions[bot]

This comment was marked as off-topic.

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 636-652
    • lines 698-697
    • lines 2311-2310
    • lines 2326-2338

Copy link
Member

@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.

Thanks for the fix here! Great to not make incorrect changes where we can avoid it.

@github-project-automation github-project-automation bot moved this from In Progress to Ready to Review in OpenRewrite May 9, 2025
…oves-necessary-generic-type-argument-from-builder-method
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 474-473
    • lines 636-652
    • lines 698-697
    • lines 2311-2310
    • lines 2326-2338

@timtebeek timtebeek merged commit 999a8f9 into main May 9, 2025
2 checks passed
@timtebeek timtebeek deleted the 1142-unnecessary-explicit-type-arguments-removes-necessary-generic-type-argument-from-builder-method branch May 9, 2025 11:35
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working recipe
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants