Conversation
|
@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)))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fixes #6143. Replaces #8911 following the feedback on maintainability.
OrderImportscan 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
finallyblock 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
removeUnusedoption still controls unused-import cleanup.Validation:
8ca2449and pass with the fix.1311862and now pass, covering imports both with and without existing markers.rewrite-javaandrewrite-java-testsuites: 2,931 passed, 31 skipped, no failures or errors.rewrite-java-21:test: 3 passed. Java 21 parserImportTestcompatibility suite: 12 passed.git diff --checkpassed.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
testtask requires JDK 11, which is unavailable locally; the results above cover the listed Java suites.