Skip to content

Commit f5913e7

Browse files
AnthonyLatsisAnthonyLatsis
committed
SwiftFixIt: Skip duplicate primary diagnostics
1 parent 55b3a29 commit f5913e7

File tree

3 files changed

+196
-53
lines changed

3 files changed

+196
-53
lines changed

‎Sources/SwiftFixIt/SwiftFixIt.swift

Lines changed: 99 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ private enum Error: Swift.Error {
4242
}
4343

4444
// FIXME: An abstraction for tests to work around missing memberwise initializers in `TSCUtility.SerializedDiagnostics`.
45-
protocol AnySourceLocation {
45+
protocol AnySourceLocation: Hashable {
4646
var filename: String { get }
4747
var line: UInt64 { get }
4848
var column: UInt64 { get }
@@ -96,9 +96,103 @@ extension AnyDiagnostic {
9696
}
9797

9898
extension SerializedDiagnostics.Diagnostic: AnyDiagnostic {}
99-
extension SerializedDiagnostics.SourceLocation: AnySourceLocation {}
99+
extension SerializedDiagnostics.SourceLocation: AnySourceLocation, @retroactive Hashable {
100+
public func hash(into hasher: inout Hasher) {
101+
hasher.combine(self.filename)
102+
hasher.combine(self.line)
103+
hasher.combine(self.column)
104+
}
105+
}
106+
100107
extension SerializedDiagnostics.FixIt: AnyFixIt {}
101108

109+
/// Encapsulates initial diagnostic skipping behavior.
110+
private struct PrimaryDiagnosticFilter<Diagnostic: AnyDiagnostic>: ~Copyable {
111+
/// A hashable type storing the minimum data necessary to uniquely identify
112+
/// a diagnostic for our purposes.
113+
private struct DiagnosticID: Hashable {
114+
private let location: Diagnostic.SourceLocation
115+
private let message: String
116+
private let level: SerializedDiagnostics.Diagnostic.Level
117+
118+
init(diagnostic: Diagnostic) {
119+
self.level = diagnostic.level
120+
self.message = diagnostic.text
121+
// Force the location. We should be filtering out diagnostics
122+
// without a location.
123+
self.location = diagnostic.location!
124+
}
125+
}
126+
127+
private var uniquePrimaryDiagnostics: Set<DiagnosticID> = []
128+
129+
let categories: Set<String>
130+
131+
init(categories: Set<String>) {
132+
self.categories = categories
133+
}
134+
135+
/// Returns a Boolean value indicating whether to skip the given primary
136+
/// diagnostic and its notes.
137+
mutating func shouldSkip(primaryDiagnosticWithNotes: some Collection<Diagnostic>) -> Bool {
138+
let diagnostic = primaryDiagnosticWithNotes[primaryDiagnosticWithNotes.startIndex]
139+
precondition(diagnostic.isPrimary)
140+
141+
// Skip if ignored.
142+
if diagnostic.isIgnored {
143+
return true
144+
}
145+
146+
// Skip if no location.
147+
if diagnostic.hasNoLocation {
148+
return true
149+
}
150+
151+
// Skip if categories are given and the diagnostic does not
152+
// belong to any of them.
153+
if !self.categories.isEmpty {
154+
guard let category = diagnostic.category, self.categories.contains(category) else {
155+
return true
156+
}
157+
}
158+
159+
let notes = primaryDiagnosticWithNotes.dropFirst()
160+
precondition(notes.allSatisfy(\.isNote))
161+
162+
// Consider the diagnostic compromised if a note does not have a
163+
// location.
164+
if notes.contains(where: \.hasNoLocation) {
165+
return true
166+
}
167+
168+
switch notes.count(where: \.hasFixIt) {
169+
case 0:
170+
// No notes have fix-its. Skip if neither does the primary
171+
// diagnostic.
172+
guard diagnostic.hasFixIt else {
173+
return true
174+
}
175+
case 1:
176+
break
177+
default:
178+
// Skip if more than 1 note has a fix-it. These diagnostics
179+
// generally require user intervention.
180+
// TODO: This will have to be done lazier once we support printing them.
181+
return true
182+
}
183+
184+
// Skip if we've seen this primary diagnostic before.
185+
//
186+
// NB: This check is done last to prevent the set from growing without
187+
// need.
188+
guard self.uniquePrimaryDiagnostics.insert(.init(diagnostic: diagnostic)).inserted else {
189+
return true
190+
}
191+
192+
return false
193+
}
194+
}
195+
102196
/// The backing API for `SwiftFixitCommand`.
103197
package struct SwiftFixIt /*: ~Copyable */ {
104198
private typealias DiagnosticsPerFile = [SourceFile: [SwiftDiagnostics.Diagnostic]]
@@ -132,50 +226,8 @@ package struct SwiftFixIt /*: ~Copyable */ {
132226
) throws {
133227
self.fileSystem = fileSystem
134228

135-
func shouldSkip(primaryDiagnosticWithNotes: some Collection<Diagnostic>) -> Bool {
136-
let diagnostic = primaryDiagnosticWithNotes[primaryDiagnosticWithNotes.startIndex]
137-
138-
// Skip if ignored.
139-
if diagnostic.isIgnored {
140-
return true
141-
}
142-
143-
// Skip if no location.
144-
if diagnostic.hasNoLocation {
145-
return true
146-
}
147-
148-
// Skip if categories were given and the diagnostic does not
149-
// belong to any of them.
150-
if !categories.isEmpty {
151-
guard let category = diagnostic.category, categories.contains(category) else {
152-
return true
153-
}
154-
}
155-
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
162-
}
163-
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
177-
}
178-
}
229+
var filter = PrimaryDiagnosticFilter<Diagnostic>(categories: categories)
230+
_ = consume categories
179231

180232
// Build a map from source files to `SwiftDiagnostics` diagnostics.
181233
var diagnosticsPerFile: DiagnosticsPerFile = [:]
@@ -193,7 +245,7 @@ package struct SwiftFixIt /*: ~Copyable */ {
193245

194246
let primaryDiagnosticWithNotes = diagnostics[currentPrimaryIndex ..< nextPrimaryIndex]
195247

196-
if shouldSkip(primaryDiagnosticWithNotes: primaryDiagnosticWithNotes) {
248+
if filter.shouldSkip(primaryDiagnosticWithNotes: primaryDiagnosticWithNotes) {
197249
continue
198250
}
199251

‎Tests/SwiftFixItTests/BasicTests.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ final class BasicTests: XCTestCase {
231231
diagnostics: [
232232
PrimaryDiagnostic(
233233
level: .error,
234-
text: "error",
234+
text: "error1",
235235
location: .init(filename: filename, line: 1, column: 1, offset: 0),
236236
fixIts: [
237237
// Applied.
@@ -244,7 +244,7 @@ final class BasicTests: XCTestCase {
244244
),
245245
PrimaryDiagnostic(
246246
level: .error,
247-
text: "error",
247+
text: "error2",
248248
location: .init(filename: filename, line: 1, column: 1, offset: 0),
249249
fixIts: [
250250
// Skipped, overlaps with previous fix-it.
@@ -271,7 +271,7 @@ final class BasicTests: XCTestCase {
271271
// filename1
272272
PrimaryDiagnostic(
273273
level: .warning,
274-
text: "warning",
274+
text: "warning1",
275275
location: .init(filename: filename1, line: 1, column: 1, offset: 0),
276276
fixIts: [
277277
.init(
@@ -283,7 +283,7 @@ final class BasicTests: XCTestCase {
283283
),
284284
PrimaryDiagnostic(
285285
level: .error,
286-
text: "error",
286+
text: "error1",
287287
location: .init(filename: filename1, line: 1, column: 1, offset: 0),
288288
fixIts: [
289289
.init(
@@ -296,7 +296,7 @@ final class BasicTests: XCTestCase {
296296
// filename2
297297
PrimaryDiagnostic(
298298
level: .warning,
299-
text: "warning",
299+
text: "warning2",
300300
location: .init(filename: filename2, line: 1, column: 1, offset: 0),
301301
fixIts: [
302302
.init(
@@ -308,7 +308,7 @@ final class BasicTests: XCTestCase {
308308
),
309309
PrimaryDiagnostic(
310310
level: .error,
311-
text: "error",
311+
text: "error2",
312312
location: .init(filename: filename2, line: 1, column: 1, offset: 0),
313313
fixIts: [
314314
.init(

‎Tests/SwiftFixItTests/FilteringTests.swift

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,97 @@ final class FilteringTests: XCTestCase {
231231
}
232232
}
233233

234+
func testDuplicatePrimaryDiag() throws {
235+
try testAPI1File { filename in
236+
.init(
237+
edits: .init(input: "var x = (1, 1)", result: "let x = (22, 13)"),
238+
diagnostics: [
239+
PrimaryDiagnostic(
240+
level: .error,
241+
text: "error1",
242+
location: .init(filename: filename, line: 1, column: 1, offset: 0),
243+
fixIts: [
244+
// Applied.
245+
.init(
246+
start: .init(filename: filename, line: 1, column: 1, offset: 0),
247+
end: .init(filename: filename, line: 1, column: 4, offset: 0),
248+
text: "let"
249+
),
250+
]
251+
),
252+
PrimaryDiagnostic(
253+
level: .warning,
254+
text: "warning1",
255+
location: .init(filename: filename, line: 1, column: 10, offset: 0),
256+
notes: [
257+
Note(
258+
text: "warning1_note1",
259+
location: .init(filename: filename, line: 1, column: 10, offset: 0),
260+
fixIts: [
261+
// Applied.
262+
.init(
263+
start: .init(filename: filename, line: 1, column: 10, offset: 0),
264+
end: .init(filename: filename, line: 1, column: 11, offset: 0),
265+
text: "22"
266+
),
267+
]
268+
),
269+
Note(
270+
text: "warning1_note2",
271+
location: .init(filename: filename, line: 1, column: 5, offset: 0),
272+
),
273+
]
274+
),
275+
PrimaryDiagnostic(
276+
level: .error,
277+
text: "error1",
278+
location: .init(filename: filename, line: 1, column: 1, offset: 0),
279+
notes: [
280+
Note(
281+
text: "error1_note1",
282+
location: .init(filename: filename, line: 1, column: 5, offset: 0),
283+
fixIts: [
284+
// Skipped, duplicate.
285+
.init(
286+
start: .init(filename: filename, line: 1, column: 5, offset: 0),
287+
end: .init(filename: filename, line: 1, column: 6, offset: 0),
288+
text: "y"
289+
),
290+
]
291+
),
292+
]
293+
),
294+
PrimaryDiagnostic(
295+
level: .warning,
296+
text: "warning1",
297+
location: .init(filename: filename, line: 1, column: 10, offset: 0),
298+
fixIts: [
299+
// Skipped, duplicate.
300+
.init(
301+
start: .init(filename: filename, line: 1, column: 7, offset: 0),
302+
end: .init(filename: filename, line: 1, column: 8, offset: 0),
303+
text: ":"
304+
),
305+
]
306+
),
307+
PrimaryDiagnostic(
308+
level: .error,
309+
text: "error2",
310+
location: .init(filename: filename, line: 1, column: 14, offset: 0),
311+
fixIts: [
312+
// Applied.
313+
.init(
314+
start: .init(filename: filename, line: 1, column: 14, offset: 0),
315+
end: .init(filename: filename, line: 1, column: 14, offset: 0),
316+
text: "3"
317+
),
318+
]
319+
),
320+
]
321+
)
322+
}
323+
}
324+
234325
func testDuplicateReplacementFixIts() throws {
235326
try testAPI1File { (filename: String) in
236327
.init(

0 commit comments

Comments
 (0)