search

swiftlang / sourcekit-lsp

Language Server Protocol implementation for Swift and C-based languages
Apache License 2.0
3.21k stars 264 forks source link

Add LSP support for showing Macro Expansions #1436

Closed lokesh-tr closed 3 weeks ago

lokesh-tr commented 1 month ago

Note: This Feature is Experimental 🧪. You have to first enable this feature by passing --experimental-feature show-macro-expansions to sourcekit-lsp

This PR aims to bring a streamlined way of showing macro expansions through the LSP.

This allows users to pick a code action on a macro which performs an ExecuteCommandRequest with an ExpandMacroCommand. The server then generates the macro expansion (using semantic refactoring request) and stores it in a temporary directory. The server also performs a ShowDocumentRequest (LSP specification) on the client. Thus, displaying the expansion to the users.

ShowDocumentRequest being a part of the LSP spec will allow us to make macro expansions accessible across wide range of editors.

The result of this PR is that macro expansions work identical to that of Xcode with the only difference being Xcode showing the expansions inline, while we don't have a standardised LSP spec for displaying inline. (Although some customisation is possible for VS Code alone through their API).

How it works?

  1. sourcekitd returns an "Inline Macro" Code Action when the editor requests for the available code actions on a macro.
  2. We inject an "Expand Macro" Code Action having a ExpandMacroCommand before passing the response of sourcekitd to the editor.
  3. If the user decides to pick the "Expand Macro" Code Action, then the editor sends an ExecuteCommandRequest containing the ExpandMacroCommand.
  4. Upon receiving the ExecuteCommandRequest containing the ExpandMacroCommand, We then ask sourcekitd for the expansion of the given macro by making a semantic refactoring request.
  5. The response is interpreted as a MacroExpansion, and for each edit (expansion / replacement) it suggested, we make a temporary file as follows: <NSTemporaryDirectory>/sourcekit-lsp/GeneratedMacroExpansions/<edit.bufferName>/<SourceFileName>_<LineColumnRange>.<SourceFileExtension>

Here,

Follow-up Work & Future Directions:

  1. The current implementation lays foundation for macro expansions of any kind, but @attached macro expansions is little buggy. LSP support for @attached macro expansions will be fixed in the following PR: https://github.com/apple/sourcekit-lsp/pull/1479
  2. This implementation is not 1:1 with Xcode's "Expand Macro" feature. The only difference being the fact that we open separate documents for each expansion edit, whereas Xcode shows them all inline. This is a known limitation due to the LSP specifications having no support for showing anything inline the document. But for people using VS Code, we are looking forward to implement some custom functionality to show expansions inline with the help of VS Code API. ~This will be implemented in a future PR.~ (Implemented in https://github.com/swiftlang/sourcekit-lsp/pull/1479 https://github.com/swiftlang/vscode-swift/pull/945)

References:

Expansion of Swift Macros in Visual Studio Code - Google Summer Of Code 2024 @lokesh-tr @ahoppen @adam-fowler

yaroslavyaroslav commented 1 month ago

Sorry to disturbing you folks in such a busy time, but just to clarify for myself does this implementation would add ExpandSnippet code action option in along with ExtractMethod on a client side for free or there's be some work on their side required?

lokesh-tr commented 1 month ago

Sorry to disturbing you folks in such a busy time, but just to clarify for myself does this implementation would add ExpandSnippet code action option in along with ExtractMethod on a client side for free or there's be some work on their side required?

@yaroslavyaroslav well I don't think this implementation would add anything related to what you have asked. There's this ExecuteCommandTests.testRangeSemanticRefactoring which tests whether extract method works (already implemented, not specific to this PR). But am not sure if the same is implemented on the client side and am not sure sourcekitd sends an ExpandSnippet code action to the LSP.

Perhaps @ahoppen can answer?

@yaroslavyaroslav I guess you can open an issue and discuss this with him to not spam this PR since its not related to this PR.

ahoppen commented 1 month ago

Sorry, I don’t fully understand your question @yaroslavyaroslav. If we want to continue discussing it, could you please file an issue so we can have a dedicated thread for it?

lokesh-tr commented 1 month ago

Based on the meeting which we had yesterday:

lokesh-tr commented 1 month ago

I was little skeptical with my implementation of Refactoring (thought I would replace it with a better name at the end but forgot) and the passing around T.Type. I did think about using associated typesbut that didn't fit well with my array-looping based implementation of executeCommandRequest. Now that I changed it to if-elseif, the associate type implementation is trivial.

I have resolved all your comments. @ahoppen Kindly review and let me know.

lokesh-tr commented 1 month ago

@ahoppen I have addressed the review comments. When possible, let me know if there is anything else which I should fix. 😌

I have also made a new commit. Instead of creating generated files with buffer name in GeneratedMacroExpansions directory, this will now create a new subdirectory for each buffer within the GeneratedMacroExpansions. Within that subdirectory, will exist a file with the name as original source file containing the macro expansion.

This is inline with what we discussed in the meeting of adding a GitHub style name notation. The only difference is that instead of having just the starting line number in the file name like SourceFile.swift#L3, this implementation adds position range to the file name as SourceFile.swift#L3C2-L4C5. This is based on my finding that GitHub supports a url fragment which denotes the starting and ending position separated with an hyphen, each denoted by the letters L and C representing Line number and Column Number respectively.

The start and end position are equal when new lines are added while expanding. Similarly, they are unequal when new lines are replacing old lines while expanding. And thus, allows the user to just look at the name of the expanded file name to decide whether it is adding or replacing new lines.

EDIT: The naming scheme has been updated, kindly check the following comment which I made.

lokesh-tr commented 1 month ago

Based on the discussion about my findings which we had, I changed the file naming scheme as follows: SourceFileName_LaCb-LcCd.swift

The benefit of the above naming is that:

  1. The user can be clear about whether the macro is adding new lines or replacing with new lines just by looking at the position range.
  2. No use of # means, no need to worry about fragments, escaping the sign, etc.
  3. Instead of putting the position range indicator after the file extension, we put it before the extension, so that the code editors can understand the actual file extension.
lokesh-tr commented 4 weeks ago

@ahoppen The last commit which I made tries to convert the sequential edit from sourcekitd into ConcurrentEdits as we discussed, but it fails the testFreestandingMacroExpansion test, the reason being the line numbers aren't matching after i convert them to concurrent edits.

I would like to have you help me debug this issue.

The test for attached macros is yet to be written.

lokesh-tr commented 4 weeks ago

As we discussed in the meeting, I removed the ConcurrentEdits commit from here... I will wrap the current state of freestanding macro expansion as such in experimental features and will make a seperate PR for concurrent edits and attached macro expansion

lokesh-tr commented 4 weeks ago

@ahoppen I wrapped the feature under ExperimentalFeatures. Let me have your comments so that I can fix them by tomorrow and also squash the commits. Hope we will merge this soon.

Further, I will follow up with a new PR for the ConcurrentEdits.

ahoppen commented 3 weeks ago

@swift-ci Please test

ahoppen commented 3 weeks ago

@swift-ci Please test Windows

ahoppen commented 3 weeks ago

@swift-ci Please test

ahoppen commented 3 weeks ago

@swift-ci Please test Windows