Add Semantic Functionality to Macro Expansion Reference Documents (including Nested Macro Expansion) 🚦 #1610
Conversation
Sources/SourceKitLSP/Swift/MacroExpansionReferenceDocumentURLData.swift
Outdated
Show resolved
Hide resolved
Sources/SourceKitLSP/Swift/MacroExpansionReferenceDocumentURLData.swift
Outdated
Show resolved
Hide resolved
Sources/SourceKitLSP/Swift/MacroExpansionReferenceDocumentURLData.swift
Outdated
Show resolved
Hide resolved
Sources/SourceKitLSP/Swift/MacroExpansionReferenceDocumentURLData.swift
Outdated
Show resolved
Hide resolved
Sources/SourceKitLSP/Swift/MacroExpansionReferenceDocumentURLData.swift
Outdated
Show resolved
Hide resolved
lokesh-tr
force-pushed
the
semantic-functionality-in-macro-expansion-reference-document
branch
from
19f139f to
f4c2399
Compare
…cluding Nested Macro Expansion)
lokesh-tr
force-pushed
the
semantic-functionality-in-macro-expansion-reference-document
branch
from
f4c2399 to
af39b02
Compare
There was a problem hiding this comment.
I didn’t like the fact that we had implicit dependencies between requests and that the workspace/getReferenceDocument request only worked after executing the expand macro refactoring action. We should not make assumptions about how often a client calls the getReferenceDocument request, they could reasonably call it every time you switch between the different the different buffers generated by a macro. Or getReferenceDocument might get executed a second time when the editor is closed and then re-opened (which could reasonably restore the peek windows).
#1631 is what I ended up with. The PR description says what it does but the gist is that it makes the macro expansion URLs stateless and just caches intermediate macro expansions so it doesn’t re-run those requests all the time. It does not implement any semantic functionality for nested macro expansions.
Could you take a look at that PR, @lokesh-tr and check if you see any issues with that approach?
| let uri = | ||
| if let primaryFileURI = (try? ReferenceDocumentURL(from: reqURI))?.primaryFile { | ||
| primaryFileURI | ||
| } else { | ||
| reqURI | ||
| } |
There was a problem hiding this comment.
Having an if expression inside a guard condition looks a little confusing to me. Can you check if there’s a way to refactor this, maybe splitting it into two guards?
| @@ -44,6 +44,21 @@ struct MacroExpansion: RefactoringResponse { | |||
| } | |||
| } | |||
|
|
|||
| actor GeneratedMacroExpansionsStorage { | |||
| static var shared = GeneratedMacroExpansionsStorage() | |||
There was a problem hiding this comment.
Instead of having a static shared instance, can this be a member on SwiftLanguageService, similar to DiagnosticReportManager? You should be able to have multiple SourceKitLSPServer instances in the same process without having them influence each other (and we do when running tests).
| let queryItems = URLComponents(string: url.absoluteString.removingPercentEncoding ?? url.absoluteString)? | ||
| .queryItems |
There was a problem hiding this comment.
Why do we need to remove the percent encoding here? Do you have a log of the requests and replies sent between SourceKit-LSP and VS Code to see where the additional percent encoding gets added?
| cancelInFlightPublishDiagnosticsTask(for: snapshot.uri) | ||
| await diagnosticReportManager.removeItemsFromCache(with: snapshot.uri) | ||
| switch try? ReferenceDocumentURL(from: notification.textDocument.uri) { | ||
| case .macroExpansion(_): |
There was a problem hiding this comment.
Nitpick: The (_) isn’t needed. Similar a couple more times below.
| case .macroExpansion(_): | |
| case .macroExpansion: |
| let document = | ||
| if let referenceDocument = try? ReferenceDocumentURL(from: req.textDocument.uri) { | ||
| referenceDocument.primaryFile | ||
| } else { | ||
| req.textDocument.uri | ||
| } | ||
|
|
||
| await semanticIndexManager?.prepareFileForEditorFunctionality(document) | ||
| let snapshot = try documentManager.latestSnapshot(req.textDocument.uri) | ||
| let buildSettings = await self.buildSettings(for: req.textDocument.uri) | ||
| let buildSettings = await self.buildSettings(for: document) |
There was a problem hiding this comment.
Why do we need to explicitly get the primaryFile from the ReferenceDocumentURL here but not to get build settings in eg. relatedIdentifiers. I would expect that we always need to get the build settings for the primary file, in which case we could implement this check in SwiftLanguageService.buildSettings.
|
Closing this in favour of #1634 |
Note: WORK IN PROGRESS
This PR migrates several requests made to sourcekitd to support Semantic Functionality in Macro Expansion Reference Documents.
What has been migrated and works?
cursorInfodocumentDiagnosticReportopenInterfacesemanticTokensrelatedIdentifiers(used bydocumentSymbolHighlight)RefactoringResponse.refactoring,getMacroExpansion, etc. (Nested macro expansions)What has been migrated but doesn’t work? (probably needs a fix in sourcekitd)
inlayHints(variableTypeInfo)What won't be migrated since they don't apply for reference documents?
Accompanying PR in vscode-swift extension repository: swiftlang/vscode-swift#990
Expansion of Swift Macros in Visual Studio Code - Google Summer Of Code 2024
@lokesh-tr @ahoppen @adam-fowler