Skip to content

Preserve Java import comments through ordering and folding - #8964

Open
Niloyyy wants to merge 2 commits into
openrewrite:mainfrom
Niloyyy:fix/order-import-comment-association
Open

Niloyyy wants to merge 2 commits into
openrewrite:mainfrom
Niloyyy:fix/order-import-comment-association

Conversation

@Niloyyy

@Niloyyy Niloyyy commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #6143. Replaces #8911 following the feedback on maintainability.

OrderImports can move an end-of-line comment to a different import, lose a leading comment when its import moves first, or duplicate an import-section header. The reported example is included verbatim, with both LF and CRLF endings.

A helper holds the section header separately and associates trailing comments with import IDs. This transient state is shared through a cursor message scoped to one source-file visit and cleared in a finally block after ordering and the scheduled cleanup visitors finish. Import markers are never changed to carry this state. An unchanged result returns the original compilation unit.

Comments from imports being folded are transferred at the existing fold site, where the import group is already available. When unused-import cleanup expands a wildcard, it associates the trailing comment with the last replacement at the expansion site. There is no qualifier matching or reconstruction of import groups. The existing threshold, configured-package, and existing-wildcard folding conditions are unchanged.

When folding removes an individual import, its comments move above the surviving wildcard; the surviving import's own trailing comment stays inline. Ordering retains duplicate imports with trailing comments so de-duplication does not discard those comments. The existing removeUnused option still controls unused-import cleanup.

Validation:

  • 15 reproduced comment cases failed on unchanged upstream 8ca2449 and pass with the fix.
  • Two marker-identity regression cases failed on the previously submitted commit 1311862 and now pass, covering imports both with and without existing markers.
  • All 30 comment tests and 33 existing import-ordering tests pass. Coverage includes unchanged compilation-unit identity with and without unused-import cleanup, separate comment state across source files, ordinary and static wildcard folding and expansion, headers, duplicates, EOF, and LF/CRLF.
  • Full rewrite-java and rewrite-java-test suites: 2,931 passed, 31 skipped, no failures or errors.
  • Full rewrite-java-21:test: 3 passed. Java 21 parser ImportTest compatibility suite: 12 passed.
  • License formatting and git diff --check passed.

Local validation used the temporary public build-plugin 2.23.1 override because the default plugin requires Code Genome credentials. No build configuration changes are included. The repository-wide test task requires JDK 11, which is unavailable locally; the results above cover the listed Java suites.

@Niloyyy

Niloyyy commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor Author

@jkschneider would like to request you to review this PR

}
J.Import anImport = imports.get(i);
Space trailing = Space.build(next.getWhitespace(), next.getComments().subList(0, count));
imports.set(i, anImport.withMarkers(anImport.getMarkers().add(new Trailing(randomId(), trailing))));

@jkschneider jkschneider Sep 27, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The implementation is overall better than the first attempt in terms of not adding a lot of complexity to the AddImport visitor, but...

I'd rather not manipulate markers here, because it does change the Markers instance regardless of whether it is later cleared, and that referential change is seen as a change by the top level recipe.

In general in OpenRewrite, we don't pass state via markers. Rather cursor messaging is designed to hold transient state per SourceFile visit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for explaining the marker identity issue. Addressed in 012eaef: the transient comment state now uses a cursor message scoped to each source-file visit, including the import cleanup visitor, with cleanup in a finally block. The temporary marker implementation has been removed.

I added regression tests that assert the original Markers instances are preserved, both with and without existing markers, plus checks for compilation-unit identity when ordering makes no changes and independent state across source files. The marker identity regressions fail against the previous revision and pass with this change. The Java module tests and ImportTest compatibility tests pass, and CI is green on the updated commit.

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

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

OrderImports incorrectly moves comments between import statements during reordering

2 participants