Skip to content

Commit 55b3a29

Browse files
AnthonyLatsisAnthonyLatsis
committed
SwiftFixIt: Skip diagnostic with 2 or more notes with fix-its
These diagnostics generally require user intervention.
1 parent ba9c736 commit 55b3a29

File tree

3 files changed

+106
-93
lines changed

3 files changed

+106
-93
lines changed

‎Sources/SwiftFixIt/SwiftFixIt.swift

Lines changed: 74 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,21 @@ extension AnyDiagnostic {
7878
self.level != .note
7979
}
8080

81+
var isNote: Bool {
82+
!self.isPrimary
83+
}
84+
8185
var isIgnored: Bool {
8286
self.level == .ignored
8387
}
88+
89+
var hasFixIt: Bool {
90+
!self.fixIts.isEmpty
91+
}
92+
93+
var hasNoLocation: Bool {
94+
self.location == nil
95+
}
8496
}
8597

8698
extension SerializedDiagnostics.Diagnostic: AnyDiagnostic {}
@@ -113,90 +125,92 @@ package struct SwiftFixIt /*: ~Copyable */ {
113125
)
114126
}
115127

116-
init(
117-
diagnostics: some Collection<some AnyDiagnostic>,
128+
init<Diagnostic: AnyDiagnostic>(
129+
diagnostics: some Collection<Diagnostic>,
118130
categories: Set<String>,
119131
fileSystem: any FileSystem
120132
) throws {
121133
self.fileSystem = fileSystem
122134

123-
// Build a map from source files to `SwiftDiagnostics` diagnostics.
124-
var diagnosticsPerFile: DiagnosticsPerFile = [:]
125-
126-
var diagnosticConverter = DiagnosticConverter(fileSystem: fileSystem)
127-
var currentPrimaryDiagnosticHasNoteWithFixIt = false
128-
129-
var index = diagnostics.startIndex
130-
while index != diagnostics.endIndex {
131-
let diagnostic = diagnostics[index]
132-
133-
func skipDiagnostic() {
134-
guard diagnostic.isPrimary else {
135-
diagnostics.formIndex(after: &index)
136-
return
137-
}
138-
139-
// Skip a primary diagnostic together with its notes.
140-
repeat {
141-
diagnostics.formIndex(after: &index)
142-
} while index != diagnostics.endIndex && !diagnostics[index].isPrimary
143-
}
135+
func shouldSkip(primaryDiagnosticWithNotes: some Collection<Diagnostic>) -> Bool {
136+
let diagnostic = primaryDiagnosticWithNotes[primaryDiagnosticWithNotes.startIndex]
144137

145-
// Skip ignored diagnostics.
146-
guard !diagnostic.isIgnored else {
147-
skipDiagnostic()
148-
continue
138+
// Skip if ignored.
139+
if diagnostic.isIgnored {
140+
return true
149141
}
150142

151-
// Skip diagnostics with no location.
152-
guard diagnostic.location != nil else {
153-
skipDiagnostic()
154-
continue
143+
// Skip if no location.
144+
if diagnostic.hasNoLocation {
145+
return true
155146
}
156147

157-
// Skip a primary diagnostic if categories were given and it does
158-
// not belong to any of them.
159-
if !categories.isEmpty && diagnostic.isPrimary {
148+
// Skip if categories were given and the diagnostic does not
149+
// belong to any of them.
150+
if !categories.isEmpty {
160151
guard let category = diagnostic.category, categories.contains(category) else {
161-
skipDiagnostic()
162-
continue
152+
return true
163153
}
164154
}
165155

166-
defer {
167-
diagnostics.formIndex(after: &index)
156+
let notes = primaryDiagnosticWithNotes.dropFirst()
157+
158+
// Consider the diagnostic compromised if a note does not have a
159+
// location.
160+
if notes.contains(where: \.hasNoLocation) {
161+
return true
168162
}
169163

170-
let hasFixits = !diagnostic.fixIts.isEmpty
171-
172-
if diagnostic.isPrimary {
173-
currentPrimaryDiagnosticHasNoteWithFixIt = false
174-
} else {
175-
if hasFixits {
176-
// The Swift compiler produces parallel fix-its by attaching
177-
// them to notes, which in turn associate to a single
178-
// error/warning. Prefer the first fix-it in this case: if
179-
// the last error/warning we saw has a note with a fix-it
180-
// and so is this one, skip it.
181-
if currentPrimaryDiagnosticHasNoteWithFixIt {
182-
continue
183-
}
184-
185-
currentPrimaryDiagnosticHasNoteWithFixIt = true
186-
}
164+
let numberOfNotesWithFixIts = notes.count(where: \.hasFixIt)
165+
switch numberOfNotesWithFixIts {
166+
case 0:
167+
// Skip if neither the primary diagnostic nor any of its notes
168+
// has a fix-it.
169+
return !diagnostic.hasFixIt
170+
case 1:
171+
return false
172+
default:
173+
// Skip if more than 1 note has a fix-it. These diagnostics
174+
// generally require user intervention.
175+
// TODO: This will have to done lazier once we support printing them.
176+
return true
187177
}
178+
}
179+
180+
// Build a map from source files to `SwiftDiagnostics` diagnostics.
181+
var diagnosticsPerFile: DiagnosticsPerFile = [:]
182+
var diagnosticConverter = DiagnosticConverter(fileSystem: fileSystem)
183+
184+
var nextPrimaryIndex = diagnostics.startIndex
185+
while nextPrimaryIndex != diagnostics.endIndex {
186+
let currentPrimaryIndex = nextPrimaryIndex
187+
precondition(diagnostics[currentPrimaryIndex].isPrimary)
188+
189+
// Shift the index to the next primary diagnostic.
190+
repeat {
191+
diagnostics.formIndex(after: &nextPrimaryIndex)
192+
} while nextPrimaryIndex != diagnostics.endIndex && diagnostics[nextPrimaryIndex].isNote
193+
194+
let primaryDiagnosticWithNotes = diagnostics[currentPrimaryIndex ..< nextPrimaryIndex]
188195

189-
// We are only interested in diagnostics with fix-its.
190-
guard hasFixits else {
196+
if shouldSkip(primaryDiagnosticWithNotes: primaryDiagnosticWithNotes) {
191197
continue
192198
}
193199

194-
let (sourceFile, convertedDiagnostic) = try diagnosticConverter.diagnostic(from: diagnostic)
200+
for diagnostic in primaryDiagnosticWithNotes {
201+
// We are only interested in diagnostics with fix-its.
202+
// TODO: This will have to change once we support printing them.
203+
guard diagnostic.hasFixIt else {
204+
continue
205+
}
195206

196-
diagnosticsPerFile[consume sourceFile, default: []].append(consume convertedDiagnostic)
207+
let (sourceFile, convertedDiagnostic) = try diagnosticConverter.diagnostic(from: diagnostic)
208+
209+
diagnosticsPerFile[consume sourceFile, default: []].append(consume convertedDiagnostic)
210+
}
197211
}
198212

199-
self.diagnosticsPerFile = consume diagnosticsPerFile
213+
self.diagnosticsPerFile = diagnosticsPerFile
200214
}
201215

202216
package func applyFixIts() throws {

‎Tests/SwiftFixItTests/BasicTests.swift

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,16 @@
1313
import XCTest
1414

1515
final class BasicTests: XCTestCase {
16+
func testNoDiagnostics() throws {
17+
// Edge case.
18+
try testAPI1File { _ in
19+
.init(
20+
edits: .init(input: "var x = 1", result: "var x = 1"),
21+
diagnostics: []
22+
)
23+
}
24+
}
25+
1626
func testPrimaryDiag() throws {
1727
try testAPI1File { (filename: String) in
1828
.init(

‎Tests/SwiftFixItTests/FilteringTests.swift

Lines changed: 22 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -154,22 +154,21 @@ final class FilteringTests: XCTestCase {
154154
}
155155
}
156156

157-
func testParallelFixIts1() throws {
158-
// First parallel fix-it is applied per emission order.
159-
try testAPI1File { (filename: String) in
157+
func testMultipleNotesWithFixIts() throws {
158+
try testAPI1File { filename in
160159
.init(
161-
edits: .init(input: "var x = 1", result: "let x = 1"),
160+
edits: .init(input: "var x = 1", result: "var x = 1"),
162161
diagnostics: [
163162
PrimaryDiagnostic(
164163
level: .error,
165-
text: "error",
164+
text: "error1",
166165
location: .init(filename: filename, line: 1, column: 1, offset: 0),
167166
notes: [
168167
Note(
169-
text: "note",
168+
text: "error1_note1",
170169
location: .init(filename: filename, line: 1, column: 1, offset: 0),
171170
fixIts: [
172-
// Applied.
171+
// Skipped, primary diagnostic has more than 1 note with fix-it.
173172
.init(
174173
start: .init(filename: filename, line: 1, column: 1, offset: 0),
175174
end: .init(filename: filename, line: 1, column: 4, offset: 0),
@@ -178,10 +177,10 @@ final class FilteringTests: XCTestCase {
178177
]
179178
),
180179
Note(
181-
text: "note",
180+
text: "error1_note2",
182181
location: .init(filename: filename, line: 1, column: 1, offset: 0),
183182
fixIts: [
184-
// Ignored, parallel to previous fix-it.
183+
// Skipped, primary diagnostic has more than 1 note with fix-it.
185184
.init(
186185
start: .init(filename: filename, line: 1, column: 9, offset: 0),
187186
end: .init(filename: filename, line: 1, column: 10, offset: 0),
@@ -191,47 +190,37 @@ final class FilteringTests: XCTestCase {
191190
),
192191
]
193192
),
194-
]
195-
)
196-
}
197-
}
198-
199-
func testParallelFixIts2() throws {
200-
// First parallel fix-it is applied per emission order.
201-
try testAPI1File { (filename: String) in
202-
.init(
203-
edits: .init(input: "var x = 1", result: "let x = 1"),
204-
diagnostics: [
205193
PrimaryDiagnostic(
206-
level: .error,
207-
text: "error",
194+
level: .warning,
195+
text: "warning1",
208196
location: .init(filename: filename, line: 1, column: 1, offset: 0),
209197
notes: [
210198
Note(
211-
text: "note",
199+
text: "warning1_note1",
212200
location: .init(filename: filename, line: 1, column: 1, offset: 0),
213201
fixIts: [
214-
// Applied.
202+
// Skipped, primary diagnostic has more than 1 note with fix-it.
215203
.init(
216-
start: .init(filename: filename, line: 1, column: 1, offset: 0),
217-
end: .init(filename: filename, line: 1, column: 4, offset: 0),
218-
text: "let"
204+
start: .init(filename: filename, line: 1, column: 5, offset: 0),
205+
end: .init(filename: filename, line: 1, column: 6, offset: 0),
206+
text: "y"
219207
),
220208
]
221209
),
210+
// This separator note should not make a difference.
222211
Note(
223-
text: "note",
212+
text: "warning1_note2",
224213
location: .init(filename: filename, line: 1, column: 1, offset: 0)
225214
),
226215
Note(
227-
text: "note",
216+
text: "warning1_note3",
228217
location: .init(filename: filename, line: 1, column: 1, offset: 0),
229218
fixIts: [
230-
// Ignored, parallel to previous fix-it.
219+
// Skipped, primary diagnostic has more than 1 note with fix-it.
231220
.init(
232-
start: .init(filename: filename, line: 1, column: 9, offset: 0),
233-
end: .init(filename: filename, line: 1, column: 10, offset: 0),
234-
text: "22"
221+
start: .init(filename: filename, line: 1, column: 7, offset: 0),
222+
end: .init(filename: filename, line: 1, column: 8, offset: 0),
223+
text: ":"
235224
),
236225
]
237226
),

0 commit comments

Comments
 (0)