Skip to content

Commit 0d147d5

Browse files
emincbemincb
committed
fix(diagnostics): differentiate linter warnings from fatal errors\n\nThis change addresses an issue where linter errors were treated as fatal, causing unnecessary retries and inefficient file writes. The getNewDiagnostics function has been updated to categorize diagnostics into problems (fatal errors) and warnings (linter errors). The DiffViewProvider has been updated to consume this new structure and present the information to the AI in a more structured way.\n\nFixes #4622 
1 parent 478869e commit 0d147d5

File tree

4 files changed

+292
-86
lines changed

4 files changed

+292
-86
lines changed

‎src/__mocks__/vscode.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
export const DiagnosticSeverity = {
2+
Error: 0,
3+
Warning: 1,
4+
Information: 2,
5+
Hint: 3,
6+
}
7+
8+
export class Range {
9+
start: { line: number; character: number }
10+
end: { line: number; character: number }
11+
12+
constructor(
13+
public startLine: number,
14+
public startChar: number,
15+
public endLine: number,
16+
public endChar: number,
17+
) {
18+
this.start = { line: startLine, character: startChar }
19+
this.end = { line: endLine, character: endChar }
20+
}
21+
}
22+
23+
export class Diagnostic {
24+
constructor(
25+
public range: Range,
26+
public message: string,
27+
public severity: number,
28+
public source?: string,
29+
) {}
30+
}
31+
32+
export const Uri = {
33+
file: (path: string) => ({
34+
path,
35+
fsPath: path,
36+
toString: () => path,
37+
}),
38+
}

‎src/integrations/diagnostics/__tests__/index.test.ts‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
import * as vscode from "vscode"
2+
import { getNewDiagnostics } from ".."
3+
4+
vi.mock("vscode", async () => {
5+
const originalModule = await vi.importActual("vscode")
6+
const mock = {
7+
...originalModule,
8+
DiagnosticSeverity: {
9+
Error: 0,
10+
Warning: 1,
11+
Information: 2,
12+
Hint: 3,
13+
},
14+
Range: class {
15+
start: { line: number; character: number }
16+
end: { line: number; character: number }
17+
18+
constructor(
19+
public startLine: number,
20+
public startChar: number,
21+
public endLine: number,
22+
public endChar: number,
23+
) {
24+
this.start = { line: startLine, character: startChar }
25+
this.end = { line: endLine, character: endChar }
26+
}
27+
},
28+
Diagnostic: class {
29+
constructor(
30+
public range: any,
31+
public message: string,
32+
public severity: number,
33+
public source?: string,
34+
) {}
35+
},
36+
Uri: {
37+
file: (path: string) => ({
38+
path,
39+
fsPath: path,
40+
toString: () => path,
41+
}),
42+
},
43+
}
44+
return mock
45+
})
46+
47+
describe("getNewDiagnostics", () => {
48+
it("should return an empty list if there are no new diagnostics", () => {
49+
const oldDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [
50+
[
51+
vscode.Uri.file("/path/to/file1.ts"),
52+
[
53+
new vscode.Diagnostic(
54+
new vscode.Range(0, 0, 0, 10),
55+
"Old error in file1",
56+
vscode.DiagnosticSeverity.Error,
57+
),
58+
],
59+
],
60+
]
61+
const newDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [
62+
[
63+
vscode.Uri.file("/path/to/file1.ts"),
64+
[
65+
new vscode.Diagnostic(
66+
new vscode.Range(0, 0, 0, 10),
67+
"Old error in file1",
68+
vscode.DiagnosticSeverity.Error,
69+
),
70+
],
71+
],
72+
]
73+
74+
const { problems, warnings } = getNewDiagnostics(oldDiagnostics, newDiagnostics)
75+
expect(problems).toEqual([])
76+
expect(warnings).toEqual([])
77+
})
78+
79+
it("should identify new fatal errors", () => {
80+
const oldDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = []
81+
const newDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [
82+
[
83+
vscode.Uri.file("/path/to/file1.ts"),
84+
[
85+
new vscode.Diagnostic(
86+
new vscode.Range(0, 0, 0, 10),
87+
"New fatal error",
88+
vscode.DiagnosticSeverity.Error,
89+
"typescript",
90+
),
91+
],
92+
],
93+
]
94+
95+
const { problems, warnings } = getNewDiagnostics(oldDiagnostics, newDiagnostics)
96+
expect(problems).toHaveLength(1)
97+
expect(warnings).toHaveLength(0)
98+
expect(problems[0][1][0].message).toBe("New fatal error")
99+
})
100+
101+
it("should identify new linter warnings", () => {
102+
const oldDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = []
103+
const newDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [
104+
[
105+
vscode.Uri.file("/path/to/file1.ts"),
106+
[
107+
new vscode.Diagnostic(
108+
new vscode.Range(0, 0, 0, 10),
109+
"New linter warning",
110+
vscode.DiagnosticSeverity.Error,
111+
"pylance",
112+
),
113+
],
114+
],
115+
]
116+
117+
const { problems, warnings } = getNewDiagnostics(oldDiagnostics, newDiagnostics)
118+
expect(problems).toHaveLength(0)
119+
expect(warnings).toHaveLength(1)
120+
expect(warnings[0][1][0].message).toBe("New linter warning")
121+
})
122+
123+
it("should correctly categorize a mix of new diagnostics", () => {
124+
const oldDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = []
125+
const newDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [
126+
[
127+
vscode.Uri.file("/path/to/file1.ts"),
128+
[
129+
new vscode.Diagnostic(
130+
new vscode.Range(0, 0, 0, 10),
131+
"New fatal error",
132+
vscode.DiagnosticSeverity.Error,
133+
"typescript",
134+
),
135+
new vscode.Diagnostic(
136+
new vscode.Range(1, 0, 1, 10),
137+
"New linter warning",
138+
vscode.DiagnosticSeverity.Error,
139+
"eslint",
140+
),
141+
],
142+
],
143+
]
144+
145+
const { problems, warnings } = getNewDiagnostics(oldDiagnostics, newDiagnostics)
146+
expect(problems).toHaveLength(1)
147+
expect(warnings).toHaveLength(1)
148+
expect(problems[0][1][0].message).toBe("New fatal error")
149+
expect(warnings[0][1][0].message).toBe("New linter warning")
150+
})
151+
})

‎src/integrations/diagnostics/index.ts‎

Lines changed: 50 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,74 +1,68 @@
11
import * as vscode from "vscode"
22
import * as path from "path"
3-
import deepEqual from "fast-deep-equal"
3+
4+
export type NewDiagnosticsByKind = {
5+
problems: [vscode.Uri, vscode.Diagnostic[]][]
6+
warnings: [vscode.Uri, vscode.Diagnostic[]][]
7+
}
8+
9+
function areDiagnosticsEqual(d1: vscode.Diagnostic, d2: vscode.Diagnostic): boolean {
10+
return (
11+
d1.message === d2.message &&
12+
d1.severity === d2.severity &&
13+
d1.source === d2.source &&
14+
d1.range.start.line === d2.range.start.line &&
15+
d1.range.start.character === d2.range.start.character &&
16+
d1.range.end.line === d2.range.end.line &&
17+
d1.range.end.character === d2.range.end.character
18+
)
19+
}
420

521
export function getNewDiagnostics(
622
oldDiagnostics: [vscode.Uri, vscode.Diagnostic[]][],
723
newDiagnostics: [vscode.Uri, vscode.Diagnostic[]][],
8-
): [vscode.Uri, vscode.Diagnostic[]][] {
9-
const newProblems: [vscode.Uri, vscode.Diagnostic[]][] = []
10-
const oldMap = new Map(oldDiagnostics)
24+
): NewDiagnosticsByKind {
25+
const LINTER_DIAGNOSTIC_SOURCES = new Set(["pylance", "pyright", "eslint", "flake8", "mypy", "ruff"])
26+
27+
const problems: [vscode.Uri, vscode.Diagnostic[]][] = []
28+
const warnings: [vscode.Uri, vscode.Diagnostic[]][] = []
29+
30+
const oldMap = new Map<string, vscode.Diagnostic[]>()
31+
for (const [uri, diags] of oldDiagnostics) {
32+
oldMap.set(uri.toString(), diags)
33+
}
1134

1235
for (const [uri, newDiags] of newDiagnostics) {
13-
const oldDiags = oldMap.get(uri) || []
14-
const newProblemsForUri = newDiags.filter((newDiag) => !oldDiags.some((oldDiag) => deepEqual(oldDiag, newDiag)))
36+
const oldDiags = oldMap.get(uri.toString()) || []
37+
const newProblemsForUri = newDiags.filter(
38+
(newDiag) => !oldDiags.some((oldDiag) => areDiagnosticsEqual(oldDiag, newDiag)),
39+
)
1540

1641
if (newProblemsForUri.length > 0) {
17-
newProblems.push([uri, newProblemsForUri])
42+
const fatalErrorsForUri = newProblemsForUri.filter(
43+
(diag) =>
44+
diag.severity === vscode.DiagnosticSeverity.Error &&
45+
!LINTER_DIAGNOSTIC_SOURCES.has(diag.source ?? ""),
46+
)
47+
48+
const linterWarningsForUri = newProblemsForUri.filter(
49+
(diag) =>
50+
diag.severity === vscode.DiagnosticSeverity.Error &&
51+
LINTER_DIAGNOSTIC_SOURCES.has(diag.source ?? ""),
52+
)
53+
54+
if (fatalErrorsForUri.length > 0) {
55+
problems.push([uri, fatalErrorsForUri])
56+
}
57+
if (linterWarningsForUri.length > 0) {
58+
warnings.push([uri, linterWarningsForUri])
59+
}
1860
}
1961
}
2062

21-
return newProblems
63+
return { problems, warnings }
2264
}
2365

24-
// Usage:
25-
// const oldDiagnostics = // ... your old diagnostics array
26-
// const newDiagnostics = // ... your new diagnostics array
27-
// const newProblems = getNewDiagnostics(oldDiagnostics, newDiagnostics);
28-
29-
// Example usage with mocks:
30-
//
31-
// // Mock old diagnostics
32-
// const oldDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [
33-
// [vscode.Uri.file("/path/to/file1.ts"), [
34-
// new vscode.Diagnostic(new vscode.Range(0, 0, 0, 10), "Old error in file1", vscode.DiagnosticSeverity.Error)
35-
// ]],
36-
// [vscode.Uri.file("/path/to/file2.ts"), [
37-
// new vscode.Diagnostic(new vscode.Range(5, 5, 5, 15), "Old warning in file2", vscode.DiagnosticSeverity.Warning)
38-
// ]]
39-
// ];
40-
//
41-
// // Mock new diagnostics
42-
// const newDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [
43-
// [vscode.Uri.file("/path/to/file1.ts"), [
44-
// new vscode.Diagnostic(new vscode.Range(0, 0, 0, 10), "Old error in file1", vscode.DiagnosticSeverity.Error),
45-
// new vscode.Diagnostic(new vscode.Range(2, 2, 2, 12), "New error in file1", vscode.DiagnosticSeverity.Error)
46-
// ]],
47-
// [vscode.Uri.file("/path/to/file2.ts"), [
48-
// new vscode.Diagnostic(new vscode.Range(5, 5, 5, 15), "Old warning in file2", vscode.DiagnosticSeverity.Warning)
49-
// ]],
50-
// [vscode.Uri.file("/path/to/file3.ts"), [
51-
// new vscode.Diagnostic(new vscode.Range(1, 1, 1, 11), "New error in file3", vscode.DiagnosticSeverity.Error)
52-
// ]]
53-
// ];
54-
//
55-
// const newProblems = getNewProblems(oldDiagnostics, newDiagnostics);
56-
//
57-
// console.log("New problems:");
58-
// for (const [uri, diagnostics] of newProblems) {
59-
// console.log(`File: ${uri.fsPath}`);
60-
// for (const diagnostic of diagnostics) {
61-
// console.log(`- ${diagnostic.message} (${diagnostic.range.start.line}:${diagnostic.range.start.character})`);
62-
// }
63-
// }
64-
//
65-
// // Expected output:
66-
// // New problems:
67-
// // File: /path/to/file1.ts
68-
// // - New error in file1 (2:2)
69-
// // File: /path/to/file3.ts
70-
// // - New error in file3 (1:1)
71-
7266
// will return empty string if no problems with the given severity are found
7367
export async function diagnosticsToProblemsString(
7468
diagnostics: [vscode.Uri, vscode.Diagnostic[]][],

0 commit comments

Comments
 (0)