Content mapper auto import formatting panic

tsc/internal/fourslash/tests/contentMapperAutoImports_test.go

@@ -250,3 +250,68 @@ profileTi/**/
 	})
 	f.BaselineAutoImportsCompletions(t, []string{""})
 }
+
+// TestContentMapperAutoImportAtHoistedImportBoundary covers a mapper that hoists the imports of a
+// component's script above the generated render function, the way svelte2tsx does. Hoisting places the
+// script text preceding the first import *after* that import in the virtual file:
+//
+//	///<reference types="svelte" />
+//	;
+//	import { existing } from "./dep";   <- original [21, 54)
+//	function $$render() {
+//	<whitespace preceding the first import>   <- original [18, 21)
+//	  const value = help;                     <- original [54, 77)
+//
+// Original position 21 therefore has two exact virtual projections: the start of the hoisted import and
+// the end of the preceding whitespace segment. A new import sorts ahead of "./dep" and is inserted at
+// exactly that position, so the change tracker formats the same new import node once per projection.
+// Printing assigns source positions to the node, so the second print reads the module specifier back out
+// of the virtual file at those stale offsets and yields text like `import { helper } from om "./de;`,
+// which then trips a formatter assertion.
+func TestContentMapperAutoImportAtHoistedImportBoundary(t *testing.T) {
+	t.Parallel()
+	defer testutil.RecoverAndFail(t, "Panic on fourslash test")
+	f, done := newContentMapperFourslash(t, `// @Filename: /aaa.ts
+export const helper = 1;
+
+// @Filename: /dep.ts
+export const existing = 2;
+
+// @Filename: /App.svelte
+<script lang="ts">
+  import { existing } from "./dep";
+  const value = help/**/;
+</script>
+`, contentmappertest.HoistingMapper, ".svelte")
+	defer done()
+
+	f.VerifyCompletions(t, "", &fourslash.CompletionsExpectedList{
+		UserPreferences: &lsutil.UserPreferences{
+			IncludeCompletionsForModuleExports:    core.TSTrue,
+			IncludeCompletionsForImportStatements: core.TSTrue,
+		},
+		ItemDefaults: &fourslash.CompletionsExpectedItemDefaults{
+			CommitCharacters: &DefaultCommitCharacters,
+			EditRange:        Ignored,
+		},
+		Items: &fourslash.CompletionsExpectedItems{Includes: []fourslash.CompletionsExpectedItem{
+			&lsproto.CompletionItem{
+				Label:               "helper",
+				SortText:            new(string(ls.SortTextAutoImportSuggestions)),
+				Data:                &lsproto.CompletionItemData{AutoImport: &lsproto.AutoImportFix{ModuleSpecifier: "./aaa"}},
+				AdditionalTextEdits: fourslash.AnyTextEdits,
+			},
+		}},
+	})
+	f.VerifyApplyCodeActionFromCompletion(t, new(""), &fourslash.ApplyCodeActionFromCompletionOptions{
+		Name:        "helper",
+		Source:      "./aaa",
+		Description: `Add import from "./aaa"`,
+		NewFileContent: new(`<script lang="ts">
+  import { helper } from "./aaa";
+  import { existing } from "./dep";
+  const value = help;
+</script>
+`),
+	})
+}

tsc/internal/fourslash/tests/contentMapperDuplicateProjections_test.go

@@ -0,0 +1,31 @@
+package fourslash_test
+
+import (
+	"testing"
+
+	"github.com/microsoft/TypeScript/tsc/internal/testutil"
+	"github.com/microsoft/TypeScript/tsc/internal/testutil/contentmappertest"
+)
+
+// A mapper may emit the same original text in more than one virtual output. Each output is a separate
+// source file to the change tracker, but they share an original file, so an edit to a span present in
+// both is recorded twice. The two edits describe the same change to the same original range, and applying
+// it more than once would corrupt the file, so only one may reach the client.
+func TestContentMapperFileRenameAcrossDuplicateProjections(t *testing.T) {
+	t.Parallel()
+	defer testutil.RecoverAndFail(t, "Panic on fourslash test")
+	f, done := newContentMapperFourslash(t, `// @Filename: /dep.ts
+export const helper = 1;
+
+// @Filename: /app.astro
+import { helper } from "./dep";
+helper;
+`, contentmappertest.DuplicateProjectionMapper, ".astro")
+	defer done()
+
+	f.VerifyWillRenameFilesEdits(t, "/dep.ts", "/renamed.ts", map[string]string{
+		"/app.astro": `import { helper } from "./renamed";
+helper;
+`,
+	}, nil)
+}

tsc/internal/fourslash/tests/importFixIndentedStatements_test.go

@@ -0,0 +1,101 @@
+package fourslash_test
+
+import (
+	"testing"
+
+	"github.com/microsoft/TypeScript/tsc/internal/fourslash"
+	"github.com/microsoft/TypeScript/tsc/internal/testutil"
+)
+
+// An added import has to line up with the imports already in the file. The formatter indents a new
+// top-level statement to column zero, since that is where a top-level statement canonically belongs, so
+// the surrounding indentation has to be reapplied when the file does something else. Statements are only
+// indented like this in hand-written TypeScript by accident, but it is the normal shape of the virtual
+// file a content mapper produces for an indented `<script>` block.
+//
+// The two tests below cover the two sides of the insertion point: the indentation can sit before it or
+// after it, and it has to end up on both lines either way.
+
+// The new import sorts first, so it is inserted directly after the existing import's indentation.
+func TestImportFixBeforeIndentedImport(t *testing.T) {
+	t.Parallel()
+	defer testutil.RecoverAndFail(t, "Panic on fourslash test")
+	f, done := fourslash.NewFourslashWithOptions(t, `// @Filename: /aaa.ts
+export const helper = 1;
+
+// @Filename: /dep.ts
+export const existing = 2;
+
+// @Filename: /main.ts
+// header
+  import { existing } from "./dep";
+  const value = help/**/;
+`, &fourslash.FourslashOptions{})
+	defer done()
+
+	f.VerifyApplyCodeActionFromCompletion(t, new(""), &fourslash.ApplyCodeActionFromCompletionOptions{
+		Name:        "helper",
+		Source:      "./aaa",
+		Description: `Add import from "./aaa"`,
+		NewFileContent: new(`// header
+  import { helper } from "./aaa";
+  import { existing } from "./dep";
+  const value = help;
+`),
+	})
+}
+
+// The new import sorts last, so it is inserted at the start of the line following the existing import.
+func TestImportFixAfterIndentedImport(t *testing.T) {
+	t.Parallel()
+	defer testutil.RecoverAndFail(t, "Panic on fourslash test")
+	f, done := fourslash.NewFourslashWithOptions(t, `// @Filename: /aaa.ts
+export const existing = 2;
+
+// @Filename: /zzz.ts
+export const helper = 1;
+
+// @Filename: /main.ts
+// header
+  import { existing } from "./aaa";
+  const value = help/**/;
+`, &fourslash.FourslashOptions{})
+	defer done()
+
+	f.VerifyApplyCodeActionFromCompletion(t, new(""), &fourslash.ApplyCodeActionFromCompletionOptions{
+		Name:        "helper",
+		Source:      "./zzz",
+		Description: `Add import from "./zzz"`,
+		NewFileContent: new(`// header
+  import { existing } from "./aaa";
+  import { helper } from "./zzz";
+  const value = help;
+`),
+	})
+}
+
+// The scanner treats a lone carriage return as a line terminator, so a file that uses CR endings has real
+// lines and real indentation, and an inserted import has to respect them.
+func TestImportFixBeforeIndentedImportWithCarriageReturns(t *testing.T) {
+	t.Parallel()
+	defer testutil.RecoverAndFail(t, "Panic on fourslash test")
+	f, done := fourslash.NewFourslashWithOptions(t, `// @Filename: /aaa.ts
+export const helper = 1;
+
+// @Filename: /dep.ts
+export const existing = 2;
+
+// @Filename: /main.ts
+`+"// header\r  import { existing } from \"./dep\";\r  const value = help/**/;\r",
+		&fourslash.FourslashOptions{})
+	defer done()
+
+	// The inserted line is separated with the tracker's newline rather than the file's, which is a
+	// pre-existing behavior unrelated to indentation; what matters here is that both imports stay indented.
+	f.VerifyApplyCodeActionFromCompletion(t, new(""), &fourslash.ApplyCodeActionFromCompletionOptions{
+		Name:           "helper",
+		Source:         "./aaa",
+		Description:    `Add import from "./aaa"`,
+		NewFileContent: new("// header\r  import { helper } from \"./aaa\";\n  import { existing } from \"./dep\";\r  const value = help;\r"),
+	})
+}

tsc/internal/ls/change/delete.go

@@ -24,7 +24,7 @@ func deleteDeclaration(t *Tracker, deletedNodesInLists map[*ast.Node]bool, sourc
 			// Lambdas with exactly one parameter are special because, after removal, there
 			// must be an empty parameter list (i.e. `()`) and this won't necessarily be the
 			// case if the parameter is simply removed (e.g. in `x => 1`).
-			t.ReplaceRangeWithText(sourceFile, t.GetAdjustedRange(sourceFile, node, node, LeadingTriviaOptionIncludeAll, TrailingTriviaOptionInclude), "()")
+			t.ReplaceTextRangeWithText(sourceFile, t.GetAdjustedRange(sourceFile, node, node, LeadingTriviaOptionIncludeAll, TrailingTriviaOptionInclude), "()")
 		} else {
 			deleteNodeInList(t, deletedNodesInLists, sourceFile, node)
 		}
@@ -114,7 +114,7 @@ func deleteDefaultImport(t *Tracker, sourceFile *ast.SourceFile, importClause *a
 		if nextToken != nil && nextToken.Kind == ast.KindCommaToken {
 			// shift first non-whitespace position after comma to the start position of the node
 			end := scanner.SkipTriviaEx(sourceFile.Text(), nextToken.End(), &scanner.SkipTriviaOptions{StopAfterLineBreak: false, StopAtComments: true})
-			t.ReplaceRangeWithText(sourceFile, t.toLSPEditRange(sourceFile, core.NewTextRange(start, end)), "")
+			t.ReplaceTextRangeWithText(sourceFile, core.NewTextRange(start, end), "")
 		} else {
 			deleteNode(t, sourceFile, name, LeadingTriviaOptionIncludeAll, TrailingTriviaOptionInclude)
 		}
@@ -130,7 +130,7 @@ func deleteImportBinding(t *Tracker, sourceFile *ast.SourceFile, node *ast.Node)
 		previousToken := astnav.GetTokenAtPosition(sourceFile, node.Pos()-1)
 		debug.Assert(previousToken != nil, "previousToken should not be nil")
 		start := astnav.GetStartOfNode(previousToken, sourceFile, false)
-		t.ReplaceRangeWithText(sourceFile, t.toLSPEditRange(sourceFile, core.NewTextRange(start, node.End())), "")
+		t.ReplaceTextRangeWithText(sourceFile, core.NewTextRange(start, node.End()), "")
 	} else {
 		// Delete the entire import declaration
 		// |import * as ns from './file'|
@@ -183,7 +183,7 @@ func deleteVariableDeclaration(t *Tracker, deletedNodesInLists map[*ast.Node]boo
 func deleteNode(t *Tracker, sourceFile *ast.SourceFile, node *ast.Node, leadingTrivia LeadingTriviaOption, trailingTrivia TrailingTriviaOption) {
 	startPosition := t.getAdjustedStartPosition(sourceFile, node, leadingTrivia, false)
 	endPosition := t.getAdjustedEndPosition(sourceFile, node, trailingTrivia)
-	t.ReplaceRangeWithText(sourceFile, t.toLSPEditRange(sourceFile, core.NewTextRange(startPosition, endPosition)), "")
+	t.ReplaceTextRangeWithText(sourceFile, core.NewTextRange(startPosition, endPosition), "")
 }
 
 func deleteNodeInList(t *Tracker, deletedNodesInLists map[*ast.Node]bool, sourceFile *ast.SourceFile, node *ast.Node) {
@@ -214,7 +214,7 @@ func deleteNodeInList(t *Tracker, deletedNodesInLists map[*ast.Node]bool, source
 		endPos = t.endPositionToDeleteNodeInList(sourceFile, node, prevNode, containingList.Nodes[index+1])
 	}
 
-	t.ReplaceRangeWithText(sourceFile, t.toLSPEditRange(sourceFile, core.NewTextRange(startPos, endPos)), "")
+	t.ReplaceTextRangeWithText(sourceFile, core.NewTextRange(startPos, endPos), "")
 }
 
 // startPositionToDeleteNodeInList finds the first non-whitespace position in the leading trivia of the node