Skip to content

Commit 6a24966

Browse files
fansenzefansenze
authored
fix(linter): prevent duplicate fix insertions when multiple tsconfigs match the same file (#452)
1 parent 6b6e555 commit 6a24966

File tree

3 files changed

+339
-1
lines changed

3 files changed

+339
-1
lines changed

‎cspell.config.cjs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ module.exports = {
1717
'packages/rslint/pkg/mod',
1818
'cmd/tsgo',
1919
'./agents',
20+
'internal/linter/*_test.go',
2021
],
2122
dictionaries: ['dictionary'],
2223
dictionaryDefinitions: [

‎internal/linter/source_code_fixer.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,15 +45,34 @@ func ApplyRuleFixes[M LintMessage](code string, diagnostics []M) (string, []M, b
4545
var builder strings.Builder
4646

4747
lastFixEnd := 0
48+
lastWasInsertion := false
4849
for _, diagnostic := range withFixes {
4950
fixes := diagnostic.Fixes()
50-
if lastFixEnd > fixes[0].Range.Pos() {
51+
firstFix := fixes[0]
52+
53+
isCurrentFixInsertion := firstFix.Range.Pos() == firstFix.Range.End()
54+
55+
// Check for overlapping fixes (e.g., [0,5] and [2,7])
56+
isOverlapping := lastFixEnd > firstFix.Range.Pos()
57+
58+
// Check for adjacent conflicts. This happens when a fix starts exactly where the last one ended,
59+
// and at least one of them is an insertion. Adjacent replacements are allowed.
60+
// - Insertion followed by insertion at same pos: duplicate
61+
// - Insertion followed by replacement at same pos: conflict (replacement starts where insertion happened)
62+
// - Replacement followed by insertion at same pos: conflict (ambiguous position after replacement)
63+
// - Replacement followed by replacement at same pos: OK (adjacent, non-overlapping)
64+
isAdjacentConflict := fixed &&
65+
lastFixEnd == firstFix.Range.Pos() &&
66+
(isCurrentFixInsertion || lastWasInsertion)
67+
68+
if isOverlapping || isAdjacentConflict {
5169
unapplied = append(unapplied, diagnostic)
5270
continue
5371
}
5472

5573
for _, fix := range fixes {
5674
fixed = true
75+
lastWasInsertion = fix.Range.Pos() == fix.Range.End()
5776

5877
builder.WriteString(code[lastFixEnd:fix.Range.Pos()])
5978
builder.WriteString(fix.Text)

‎internal/linter/source_code_fixer_test.go‎

Lines changed: 318 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,318 @@
1+
package linter
2+
3+
import (
4+
"testing"
5+
6+
"github.com/microsoft/typescript-go/shim/core"
7+
"github.com/web-infra-dev/rslint/internal/rule"
8+
)
9+
10+
// mockDiagnostic implements LintMessage interface for testing
11+
type mockDiagnostic struct {
12+
fixes []rule.RuleFix
13+
}
14+
15+
func (m mockDiagnostic) Fixes() []rule.RuleFix {
16+
return m.fixes
17+
}
18+
19+
func newMockDiagnostic(fixes ...rule.RuleFix) mockDiagnostic {
20+
return mockDiagnostic{fixes: fixes}
21+
}
22+
23+
func newInsertFix(pos int, text string) rule.RuleFix {
24+
return rule.RuleFix{
25+
Text: text,
26+
Range: core.NewTextRange(pos, pos),
27+
}
28+
}
29+
30+
func newReplaceFix(start, end int, text string) rule.RuleFix {
31+
return rule.RuleFix{
32+
Text: text,
33+
Range: core.NewTextRange(start, end),
34+
}
35+
}
36+
37+
func TestApplyRuleFixes(t *testing.T) {
38+
tests := []struct {
39+
name string
40+
code string
41+
diagnostics []mockDiagnostic
42+
expectedCode string
43+
expectedUnapplied int
44+
expectedFixedStatus bool
45+
}{
46+
{
47+
name: "single insertion fix",
48+
code: "function foo() {}",
49+
diagnostics: []mockDiagnostic{
50+
newMockDiagnostic(newInsertFix(0, "async ")),
51+
},
52+
expectedCode: "async function foo() {}",
53+
expectedUnapplied: 0,
54+
expectedFixedStatus: true,
55+
},
56+
{
57+
name: "multiple non-overlapping insertion fixes",
58+
code: "function foo() { return bar(); }",
59+
diagnostics: []mockDiagnostic{
60+
newMockDiagnostic(newInsertFix(0, "async ")),
61+
newMockDiagnostic(newInsertFix(24, "await ")),
62+
},
63+
expectedCode: "async function foo() { return await bar(); }",
64+
expectedUnapplied: 0,
65+
expectedFixedStatus: true,
66+
},
67+
{
68+
name: "duplicate insertion fixes at same position - should skip duplicates",
69+
code: "function foo() {}",
70+
diagnostics: []mockDiagnostic{
71+
newMockDiagnostic(newInsertFix(0, "async ")),
72+
newMockDiagnostic(newInsertFix(0, "async ")),
73+
newMockDiagnostic(newInsertFix(0, "async ")),
74+
},
75+
expectedCode: "async function foo() {}",
76+
expectedUnapplied: 2,
77+
expectedFixedStatus: true,
78+
},
79+
{
80+
name: "overlapping replacement fixes - should skip overlapping",
81+
code: "const foo = 'bar';",
82+
diagnostics: []mockDiagnostic{
83+
newMockDiagnostic(newReplaceFix(6, 9, "baz")),
84+
newMockDiagnostic(newReplaceFix(8, 11, "qux")),
85+
},
86+
expectedCode: "const baz = 'bar';",
87+
expectedUnapplied: 1,
88+
expectedFixedStatus: true,
89+
},
90+
{
91+
name: "consecutive replacement fixes - should apply both",
92+
code: "const foo = bar;",
93+
diagnostics: []mockDiagnostic{
94+
newMockDiagnostic(newReplaceFix(6, 9, "baz")),
95+
newMockDiagnostic(newReplaceFix(12, 15, "qux")),
96+
},
97+
expectedCode: "const baz = qux;",
98+
expectedUnapplied: 0,
99+
expectedFixedStatus: true,
100+
},
101+
{
102+
name: "adjacent replacement fixes (end == start) - should apply both",
103+
code: "abcdef",
104+
diagnostics: []mockDiagnostic{
105+
newMockDiagnostic(newReplaceFix(0, 3, "XXX")),
106+
newMockDiagnostic(newReplaceFix(3, 6, "YYY")),
107+
},
108+
expectedCode: "XXXYYY",
109+
expectedUnapplied: 0,
110+
expectedFixedStatus: true,
111+
},
112+
{
113+
name: "no fixes",
114+
code: "const foo = 'bar';",
115+
diagnostics: []mockDiagnostic{},
116+
expectedCode: "const foo = 'bar';",
117+
expectedUnapplied: 0,
118+
expectedFixedStatus: false,
119+
},
120+
{
121+
name: "diagnostic without fixes",
122+
code: "const foo = 'bar';",
123+
diagnostics: []mockDiagnostic{
124+
{fixes: []rule.RuleFix{}},
125+
},
126+
expectedCode: "const foo = 'bar';",
127+
expectedUnapplied: 1,
128+
expectedFixedStatus: false,
129+
},
130+
{
131+
name: "mixed: insertion at position 0 followed by another at same position",
132+
code: "foo()",
133+
diagnostics: []mockDiagnostic{
134+
newMockDiagnostic(newInsertFix(0, "await ")),
135+
newMockDiagnostic(newInsertFix(0, "await ")),
136+
},
137+
expectedCode: "await foo()",
138+
expectedUnapplied: 1,
139+
expectedFixedStatus: true,
140+
},
141+
{
142+
name: "insertion followed by replacement at different positions",
143+
code: "function foo() { return bar; }",
144+
diagnostics: []mockDiagnostic{
145+
newMockDiagnostic(newInsertFix(0, "async ")),
146+
newMockDiagnostic(newReplaceFix(24, 27, "baz")),
147+
},
148+
expectedCode: "async function foo() { return baz; }",
149+
expectedUnapplied: 0,
150+
expectedFixedStatus: true,
151+
},
152+
// Tests for diagnostics with multiple fixes
153+
{
154+
name: "single diagnostic with multiple fixes at different positions",
155+
code: "function foo() { return bar(); }",
156+
diagnostics: []mockDiagnostic{
157+
// One diagnostic with two fixes: add async and add await
158+
newMockDiagnostic(newInsertFix(0, "async "), newInsertFix(24, "await ")),
159+
},
160+
expectedCode: "async function foo() { return await bar(); }",
161+
expectedUnapplied: 0,
162+
expectedFixedStatus: true,
163+
},
164+
{
165+
name: "two diagnostics with multiple fixes - second conflicts with first",
166+
code: "function foo() { return bar(); }",
167+
diagnostics: []mockDiagnostic{
168+
// First diagnostic: add async at 0, add await at 24
169+
newMockDiagnostic(newInsertFix(0, "async "), newInsertFix(24, "await ")),
170+
// Second diagnostic: tries to insert at position 10 (between first diagnostic's fixes)
171+
newMockDiagnostic(newInsertFix(10, "CONFLICT")),
172+
},
173+
expectedCode: "async function foo() { return await bar(); }",
174+
expectedUnapplied: 1,
175+
expectedFixedStatus: true,
176+
},
177+
{
178+
name: "duplicate diagnostic with multiple fixes - should skip entire duplicate",
179+
code: "function foo() { return bar(); }",
180+
diagnostics: []mockDiagnostic{
181+
newMockDiagnostic(newInsertFix(0, "async "), newInsertFix(24, "await ")),
182+
// Duplicate: same first fix position
183+
newMockDiagnostic(newInsertFix(0, "async "), newInsertFix(24, "await ")),
184+
},
185+
expectedCode: "async function foo() { return await bar(); }",
186+
expectedUnapplied: 1,
187+
expectedFixedStatus: true,
188+
},
189+
// Edge cases
190+
{
191+
name: "delete operation (empty replacement text)",
192+
code: "var foo = 1;",
193+
diagnostics: []mockDiagnostic{
194+
newMockDiagnostic(newReplaceFix(0, 4, "")), // delete "var "
195+
},
196+
expectedCode: "foo = 1;",
197+
expectedUnapplied: 0,
198+
expectedFixedStatus: true,
199+
},
200+
{
201+
name: "insertion at end of code",
202+
code: "const x = 1",
203+
diagnostics: []mockDiagnostic{
204+
newMockDiagnostic(newInsertFix(11, ";")),
205+
},
206+
expectedCode: "const x = 1;",
207+
expectedUnapplied: 0,
208+
expectedFixedStatus: true,
209+
},
210+
{
211+
name: "replacement at end of code",
212+
code: "const x = 1",
213+
diagnostics: []mockDiagnostic{
214+
newMockDiagnostic(newReplaceFix(10, 11, "2;")),
215+
},
216+
expectedCode: "const x = 2;",
217+
expectedUnapplied: 0,
218+
expectedFixedStatus: true,
219+
},
220+
{
221+
name: "empty code with insertion",
222+
code: "",
223+
diagnostics: []mockDiagnostic{
224+
newMockDiagnostic(newInsertFix(0, "hello")),
225+
},
226+
expectedCode: "hello",
227+
expectedUnapplied: 0,
228+
expectedFixedStatus: true,
229+
},
230+
{
231+
name: "different insertions at same position - should skip second",
232+
code: "function foo() {}",
233+
diagnostics: []mockDiagnostic{
234+
newMockDiagnostic(newInsertFix(0, "async ")),
235+
newMockDiagnostic(newInsertFix(0, "export ")), // different content, same position
236+
},
237+
expectedCode: "async function foo() {}",
238+
expectedUnapplied: 1,
239+
expectedFixedStatus: true,
240+
},
241+
{
242+
name: "replacement followed by insertion at same end position - should skip insertion",
243+
code: "ABCDEFGHIJ",
244+
diagnostics: []mockDiagnostic{
245+
newMockDiagnostic(newReplaceFix(0, 5, "XXX")), // Replace ABCDE with XXX
246+
newMockDiagnostic(newInsertFix(5, "YYY")), // Insert at position 5
247+
},
248+
expectedCode: "XXXFGHIJ",
249+
expectedUnapplied: 1,
250+
expectedFixedStatus: true,
251+
},
252+
{
253+
name: "insertion followed by replacement starting at same position - should skip replacement",
254+
code: "ABCDEFGHIJ",
255+
diagnostics: []mockDiagnostic{
256+
newMockDiagnostic(newInsertFix(0, "XXX")), // Insert at 0
257+
newMockDiagnostic(newReplaceFix(0, 3, "YYY")), // Replace ABC with YYY
258+
},
259+
expectedCode: "XXXABCDEFGHIJ",
260+
expectedUnapplied: 1,
261+
expectedFixedStatus: true,
262+
},
263+
}
264+
265+
for _, tt := range tests {
266+
t.Run(tt.name, func(t *testing.T) {
267+
result, unapplied, fixed := ApplyRuleFixes(tt.code, tt.diagnostics)
268+
269+
if result != tt.expectedCode {
270+
t.Errorf("ApplyRuleFixes() code = %q, want %q", result, tt.expectedCode)
271+
}
272+
273+
if len(unapplied) != tt.expectedUnapplied {
274+
t.Errorf("ApplyRuleFixes() unapplied count = %d, want %d", len(unapplied), tt.expectedUnapplied)
275+
}
276+
277+
if fixed != tt.expectedFixedStatus {
278+
t.Errorf("ApplyRuleFixes() fixed = %v, want %v", fixed, tt.expectedFixedStatus)
279+
}
280+
})
281+
}
282+
}
283+
284+
// TestApplyRuleFixes_DuplicateInsertionRegression is a regression test for issue #451
285+
// where multiple diagnostics at the same position caused "async async async" to be inserted
286+
func TestApplyRuleFixes_DuplicateInsertionRegression(t *testing.T) {
287+
// Simulate what happens when the same file is linted by multiple tsconfig projects
288+
// Each project reports the same diagnostic, resulting in duplicate fixes
289+
code := "function fetchData(): Promise<string> { return Promise.resolve('data'); }"
290+
291+
// Three diagnostics from three different tsconfig projects, all pointing to the same fix
292+
diagnostics := []mockDiagnostic{
293+
newMockDiagnostic(newInsertFix(0, " async ")),
294+
newMockDiagnostic(newInsertFix(0, " async ")),
295+
newMockDiagnostic(newInsertFix(0, " async ")),
296+
}
297+
298+
result, unapplied, fixed := ApplyRuleFixes(code, diagnostics)
299+
300+
expectedCode := " async function fetchData(): Promise<string> { return Promise.resolve('data'); }"
301+
302+
if result != expectedCode {
303+
t.Errorf("Regression test failed: got %q, want %q", result, expectedCode)
304+
}
305+
306+
if len(unapplied) != 2 {
307+
t.Errorf("Expected 2 unapplied diagnostics (duplicates), got %d", len(unapplied))
308+
}
309+
310+
if !fixed {
311+
t.Error("Expected fixed to be true")
312+
}
313+
314+
// Verify we don't have "async async async"
315+
if result == " async async async function fetchData(): Promise<string> { return Promise.resolve('data'); }" {
316+
t.Error("Regression: duplicate insertions were not prevented!")
317+
}
318+
}

0 commit comments

Comments
 (0)