Organize Imports

[johnfav03]

Implementation of organize imports

[jakebailey]

I haven't yet read the main code, but here are some quick ones in other files.

I would unmark this as draft if it's ready for review (then you'll get a copilot review that could note things too).

[jakebailey]

It's sort of spooky that this is needed; my assumption here was that the node is not the one from the original source file, so the text won't line up, and therefore we want empty so that we know when we are mistakenly accessing it. Is that not the case here?

[iisaduan]

I agree, very spooky. What goes wrong if you dont do this change?

[johnfav03]

nodeIn is passed to Write(), and further down the call stack functions like SkipTrivia() use p.currentSourceFile which tries to index the empty text using the old positions, which results in out of bounds errors on many tests. I'm investigating an alternate solution that leaves the empty string unmodified

[jakebailey]

But are the nodes we pass in actually from the real tree? If they're synthetic, the positions won't map to the text, I don't think. (Is this something that the original code did?)

[johnfav03]

Yes, this was something the original code did - I just discovered it after using the tracker in my code

[iisaduan]

Organize imports uses the nodes that existed in the old tree since it never has to generate new nodes. This allows us to re-emit nodes while keeping comments (and other benefits)

I took a closer look, and I think the change is generally correct. While this specifically isn't necessary for organize imports to work correctly (we could do something else to skip name generation), it's probably the easiest way to fix it and should hold up for future codefixes that reuse nodes

[iisaduan]

Suggestion to

• change the if statement to something like !ast.IsSynthesized() or node.Pos != -1 to make it more clear that it's testing if the node is a real node or not
• add a comment with some explanation (perhaps routing back to this PR). I think there's a chance that some of the code in the latter half of the function might need to be updated because of this change, but we wouldn't know until we hit a specific case (a mix of synthesized and old nodes in nodeIn)

[andrewbranch]

Yeah, I think this is right. At least in Strada, the emitter is very smart about detecting per-node whether it can take a slice of the source file text or if it needs to be printed fresh.

[Copilot]

Pull request overview

Implements “Organize Imports” end-to-end in the language server, including import removal/coalescing/sorting logic and integration via LSP code actions, with extensive fourslash coverage to validate behaviors (type-only ordering, unicode collation, grouping by blank lines, import attributes, JSX cases).

Changes:

• Added a full organize-imports engine (remove unused, coalesce, sort) and export coalescing support.
• Integrated organize-imports as LSP textDocument/codeAction source actions (including mode variants).
• Extended change tracking + printer behavior and added a large suite of generated fourslash tests plus conversion-script support.

Reviewed changes

Copilot reviewed 61 out of 61 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/printer/printer.go	Adjusts list formatting flags to better support multi-line/indented output during rewritten imports.
internal/ls/organizeimports/organizeimports.go	Core organize-imports implementation (remove unused, coalesce, sort; plus export coalescing and preference-driven compare/detection).
internal/ls/codeactions.go	Exposes organize-imports (and variants) as LSP source code actions.
internal/ls/change/trackerimpl.go	Improves statement printing by providing source text when available during change tracking.
internal/ls/change/tracker.go	Adds helper to replace one node with multiple nodes (used by organize-imports rewrites).
internal/checker/exports.go	Adds GetJsxFragmentFactory support needed for JSX-related unused-import detection.
internal/fourslash/fourslash.go	Adds a fourslash helper to invoke/verify organize-imports via code actions.
internal/fourslash/_scripts/convertFourslash.mts	Adds conversion support for verify.organizeImports(...) into Go fourslash tests.
internal/fourslash/tests/gen/organizeImports_removeOnly_test.go	Tests remove-unused-only mode behavior.
internal/fourslash/tests/gen/organizeImportsUnicode4_test.go	Tests unicode collation with case-first preferences for named imports.
internal/fourslash/tests/gen/organizeImportsUnicode3_test.go	Tests unicode accent collation behavior for named imports.
internal/fourslash/tests/gen/organizeImportsUnicode2_test.go	Tests unicode numeric collation behavior for named imports.
internal/fourslash/tests/gen/organizeImportsUnicode1_test.go	Tests ordinal vs unicode collation behavior for named imports.
internal/fourslash/tests/gen/organizeImportsType9_test.go	Tests type-order variants for mixed type/value named imports.
internal/fourslash/tests/gen/organizeImportsType8_test.go	Tests type-order variants for mixed type/value named imports (different input ordering).
internal/fourslash/tests/gen/organizeImportsType7_test.go	Tests inline type-order behavior under different ignore-case settings.
internal/fourslash/tests/gen/organizeImportsType6_test.go	Tests inline type-order behavior under different ignore-case settings (type-first element).
internal/fourslash/tests/gen/organizeImportsType5_test.go	Tests multi-line named import sorting with type-only and aliases.
internal/fourslash/tests/gen/organizeImportsType4_test.go	Tests inline type-order with ignore-case true.
internal/fourslash/tests/gen/organizeImportsType3_test.go	Tests inline type-order with ignore-case false.
internal/fourslash/tests/gen/organizeImportsType2_test.go	Tests export specifier organization with type-order options.
internal/fourslash/tests/gen/organizeImportsType1_test.go	Tests coalescing multiple imports from same module with type-only + type-order options.
internal/fourslash/tests/gen/organizeImportsType11_test.go	Tests large named-import sorting stability/ordering.
internal/fourslash/tests/gen/organizeImportsType10_test.go	Tests large named-import sorting with ignore-case true.
internal/fourslash/tests/gen/organizeImportsReactJsx_test.go	Tests unused import removal in react-jsx mode.
internal/fourslash/tests/gen/organizeImportsReactJsxDev_test.go	Tests unused import removal in react-jsxdev mode.
internal/fourslash/tests/gen/organizeImportsPathsUnicode4_test.go	Tests unicode collation for module specifiers (paths) with case-first preferences.
internal/fourslash/tests/gen/organizeImportsPathsUnicode3_test.go	Tests unicode accent collation for module specifiers (paths).
internal/fourslash/tests/gen/organizeImportsPathsUnicode2_test.go	Tests unicode numeric collation for module specifiers (paths).
internal/fourslash/tests/gen/organizeImportsPathsUnicode1_test.go	Tests ordinal vs unicode collation for module specifiers (paths).
internal/fourslash/tests/gen/organizeImportsGroup_Newline_test.go	Tests grouping behavior across blank-line-separated import blocks.
internal/fourslash/tests/gen/organizeImportsGroup_MultilineCommentInNewline_test.go	Tests grouping behavior when a multiline comment exists between imports.
internal/fourslash/tests/gen/organizeImportsGroup_MultiNewlines_test.go	Tests grouping behavior with multiple blank lines between import blocks.
internal/fourslash/tests/gen/organizeImportsGroup_CommentInNewline_test.go	Tests grouping behavior when line comments exist between imports.
internal/fourslash/tests/gen/organizeImportsAttributes_test.go	Tests coalescing/sorting across different import-attribute forms (assert/with).
internal/fourslash/tests/gen/organizeImportsAttributes4_test.go	Tests attribute-key normalization/sorting for attributes with multiple keys and value types.
internal/fourslash/tests/gen/organizeImportsAttributes3_test.go	Tests attribute normalization with comments/spacing and grouping by attribute identity.
internal/fourslash/tests/gen/organizeImportsAttributes2_test.go	Tests attribute grouping across assert vs with and module differences.
internal/fourslash/tests/gen/organizeImports9_test.go	Tests normalization of redundant aliases (a as a, d as d) plus unused removal.
internal/fourslash/tests/gen/organizeImports8_test.go	Tests normalization of foo as foo and unused removal behavior.
internal/fourslash/tests/gen/organizeImports7_test.go	Tests comment preservation/attachment during sorting.
internal/fourslash/tests/gen/organizeImports6_test.go	Tests comment/trivia handling and unused removal across multiple imports.
internal/fourslash/tests/gen/organizeImports5_test.go	Tests full-file removal output case.
internal/fourslash/tests/gen/organizeImports4_test.go	Tests full-file removal output case (variant).
internal/fourslash/tests/gen/organizeImports3_test.go	Tests formatting normalization in oddly-formatted multi-line named imports.
internal/fourslash/tests/gen/organizeImports2_test.go	Tests formatting normalization in oddly-formatted multi-line named imports (variant).
internal/fourslash/tests/gen/organizeImports23_test.go	Tests sorting between imports plus named specifier sorting with type-only elements.
internal/fourslash/tests/gen/organizeImports22_test.go	Tests sorting between imports plus named specifier sorting (non-type-only).
internal/fourslash/tests/gen/organizeImports21_test.go	Tests export specifier formatting/trailing comma adjustments with JSDoc/deprecation comments.
internal/fourslash/tests/gen/organizeImports20_test.go	Tests coalescing multiple export declarations into a single export list.
internal/fourslash/tests/gen/organizeImports19_test.go	Tests preserving separated export groups (should not coalesce across gaps).
internal/fourslash/tests/gen/organizeImports18_test.go	Tests sorting/coalescing re-exports across multiple modules with grouping.
internal/fourslash/tests/gen/organizeImports17_test.go	Tests module-specifier sorting with detected case-sensitivity preferences.
internal/fourslash/tests/gen/organizeImports16_test.go	Tests ignore-case variants for named-import sorting.
internal/fourslash/tests/gen/organizeImports15_test.go	Tests header comment preservation when imports are removed.
internal/fourslash/tests/gen/organizeImports14_test.go	Tests header comment preservation when duplicate imports are removed.
internal/fourslash/tests/gen/organizeImports13_test.go	Tests large named-import sorting behavior under ignore-case variants.
internal/fourslash/tests/gen/organizeImports12_test.go	Tests sorting of declare export forms.
internal/fourslash/tests/gen/organizeImports11_test.go	Tests unused removal with type references inside JSDoc ({@link ...}).
internal/fourslash/tests/gen/organizeImports10_test.go	Tests preserving type-only imports referenced from JSDoc text.
internal/fourslash/tests/gen/organizeImports1_test.go	Tests coalescing multiple imports from the same module into a single sorted multi-line import.

Comments suppressed due to low confidence (1)

internal/ls/codeactions.go:126

• When params.Context.Only is provided, the LSP expects the server to return only matching kinds. This implementation still always generates diagnostic quick-fix actions regardless of Only, so a request for only source actions (e.g. ["source.organizeImports"]) can incorrectly return quick fixes too. Consider gating the diagnostics loop on Only == nil or on Only containing quickfix (or a matching prefix).

[iisaduan]

I'm still working through my in-depth pass through the code, but I have some concerns about package organization and shared code

[iisaduan]

Do we want to continue supporting these? SortAndCombine and RemoveUnused aren't (currently) triggerable in the ts-go extension.
We also had a separate (not yet ported) codefix to "remove unused imports" which would cover the RemoveUnused case (the more popular sub command).
@andrewbranch @mjbvz

Discussed offline--We will continue to have the SortAndCombine and RemoveUnused actions. The change requested is to register them so they are user-invokable

[iisaduan]

:D