Skip to content

Commit e26b8d2

Browse files
andrewbranchandrewbranch
authored
Content mapper auto import formatting panic (#64042)
1 parent 8330d40 commit e26b8d2

11 files changed

Lines changed: 609 additions & 115 deletions

‎tsc/internal/fourslash/tests/contentMapperAutoImports_test.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,3 +250,68 @@ profileTi/**/
250250
})
251251
f.BaselineAutoImportsCompletions(t, []string{""})
252252
}
253+
254+
// TestContentMapperAutoImportAtHoistedImportBoundary covers a mapper that hoists the imports of a
255+
// component's script above the generated render function, the way svelte2tsx does. Hoisting places the
256+
// script text preceding the first import *after* that import in the virtual file:
257+
//
258+
// ///<reference types="svelte" />
259+
// ;
260+
// import { existing } from "./dep"; <- original [21, 54)
261+
// function $$render() {
262+
// <whitespace preceding the first import> <- original [18, 21)
263+
// const value = help; <- original [54, 77)
264+
//
265+
// Original position 21 therefore has two exact virtual projections: the start of the hoisted import and
266+
// the end of the preceding whitespace segment. A new import sorts ahead of "./dep" and is inserted at
267+
// exactly that position, so the change tracker formats the same new import node once per projection.
268+
// Printing assigns source positions to the node, so the second print reads the module specifier back out
269+
// of the virtual file at those stale offsets and yields text like `import { helper } from om "./de;`,
270+
// which then trips a formatter assertion.
271+
func TestContentMapperAutoImportAtHoistedImportBoundary(t *testing.T) {
272+
t.Parallel()
273+
defer testutil.RecoverAndFail(t, "Panic on fourslash test")
274+
f, done := newContentMapperFourslash(t, `// @Filename: /aaa.ts
275+
export const helper = 1;
276+
277+
// @Filename: /dep.ts
278+
export const existing = 2;
279+
280+
// @Filename: /App.svelte
281+
<script lang="ts">
282+
import { existing } from "./dep";
283+
const value = help/**/;
284+
</script>
285+
`, contentmappertest.HoistingMapper, ".svelte")
286+
defer done()
287+
288+
f.VerifyCompletions(t, "", &fourslash.CompletionsExpectedList{
289+
UserPreferences: &lsutil.UserPreferences{
290+
IncludeCompletionsForModuleExports: core.TSTrue,
291+
IncludeCompletionsForImportStatements: core.TSTrue,
292+
},
293+
ItemDefaults: &fourslash.CompletionsExpectedItemDefaults{
294+
CommitCharacters: &DefaultCommitCharacters,
295+
EditRange: Ignored,
296+
},
297+
Items: &fourslash.CompletionsExpectedItems{Includes: []fourslash.CompletionsExpectedItem{
298+
&lsproto.CompletionItem{
299+
Label: "helper",
300+
SortText: new(string(ls.SortTextAutoImportSuggestions)),
301+
Data: &lsproto.CompletionItemData{AutoImport: &lsproto.AutoImportFix{ModuleSpecifier: "./aaa"}},
302+
AdditionalTextEdits: fourslash.AnyTextEdits,
303+
},
304+
}},
305+
})
306+
f.VerifyApplyCodeActionFromCompletion(t, new(""), &fourslash.ApplyCodeActionFromCompletionOptions{
307+
Name: "helper",
308+
Source: "./aaa",
309+
Description: `Add import from "./aaa"`,
310+
NewFileContent: new(`<script lang="ts">
311+
import { helper } from "./aaa";
312+
import { existing } from "./dep";
313+
const value = help;
314+
</script>
315+
`),
316+
})
317+
}

‎tsc/internal/fourslash/tests/contentMapperDuplicateProjections_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
package fourslash_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/microsoft/TypeScript/tsc/internal/testutil"
7+
"github.com/microsoft/TypeScript/tsc/internal/testutil/contentmappertest"
8+
)
9+
10+
// A mapper may emit the same original text in more than one virtual output. Each output is a separate
11+
// source file to the change tracker, but they share an original file, so an edit to a span present in
12+
// both is recorded twice. The two edits describe the same change to the same original range, and applying
13+
// it more than once would corrupt the file, so only one may reach the client.
14+
func TestContentMapperFileRenameAcrossDuplicateProjections(t *testing.T) {
15+
t.Parallel()
16+
defer testutil.RecoverAndFail(t, "Panic on fourslash test")
17+
f, done := newContentMapperFourslash(t, `// @Filename: /dep.ts
18+
export const helper = 1;
19+
20+
// @Filename: /app.astro
21+
import { helper } from "./dep";
22+
helper;
23+
`, contentmappertest.DuplicateProjectionMapper, ".astro")
24+
defer done()
25+
26+
f.VerifyWillRenameFilesEdits(t, "/dep.ts", "/renamed.ts", map[string]string{
27+
"/app.astro": `import { helper } from "./renamed";
28+
helper;
29+
`,
30+
}, nil)
31+
}

‎tsc/internal/fourslash/tests/importFixIndentedStatements_test.go‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
package fourslash_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/microsoft/TypeScript/tsc/internal/fourslash"
7+
"github.com/microsoft/TypeScript/tsc/internal/testutil"
8+
)
9+
10+
// An added import has to line up with the imports already in the file. The formatter indents a new
11+
// top-level statement to column zero, since that is where a top-level statement canonically belongs, so
12+
// the surrounding indentation has to be reapplied when the file does something else. Statements are only
13+
// indented like this in hand-written TypeScript by accident, but it is the normal shape of the virtual
14+
// file a content mapper produces for an indented `<script>` block.
15+
//
16+
// The two tests below cover the two sides of the insertion point: the indentation can sit before it or
17+
// after it, and it has to end up on both lines either way.
18+
19+
// The new import sorts first, so it is inserted directly after the existing import's indentation.
20+
func TestImportFixBeforeIndentedImport(t *testing.T) {
21+
t.Parallel()
22+
defer testutil.RecoverAndFail(t, "Panic on fourslash test")
23+
f, done := fourslash.NewFourslashWithOptions(t, `// @Filename: /aaa.ts
24+
export const helper = 1;
25+
26+
// @Filename: /dep.ts
27+
export const existing = 2;
28+
29+
// @Filename: /main.ts
30+
// header
31+
import { existing } from "./dep";
32+
const value = help/**/;
33+
`, &fourslash.FourslashOptions{})
34+
defer done()
35+
36+
f.VerifyApplyCodeActionFromCompletion(t, new(""), &fourslash.ApplyCodeActionFromCompletionOptions{
37+
Name: "helper",
38+
Source: "./aaa",
39+
Description: `Add import from "./aaa"`,
40+
NewFileContent: new(`// header
41+
import { helper } from "./aaa";
42+
import { existing } from "./dep";
43+
const value = help;
44+
`),
45+
})
46+
}
47+
48+
// The new import sorts last, so it is inserted at the start of the line following the existing import.
49+
func TestImportFixAfterIndentedImport(t *testing.T) {
50+
t.Parallel()
51+
defer testutil.RecoverAndFail(t, "Panic on fourslash test")
52+
f, done := fourslash.NewFourslashWithOptions(t, `// @Filename: /aaa.ts
53+
export const existing = 2;
54+
55+
// @Filename: /zzz.ts
56+
export const helper = 1;
57+
58+
// @Filename: /main.ts
59+
// header
60+
import { existing } from "./aaa";
61+
const value = help/**/;
62+
`, &fourslash.FourslashOptions{})
63+
defer done()
64+
65+
f.VerifyApplyCodeActionFromCompletion(t, new(""), &fourslash.ApplyCodeActionFromCompletionOptions{
66+
Name: "helper",
67+
Source: "./zzz",
68+
Description: `Add import from "./zzz"`,
69+
NewFileContent: new(`// header
70+
import { existing } from "./aaa";
71+
import { helper } from "./zzz";
72+
const value = help;
73+
`),
74+
})
75+
}
76+
77+
// The scanner treats a lone carriage return as a line terminator, so a file that uses CR endings has real
78+
// lines and real indentation, and an inserted import has to respect them.
79+
func TestImportFixBeforeIndentedImportWithCarriageReturns(t *testing.T) {
80+
t.Parallel()
81+
defer testutil.RecoverAndFail(t, "Panic on fourslash test")
82+
f, done := fourslash.NewFourslashWithOptions(t, `// @Filename: /aaa.ts
83+
export const helper = 1;
84+
85+
// @Filename: /dep.ts
86+
export const existing = 2;
87+
88+
// @Filename: /main.ts
89+
`+"// header\r import { existing } from \"./dep\";\r const value = help/**/;\r",
90+
&fourslash.FourslashOptions{})
91+
defer done()
92+
93+
// The inserted line is separated with the tracker's newline rather than the file's, which is a
94+
// pre-existing behavior unrelated to indentation; what matters here is that both imports stay indented.
95+
f.VerifyApplyCodeActionFromCompletion(t, new(""), &fourslash.ApplyCodeActionFromCompletionOptions{
96+
Name: "helper",
97+
Source: "./aaa",
98+
Description: `Add import from "./aaa"`,
99+
NewFileContent: new("// header\r import { helper } from \"./aaa\";\n import { existing } from \"./dep\";\r const value = help;\r"),
100+
})
101+
}

‎tsc/internal/ls/change/delete.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ func deleteDeclaration(t *Tracker, deletedNodesInLists map[*ast.Node]bool, sourc
2424
// Lambdas with exactly one parameter are special because, after removal, there
2525
// must be an empty parameter list (i.e. `()`) and this won't necessarily be the
2626
// case if the parameter is simply removed (e.g. in `x => 1`).
27-
t.ReplaceRangeWithText(sourceFile, t.GetAdjustedRange(sourceFile, node, node, LeadingTriviaOptionIncludeAll, TrailingTriviaOptionInclude), "()")
27+
t.ReplaceTextRangeWithText(sourceFile, t.GetAdjustedRange(sourceFile, node, node, LeadingTriviaOptionIncludeAll, TrailingTriviaOptionInclude), "()")
2828
} else {
2929
deleteNodeInList(t, deletedNodesInLists, sourceFile, node)
3030
}
@@ -114,7 +114,7 @@ func deleteDefaultImport(t *Tracker, sourceFile *ast.SourceFile, importClause *a
114114
if nextToken != nil && nextToken.Kind == ast.KindCommaToken {
115115
// shift first non-whitespace position after comma to the start position of the node
116116
end := scanner.SkipTriviaEx(sourceFile.Text(), nextToken.End(), &scanner.SkipTriviaOptions{StopAfterLineBreak: false, StopAtComments: true})
117-
t.ReplaceRangeWithText(sourceFile, t.toLSPEditRange(sourceFile, core.NewTextRange(start, end)), "")
117+
t.ReplaceTextRangeWithText(sourceFile, core.NewTextRange(start, end), "")
118118
} else {
119119
deleteNode(t, sourceFile, name, LeadingTriviaOptionIncludeAll, TrailingTriviaOptionInclude)
120120
}
@@ -130,7 +130,7 @@ func deleteImportBinding(t *Tracker, sourceFile *ast.SourceFile, node *ast.Node)
130130
previousToken := astnav.GetTokenAtPosition(sourceFile, node.Pos()-1)
131131
debug.Assert(previousToken != nil, "previousToken should not be nil")
132132
start := astnav.GetStartOfNode(previousToken, sourceFile, false)
133-
t.ReplaceRangeWithText(sourceFile, t.toLSPEditRange(sourceFile, core.NewTextRange(start, node.End())), "")
133+
t.ReplaceTextRangeWithText(sourceFile, core.NewTextRange(start, node.End()), "")
134134
} else {
135135
// Delete the entire import declaration
136136
// |import * as ns from './file'|
@@ -183,7 +183,7 @@ func deleteVariableDeclaration(t *Tracker, deletedNodesInLists map[*ast.Node]boo
183183
func deleteNode(t *Tracker, sourceFile *ast.SourceFile, node *ast.Node, leadingTrivia LeadingTriviaOption, trailingTrivia TrailingTriviaOption) {
184184
startPosition := t.getAdjustedStartPosition(sourceFile, node, leadingTrivia, false)
185185
endPosition := t.getAdjustedEndPosition(sourceFile, node, trailingTrivia)
186-
t.ReplaceRangeWithText(sourceFile, t.toLSPEditRange(sourceFile, core.NewTextRange(startPosition, endPosition)), "")
186+
t.ReplaceTextRangeWithText(sourceFile, core.NewTextRange(startPosition, endPosition), "")
187187
}
188188

189189
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
214214
endPos = t.endPositionToDeleteNodeInList(sourceFile, node, prevNode, containingList.Nodes[index+1])
215215
}
216216

217-
t.ReplaceRangeWithText(sourceFile, t.toLSPEditRange(sourceFile, core.NewTextRange(startPos, endPos)), "")
217+
t.ReplaceTextRangeWithText(sourceFile, core.NewTextRange(startPos, endPos), "")
218218
}
219219

220220
// startPositionToDeleteNodeInList finds the first non-whitespace position in the leading trivia of the node

0 commit comments

Comments
 (0)