Skip to content

Preserve XML comments that auto-format or content removal used to drop - #8355

Merged
timtebeek merged 1 commit into
mainfrom
tim/reflect-issue-8353
Jul 30, 2026
Merged

Preserve XML comments that auto-format or content removal used to drop#8355
timtebeek merged 1 commit into
mainfrom
tim/reflect-issue-8353

Conversation

@timtebeek

@timtebeek timtebeek commented Jul 30, 2026

Copy link
Copy Markdown
Member
  • Follow-up to Keep XML comments trailing an element on the same line #8353. That PR made a trailing comment's prefix authoritative by dropping the unconditional line break in LineBreaksVisitor. That line break had also been acting as an accidental repair for visitors that destroy a comment's spacing, so three latent defects are now user-visible. All three are reproduced by tests that fail on main.

1. Auto-format silently deletes anything after the root element

Xml.Document has no trailing-misc list — a comment or processing instruction after </project> is stored verbatim in the eof String. RemoveTrailingWhitespaceVisitor.visitDocument filtered eof down to its \n/\r characters, so every recipe that auto-formats dropped it:

 <project>
   <excludes/>
 </project>
-<!--why project-->
  • This is data loss rather than reflow, and it predates Keep XML comments trailing an element on the same line #8353 (the own-line case fails there too); an identity parse/print round-trips the comment fine, so it is the formatter. Now only horizontal whitespace at line ends is stripped, which is equivalent to the old filter for the whitespace-only eof that is the common case.

2. RemoveContentVisitor collapses an orphaned trailing comment

 <dependency>
     <groupId>group</groupId>
-    <version/><!-- why version -->
+<!-- why version -->
 </dependency>

The removed content's prefix is now transferred to the following sibling when that sibling has no line break of its own. This one is independent of auto-format — it reproduces with RemoveContentVisitor alone.

3. removePrecedingComment eats the previous sibling's trailing comment

 <dependency>
-    <groupId>group</groupId> <!-- why group -->
+    <groupId>group</groupId>
 </dependency>

Gated on the same predicate LineBreaksVisitor uses: a comment sharing a line with an earlier sibling is that sibling's. A comment on its own line, or one that is the first content of its tag, is still removed.

Tests

New, each failing on main:

  • AutoFormatTest#commentAfterRootElementIsRetained, #commentTrailingRootElementIsRetained
  • RemoveContentTest#trailingCommentKeepsItsOwnLine, #precedingCommentOnTheLineOfAnEarlierSiblingIsRetained

New regression guards for the behavior that must not change:

  • AutoFormatTest#trailingWhitespaceAfterRootElementIsRemoved
  • RemoveContentTest#precedingCommentOnItsOwnLineIsRemoved

:rewrite-xml:test, :rewrite-maven:test and :rewrite-gradle:test are green.

Not covered here

  • A comment trailing the prolog (<?xml version="1.0"?> <!--generated-->) is still moved onto its own line, because Keep XML comments trailing an element on the same line #8353's predicate bails when the parent is not an Xml.Tag. Consistent with that PR's stated scope, inconsistent as a rule; worth a separate look if anyone hits it.

Three defects around comments whose only carrier of intent is their prefix,
surfaced by #8353 removing the accidental repair that unconditional line
breaks were performing:

- `RemoveTrailingWhitespaceVisitor` filtered `Xml.Document#eof` down to its
  newline characters. `eof` holds everything after the root element, so any
  comment or processing instruction there was silently deleted by every
  auto-formatting recipe. Strip only horizontal whitespace at line ends.
- `RemoveContentVisitor` left a following sibling without a line break when
  the removed content's prefix carried it, collapsing an orphaned trailing
  comment onto the previous line. Transfer the prefix instead.
- `removePrecedingComment` deleted a comment sharing a line with an earlier
  sibling, which documents that sibling rather than the removed content.
@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Jul 30, 2026
@timtebeek
timtebeek merged commit a267819 into main Jul 30, 2026
1 check passed
@timtebeek
timtebeek deleted the tim/reflect-issue-8353 branch July 30, 2026 18:51
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite Jul 30, 2026
timtebeek added a commit to openrewrite/rewrite-migrate-java that referenced this pull request Jul 30, 2026
openrewrite/rewrite#8355 taught `RemoveContentVisitor` to hand a removed
element's prefix to a comment that trailed it, so the comment keeps its own
line instead of collapsing onto the preceding sibling. `JpaCacheProperties`
removed via `filterTagChildren`, which drops content outright and so never
saw that fix; switching the two single-element removals over picks it up and
restores the original expectations for 13 of the 20 cases.

The remaining 7 remove nothing: the element the comment trails is retained
(or merely has an attribute updated), so #8353 now faithfully reproduces the
inline placement the input used. Those expectations were asserting the old
reflow and are updated to match the source.
timtebeek added a commit to openrewrite/rewrite-migrate-java that referenced this pull request Jul 30, 2026
* Adopt upstream XML trailing-comment formatting

openrewrite/rewrite#8353 made XML auto-format keep a comment on the same
line as the element it trails, instead of reflowing it onto its own line.

`AddMockitoJavaAgentToMavenSurefirePlugin` relied on that reflow: when its
`<argLine>` is appended into an existing `<configuration>`, the accompanying
`<!--suppress MavenModelInspection -->` is no longer the first content of the
tag, so it stayed glued to the preceding sibling and read as annotating that
element rather than the `<argLine>` below it. Seed the comment with a newline
prefix so auto-format keeps it on its own line either way.

`JpaCachePropertiesTest` expectations reflowed comments that the input had
placed on the same line; they now assert the input's own placement.

* Revert JpaCachePropertiesTest expectations to own-line comments

Restores the pre-existing expectations, which assert that auto-format moves
a trailing comment onto its own line. All 20 cases fail against the current
rewrite snapshot: openrewrite/rewrite#8353 now keeps such comments on the
line of the element they trail.

Leaving the expectations as the desired behaviour rather than absorbing the
new output into the test. Red until the formatting is addressed.

* Route JpaCacheProperties removals through RemoveContentVisitor

openrewrite/rewrite#8355 taught `RemoveContentVisitor` to hand a removed
element's prefix to a comment that trailed it, so the comment keeps its own
line instead of collapsing onto the preceding sibling. `JpaCacheProperties`
removed via `filterTagChildren`, which drops content outright and so never
saw that fix; switching the two single-element removals over picks it up and
restores the original expectations for 13 of the 20 cases.

The remaining 7 remove nothing: the element the comment trails is retained
(or merely has an attribute updated), so #8353 now faithfully reproduces the
inline placement the input used. Those expectations were asserting the old
reflow and are updated to match the source.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant