Skip to content
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

Implement completionItem/resolve #432

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Sources/LanguageServerProtocol/Messages.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public let builtinRequests: [_RequestType.Type] = [
ShutdownRequest.self,
WorkspaceFoldersRequest.self,
CompletionRequest.self,
CompletionItem.self,
HoverRequest.self,
WorkspaceSemanticTokensRefreshRequest.self,
WorkspaceSymbolsRequest.self,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,23 @@
//===----------------------------------------------------------------------===//

/// A single completion result.
public struct CompletionItem: Codable, Hashable {
public struct CompletionItem: TextDocumentRequest, RequestType, ResponseType, Codable, Hashable {
ahoppen marked this conversation as resolved.
Show resolved Hide resolved
public typealias Response = CompletionItem

public var textDocument: TextDocumentIdentifier {
if
let data = self.data,
case let .dictionary(dict) = data,
case let .string(textDocURI) = dict["textDocURI"]
{
return TextDocumentIdentifier(DocumentURI(string: textDocURI))
} else {
fatalError("Missing textDocURI in CompletionItem.data: \(data ?? "(CompletionItem.data is nil)")")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think we should make any assumptions about the requests sent to us by the client and, in particular, not crash if it is malformed. Could we return a failure value (e.g.nil) here instead of fatalErroring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is TextDocumentIdentifier(DocumentURI(string: "")) ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you are not using this computed property at all, so let’s just remove it.

}
}

public static var method: String = "completionItem/resolve"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the method property is always the first property in a request type. Could you move it up to match this style?



/// The display name of the completion.
public var label: String
Expand Down Expand Up @@ -48,6 +64,9 @@ public struct CompletionItem: Codable, Hashable {

/// Whether the completion is for a deprecated symbol.
public var deprecated: Bool?

/// A data entry field that is preserved on a `CompletionItem` between a completion and a completion resolve request.
public var data: LSPAny?

public init(
label: String,
Expand All @@ -59,7 +78,8 @@ public struct CompletionItem: Codable, Hashable {
textEdit: TextEdit? = nil,
insertText: String? = nil,
insertTextFormat: InsertTextFormat? = nil,
deprecated: Bool? = nil)
deprecated: Bool? = nil,
data: LSPAny? = nil)
{
self.label = label
self.detail = detail
Expand All @@ -71,6 +91,21 @@ public struct CompletionItem: Codable, Hashable {
self.insertTextFormat = insertTextFormat
self.kind = kind
self.deprecated = deprecated
self.data = data
}

enum CodingKeys: String, CodingKey {
ahoppen marked this conversation as resolved.
Show resolved Hide resolved
case label,
detail,
documentation,
sortText,
filterText,
textEdit,
insertText,
insertTextFormat,
kind,
deprecated,
data
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
public struct Lib {
/// Documentation for `foo`.
public func /*Lib.foo:def*/foo() {}

public init() {}
Expand Down
4 changes: 4 additions & 0 deletions Sources/SourceKitLSP/Clang/ClangLanguageServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,10 @@ extension ClangLanguageServerShim {
func completion(_ req: Request<CompletionRequest>) {
forwardRequestToClangdOnQueue(req)
}

func completionResolve(_ req: Request<CompletionItem>) {
forwardRequestToClangdOnQueue(req)
}

func hover(_ req: Request<HoverRequest>) {
forwardRequestToClangdOnQueue(req)
Expand Down
9 changes: 9 additions & 0 deletions Sources/SourceKitLSP/SourceKitServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public final class SourceKitServer: LanguageServer {

registerToolchainTextDocumentRequest(SourceKitServer.completion,
CompletionList(isIncomplete: false, items: []))
registerToolchainTextDocumentRequest(SourceKitServer.completionResolve, CompletionItem(label: "", kind: .class))
registerToolchainTextDocumentRequest(SourceKitServer.hover, nil)
registerToolchainTextDocumentRequest(SourceKitServer.definition, .locations([]))
registerToolchainTextDocumentRequest(SourceKitServer.references, [])
Expand Down Expand Up @@ -757,6 +758,14 @@ extension SourceKitServer {
) {
languageService.completion(req)
}

func completionResolve(
_ req: Request<CompletionItem>,
workspace: Workspace,
languageService: ToolchainLanguageServer
) {
languageService.completionResolve(req)
}

func hover(
_ req: Request<HoverRequest>,
Expand Down
73 changes: 70 additions & 3 deletions Sources/SourceKitLSP/Swift/CodeCompletion.swift
Original file line number Diff line number Diff line change
Expand Up @@ -169,18 +169,33 @@ extension SwiftLanguageServer {
// Map SourceKit's not_recommended field to LSP's deprecated
let notRecommended = (value[self.keys.not_recommended] as Int?).map({ $0 != 0 })

let kind: sourcekitd_uid_t? = value[self.keys.kind]
let kind = (value[self.keys.kind] as sourcekitd_uid_t?)?.asCompletionItemKind(self.values) ?? .value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why you need to refactor kind anymore? I think the previous version read better 😉

var usr: String? = nil

switch kind {
// Don't fill in `usr` for things other than Swift symbols.
case .class, .struct, .constant, .variable, .enum, .enumMember, .constructor,
.field, .function, .interface, .method, .module, .operator, .property, .typeParameter:
ahoppen marked this conversation as resolved.
Show resolved Hide resolved
usr = value[self.keys.associated_usrs]
default: break
}

var data: [String: LSPAny] = ["textDocURI": .string(snapshot.document.uri.stringValue)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to define the magic "textDocURI" string as a static property on CompletionItem instead of repeating it throughout the codebase? IIUC it’s not part of the LSP standard but just a key that you’re using for the opaque data dictionary.
Same for "usr"


if usr != nil { data["usr"] = .string(usr!) }

result.items.append(CompletionItem(
label: name,
kind: kind?.asCompletionItemKind(self.values) ?? .value,
kind: kind,
detail: typeName,
documentation: docBrief != nil ? .markupContent(MarkupContent(kind: .markdown, value: docBrief!)) : nil,
sortText: nil,
filterText: filterName,
textEdit: textEdit,
insertText: text,
insertTextFormat: isInsertTextSnippet ? .snippet : .plain,
deprecated: notRecommended ?? false
deprecated: notRecommended ?? false,
data: .dictionary(data)
))

return true
Expand Down Expand Up @@ -283,4 +298,56 @@ extension SwiftLanguageServer {

return TextEdit(range: textEditRangeStart..<requestPosition, newText: newText)
}

/// Must be called on `queue`.
ahoppen marked this conversation as resolved.
Show resolved Hide resolved
func _completionResolve(_ req: Request<CompletionItem>) {
if
let data = req.params.data,
case let .dictionary(dict) = data,
case let .string(usr) = dict["usr"],
case let .string(textDocURI) = dict["textDocURI"]
{
let uri = DocumentURI(string: textDocURI)
let skreq = SKDRequestDictionary(sourcekitd: sourcekitd)

skreq[keys.request] = requests.cursorinfo
skreq[keys.usr] = usr
skreq[keys.sourcefile] = uri.pseudoPath

// FIXME: SourceKit should probably cache this for us.
if let compileCommand = commandsByFile[uri] {
skreq[keys.compilerargs] = compileCommand.compilerArgs
}

let handle = sourcekitd.send(skreq, queue) { [weak self] result in
guard let dict = result.success else {
req.reply(.failure(ResponseError(result.failure!)))
return
}

if let documentationXML: String = dict[self?.keys.doc_full_as_xml] {

do {
let documentationMarkdown = try xmlDocumentationToMarkdown(documentationXML)
var completionItem = req.params

completionItem.documentation = .markupContent(MarkupContent(kind: .markdown, value: documentationMarkdown))
req.reply(completionItem)
} catch {
log("Completion Resolve: Unable to convert XML documentation to Markdown", level: .debug)
req.reply(req.params)
}

} else {
log("Completion Resolve: No doc_full_as_xml in SourceKit response", level: .debug)
req.reply(req.params)
}
}

// FIXME: cancellation
_ = handle
} else {
req.reply(req.params)
}
}
}
8 changes: 7 additions & 1 deletion Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ extension SwiftLanguageServer {
save: .value(TextDocumentSyncOptions.SaveOptions(includeText: false))),
hoverProvider: true,
completionProvider: CompletionOptions(
resolveProvider: false,
resolveProvider: true,
triggerCharacters: [".", "("]),
definitionProvider: nil,
implementationProvider: .bool(true),
Expand Down Expand Up @@ -565,6 +565,12 @@ extension SwiftLanguageServer {
self._completion(req)
}
}

public func completionResolve(_ req: Request<CompletionItem>) {
queue.async {
self._completionResolve(req)
}
}

public func hover(_ req: Request<HoverRequest>) {
let uri = req.params.textDocument.uri
Expand Down
1 change: 1 addition & 0 deletions Sources/SourceKitLSP/ToolchainLanguageServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public protocol ToolchainLanguageServer: AnyObject {
// MARK: - Text Document

func completion(_ req: Request<CompletionRequest>)
func completionResolve(_ req: Request<CompletionItem>)
func hover(_ req: Request<HoverRequest>)
func symbolInfo(_ request: Request<SymbolInfoRequest>)

Expand Down
6 changes: 4 additions & 2 deletions Tests/SourceKitLSPTests/SourceKitTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@ final class SKTests: XCTestCase {
textEdit: TextEdit(range: Position(line: 1, utf16index: 14)..<Position(line: 1, utf16index: 14), newText: "method(a: )"),
insertText: "method(a: )",
insertTextFormat: .plain,
deprecated: false),
deprecated: false,
data: .dictionary(["textDocURI": .string(loc.url.absoluteString), "usr": .string("s:4main1AV6method1aySi_tF")])),
CompletionItem(
label: "self",
kind: .keyword,
Expand All @@ -186,7 +187,8 @@ final class SKTests: XCTestCase {
textEdit: TextEdit(range: Position(line: 1, utf16index: 14)..<Position(line: 1, utf16index: 14), newText: "self"),
insertText: "self",
insertTextFormat: .plain,
deprecated: false),
deprecated: false,
data: .dictionary(["textDocURI": .string(loc.url.absoluteString)])),
])
}

Expand Down
7 changes: 7 additions & 0 deletions Tests/SourceKitLSPTests/SwiftCompletionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,13 @@ final class SwiftCompletionTests: XCTestCase {
XCTAssertEqual(abc.textEdit, TextEdit(range: Position(line: 5, utf16index: 9)..<Position(line: 5, utf16index: 9), newText: "abc"))
XCTAssertEqual(abc.insertText, "abc")
XCTAssertEqual(abc.insertTextFormat, .plain)
XCTAssertEqual(abc.data, .dictionary(["textDocURI": .string(url.absoluteString), "usr": .string("s:1a1SV3abcSivp")]))

// FIXME: SourceKit is "Unable to resolve type from" `s:1a1SV3abcSivp`.

// `completionItem/resolve` should return full documentation.
//let completionResolveResponse = try! sk.sendSync(abc)
//XCTAssertEqual(completionResolveResponse.documentation, .markupContent(MarkupContent(kind: .markdown, value: "Documentation for `abc`.")))
}

for col in 10...12 {
Expand Down
13 changes: 11 additions & 2 deletions Tests/SourceKitLSPTests/SwiftPMIntegration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,14 @@ final class SwiftPMIntegrationTests: XCTestCase {
label: "foo()",
kind: .method,
detail: "Void",
documentation: .markupContent(MarkupContent(kind: .markdown, value: "Documentation for foo.")),
sortText: nil,
filterText: "foo()",
textEdit: TextEdit(range: Position(line: 2, utf16index: 24)..<Position(line: 2, utf16index: 24), newText: "foo()"),
insertText: "foo()",
insertTextFormat: .plain,
deprecated: false),
deprecated: false,
data: .dictionary(["textDocURI": .string(call.url.absoluteString), "usr": .string("s:3lib3LibV3fooyyF")])),
CompletionItem(
label: "self",
kind: .keyword,
Expand All @@ -53,7 +55,14 @@ final class SwiftPMIntegrationTests: XCTestCase {
textEdit: TextEdit(range: Position(line: 2, utf16index: 24)..<Position(line: 2, utf16index: 24), newText: "self"),
insertText: "self",
insertTextFormat: .plain,
deprecated: false),
deprecated: false,
data: .dictionary(["textDocURI": .string(call.url.absoluteString)])),
])

// FIXME: SourceKit is "Unable to resolve type from" `s:3lib3LibV3fooyyF`.

// `completionItem/resolve` should return full documentation.
//let completionItemResolveResponse = try ws.sk.sendSync(completions.items[0])
//XCTAssertEqual(completionItemResolveResponse.documentation, .markupContent(MarkupContent(kind: .markdown, value: "Documentation for `foo`.")))
}
}