-
Notifications
You must be signed in to change notification settings - Fork 320
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
24de753
283e08c
fce98d5
ae3f00e
2b40e78
db44320
3c66365
4df7ea1
ed77735
4f8d69e
d3ed8b1
9d56a93
2f77852
c0b4ca2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we pass ownership to the client, I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just modified 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]?, | ||
let 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,10 +10,13 @@ | |
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import Csourcekitd | ||
import LanguageServerProtocol | ||
import SKTestSupport | ||
import SourceKitD | ||
import SourceKitLSP | ||
import SwiftExtensions | ||
import ToolchainRegistry | ||
import XCTest | ||
|
||
final class SwiftCompletionTests: XCTestCase { | ||
|
@@ -67,7 +70,16 @@ 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."))) | ||
try await assertDocumentation( | ||
documentation: abc.documentation, | ||
expectedBrief: "Documentation for abc.", | ||
expectedFull: """ | ||
```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") | ||
|
@@ -87,7 +99,16 @@ 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."))) | ||
try await assertDocumentation( | ||
documentation: abc.documentation, | ||
expectedBrief: "Documentation for abc.", | ||
expectedFull: """ | ||
```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") | ||
|
@@ -1187,9 +1208,45 @@ 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")) | ||
try await assertDocumentation( | ||
documentation: resolvedItem.documentation, | ||
expectedBrief: "Creates a true value", | ||
expectedFull: """ | ||
```swift | ||
func makeBool() -> Bool | ||
``` | ||
Creates a true value | ||
""" | ||
) | ||
} | ||
|
||
func testCompletionBriefDocumentationFallback() async throws { | ||
try await SkipUnless.sourcekitdSupportsPlugin() | ||
|
||
let fullDocumentationSupported = try await sourcekitdSupportsFullDocumentation() | ||
try XCTSkipUnless(fullDocumentationSupported) | ||
|
||
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" | ||
) | ||
} | ||
|
||
|
@@ -1253,6 +1310,39 @@ 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))) | ||
} | ||
|
||
/// Asserts that documentation matches the expected values based on whether full documentation is supported in sourcekitd or not. | ||
private func assertDocumentation( | ||
documentation: StringOrMarkupContent?, | ||
expectedBrief: String, | ||
expectedFull: String, | ||
file: StaticString = #filePath, | ||
line: UInt = #line | ||
) async throws { | ||
let supportsFullDocumentation = try await sourcekitdSupportsFullDocumentation() | ||
let expected = supportsFullDocumentation ? expectedFull : expectedBrief | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m a little concerned with executing a different test path depending on whether sourcekitd supports full documentation because in Swift CI I imagine the sourcekitd will always support full documentation which means that the Instead, I would prefer to add a function to Same comment for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only concern is that tests like What do you think? Should we just assume full documentation exists with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed we'd just skip them if full documentation is not supported with no tests for brief documentation alone (avoiding duplication & relying on the fact that Swift CI is going to have full documentation support anyway) to speedup review. If you'd prefer another approach please let me know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CI should be able to execute all tests. Otherwise tests can break and you don’t notice until you run them locally. So, since CI will only able to test full documentation and the brief documentation part is being phased out, we should just test full documentation support. |
||
|
||
assertMarkdown(documentation: documentation, expected: expected, file: file, line: line) | ||
} | ||
|
||
private func sourcekitdSupportsFullDocumentation() async throws -> Bool { | ||
let sourcekitdPath = await ToolchainRegistry.forTesting.default!.sourcekitd! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed now that we're using |
||
let sourcekitd = try await SourceKitD.getOrCreate( | ||
dylibPath: sourcekitdPath, | ||
pluginPaths: sourceKitPluginPaths | ||
) | ||
|
||
return sourcekitd.ideApi.completion_item_get_doc_full_copy != nil | ||
} | ||
|
||
fileprivate extension Position { | ||
func adding(columns: Int) -> Position { | ||
return Position(line: line, utf16index: utf16index + columns) | ||
|
There was a problem hiding this comment.
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
overtry?
because it means that the error will get logged instead of silently swallowing it, which helps debugging.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅