From ceb8e8d99c473447ca39b93ac0ca9fb5eb79d34e Mon Sep 17 00:00:00 2001 From: dongwoo Date: Wed, 13 Aug 2025 01:44:05 +0900 Subject: [PATCH 1/3] fix: preserve spacing when removing access modifiers Properly merge whitespace and comments from removed access modifier with the next modifier when changing to package-private access level. - Add Space merging logic for removed modifiers - Handle comment relocation during modifier removal - Add test for line comment movement --- .../java/ChangeMethodAccessLevelTest.java | 28 +++++++++++++++ .../java/ChangeMethodAccessLevelVisitor.java | 34 +++++++++++++++++-- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java index 3cb2e3c65a..1254c024cc 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java @@ -427,4 +427,32 @@ static void aMethod(Double d) { ) ); } + + @Test + void publicToPackagePrivateMovesLineComment() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodAccessLevel("com.abc.DatabaseConfiguration dataSource(..)", "package", null)), + java( + """ + package com.abc; + + class DatabaseConfiguration { + @SuppressWarnings("ALL") + // comments + public static void dataSource() { + } + } + """, + """ + package com.abc; + + class DatabaseConfiguration { + @SuppressWarnings("ALL") // comments + static void dataSource() { + } + } + """ + ) + ); + } } diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodAccessLevelVisitor.java b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodAccessLevelVisitor.java index 63b00787e8..fef69b0331 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodAccessLevelVisitor.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodAccessLevelVisitor.java @@ -104,16 +104,46 @@ else if (currentMethodAccessLevel == null) { } // If target access level is package-private (no modifier), remove the current access level modifier - // and copy any associated comments + // and copy any associated comments and space else if (newAccessLevel == null) { final List modifierComments = new ArrayList<>(); + final List removedModifierPrefix = new ArrayList<>(); List modifiers = ListUtils.map(m.getModifiers(), mod -> { if (mod.getType() == currentMethodAccessLevel) { modifierComments.addAll(mod.getComments()); + removedModifierPrefix.add(mod.getPrefix()); return null; } - // copy access level modifier comment to next modifier if it exists + if (!removedModifierPrefix.isEmpty()) { + // Merge the removed modifier's prefix with the current modifier's prefix + // Preserve the whitespace structure while moving comments + Space removedPrefix = removedModifierPrefix.get(0); + final List allComments = ListUtils.concatAll(removedPrefix.getComments(), mod.getPrefix().getComments()); + + // Use the removed modifier's whitespace if it has meaningful content, otherwise use current + String currentWhitespace = mod.getPrefix().getWhitespace(); + if (currentWhitespace.isEmpty()) { + String removedWhitespace = removedPrefix.getWhitespace(); + if (!removedWhitespace.isEmpty()) { + currentWhitespace = removedWhitespace; + } + } + + Space mergedPrefix = Space.build(currentWhitespace, allComments); + J.Modifier nextModifier = mod.withPrefix(mergedPrefix); + removedModifierPrefix.clear(); + + // Also handle modifier's own comments if any + if (!modifierComments.isEmpty()) { + nextModifier = nextModifier.withComments(ListUtils.concatAll(new ArrayList<>(modifierComments), mod.getComments())); + modifierComments.clear(); + } + + return nextModifier; + } + + // copy access level modifier comment to next modifier if it exists (fallback) if (!modifierComments.isEmpty()) { J.Modifier nextModifier = mod.withComments(ListUtils.concatAll(new ArrayList<>(modifierComments), mod.getComments())); modifierComments.clear(); From 32b993a0a56656c4c4b12380ffc1552e5b12fddf Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 12 Aug 2025 22:24:23 +0200 Subject: [PATCH 2/3] Update rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../java/org/openrewrite/java/ChangeMethodAccessLevelTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java index 1254c024cc..f3553e7966 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java @@ -427,7 +427,6 @@ static void aMethod(Double d) { ) ); } - @Test void publicToPackagePrivateMovesLineComment() { rewriteRun( From 1fa7d8a989340ff611c69c17af00704fb5ab0f67 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 13 Aug 2025 13:46:18 +0200 Subject: [PATCH 3/3] Slight polish --- .../java/ChangeMethodAccessLevelTest.java | 1 + .../java/ChangeMethodAccessLevelVisitor.java | 27 +++++-------------- 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java index f3553e7966..abc7f028cc 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodAccessLevelTest.java @@ -427,6 +427,7 @@ static void aMethod(Double d) { ) ); } + @Test void publicToPackagePrivateMovesLineComment() { rewriteRun( diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodAccessLevelVisitor.java b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodAccessLevelVisitor.java index fef69b0331..7d432db063 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodAccessLevelVisitor.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodAccessLevelVisitor.java @@ -30,6 +30,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import static java.util.Collections.emptyList; @@ -107,32 +108,18 @@ else if (currentMethodAccessLevel == null) { // and copy any associated comments and space else if (newAccessLevel == null) { final List modifierComments = new ArrayList<>(); - final List removedModifierPrefix = new ArrayList<>(); + final AtomicReference<@Nullable Space> removedModifierPrefix = new AtomicReference<>(null); List modifiers = ListUtils.map(m.getModifiers(), mod -> { if (mod.getType() == currentMethodAccessLevel) { modifierComments.addAll(mod.getComments()); - removedModifierPrefix.add(mod.getPrefix()); + removedModifierPrefix.set(mod.getPrefix()); return null; } - if (!removedModifierPrefix.isEmpty()) { - // Merge the removed modifier's prefix with the current modifier's prefix - // Preserve the whitespace structure while moving comments - Space removedPrefix = removedModifierPrefix.get(0); - final List allComments = ListUtils.concatAll(removedPrefix.getComments(), mod.getPrefix().getComments()); - - // Use the removed modifier's whitespace if it has meaningful content, otherwise use current - String currentWhitespace = mod.getPrefix().getWhitespace(); - if (currentWhitespace.isEmpty()) { - String removedWhitespace = removedPrefix.getWhitespace(); - if (!removedWhitespace.isEmpty()) { - currentWhitespace = removedWhitespace; - } - } - - Space mergedPrefix = Space.build(currentWhitespace, allComments); - J.Modifier nextModifier = mod.withPrefix(mergedPrefix); - removedModifierPrefix.clear(); + Space removedSpace = removedModifierPrefix.getAndSet(null); + if (removedSpace != null) { + J.Modifier nextModifier = mod.withPrefix(mod.getPrefix().withComments( + ListUtils.concatAll(removedSpace.getComments(), mod.getComments()))); // Also handle modifier's own comments if any if (!modifierComments.isEmpty()) {