test(organizeimports): add failing IO tests for inline comment behavior (issue #2267)#2324
test(organizeimports): add failing IO tests for inline comment behavior (issue #2267)#2324arnavsharma990 wants to merge 8 commits intoscalacenter:mainfrom
Conversation
|
Test cases included I added failing test cases in the following files: InlineCommentMultiple.scala InlineCommentMoves.scala InlineCommentRemoved.scala These tests capture scenarios where inline comments are not preserved, shifted incorrectly, or dropped during import organization. |
|
I think the test cases look fine, we might not need that many though. |
|
Thanks for the clarification and for reviewing the tests! Also, should I start working on the actual fix for the issue now? |
|
Sure, just make sure that if you use AI that you properly vet the output and try to minimize the changes. |
bjaglin
left a comment
bjaglin
left a comment
There was a problem hiding this comment.
That's a great start 👍 I think we could add heading/trailing examples, as well as multi-line /* */ comments to have a better coverage.
Feel free to start working on implementation.
scalafix-tests/input/src/main/scala/test/organizeImports/StandaloneComment.scala
Outdated
Show resolved
Hide resolved
| import c.C | ||
|
|
||
| object InlineCommentMultiple { | ||
| val keep = 1 |
There was a problem hiding this comment.
nit: we can remove that if removeUnused is not tested
…aloneComment.scala Co-authored-by: Brice Jaglin <bjaglin@gmail.com>
Removed unused import 'c.C' to clean up code.
There was a problem hiding this comment.
Pull request overview
This PR adds new IO test cases to capture the current (incorrect) behavior of the OrganizeImports rule when handling inline comments. The tests document scenarios where inline comments are lost, reordered incorrectly, or not properly associated with their imports, establishing a baseline for future fixes to issue #2267.
- Adds fixture file
InlineCommentFixtures.scalawith test packages and classes - Creates 4 test scenarios covering standalone comments, inline comment preservation during sorting, inline comment behavior with unused import removal, and inline comment movement
- Establishes expected behavior patterns for subsequent OrganizeImports rule improvements
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scalafix-tests/shared/src/main/scala/test/organizeImports/InlineCommentFixtures.scala | Provides fixture classes in packages a, b, c, z, my.pkg, other, x, y for testing import organization with inline comments |
| scalafix-tests/input/src/main/scala/test/organizeImports/StandaloneComment.scala | Tests that standalone comments (not inline) are preserved when imports are sorted |
| scalafix-tests/output/src/main/scala/test/organizeImports/StandaloneComment.scala | Expected output showing standalone comment preserved with sorted imports |
| scalafix-tests/input/src/main/scala/test/organizeImports/InlineCommentRemoved.scala | Tests inline comment behavior when associated import is removed as unused |
| scalafix-tests/output/src/main/scala/test/organizeImports/InlineCommentRemoved.scala | Expected output showing unused import and its inline comment are both removed |
| scalafix-tests/input/src/main/scala/test/organizeImports/InlineCommentMultiple.scala | Tests preservation of multiple inline comments during import sorting |
| scalafix-tests/output/src/main/scala/test/organizeImports/InlineCommentMultiple.scala | Expected output showing inline comments move with their associated imports when sorted |
| scalafix-tests/input/src/main/scala/test/organizeImports/InlineCommentMoves.scala | Tests inline comment staying with its import when imports are reordered |
| scalafix-tests/output/src/main/scala/test/organizeImports/InlineCommentMoves.scala | Expected output showing inline comment moved with its import during sorting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scalafix-tests/input/src/main/scala/test/organizeImports/InlineCommentMultiple.scala
Show resolved
Hide resolved
scalafix-tests/input/src/main/scala/test/organizeImports/InlineCommentMoves.scala
Show resolved
Hide resolved
|
I’ve addressed the inline comment preservation issue and expanded test coverage to include: trailing // and /* */ comments leading (heading) comments mixed line and block comments correct removal of unused imports along with their comments Formatting has been applied with scalafmt, and the PR should be ready for review / merge. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scalafix-tests/output/src/main/scala/test/organizeImports/InlineCommentLeading.scala
Outdated
Show resolved
Hide resolved
scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImports.scala
Outdated
Show resolved
Hide resolved
| } | ||
| .lastOption | ||
| .collect { case c: Token.Comment => | ||
| allTokens.indexOf(c) + 1 |
There was a problem hiding this comment.
The allTokens.indexOf(c) call performs a linear search through all tokens which is inefficient. Since tokensAfter is already derived from allTokens, the index can be calculated more efficiently using lastImportTokenEnd + tokensAfter.indexOf(c).
| allTokens.indexOf(c) + 1 | |
| lastImportTokenEnd + tokensAfter.indexOf(c) + 1 |
| // Collect trailing comments for each importer before reordering | ||
| val trailingComments: Map[Int, String] = { | ||
| val comments = doc.comments | ||
| (for { | ||
| imp <- imports | ||
| importer <- imp.importers | ||
| comment <- comments.trailing(importer).headOption | ||
| } yield importer.pos.start -> (" " + comment.syntax)).toMap | ||
| } |
There was a problem hiding this comment.
This implementation only collects trailing comments (comments on the same line after an import) but does not handle leading comments (comments on the line before an import). The test case InlineCommentLeading.scala demonstrates this gap - leading comments should move with their associated imports but this implementation doesn't support that.
scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImports.scala
Outdated
Show resolved
Hide resolved
| imp <- imports | ||
| importer <- imp.importers | ||
| comment <- comments.trailing(importer).headOption | ||
| } yield importer.pos.start -> (" " + comment.syntax)).toMap |
There was a problem hiding this comment.
Using importer.pos.start as the key could lead to collisions if multiple importers have the same start position. While unlikely in normal code, using a more unique identifier or the importer itself as a key would be more robust.
| (for { | ||
| imp <- imports | ||
| importer <- imp.importers | ||
| comment <- comments.trailing(importer).headOption | ||
| } yield importer.pos.start -> (" " + comment.syntax)).toMap |
There was a problem hiding this comment.
Prepending a space character to the comment syntax is a formatting decision that should be parameterized or documented. Different comment styles (block vs line) may require different spacing.
| (for { | |
| imp <- imports | |
| importer <- imp.importers | |
| comment <- comments.trailing(importer).headOption | |
| } yield importer.pos.start -> (" " + comment.syntax)).toMap | |
| // Separator between an import and its trailing comment (single space by default) | |
| val trailingCommentSeparator = " " | |
| (for { | |
| imp <- imports | |
| importer <- imp.importers | |
| comment <- comments.trailing(importer).headOption | |
| } yield importer.pos.start -> (trailingCommentSeparator + comment.syntax)).toMap |
There was a problem hiding this comment.
Thanks for the taking a stab at the implementation @arnavsharma990 !
- Could you have a look at the remaining comments? I find them all relevant. As noted here, there is a new scalameta feature available on main that should make the implementation much simpler
- Could you update the title and the description now that this PR is actually an implementation?
…neCommentLeading.scala Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…mports.scala Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…mports.scala Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes #2267
This PR adds new IO test cases for OrganizeImports to capture the incorrect handling of inline comments (e.g., comments to the right of imports being lost, reordered incorrectly, or not associated with the correct import). These test
s encode the expected behavior so the underlying rule can later be updated to preserve inline comments consistently.
Locally, the new tests pass, but some unrelated expect*/testOnly suites occasionally behave inconsistently on my machine (sometimes showing passed, canceled, or failed). I’ve attached a screenshot in the discussion, but this inconsistency does not involve the new OrganizeImports tests themselves.
Please review the added test cases and confirm whether this structure matches what the maintainers expect before I proceed to implement the actual fix to OrganizeImports.
