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

Add LSP extension request for retrieving macro expansions #892

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions Sources/LanguageServerProtocol/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ add_library(LanguageServerProtocol STATIC
Requests/InlineValueRefreshRequest.swift
Requests/InlineValueRequest.swift
Requests/LinkedEditingRangeRequest.swift
Requests/MacroExpansionRequest.swift
Requests/MonikersRequest.swift
Requests/OpenInterfaceRequest.swift
Requests/PollIndexRequest.swift
Expand Down Expand Up @@ -107,6 +108,7 @@ add_library(LanguageServerProtocol STATIC
SupportTypes/LocationLink.swift
SupportTypes/LocationsOrLocationLinksResponse.swift
SupportTypes/LSPAny.swift
SupportTypes/MacroExpansion.swift
SupportTypes/MarkupContent.swift
SupportTypes/NotebookCellTextDocumentFilter.swift
SupportTypes/NotebookDocument.swift
Expand Down
5 changes: 5 additions & 0 deletions Sources/LanguageServerProtocol/Error.swift
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ extension ResponseError {
message: "request cancelled by server"
)

public static var unsupportedMethod: ResponseError = ResponseError(
code: .unknownErrorCode,
message: "unsupported method"
)

public static func workspaceNotOpen(_ uri: DocumentURI) -> ResponseError {
return ResponseError(code: .workspaceNotOpen, message: "No workspace containing '\(uri)' found")
}
Expand Down
1 change: 1 addition & 0 deletions Sources/LanguageServerProtocol/Messages.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public let builtinRequests: [_RequestType.Type] = [
InlineValueRefreshRequest.self,
InlineValueRequest.self,
LinkedEditingRangeRequest.self,
MacroExpansionRequest.self,
MonikersRequest.self,
OpenInterfaceRequest.self,
PollIndexRequest.self,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

/// Request the expansion of the macro at a given use site.
/// **LSP Extension**.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it would be valuable to link to the rust-analyzer definition of the request? Just so we can check in the future that we’re still in sync with theirs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We've already diverged a bit from rust-analyzer by passing a range in the request and by returning positions along with the expansions in the response.

After researching this a bit more, I have found that upstream (LSP) seems to favor a slightly more general approach where the server acts as a content provider for a custom URI scheme, see

Under that model it would probably make more sense to implement macro expansion as a command where the server then invokes workspace/openDocument/workspace/peekDocument (or whichever request ends up getting implemented) with a custom URI (swift-macro-expansion://...).

Until upstream has figured out those details, my initial gut feeling was to stick with a custom request here, since that affords us some flexibility with the precise request design. It shouldn't be too hard to move over to a command-based implementation eventually either.

Reading #892 (comment) however got me thinking, that would actually work really well with the model that upstream LSP favors. I think the request-based design is a bit simpler for now, but maybe we should still move our own implementation in that direction too?

Perhaps

  • sourcekit-lsp/getReferenceDocument (client -> server request)
  • sourcekit-lsp/peekDocument (server -> client request)

and then add a new command that peeks the macro expansion at the given position?

One complication would be that the client would have to support a custom request too, though I don't think that should be that hard to implement.

Another complication would be that SourceKit-LSP already seems to support semantic refactorings via commands. This is super nice in theory, since we wouldn't have to do anything (in principle) to support macro expansions, however (if I understand them correctly), they would actually insert the expansion currently due to being modeled as text edits.

A crazy thought: Let the client pass an additional argument to ExecuteCommandRequest that contains e.g. a config for how the refactoring is to be applied (e.g. whether the edits should actually be performed or just previewed, e.g. with the proposed sourcekit-lsp/peekDocument request). This could then be used to show macro expansions without actually applying them, but would even work for other semantic refactorings.

So many options! Perhaps you have some more thoughts on this.

Copy link
Collaborator

@ahoppen ahoppen Oct 13, 2023

Choose a reason for hiding this comment

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

I like the idea of having a URL scheme to describe the expanded macro buffers and implementing a sourcekit-lsp/getReferenceDocument request to get the generated contents to the client. The main advantage I see with it are:

  • We have a URI that we can use to talk about the expanded macro buffer. Adding support for semantic functionality within the generated buffer is just a matter of implementing LSP requests for that URL scheme.
  • We can use it to show the content of generated interfaces where we currently write the generated interface to disk and then open it.

I don’t quite understand where the sourcekit-lsp/peekDocument request comes in. My design of this feature would be using ExecuteCommandRequest.

  1. The client sends a textDocument/codeAction request to the server
  2. If we are at a macro, the server responds with a Expand Macro code action that has a command with the custom URI scheme in the arguments.
  3. If the user decides to pick that action, the client sends a workspace/executeCommand for that command.
  4. And now is where it get’s a little tricky: The server replies with some kind of special response (reply of workspace/executeCommand is LSPAny?) that the client understands as: OK, I need to show an inline text buffer with the text of this URI scheme. E.g. the reply is
{ 
  "action": "showInlineBuffer",
  "uri": "swift-macro-expansion:///path/to/file.swift?buffer=@swiftmacro_…&line=6&column=8"
}
  1. The client then sends a sourcekit-lsp/getReferenceDocument request to the server with that URI
  2. The server replies with the contents of the buffer to display

Steps 3 and 4 are a little bit roundabout and might not actually be necessary if we just short-circuit the command in the editor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, your showInlineBuffer reply is effectively what I meant by sourcekit-lsp/peekDocument (only that it wouldn't be a reply, but a separate request (which would effectively let the server invoke it multiple times, if it so desires).

But until upstream has decided on a specific design, I would be fine either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In my mind the client should always be driving the user interaction and the LSP server should just reply to the user’s request, that’s why I think peekDocument would be a little weird.

Is peekDocument a request you came up with or is it based on some proposal? I couldn’t find anything related to it in my short search.

Copy link
Collaborator

Choose a reason for hiding this comment

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

and simply return a swift-macro-expansion:// URI

Which request would this be a reply to (and in which field)? AFAICT CodeAction doesn’t have a URI field (unless we just shove it in the arguments: [LSPAny]?)

Edit: I am not sure if there's a way to handle custom responses for commands triggered by a code action though, since code actions are handled by the vscode-languageclient library.

Oh, that would indeed be a major limitation, unfortunately.

Copy link
Collaborator Author

@fwcd fwcd Oct 16, 2023

Choose a reason for hiding this comment

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

keep our non-standard macro expansion request for now and simply return a swift-macro-expansion://

with that I meant using the current design from this PR and returning something along the lines of

struct MacroExpansion {
  let uri: DocumentURI // = DocumentURI("swift-macro-expansion://...")
  let position: Position
}

from sourcekit-lsp/macroExpansion, which the client then resolves. But, yeah that wouldn't work as a code action, unfortunately, the client would have to implement the command manually (as my vscode-swift PR currently does).

Copy link
Collaborator

Choose a reason for hiding this comment

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

But, yeah that wouldn't work as a code action, unfortunately, the client would have to implement the command manually (as my vscode-swift PR currently does).

That’s the major downside I see with the current design. IIUC it currently offers “Expand Macro” everywhere, even when the cursor isn’t currently on a macro. I think implementing it on top of Code Actions would be the cleanest design that also yields the best UI results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So implementing it as a command where the server calls the client back seems like the only option that fulfills all requirements (works as a code action and doesn't need a custom response)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think that is the case. So your design was the right one all along 😄

///
/// - Parameters:
/// - textDocument: The document in which the macro is used.
/// - range: The range at which the macro is used.
///
/// - Returns: The macro expansion.
public struct MacroExpansionRequest: TextDocumentRequest, Hashable {
public static let method: String = "sourcekit-lsp/macroExpansion"
public typealias Response = [MacroExpansion]

/// The document in which the macro is used.
public var textDocument: TextDocumentIdentifier

/// The position at which the macro is used.
@CustomCodable<PositionRange>
public var range: Range<Position>

public init(
textDocument: TextDocumentIdentifier,
range: Range<Position>
) {
self.textDocument = textDocument
self._range = CustomCodable(wrappedValue: range)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

/// The expansion of a macro in a source file.
public struct MacroExpansion: ResponseType, Hashable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just an idle thought: This is different from rust-analyzer’s macro expansion, right. Do you know what they use their name property for?

All I’m thinking here is that the closer we keep the two requests, the easier it might make life for us when/if expand macro becomes part of the LSP spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See my comments regarding staying in sync with rust-analyzer/LSP above.

/// The position in the source file where the expansion would be inserted.
public let position: Position

/// The Swift code that the macro expands to.
public let sourceText: String
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to include the buffer name here as well. My thinking is that we could to encode it like

swift-macro-expansion:///path/to/file.swift?buffer=@swiftmacro_...

That way the editor could open the buffer with that URI and we could start supporting semantic functionality (like nested expansion) inside the expansion buffer as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I like the idea (I'll also refer to my longer comment here)


public init(position: Position, sourceText: String) {
self.position = position
self.sourceText = sourceText
}
}
6 changes: 5 additions & 1 deletion Sources/SourceKitLSP/Clang/ClangLanguageServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,10 @@ extension ClangLanguageServerShim {
return try await forwardRequestToClangd(req)
}

func macroExpansion(_ req: MacroExpansionRequest) async throws -> [MacroExpansion] {
throw ResponseError.unsupportedMethod
}

func foldingRange(_ req: FoldingRangeRequest) async throws -> [FoldingRange]? {
guard self.capabilities?.foldingRangeProvider?.isSupported ?? false else {
return nil
Expand All @@ -580,7 +584,7 @@ extension ClangLanguageServerShim {
}

func openInterface(_ request: OpenInterfaceRequest) async throws -> InterfaceDetails? {
throw ResponseError.unknown("unsupported method")
throw ResponseError.unsupportedMethod
}

// MARK: - Other
Expand Down
10 changes: 10 additions & 0 deletions Sources/SourceKitLSP/SourceKitServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,8 @@ extension SourceKitServer: MessageHandler {
requestHandler: self.documentDiagnostic,
fallback: .full(.init(items: []))
)
case let request as Request<MacroExpansionRequest>:
await self.handleRequest(for: request, requestHandler: self.macroExpansion, fallback: [])
default:
reply(.failure(ResponseError.methodNotFound(R.method)))
}
Expand Down Expand Up @@ -1386,6 +1388,14 @@ extension SourceKitServer {
return try await languageService.documentDiagnostic(req)
}

func macroExpansion(
_ request: MacroExpansionRequest,
workspace: Workspace,
languageService: ToolchainLanguageServer
) async throws -> [MacroExpansion] {
return try await languageService.macroExpansion(request)
}

/// Converts a location from the symbol index to an LSP location.
///
/// - Parameter location: The symbol index location
Expand Down
30 changes: 28 additions & 2 deletions Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -577,11 +577,11 @@ extension SwiftLanguageServer {

/// Returns true if the `ToolchainLanguageServer` will take ownership of the request.
public func definition(_ request: DefinitionRequest) async throws -> LocationsOrLocationLinksResponse? {
throw ResponseError.unknown("unsupported method")
throw ResponseError.unsupportedMethod
}

public func declaration(_ request: DeclarationRequest) async throws -> LocationsOrLocationLinksResponse? {
throw ResponseError.unknown("unsupported method")
throw ResponseError.unsupportedMethod
}

public func hover(_ req: HoverRequest) async throws -> HoverResponse? {
Expand Down Expand Up @@ -1299,6 +1299,32 @@ extension SwiftLanguageServer {
return .full(RelatedFullDocumentDiagnosticReport(items: diagnostics))
}

public func macroExpansion(_ req: MacroExpansionRequest) async throws -> [MacroExpansion] {
let command = SemanticRefactorCommand(
title: "Expand Macro",
actionString: "source.refactoring.kind.expand.macro",
positionRange: req.range,
textDocument: req.textDocument
)

do {
let refactor = try await semanticRefactoring(command)

guard let edits = refactor.edit.changes?[req.textDocument.uri] else {
return []
}

return edits.map { edit in
MacroExpansion(
position: edit.range.lowerBound,
sourceText: edit.newText
)
}
} catch SemanticRefactoringError.noEditsNeeded {
return []
}
}

public func executeCommand(_ req: ExecuteCommandRequest) async throws -> LSPAny? {
// TODO: If there's support for several types of commands, we might need to structure this similarly to the code actions request.
guard let sourceKitServer else {
Expand Down
1 change: 1 addition & 0 deletions Sources/SourceKitLSP/ToolchainLanguageServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ public protocol ToolchainLanguageServer: AnyObject {
func codeAction(_ req: CodeActionRequest) async throws -> CodeActionRequestResponse?
func inlayHint(_ req: InlayHintRequest) async throws -> [InlayHint]
func documentDiagnostic(_ req: DocumentDiagnosticsRequest) async throws -> DocumentDiagnosticReport
func macroExpansion(_ req: MacroExpansionRequest) async throws -> [MacroExpansion]

// MARK: - Other

Expand Down