-
Notifications
You must be signed in to change notification settings - Fork 285
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
Changes from 10 commits
77e2368
a3d8eaa
f4eef6a
a22e6c4
ceb4063
3ba673d
c03d10c
3a24271
4660c5e
150af0b
b722030
424a4e1
69c3787
4e627a2
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 |
---|---|---|
@@ -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**. | ||
/// | ||
/// - 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? | ||
fwcd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// 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 { | ||
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. Just an idle thought: This is different from rust-analyzer’s macro expansion, right. Do you know what they use their 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. 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. 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 | ||
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 would like to include the buffer name here as well. My thinking is that we could to encode it like
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. 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. 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 | ||
} | ||
} |
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.
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.
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.
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 proposedsourcekit-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.
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.
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:I don’t quite understand where the
sourcekit-lsp/peekDocument
request comes in. My design of this feature would be usingExecuteCommandRequest
.textDocument/codeAction
request to the serverExpand Macro
code action that has a command with the custom URI scheme in thearguments
.workspace/executeCommand
for that command.workspace/executeCommand
isLSPAny?
) 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 issourcekit-lsp/getReferenceDocument
request to the server with that URISteps 3 and 4 are a little bit roundabout and might not actually be necessary if we just short-circuit the command in the editor.
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.
Yes, your
showInlineBuffer
reply is effectively what I meant bysourcekit-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.
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.
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.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.
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 thearguments: [LSPAny]?
)Oh, that would indeed be a major limitation, unfortunately.
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.
with that I meant using the current design from this PR and returning something along the lines of
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 myvscode-swift
PR currently does).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.
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.
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.
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)?
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.
Yes, I think that is the case. So your design was the right one all along 😄