Content mapper auto import formatting panic

[Andrew Branch (andrewbranch)]

Fixes sveltejs/language-tools#3111 (comment)

The crash was a consequence of the very sketchy comment (inherited from Strada)

/** Note: this may mutate `nodeIn`. */
func (t *Tracker) getFormattedTextOfNode(nodeIn *ast.Node, targetSourceFile *ast.SourceFile, sourceFile *ast.SourceFile, pos int, options NodeOptions) string {

This was ok-ish before because nodeIn was freshly synthesized and only used once. But with content mappers, when a node was inserted into an original text position that maps to more than one verbatim location in the virtual text, we formatted the same synthesized node in both virtual locations to make sure it formats the same way. During this process, the assignment of node positions during the first pass messes up state for the second pass. The crash is fixed just by cloning the node instead of mutating it.

However, this made me realize that the change tracker was modeling this poorly. Edits were being computed in virtual source files, then converted into original text coordinates for storage, and then converting back into virtual text coordinates for formatting, which is a lossy/ambiguous operation: original to virtual position is potentially a one-to-many. So this PR applies a refactor to keep change tracker edits in virtual coordinates until the very end when retrieving the final LSP edits.

[Copilot]

Pull request overview

Refactors change tracking to retain virtual coordinates until final LSP edit generation, preventing content-mapped auto-import formatting crashes.

Changes:

• Preserves virtual edit ranges through formatting.
• Clones nodes before assigning print positions.
• Adds hoisting and indentation regression tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tsc/internal/testutil/contentmappertest/registry.go	Registers the hoisting mapper.
tsc/internal/testutil/contentmappertest/hoisting.go	Implements the test mapper.
tsc/internal/printer/changetrackerwriter.go	Avoids mutating printed nodes.
tsc/internal/ls/change/trackerimpl.go	Formats virtual edits and performs final mapping.
tsc/internal/ls/change/tracker.go	Stores edits in virtual coordinates.
tsc/internal/ls/change/delete.go	Uses virtual ranges for deletions.
tsc/internal/fourslash/tests/importFixIndentedStatements_test.go	Tests indented import insertion.
tsc/internal/fourslash/tests/contentMapperAutoImports_test.go	Tests auto-imports at hoisted boundaries.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (2)

tsc/internal/printer/changetrackerwriter.go:145

• The added fourslash regression no longer exercises this non-mutation guarantee: with virtual ranges retained, computeNewText formats its stored edit once rather than iterating all projections. Reverting this clone would therefore still leave that test passing. Add a focused printer test that positions/prints the same synthesized tree twice, verifies identical output, and verifies the caller-owned nodes did not acquire locations or changed parent links.

	if visited == node {
		newNode = visited.Clone(v.Factory)
	}

tsc/internal/ls/change/trackerimpl.go:70

• Projection identity has already been discarded here, so projections[fileName] > 1 is too coarse. If one projection intentionally contains ordered same-position inserts (a, then b) and another projection contributes any edit—or duplicates the same sequence—this code either removes a legitimate identical insert or treats the same-projection a/b pair as a cross-projection conflict and drops the whole file. Keep the originating source file with each converted edit; preserve/validate ordering within each projection, and deduplicate or reject only edits compared across distinct projections.

		if projections[fileName] > 1 {
			textChanges = dedupeIdenticalEdits(textChanges)
			changes[fileName] = textChanges
		}

[Gabriela Araujo Britto (gabritto)]

Left a minor comment on comment clarity, but I think the approach here makes sense.