Support expansion of nested macros #1631
Conversation
The basic idea is that a `sourcekit-lsp://swift-macro-expansion` URL should have sufficient information to reconstruct the contents of that macro buffer without relying on any state in SourceKit-LSP. The benefit of not having any cross-request state in SourceKit-LSP is that an editor might can send the `workspace/getReferenceDocument` request at any time and it will succeed independent of the previous requests. Furthermore, we can always get the contents of the macro expansion to form a `DocumentSnapshot`, which can be used to provide semantic functionality inside macro expansion buffers. To do that, the `sourcekit-lsp:` URL scheme was changed to have a parent instead of a `primary`, which is the URI of the document that the buffer was expanded from. For nested macro expansions, this will be a `sourcekit-lsp://swift-macro-expansion` URL itself. With that parent, we can reconstruct the macro expansion chain all the way from the primary source file. To avoid sending the same expand macro request to sourcekitd all the time, we introduce `MacroExpansionManager`, which caches the last 10 macro expansions. `SwiftLanguageService` now has a `latestSnapshot` method that returns the contents of the reference document when asked for a reference document URL and only consults the document manager for other URIs. To support semantic functionality in macro expansion buffers, we need to call that `latestSnapshot` method so we have a document snapshot of the macro expansion buffer for position conversions and pass the following to the sourcekitd requests. ``` keys.sourceFile: snapshot.uri.sourcekitdSourceFile, keys.primaryFile: snapshot.uri.primaryFile?.pseudoPath, ``` We should consider if there’s a way to make the `latestSnapshot` method on `documentManager` less accessible so that the method which also returns snapshots for reference documents is the one being used by default. Co-Authored-By: Lokesh T R <[email protected]>
ahoppen
force-pushed
the
nested-macro-expansions
branch
from
015d626 to
90e0f3f
Compare
|
@swift-ci Please test |
There was a problem hiding this comment.
I love how you implemented it and am glad that my initial idea of having nested URLs is good and that the macro expansions storage idea took its shape into a Cache system which is pretty neat.
Thank you so much for lending me an helping hand. ❤️
I will look into building semantic functionality on top of this and see if there exists any issues in integrating this with VS Code / Extension.
|
@hamishknight @bnbarham If you have review comments, I will address them in a follow-up PR so that @lokesh-tr can continue building semantic functionality in macro expansion buffers on top of this. |
|
Here's the new one: #1634 |
| /// | ||
| /// The primary file is used to determine the workspace and language service that is used to generate the reference | ||
| /// document as well as getting the reference document's build settings. | ||
| var primaryFile: DocumentURI? { |
There was a problem hiding this comment.
It seems like all-but-one uses of this immediately do ?? self, would it be worth making this non-optional? For the one use that cares about nil, maybe we could expose isPrimaryFile?
There was a problem hiding this comment.
The usages that will be introduced by #1634 will expect primaryFile to be nil if the URI is an actual on-disk file, so I think keeping it optional makes sense.
| guard let primaryFileURL = expandMacroCommand.textDocument.uri.fileURL else { | ||
| throw ResponseError.unknown("Given URI is not a file URL") | ||
| } | ||
| let primaryFileDisplayName = |
There was a problem hiding this comment.
Would it be worth renaming this to currentFileDisplayName/parentFileDisplayName or something to avoid confusion?
There was a problem hiding this comment.
Oh yes, good suggestion.
| switch try? ReferenceDocumentURL(from: notification.textDocument.uri) { | ||
| case .macroExpansion: | ||
| break |
There was a problem hiding this comment.
Could this be an early return? Same goes for open and close below
There was a problem hiding this comment.
I want to migrate generated interfaces to reference documents next, which will allow us to provide semantic functionality in them. They will require logic for open/close/reopen, so I don’t think that the early return scales. That’s why I would just keep it as a switch like it is today.
Address review comments to #1631
The basic idea is that a
sourcekit-lsp://swift-macro-expansionURL should have sufficient information to reconstruct the contents of that macro buffer without relying on any state in SourceKit-LSP. The benefit of not having any cross-request state in SourceKit-LSP is that an editor might can send theworkspace/getReferenceDocumentrequest at any time and it will succeed independent of the previous requests. Furthermore, we can always get the contents of the macro expansion to form aDocumentSnapshot, which can be used to provide semantic functionality inside macro expansion buffers.To do that, the
sourcekit-lsp:URL scheme was changed to have a parent instead of aprimary, which is the URI of the document that the buffer was expanded from. For nested macro expansions, this will be asourcekit-lsp://swift-macro-expansionURL itself.With that parent, we can reconstruct the macro expansion chain all the way from the primary source file. To avoid sending the same expand macro request to sourcekitd all the time, we introduce
MacroExpansionManager, which caches the last 10 macro expansions.SwiftLanguageServicenow has alatestSnapshotmethod that returns the contents of the reference document when asked for a reference document URL and only consults the document manager for other URIs. To support semantic functionality in macro expansion buffers, we need to call thatlatestSnapshotmethod so we have a document snapshot of the macro expansion buffer for position conversions and pass the following to the sourcekitd requests.We should consider if there’s a way to make the
latestSnapshotmethod ondocumentManagerless accessible so that the method which also returns snapshots for reference documents is the one being used by default.CC @lokesh-tr