Skip to content

Add additional test cases to TestStaticImportRuleNormalizer#23

Open
ksobolew wants to merge 1 commit into
airlift:mainfrom
ksobolew:kudi/static-imports-fixes
Open

Add additional test cases to TestStaticImportRuleNormalizer#23
ksobolew wants to merge 1 commit into
airlift:mainfrom
ksobolew:kudi/static-imports-fixes

Conversation

@ksobolew

@ksobolew ksobolew commented May 29, 2026

Copy link
Copy Markdown
Contributor

There are various ways the type can remain used; the existing test case covers the case when the type is used to invoke a static method (other than the ones that are meant to be static-imported), these new test cases covers when the type is used as a method parameter or return type. Fortunately the fix in #24 fixes those cases as well.

@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2ef33c0a-6e3d-44d4-8799-25332f51edb3

📥 Commits

Reviewing files that changed from the base of the PR and between 2d5259c and 6d2f852.

📒 Files selected for processing (2)
  • airstyle-core/src/main/java/io/airlift/airstyle/normalizer/StaticImportRuleNormalizer.java
  • airstyle-core/src/test/java/io/airlift/airstyle/normalizer/TestStaticImportRuleNormalizer.java

📝 Walkthrough

Walkthrough

This PR extends StaticImportRuleNormalizer to handle classes that require static imports while also having other non-required method calls. A new helper method scans the AST to detect such classes by examining method invocations and type references within required-static-import scenarios. The normalizer then conditionally preserves the plain import for these mixed-usage classes instead of removing it, while still applying the required static imports. Tests are updated to reflect the expected behavior with stream-based collectors and new assertions validate the case where immutable types appear as parameter types.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@electrum

Copy link
Copy Markdown
Member

Sorry, I missed seeing this before merging #24, though I think that one fixed it in a much simpler way. It looks like the test changes from this one are still useful.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a case where StaticImportRuleNormalizer would remove a class import when rewriting qualified method calls into required static imports, even though the class is still referenced elsewhere (e.g., other static calls like copyOf(...) or as a type in method parameters).

Changes:

  • Updates the normalizer to keep class imports when the class is still referenced via non-rewritten calls or in type positions.
  • Fixes and extends TestStaticImportRuleNormalizer to cover the previously broken case and add two regression scenarios.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
airstyle-core/src/main/java/io/airlift/airstyle/normalizer/StaticImportRuleNormalizer.java Tracks additional class/type usages so required-static-import rewrites don’t incorrectly remove still-needed imports.
airstyle-core/src/test/java/io/airlift/airstyle/normalizer/TestStaticImportRuleNormalizer.java Adjusts the existing required-static-import test and adds regression tests for “class still used” scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ksobolew

Copy link
Copy Markdown
Contributor Author

though I think that one fixed it in a much simpler way

Looks like it :) I'll see if there are any test duplication and repurpose this PR for additional test coverage.

@ksobolew ksobolew force-pushed the kudi/static-imports-fixes branch from 6d2f852 to 873641a Compare June 17, 2026 08:45
@ksobolew ksobolew changed the title Fix some overeager removal of imports when we consider methods that needs to be static-imported Add additional test cases to TestStaticImportRuleNormalizer Jun 17, 2026
There are various ways the type can remain used; the existing test case
covers the case when the type is used to invoke a static method (other
than the ones that are meant to be static-imported), these new test
cases covers when the type is used as a method parameter or return type.
Fortunately the fix in airlift#24 fixes those cases as well.
@ksobolew ksobolew force-pushed the kudi/static-imports-fixes branch from 873641a to 1575391 Compare June 17, 2026 08:46
@ksobolew

Copy link
Copy Markdown
Contributor Author

@electrum I decided not to modify the existing test(s) as in the original version of this PR, instead I added two more test cases, one that was needed for my original fix (but the fix in #24 covers it already), and one for completeness. So these are now mostly just a way to expand test coverage.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants