Skip to content

Fetch full documentation in code completion #2207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Sources/Csourcekitd/include/CodeCompletionSwiftInterop.h
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,12 @@ typedef struct {
void (^_Null_unspecified handler)(const char *_Null_unspecified)
);

void (*_Nonnull completion_item_get_doc_full_copy)(
_Null_unspecified swiftide_api_completion_response_t,
_Null_unspecified swiftide_api_completion_item_t,
void (^_Null_unspecified handler)(char *_Null_unspecified)
);

void (*_Nonnull completion_item_get_associated_usrs)(
_Null_unspecified swiftide_api_completion_response_t,
_Null_unspecified swiftide_api_completion_item_t,
Expand Down
1 change: 1 addition & 0 deletions Sources/SourceKitD/sourcekitd_functions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ extension sourcekitd_ide_api_functions_t {
completion_item_get_source_text: try loadRequired("swiftide_completion_item_get_source_text"),
completion_item_get_type_name: try loadRequired("swiftide_completion_item_get_type_name"),
completion_item_get_doc_brief: try loadRequired("swiftide_completion_item_get_doc_brief"),
completion_item_get_doc_full_copy: try loadRequired("swiftide_completion_item_get_doc_full_copy"),
completion_item_get_associated_usrs: try loadRequired("swiftide_completion_item_get_associated_usrs"),
completion_item_get_kind: try loadRequired("swiftide_completion_item_get_kind"),
completion_item_get_associated_kind: try loadRequired("swiftide_completion_item_get_associated_kind"),
Expand Down
12 changes: 11 additions & 1 deletion Sources/SourceKitLSP/Swift/CodeCompletionSession.swift
Original file line number Diff line number Diff line change
Expand Up @@ -574,13 +574,23 @@ class CodeCompletionSession {
fileContents: nil
)
}
if let docString: String = documentationResponse?[sourcekitd.keys.docBrief] {

if let response = documentationResponse,
let docString = documentationString(from: response, sourcekitd: sourcekitd) {
item.documentation = .markupContent(MarkupContent(kind: .markdown, value: docString))
}
}
return item
}

private static func documentationString(from response: SKDResponseDictionary, sourcekitd: SourceKitD) -> String? {
if let docFullAsXML: String = response[sourcekitd.keys.docFullAsXML] {
return try? xmlDocumentationToMarkdown(docFullAsXML)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have noticed this before: We generally prefer to use orLog over try? because it means that the error will get logged instead of silently swallowing it, which helps debugging.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ✅

}

return response[sourcekitd.keys.docBrief]
}

private func computeCompletionTextEdit(
completionPos: Position,
requestPosition: Position,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,17 @@ struct ExtendedCompletionInfo {
return result
}

var fullDocumentation: String? {
var result: String? = nil
session.sourcekitd.ideApi.completion_item_get_doc_full_copy(session.response, rawItem) {
if let cstr = $0 {
result = String(cString: cstr)
free(cstr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to free the pointer here? I would assume that the memory is still owned by sourcedkitd since it only yields the pointer to us in a closure and is thus still responsible for its lifetime.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourcekitd doesn't keep full documentation comments in-memory, it prints them in a temporary buffer and passes a copy through strdup that clients should free (hence the name is suffixed with _copy).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we pass ownership to the client, I think completion_item_get_doc_full_copy should return the pointer. E.g. like swiftide_completion_result_description_copy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just modified completion_item_get_doc_full to not strdup the string and pass it as is to the handler, and SourceKit-LSP will copy it into a Swift String anyway and it doesn't have to manually call free.

Can you please recheck?

}
}
return result
}

var associatedUSRs: [String] {
var result: [String] = []
session.sourcekitd.ideApi.completion_item_get_associated_usrs(session.response, rawItem) { ptr, len in
Expand Down
13 changes: 10 additions & 3 deletions Sources/SwiftSourceKitPlugin/CompletionProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -262,10 +262,17 @@ actor CompletionProvider {
func handleCompletionDocumentation(_ request: SKDRequestDictionaryReader) throws -> SKDResponseDictionaryBuilder {
let info = try handleExtendedCompletionRequest(request)

return request.sourcekitd.responseDictionary([
request.sourcekitd.keys.docBrief: info.briefDocumentation,
request.sourcekitd.keys.associatedUSRs: info.associatedUSRs as [SKDResponseValue]?,
var response = request.sourcekitd.responseDictionary([
request.sourcekitd.keys.associatedUSRs: info.associatedUSRs as [SKDResponseValue]?
])

if let fullDocumentation = info.fullDocumentation {
response.set(request.sourcekitd.keys.docFullAsXML, to: fullDocumentation)
} else {
response.set(request.sourcekitd.keys.docBrief, to: info.briefDocumentation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: Does it make sense to also return the brief documentation in addition to the full documentation? Eg. an editor might want to display a brief documentation of the symbol by default and then expand that to full documentation once the user performs an action.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a specific flow in mind? AFAIK this doesn't happen in code completion since documentation is only shown when the completion item is selected in the completions list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, you could think about a UI where the brief documentation is displayed in the completion list (similar to how Xcode shows documentation) and only showing the full documentation when the user requests it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the plugin is used in SourceKit-LSP only, no? If so, then we don't have to return the brief documentation now as we don't need it. We can always add it later if we found a use case for it.

If not, it'd probably make sense to return both yes, especially if it's used by Xcode which already has this use case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin can potentially be used by any SourceKit client yes, so I think we should return both by default. We can always add options to the request for cases where the client only wants the brief or full doc

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it makes sense, thanks for the clarification @ahoppen and @hamishknight 🙏🏼

I've changed the request to return both now.

}

return response
}

func handleCompletionDiagnostic(_ dict: SKDRequestDictionaryReader) throws -> SKDResponseDictionaryBuilder {
Expand Down
67 changes: 62 additions & 5 deletions Tests/SourceKitLSPTests/SwiftCompletionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,15 @@ final class SwiftCompletionTests: XCTestCase {
if let abc = abc {
XCTAssertEqual(abc.kind, .property)
XCTAssertEqual(abc.detail, "Int")
XCTAssertEqual(abc.documentation, .markupContent(MarkupContent(kind: .markdown, value: "Documentation for abc.")))
assertMarkdown(
documentation: abc.documentation,
expected: """
```swift
var abc: Int
```
Documentation for `abc`.
"""
)
XCTAssertEqual(abc.filterText, "abc")
XCTAssertEqual(abc.textEdit, .textEdit(TextEdit(range: Range(positions["1️⃣"]), newText: "abc")))
XCTAssertEqual(abc.insertText, "abc")
Expand All @@ -87,7 +95,15 @@ final class SwiftCompletionTests: XCTestCase {
// If we switch to server-side filtering this will change.
XCTAssertEqual(abc.kind, .property)
XCTAssertEqual(abc.detail, "Int")
XCTAssertEqual(abc.documentation, .markupContent(MarkupContent(kind: .markdown, value: "Documentation for abc.")))
assertMarkdown(
documentation: abc.documentation,
expected: """
```swift
var abc: Int
```
Documentation for `abc`.
"""
)
XCTAssertEqual(abc.filterText, "abc")
XCTAssertEqual(abc.textEdit, .textEdit(TextEdit(range: positions["1️⃣"]..<offsetPosition, newText: "abc")))
XCTAssertEqual(abc.insertText, "abc")
Expand Down Expand Up @@ -1187,9 +1203,41 @@ final class SwiftCompletionTests: XCTestCase {
let item = try XCTUnwrap(completions.items.only)
XCTAssertNil(item.documentation)
let resolvedItem = try await testClient.send(CompletionItemResolveRequest(item: item))
XCTAssertEqual(
resolvedItem.documentation,
.markupContent(MarkupContent(kind: .markdown, value: "Creates a true value"))
assertMarkdown(
documentation: resolvedItem.documentation,
expected: """
```swift
func makeBool() -> Bool
```
Creates a true value
"""
)
}

func testCompletionBriefDocumentationFallback() async throws {
try await SkipUnless.sourcekitdSupportsPlugin()

let testClient = try await TestSourceKitLSPClient()
let uri = DocumentURI(for: .swift)

// We test completion for result builder build functions since they don't have full documentation
// but still have brief documentation.
let positions = testClient.openDocument(
"""
@resultBuilder
struct AnyBuilder {
static func 1️⃣
}
""",
uri: uri
)
let completions = try await testClient.send(
CompletionRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
)
let item = try XCTUnwrap(completions.items.filter { $0.label.contains("buildBlock") }.only)
assertMarkdown(
documentation: item.documentation,
expected: "Required by every result builder to build combined results from statement blocks"
)
}

Expand Down Expand Up @@ -1253,6 +1301,15 @@ private func countFs(_ response: CompletionList) -> Int {
return response.items.filter { $0.label.hasPrefix("f") }.count
}

private func assertMarkdown(
documentation: StringOrMarkupContent?,
expected: String,
file: StaticString = #filePath,
line: UInt = #line
) {
XCTAssertEqual(documentation, .markupContent(MarkupContent(kind: .markdown, value: expected)))
}

fileprivate extension Position {
func adding(columns: Int) -> Position {
return Position(line: line, utf16index: utf16index + columns)
Expand Down
31 changes: 25 additions & 6 deletions Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ final class SwiftSourceKitPluginTests: XCTestCase {
XCTAssertEqual(result2.items.count, 1)
XCTAssertEqual(result2.items[0].name, "")
let doc = try await sourcekitd.completeDocumentation(id: result2.items[0].id)
XCTAssertEqual(doc.docBrief, nil)
XCTAssertEqual(doc.docFullAsXML, nil)
}

func testMultipleFiles() async throws {
Expand Down Expand Up @@ -436,15 +436,34 @@ final class SwiftSourceKitPluginTests: XCTestCase {
let sym3 = try unwrap(result.items.first(where: { $0.name == "foo3()" }), "did not find foo3; got \(result.items)")

let sym1Doc = try await sourcekitd.completeDocumentation(id: sym1.id)
XCTAssertEqual(sym1Doc.docBrief, "Protocol P foo1")
XCTAssertEqual(sym1Doc.docFullAsXML,
"<Function file=\"\(path)\" line=\"3\" column=\"8\">"
+ "<Name>foo1()</Name>"
+ "<USR>s:1a1PP4foo1yyF</USR>"
+ "<Declaration>func foo1()</Declaration>"
+ "<CommentParts>"
+ "<Abstract><Para>Protocol P foo1</Para></Abstract>"
+ "<Discussion><Note>"
+ "<Para>This documentation comment was inherited from <codeVoice>P</codeVoice>.</Para>"
+ "</Note></Discussion>"
+ "</CommentParts>"
+ "</Function>")
XCTAssertEqual(sym1Doc.associatedUSRs, ["s:1a1SV4foo1yyF", "s:1a1PP4foo1yyF"])

let sym2Doc = try await sourcekitd.completeDocumentation(id: sym2.id)
XCTAssertEqual(sym2Doc.docBrief, "Struct S foo2")
XCTAssertEqual(sym2Doc.docFullAsXML,
"<Function file=\"\(path)\" line=\"8\" column=\"8\">"
+ "<Name>foo2()</Name>"
+ "<USR>s:1a1SV4foo2yyF</USR>"
+ "<Declaration>func foo2()</Declaration>"
+ "<CommentParts>"
+ "<Abstract><Para>Struct S foo2</Para></Abstract>"
+ "</CommentParts>"
+ "</Function>")
XCTAssertEqual(sym2Doc.associatedUSRs, ["s:1a1SV4foo2yyF"])

let sym3Doc = try await sourcekitd.completeDocumentation(id: sym3.id)
XCTAssertNil(sym3Doc.docBrief)
XCTAssertNil(sym3Doc.docFullAsXML)
XCTAssertEqual(sym3Doc.associatedUSRs, ["s:1a1SV4foo3yyF"])
}

Expand Down Expand Up @@ -1766,12 +1785,12 @@ fileprivate struct CompletionResult: Equatable, Sendable {
}

fileprivate struct CompletionDocumentation {
var docBrief: String? = nil
var docFullAsXML: String? = nil
var associatedUSRs: [String] = []

init(_ dict: SKDResponseDictionary) {
let keys = dict.sourcekitd.keys
self.docBrief = dict[keys.docBrief]
self.docFullAsXML = dict[keys.docFullAsXML]
self.associatedUSRs = dict[keys.associatedUSRs]?.asStringArray ?? []
}
}
Expand Down