Skip to content

Commit fa8acc1

Browse files
benlangmuirbenlangmuir
committed
[fixit] Handle multi-edit fixits
We were treating arrays of fixits as if they were independent actions, but in reality we have at most one quick-fix per diagnostic, which is composed of multiple edits. This fixes cases like renaming a deprecated method where there are multiple edits that need to be combined.
1 parent 206e364 commit fa8acc1

File tree

4 files changed

+209
-36
lines changed

4 files changed

+209
-36
lines changed

‎Sources/LSPLogging/Logging.swift

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,16 @@ public func logAsync(level: LogLevel = .default, messageProducer: @escaping (_ c
3737
Logger.shared.logAsync(level: level, messageProducer: messageProducer)
3838
}
3939

40+
/// Log an error and trigger an assertion failure (if compiled with assertions).
41+
///
42+
/// If `level >= Logger.shared.currentLevel`, it will be emitted. However, the converse is not necessarily true: on platforms that provide `os_log`, the message may be emitted by `os_log` according to its own rules about log level.
43+
///
44+
/// - parameter message: The message to print.
45+
public func logAssertionFailure(_ message: String, file: StaticString = #file, line: UInt = #line) {
46+
Logger.shared.log(message, level: .error)
47+
assertionFailure(message, file: file, line: line)
48+
}
49+
4050
/// Like `try?`, but logs the error on failure.
4151
public func orLog<R>(
4252
_ prefix: String = "",

‎Sources/SourceKit/sourcekitd/Diagnostic.swift

Lines changed: 102 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -16,32 +16,56 @@ import SKSupport
1616
import sourcekitd
1717

1818
extension CodeAction {
19-
init?(fixit: SKResponseDictionary, in snapshot: DocumentSnapshot) {
20-
let keys = fixit.sourcekitd.keys
2119

22-
guard let utf8Offset: Int = fixit[keys.offset],
23-
let length: Int = fixit[keys.length],
24-
let replacement: String = fixit[keys.sourcetext],
25-
let position = snapshot.positionOf(utf8Offset: utf8Offset),
26-
let endPosition = snapshot.positionOf(utf8Offset: utf8Offset + length),
27-
let startIndex = snapshot.indexOf(utf8Offset: utf8Offset),
28-
let endIndex = snapshot.indexOf(utf8Offset: utf8Offset + length),
29-
length > 0 || !replacement.isEmpty
30-
else {
20+
/// Creates a CodeAction from a list for sourcekit fixits.
21+
///
22+
/// If this is from a note, the note's description should be passed as `fromNote`.
23+
init?(fixits: SKResponseArray, in snapshot: DocumentSnapshot, fromNote: String?) {
24+
var edits: [TextEdit] = []
25+
let editsMapped = fixits.forEach { (_, skfixit) -> Bool in
26+
if let edit = TextEdit(fixit: skfixit, in: snapshot) {
27+
edits.append(edit)
28+
return true
29+
}
30+
return false
31+
}
32+
33+
if !editsMapped {
34+
log("failed to construct TextEdits from response \(fixits)", level: .warning)
3135
return nil
3236
}
3337

34-
let range = position..<endPosition
35-
let original = String(snapshot.text[startIndex..<endIndex])
36-
let title = Self.fixitTitle(replace: original, with: replacement)
37-
let workspaceEdit = WorkspaceEdit(
38-
changes: [snapshot.document.uri:[TextEdit(range: range, newText: replacement)]])
38+
if edits.isEmpty {
39+
return nil
40+
}
41+
42+
let title: String
43+
if let fromNote = fromNote {
44+
title = fromNote
45+
} else {
46+
guard let startIndex = snapshot.index(of: edits[0].range.lowerBound),
47+
let endIndex = snapshot.index(of: edits[0].range.upperBound),
48+
startIndex <= endIndex,
49+
snapshot.text.indices.contains(startIndex),
50+
endIndex <= snapshot.text.endIndex
51+
else {
52+
logAssertionFailure("position mapped, but indices failed for edit range \(edits[0])")
53+
return nil
54+
}
55+
let oldText = String(snapshot.text[startIndex..<endIndex])
56+
let description = Self.fixitTitle(replace: oldText, with: edits[0].newText)
57+
if edits.count == 1 {
58+
title = description
59+
} else {
60+
title = description + "..."
61+
}
62+
}
3963

4064
self.init(
4165
title: title,
4266
kind: .quickFix,
4367
diagnostics: nil,
44-
edit: workspaceEdit)
68+
edit: WorkspaceEdit(changes: [snapshot.document.uri:edits]))
4569
}
4670

4771
/// Describe a fixit's edit briefly.
@@ -61,6 +85,25 @@ extension CodeAction {
6185
}
6286
}
6387

88+
extension TextEdit {
89+
90+
/// Creates a TextEdit from a sourcekitd fixit response dictionary.
91+
init?(fixit: SKResponseDictionary, in snapshot: DocumentSnapshot) {
92+
let keys = fixit.sourcekitd.keys
93+
if let utf8Offset: Int = fixit[keys.offset],
94+
let length: Int = fixit[keys.length],
95+
let replacement: String = fixit[keys.sourcetext],
96+
let position = snapshot.positionOf(utf8Offset: utf8Offset),
97+
let endPosition = snapshot.positionOf(utf8Offset: utf8Offset + length),
98+
length > 0 || !replacement.isEmpty
99+
{
100+
self.init(range: position..<endPosition, newText: replacement)
101+
} else {
102+
return nil
103+
}
104+
}
105+
}
106+
64107
extension Diagnostic {
65108

66109
/// Creates a diagnostic from a sourcekitd response dictionary.
@@ -98,26 +141,18 @@ extension Diagnostic {
98141
}
99142
}
100143

101-
var fixits: [CodeAction]? = nil
102-
if let skfixits: SKResponseArray = diag[keys.fixits] {
103-
fixits = []
104-
skfixits.forEach { (_, skfixit) -> Bool in
105-
if let codeAction = CodeAction(fixit: skfixit, in: snapshot) {
106-
fixits?.append(codeAction)
107-
}
108-
return true
109-
}
144+
var actions: [CodeAction]? = nil
145+
if let skfixits: SKResponseArray = diag[keys.fixits],
146+
let action = CodeAction(fixits: skfixits, in: snapshot, fromNote: nil) {
147+
actions = [action]
110148
}
111149

112150
var notes: [DiagnosticRelatedInformation]? = nil
113151
if let sknotes: SKResponseArray = diag[keys.diagnostics] {
114152
notes = []
115153
sknotes.forEach { (_, sknote) -> Bool in
116-
guard let note = Diagnostic(sknote, in: snapshot) else { return true }
117-
notes?.append(DiagnosticRelatedInformation(
118-
location: Location(uri: snapshot.document.uri, range: note.range),
119-
message: note.message,
120-
codeActions: note.codeActions))
154+
guard let note = DiagnosticRelatedInformation(sknote, in: snapshot) else { return true }
155+
notes?.append(note)
121156
return true
122157
}
123158
}
@@ -129,7 +164,42 @@ extension Diagnostic {
129164
source: "sourcekitd",
130165
message: message,
131166
relatedInformation: notes,
132-
codeActions: fixits)
167+
codeActions: actions)
168+
}
169+
}
170+
171+
extension DiagnosticRelatedInformation {
172+
173+
/// Creates related information from a sourcekitd note response dictionary.
174+
init?(_ diag: SKResponseDictionary, in snapshot: DocumentSnapshot) {
175+
let keys = diag.sourcekitd.keys
176+
177+
var position: Position? = nil
178+
if let line: Int = diag[keys.line],
179+
let utf8Column: Int = diag[keys.column],
180+
line > 0, utf8Column > 0
181+
{
182+
position = snapshot.positionOf(zeroBasedLine: line - 1, utf8Column: utf8Column - 1)
183+
} else if let utf8Offset: Int = diag[keys.offset] {
184+
position = snapshot.positionOf(utf8Offset: utf8Offset)
185+
}
186+
187+
if position == nil {
188+
return nil
189+
}
190+
191+
guard let message: String = diag[keys.description] else { return nil }
192+
193+
var actions: [CodeAction]? = nil
194+
if let skfixits: SKResponseArray = diag[keys.fixits],
195+
let action = CodeAction(fixits: skfixits, in: snapshot, fromNote: message) {
196+
actions = [action]
197+
}
198+
199+
self.init(
200+
location: Location(uri: snapshot.document.uri, range: Range(position!)),
201+
message: message,
202+
codeActions: actions)
133203
}
134204
}
135205

‎Tests/SourceKitTests/LocalSwiftTests.swift

Lines changed: 95 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -478,7 +478,7 @@ final class LocalSwiftTests: XCTestCase {
478478
// Expected Fix-it: Replace `let a` with `_` because it's never used
479479
let expectedTextEdit = TextEdit(range: Position(line: 1, utf16index: 7)..<Position(line: 1, utf16index: 7), newText: "?")
480480
XCTAssertEqual(fixit, CodeAction(
481-
title: "Insert '?'",
481+
title: "chain the optional using '?' to access member 'bigEndian' only for non-'nil' base values",
482482
kind: .quickFix,
483483
diagnostics: nil,
484484
edit: WorkspaceEdit(changes: [uri: [expectedTextEdit]], documentChanges: nil),
@@ -493,7 +493,7 @@ final class LocalSwiftTests: XCTestCase {
493493
// Expected Fix-it: Replace `let a` with `_` because it's never used
494494
let expectedTextEdit = TextEdit(range: Position(line: 1, utf16index: 7)..<Position(line: 1, utf16index: 7), newText: "!")
495495
XCTAssertEqual(fixit, CodeAction(
496-
title: "Insert '!'",
496+
title: "force-unwrap using '!' to abort execution if the optional value contains 'nil'",
497497
kind: .quickFix,
498498
diagnostics: nil,
499499
edit: WorkspaceEdit(changes: [uri: [expectedTextEdit]], documentChanges: nil),
@@ -638,11 +638,11 @@ final class LocalSwiftTests: XCTestCase {
638638

639639
for fixit in quickFixes {
640640
if fixit.title.contains("!") {
641-
XCTAssertEqual(fixit.title, "Insert '!'")
641+
XCTAssert(fixit.title.starts(with: "force-unwrap using '!'"))
642642
expectedTextEdit.newText = "!"
643643
XCTAssertEqual(fixit.edit, WorkspaceEdit(changes: [uri: [expectedTextEdit]], documentChanges: nil))
644644
} else {
645-
XCTAssertEqual(fixit.title, "Insert '?'")
645+
XCTAssert(fixit.title.starts(with: "chain the optional using '?'"))
646646
expectedTextEdit.newText = "?"
647647
XCTAssertEqual(fixit.edit, WorkspaceEdit(changes: [uri: [expectedTextEdit]], documentChanges: nil))
648648
}
@@ -654,6 +654,97 @@ final class LocalSwiftTests: XCTestCase {
654654
}
655655
}
656656

657+
func testMuliEditFixitCodeActionPrimary() {
658+
let url = URL(fileURLWithPath: "/a.swift")
659+
let uri = DocumentURI(url)
660+
661+
var diagnostic: Diagnostic! = nil
662+
sk.sendNoteSync(DidOpenTextDocumentNotification(textDocument: TextDocumentItem(
663+
uri: uri, language: .swift, version: 12,
664+
text: """
665+
@available(*, introduced: 10, deprecated: 11)
666+
func foo() {}
667+
"""
668+
)), { (note: Notification<PublishDiagnosticsNotification>) in
669+
log("Received diagnostics for open - syntactic")
670+
}, { (note: Notification<PublishDiagnosticsNotification>) in
671+
log("Received diagnostics for open - semantic")
672+
XCTAssertEqual(note.params.diagnostics.count, 1)
673+
diagnostic = note.params.diagnostics.first!
674+
})
675+
676+
let request = CodeActionRequest(
677+
range: Position(line: 0, utf16index: 1)..<Position(line: 0, utf16index: 10),
678+
context: CodeActionContext(diagnostics: [diagnostic], only: nil),
679+
textDocument: TextDocumentIdentifier(uri)
680+
)
681+
let response = try! sk.sendSync(request)
682+
683+
XCTAssertNotNil(response)
684+
guard case .codeActions(let codeActions) = response else {
685+
XCTFail("Expected code actions as response")
686+
return
687+
}
688+
let quickFixes = codeActions.filter{ $0.kind == .quickFix }
689+
XCTAssertEqual(quickFixes.count, 1)
690+
guard let fixit = quickFixes.first else { return }
691+
692+
XCTAssertEqual(fixit.title, "Remove ': 10'...")
693+
XCTAssertEqual(fixit.diagnostics?.count, 1)
694+
XCTAssertEqual(fixit.edit?.changes?[uri], [
695+
TextEdit(range: Position(line: 0, utf16index: 24)..<Position(line: 0, utf16index: 28), newText: ""),
696+
TextEdit(range: Position(line: 0, utf16index: 40)..<Position(line: 0, utf16index: 44), newText: ""),
697+
])
698+
}
699+
700+
func testMuliEditFixitCodeActionNote() {
701+
let url = URL(fileURLWithPath: "/a.swift")
702+
let uri = DocumentURI(url)
703+
704+
var diagnostic: Diagnostic! = nil
705+
sk.sendNoteSync(DidOpenTextDocumentNotification(textDocument: TextDocumentItem(
706+
uri: uri, language: .swift, version: 12,
707+
text: """
708+
@available(*, deprecated, renamed: "new(_:hotness:)")
709+
func old(and: Int, busted: Int) {}
710+
func test() {
711+
old(and: 1, busted: 2)
712+
}
713+
"""
714+
)), { (note: Notification<PublishDiagnosticsNotification>) in
715+
log("Received diagnostics for open - syntactic")
716+
}, { (note: Notification<PublishDiagnosticsNotification>) in
717+
log("Received diagnostics for open - semantic")
718+
XCTAssertEqual(note.params.diagnostics.count, 1)
719+
diagnostic = note.params.diagnostics.first!
720+
})
721+
722+
let request = CodeActionRequest(
723+
range: Position(line: 3, utf16index: 2)..<Position(line: 3, utf16index: 2),
724+
context: CodeActionContext(diagnostics: [diagnostic], only: nil),
725+
textDocument: TextDocumentIdentifier(uri)
726+
)
727+
let response = try! sk.sendSync(request)
728+
729+
XCTAssertNotNil(response)
730+
guard case .codeActions(let codeActions) = response else {
731+
XCTFail("Expected code actions as response")
732+
return
733+
}
734+
let quickFixes = codeActions.filter{ $0.kind == .quickFix }
735+
XCTAssertEqual(quickFixes.count, 1)
736+
guard let fixit = quickFixes.first else { return }
737+
738+
XCTAssertEqual(fixit.title, "use 'new(_:hotness:)' instead")
739+
XCTAssertEqual(fixit.diagnostics?.count, 1)
740+
XCTAssert(fixit.diagnostics?.first?.message.contains("is deprecated") == true)
741+
XCTAssertEqual(fixit.edit?.changes?[uri], [
742+
TextEdit(range: Position(line: 3, utf16index: 2)..<Position(line: 3, utf16index: 5), newText: "new"),
743+
TextEdit(range: Position(line: 3, utf16index: 6)..<Position(line: 3, utf16index: 11), newText: ""),
744+
TextEdit(range: Position(line: 3, utf16index: 14)..<Position(line: 3, utf16index: 20), newText: "hotness"),
745+
])
746+
}
747+
657748
func testXMLToMarkdownDeclaration() {
658749
XCTAssertEqual(try! xmlDocumentationToMarkdown("""
659750
<Declaration>func foo(_ bar: <Type usr="fake">Baz</Type>)</Declaration>

‎Tests/SourceKitTests/XCTestManifests.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,8 @@ extension LocalSwiftTests {
117117
("testFixitTitle", testFixitTitle),
118118
("testHover", testHover),
119119
("testHoverNameEscaping", testHoverNameEscaping),
120+
("testMuliEditFixitCodeActionNote", testMuliEditFixitCodeActionNote),
121+
("testMuliEditFixitCodeActionPrimary", testMuliEditFixitCodeActionPrimary),
120122
("testSymbolInfo", testSymbolInfo),
121123
("testXMLToMarkdownComment", testXMLToMarkdownComment),
122124
("testXMLToMarkdownDeclaration", testXMLToMarkdownDeclaration),

0 commit comments

Comments
 (0)