Skip to content
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

ChangeType should fully qualify type usage in the case of conflicting imports #4458

Merged
merged 5 commits into from
Aug 30, 2024
Merged

ChangeType should fully qualify type usage in the case of conflicting imports #4458

merged 5 commits into from
Aug 30, 2024

Conversation

Laurens-W
Copy link
Contributor

@Laurens-W Laurens-W commented Aug 29, 2024 •
edited by timtebeek
Loading

What's changed?

Detect when an import becomes ambiguous and once it does start fully qualifying references to the new type

What's your motivation?

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

Anyone you would like to review specifically?

@timtebeek

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

@Laurens-W Laurens-W force-pushed the changetype-removes-import-instead-of-fqn branch from ce0dc2c to 6285ed0 Compare August 29, 2024 15:35
@Laurens-W Laurens-W requested a review from timtebeek August 30, 2024 09:47
@Laurens-W Laurens-W self-assigned this Aug 30, 2024
@Laurens-W Laurens-W added the bug Something isn't working label Aug 30, 2024
@Laurens-W Laurens-W marked this pull request as ready for review August 30, 2024 10:01
github-actions[bot]
github-actions bot reviewed Aug 30, 2024
View reviewed changes
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:

  • rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java
    • lines 464-464

timtebeek
timtebeek reviewed Aug 30, 2024
View reviewed changes
rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java Outdated
@@ -505,6 +511,18 @@ private Expression updateOuterClassTypes(Expression typeTree) {
private boolean isTargetFullyQualifiedType(JavaType.@Nullable FullyQualified fq) {
return fq != null && TypeUtils.isOfClassType(fq, originalType.getFullyQualifiedName()) && targetType instanceof JavaType.FullyQualified;
}

private boolean becomesAmbiguous(JavaSourceFile cu) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This once again gave me warnings when opened in the IDE; please keep that F2 before commit in mind, and
image

Also be sure to enable this option for yet another nudge to not push any code with new warnings; that helps me review & improve code quality.
image

timtebeek
timtebeek reviewed Aug 30, 2024
View reviewed changes
rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java Outdated
JavaType.FullyQualified newType = TypeUtils.asFullyQualified(targetType);
JavaType.FullyQualified oldType = TypeUtils.asFullyQualified(originalType);

return newType != null && oldType != null && cu.getImports().stream().anyMatch(imprt -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to avoid using the streams API, even when convenient, but especially on paths that might get evaluated often; Adding imports is one of those hot paths where we really want to avoid these object allocations where we can.

timtebeek
timtebeek approved these changes Aug 30, 2024
View reviewed changes
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.

Great to see this case fixed! Thanks!

github-actions[bot]
github-actions bot reviewed Aug 30, 2024
View reviewed changes
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:

  • rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java
    • lines 464-464

@timtebeek timtebeek changed the title ChangeType removes ambiguous import instead of fully qualifying it ChangeType should fully qualify type usage in the case of conflicting imports Aug 30, 2024
@timtebeek timtebeek merged commit a593ef6 into main Aug 30, 2024
2 checks passed
@timtebeek timtebeek deleted the changetype-removes-import-instead-of-fqn branch August 30, 2024 17:27
@timtebeek
Copy link
Contributor

We now see some failures downstream related to imports:
https://ge.openrewrite.org/s/fyq3wpbm56qgk/tests/task/:test/details/org.openrewrite.apache.httpclient4.CookieConstantsTest/cookieConstantsMapping()?top-execution=1

@knutwannheden
Copy link
Contributor

While working on #4460 I noticed that TypeTree#build() (which is used by ChangeType) produces J.FieldAccess (or J.Identifier) trees, where the type attribution isn't entirely correct. For the last segment the corresponding J.Identifier doesn't have a type. As a consequence of that the SimplifyQualifiedTypeNames visitor didn't work correctly. I tried to fix this behavior but then some ChangeType tests started failing, so I ended up reverting those changes and do that locally as part of the AddMethodParameter visitor instead.

I just wanted to mention this here, as the PR seems somewhat related and I think it would be great if we could fix that TypeTree#build() bug at some point. Possibly you have some insights regarding this.

@Laurens-W
Copy link
Contributor Author

Laurens-W commented Sep 2, 2024 •
edited
Loading

We now see some failures downstream related to imports: https://ge.openrewrite.org/s/fyq3wpbm56qgk/tests/task/:test/details/org.openrewrite.apache.httpclient4.CookieConstantsTest/cookieConstantsMapping()?top-execution=1

A situation where newType was already being used wasn't covered, this fixes it

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

@github-actions github-actions[bot] github-actions[bot] left review comments

@timtebeek timtebeek timtebeek approved these changes

Assignees

@Laurens-W Laurens-W

Labels
bug Something isn't working
Projects
OpenRewrite
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ChangeType is not addressing import conflicts
3 participants
@Laurens-W @timtebeek @knutwannheden