Keep plain import when type still used after static-import rewrite#24
Conversation
910c317 to
1fb6104
Compare
StaticImportRuleNormalizer rewrites required-static-import calls such as ImmutableSet.toImmutableSet() to their unqualified form and adds the static import. It also unconditionally removed the plain import for the type, which dropped a still-needed import when the type was referenced elsewhere (e.g. ImmutableSet.of alongside ImmutableSet.toImmutableSet), producing uncompilable output. Stop removing the plain import here and defer to the UnusedImportNormalizer phase that already runs afterward, which removes it only when the type is no longer referenced.
1fb6104 to
8bdae09
Compare
|
@electrum PTAL when you have a chance |
|
Warning Review limit reached
More reviews will be available in 57 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR modifies 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ 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. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes an import-removal bug in the formatter pipeline where StaticImportRuleNormalizer could drop a still-needed non-static (plain) type import when rewriting required static-import calls, leading to uncompilable output. The fix defers plain-import removal to the existing UnusedImportNormalizer phase, which can safely remove the import only when the type is truly unused.
Changes:
- Stop removing the plain import for a type when rewriting required static-import method invocations (leave cleanup to
UnusedImportNormalizer). - Update the existing required-static-import formatting expectation to keep still-used plain imports.
- Add regression tests covering both “plain type still used” (import kept) and “type only used for static rewrite” (import removed by unused-import pass).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
airstyle-core/src/main/java/io/airlift/airstyle/normalizer/StaticImportRuleNormalizer.java |
Prevents premature removal of plain imports during required static-import rewrites, avoiding dropped-needed-import compilation failures. |
airstyle-core/src/test/java/io/airlift/airstyle/normalizer/TestStaticImportRuleNormalizer.java |
Updates expectations and adds regression coverage for keeping/removing plain imports appropriately after static-import rewrites. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
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.
Summary
StaticImportRuleNormalizerrewrites required-static-import calls (e.g.ImmutableSet.toImmutableSet()) to their unqualified form and adds the static import. It also unconditionally removed the plain import for the type, which dropped a still-needed import whenever the type was referenced elsewhere — for exampleImmutableSet.of("5")next toImmutableSet.toImmutableSet()— producing uncompilable output.UnusedImportNormalizerphase that already runs afterward, which removes the import only when the type is no longer referenced.testFormatterFixesRequiredStaticImports(its prior expectation encoded the buggy dropped imports) and added two regression tests: one for the reportedImmutableSet.of+toImmutableSetcase (import kept) and a control where onlytoImmutableSetis used (import removed).Test plan
mvnd test -pl airstyle-core -am— all 1549 tests pass, including checkstyle and license checks