Skip to content

AddImport: Handle import ambiguity#5439

Merged
sambsnyd merged 4 commits into
openrewrite:mainfrom
amishra-u:add_import
May 16, 2025
Merged

AddImport: Handle import ambiguity#5439
sambsnyd merged 4 commits into
openrewrite:mainfrom
amishra-u:add_import

Conversation

@amishra-u

Copy link
Copy Markdown
Contributor

What's changed?

Previously, the AddImport logic did not account for import ambiguity, leading to compilation errors when ambiguous imports were added. This update introduces a check for ambiguity before adding an import. If ambiguity is detected, the code now fully qualifies the relevant identifiers instead of adding the import

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-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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-gradle/src/main/java/org/openrewrite/gradle/plugins/UpgradePluginVersion.java
    • lines 254-255

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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-gradle/src/main/java/org/openrewrite/gradle/plugins/UpgradePluginVersion.java
    • lines 254-255

sambsnyd and others added 2 commits May 16, 2025 13:14
…mberReference.java

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

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

Copy link
Copy Markdown
Member

Nice work @amishra-u !

@sambsnyd sambsnyd merged commit 70921f2 into openrewrite:main May 16, 2025
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite May 16, 2025
@knutwannheden

Copy link
Copy Markdown
Contributor

Personally, I prefer doing this the other way around by using fully qualified names in the template and then apply the ShortenFullyQualifiedTypeReferences visitor to add imports where possible.

@amishra-u

amishra-u commented May 17, 2025

Copy link
Copy Markdown
Contributor Author

@knutwannheden Just to clarify—are you suggesting that we update all existing recipes to use fully qualified names in the template, and then rely on the ShortenFullyQualifiedTypeReferences visitor to handle the imports?

Somewhat similar idea #5435

@amishra-u

amishra-u commented May 17, 2025

Copy link
Copy Markdown
Contributor Author

@sambsnyd @knutwannheden When you have a moment, could you please share your thoughts on this issue?

@knutwannheden

Copy link
Copy Markdown
Contributor

@amishra-u No, I think it makes sense for AddImport to have this extra safety, as it otherwise ends up braking code. This way we can keep both alternatives in place and let people use either one.

@amishra-u amishra-u deleted the add_import branch May 17, 2025 20:14
@timtebeek

Copy link
Copy Markdown
Member

@amishra-u

Copy link
Copy Markdown
Contributor Author

@timtebeek fixed it here
#5452

also fixed all the test in rewrite-testing-frameworks. openrewrite/rewrite-testing-frameworks#731

we may use the same approach for rewrite-apache and rewrite-migrate-java

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

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants