Skip to content

Keep XML comments trailing an element on the same line - #8353

Merged
timtebeek merged 2 commits into
mainfrom
tim/dodoma
Jul 30, 2026
Merged

Keep XML comments trailing an element on the same line#8353
timtebeek merged 2 commits into
mainfrom
tim/dodoma

Conversation

@timtebeek

@timtebeek timtebeek commented Jul 30, 2026

Copy link
Copy Markdown
Member

Fixes https://github.com/moderneinc/customer-requests/issues/2926

What

XML auto-format moved a comment that trailed an element on the same line onto its own following line:

-  <exclude>com.example.profit.ProfitFactory.processProfit.1</exclude> <!--tmp logic, tested elsewhere-->
-  <exclude>com.example.profit.io.ProfitUtils</exclude><!--tmp logic, tested elsewhere-->
+  <exclude>com.example.profit.ProfitFactory.processProfit.1</exclude>
+  <!--tmp logic, tested elsewhere-->
+  <exclude>com.example.profit.io.ProfitUtils</exclude>
+  <!--tmp logic, tested elsewhere-->

Besides being pure review noise on a migration PR, this silently changes what each comment documents: by XML convention the reflowed comment now reads as annotating the next <exclude>, and the last one dangles with nothing after it.

Why

The XML LST has no notion of a trailing comment — the whitespace separating it from the preceding sibling lives in the comment's own prefix, so that prefix is the only carrier of the author's intent. LineBreaksVisitor.visitComment called minimumLines(..., 1) unconditionally, which prepends "\n" to any newline-free prefix; TabsAndIndentsVisitor then indented the now-multiline prefix. Latent since XML auto-format was written (0b1efb7706, 2022), and untested — grepping rewrite-xml/rewrite-maven/rewrite-gradle for </tag> <!-- returned zero fixtures.

How

Skip the line break when the comment is on the same line as a preceding sibling — i.e. its prefix contains no newline and it is not the first content of its parent tag. Existing behavior is preserved for:

  • a comment that is the first content of a tag (<dependencies><!--comment-->), which the existing LineBreaksTest#comments case asserts should reflow;
  • a comment already on its own line, which still gets re-indented;
  • prolog/misc comments, which are untouched.

Tests

  • LineBreaksTest#trailingCommentsStayOnSameLine
  • AutoFormatTest#trailingCommentStaysOnSameLine (the reproducer from the issue)
  • AutoFormatTest#trailingCommentAfterTagContentStaysOnSameLine
  • AutoFormatTest#misindentedTrailingCommentIsStillIndented (guards the contrast case)

All three new same-line tests fail on main and pass with the fix. :rewrite-xml:test, :rewrite-maven:test and :rewrite-gradle:test are green.

Not covered here

The issue also asks which recipe auto-formatted a scope far wider than it changed (OrderPomElements, AddAnnotationProcessor over the whole <plugins> block are the likely candidates). This fix stops the comment reflow regardless of the caller; the over-wide auto-format scope is a separate question.

`LineBreaksVisitor.visitComment` unconditionally prepended a newline to
every comment prefix, so `<exclude>a</exclude> <!--why a-->` was reflowed
onto its own line, silently re-associating the comment with the *next*
element. Skip the line break when the comment already sits on the same
line as a preceding sibling; a comment that is the first content of its
tag (or already on its own line) keeps the existing behavior.
@timtebeek
timtebeek marked this pull request as ready for review July 30, 2026 12:05
@timtebeek
timtebeek merged commit 99c206e into main Jul 30, 2026
1 check passed
@timtebeek
timtebeek deleted the tim/dodoma branch July 30, 2026 12:37
@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
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.
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