Skip to content

Commit f4d93d4

Browse files
lokesh-trlokesh-tr
committed
Address Review Comments
1 parent 1bd0bc4 commit f4d93d4

File tree

6 files changed

+79
-54
lines changed

6 files changed

+79
-54
lines changed

‎Sources/SourceKitLSP/Swift/MacroExpansion.swift

Lines changed: 50 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ struct MacroExpansion: RefactoringResponse {
3333
self.title = title
3434
self.uri = uri
3535
self.edits = refactoringEdits.compactMap { refactoringEdit in
36-
if refactoringEdit.bufferName.isEmpty {
36+
if refactoringEdit.bufferName == nil && !refactoringEdit.newText.isEmpty {
3737
logger.fault("Unable to retrieve some parts of the expansion")
3838
return nil
3939
}
@@ -46,14 +46,14 @@ struct MacroExpansion: RefactoringResponse {
4646
extension SwiftLanguageService {
4747
/// Handles the `ExpandMacroCommand`.
4848
///
49-
/// Makes request to sourcekitd and wraps the result into a `MacroExpansion`
50-
/// and then makes `ShowDocumentRequest` to the client side for each
49+
/// Makes a request to sourcekitd and wraps the result into a `MacroExpansion`
50+
/// and then makes a `ShowDocumentRequest` to the client side for each
5151
/// expansion to be displayed.
5252
///
5353
/// - Parameters:
5454
/// - expandMacroCommand: The `ExpandMacroCommand` that triggered this request.
5555
///
56-
/// - Returns: An `[RefactoringEdit]` with the necessary edits and buffer name as a `LSPAny`
56+
/// - Returns: A `[RefactoringEdit]` with the necessary edits and buffer name as a `LSPAny`
5757
func expandMacro(
5858
_ expandMacroCommand: ExpandMacroCommand
5959
) async throws -> LSPAny {
@@ -70,46 +70,57 @@ extension SwiftLanguageService {
7070
let expansion = try await self.refactoring(expandMacroCommand)
7171

7272
for macroEdit in expansion.edits {
73-
// buffer name without ".swift"
74-
let macroExpansionBufferDirectoryName =
75-
macroEdit.bufferName.hasSuffix(".swift")
76-
? String(macroEdit.bufferName.dropLast(6))
77-
: macroEdit.bufferName
78-
79-
let macroExpansionBufferDirectoryURL = self.generatedMacroExpansionsPath
80-
.appendingPathComponent(macroExpansionBufferDirectoryName)
81-
do {
82-
try FileManager.default.createDirectory(at: macroExpansionBufferDirectoryURL, withIntermediateDirectories: true)
83-
} catch {
84-
throw ResponseError.unknown(
85-
"Failed to create directory for Macro Expansion Buffer at path: \(macroExpansionBufferDirectoryURL.path)"
86-
)
87-
}
88-
89-
let macroExpansionFileName = sourceFileURL.deletingPathExtension().lastPathComponent // name of the source file
90-
let macroExpansionPositionRangeIndicator =
91-
"L\(macroEdit.range.lowerBound.line)C\(macroEdit.range.lowerBound.utf16index)-L\(macroEdit.range.upperBound.line)C\(macroEdit.range.upperBound.utf16index)" // github permalink notation for position range
92-
93-
let macroExpansionFilePath = macroExpansionBufferDirectoryURL.appendingPathComponent(
94-
"\(macroExpansionFileName)_\(macroExpansionPositionRangeIndicator).\(sourceFileURL.pathExtension)"
95-
)
73+
if let bufferName = macroEdit.bufferName {
74+
// buffer name without ".swift"
75+
let macroExpansionBufferDirectoryName =
76+
bufferName.hasSuffix(".swift")
77+
? String(bufferName.dropLast(6))
78+
: bufferName
79+
80+
let macroExpansionBufferDirectoryURL = self.generatedMacroExpansionsPath
81+
.appendingPathComponent(macroExpansionBufferDirectoryName)
82+
do {
83+
try FileManager.default.createDirectory(
84+
at: macroExpansionBufferDirectoryURL,
85+
withIntermediateDirectories: true
86+
)
87+
} catch {
88+
throw ResponseError.unknown(
89+
"Failed to create directory for macro expansion buffer at path: \(macroExpansionBufferDirectoryURL.path)"
90+
)
91+
}
9692

97-
do {
98-
try macroEdit.newText.write(to: macroExpansionFilePath, atomically: true, encoding: .utf8)
99-
} catch {
100-
throw ResponseError.unknown("Unable to write macro expansion to file path: \"\(macroExpansionFilePath.path)\"")
101-
}
93+
let macroExpansionFileName = sourceFileURL.deletingPathExtension().lastPathComponent // name of the source file
94+
let macroExpansionPositionRangeIndicator =
95+
"L\(macroEdit.range.lowerBound.line)C\(macroEdit.range.lowerBound.utf16index)-L\(macroEdit.range.upperBound.line)C\(macroEdit.range.upperBound.utf16index)" // github permalink notation for position range
96+
97+
let macroExpansionFilePath =
98+
macroExpansionBufferDirectoryURL
99+
.appendingPathComponent(
100+
"\(macroExpansionFileName)_\(macroExpansionPositionRangeIndicator).\(sourceFileURL.pathExtension)"
101+
)
102+
103+
do {
104+
try macroEdit.newText.write(to: macroExpansionFilePath, atomically: true, encoding: .utf8)
105+
} catch {
106+
throw ResponseError.unknown(
107+
"Unable to write macro expansion to file path: \"\(macroExpansionFilePath.path)\""
108+
)
109+
}
102110

103-
Task {
104-
let req = ShowDocumentRequest(uri: DocumentURI(macroExpansionFilePath), selection: macroEdit.range)
111+
Task {
112+
let req = ShowDocumentRequest(uri: DocumentURI(macroExpansionFilePath), selection: macroEdit.range)
105113

106-
let response = await orLog("Sending ShowDocumentRequest to Client") {
107-
try await sourceKitLSPServer.sendRequestToClient(req)
108-
}
114+
let response = await orLog("Sending ShowDocumentRequest to Client") {
115+
try await sourceKitLSPServer.sendRequestToClient(req)
116+
}
109117

110-
if let response, !response.success {
111-
logger.error("client refused to show document for \(expansion.title, privacy: .public)")
118+
if let response, !response.success {
119+
logger.error("client refused to show document for \(expansion.title, privacy: .public)")
120+
}
112121
}
122+
} else if !macroEdit.newText.isEmpty {
123+
logger.fault("Unable to retrieve some parts of macro expansion")
113124
}
114125
}
115126

‎Sources/SourceKitLSP/Swift/Refactoring.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ extension RefactoringResponse {
6868
RefactoringEdit(
6969
range: startPosition..<endPosition,
7070
newText: textWithSnippets,
71-
bufferName: edit[keys.bufferName] ?? ""
71+
bufferName: edit[keys.bufferName]
7272
)
7373
)
7474
return true

‎Sources/SourceKitLSP/Swift/RefactoringEdit.swift

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@ import SourceKitD
2626
/// file but to a separate buffer, a fake name for that buffer. For example
2727
/// for expansion of macros, this is @ followed by the mangled name of the
2828
/// macro expansion, followed by .swift.
29-
public var bufferName: String
29+
public var bufferName: String?
3030

31-
public init(range: Range<Position>, newText: String, bufferName: String) {
31+
public init(range: Range<Position>, newText: String, bufferName: String?) {
3232
self._range = CustomCodable<PositionRange>(wrappedValue: range)
3333
self.newText = newText
3434
self.bufferName = bufferName
@@ -38,20 +38,33 @@ import SourceKitD
3838
extension RefactoringEdit: LSPAnyCodable {
3939
public init?(fromLSPDictionary dictionary: [String: LSPAny]) {
4040
guard case .dictionary(let rangeDict) = dictionary[CodingKeys.range.stringValue],
41-
case .string(let newText) = dictionary[CodingKeys.newText.stringValue],
42-
case .string(let bufferName) = dictionary[CodingKeys.bufferName.stringValue]
41+
case .string(let newText) = dictionary[CodingKeys.newText.stringValue]
4342
else {
4443
return nil
4544
}
45+
4646
guard let range = Range<Position>(fromLSPDictionary: rangeDict) else {
4747
return nil
4848
}
49+
4950
self._range = CustomCodable<PositionRange>(wrappedValue: range)
5051
self.newText = newText
51-
self.bufferName = bufferName
52+
53+
if case .string(let bufferName) = dictionary[CodingKeys.bufferName.stringValue] {
54+
self.bufferName = bufferName
55+
} else {
56+
self.bufferName = nil
57+
}
5258
}
5359

5460
public func encodeToLSPAny() -> LSPAny {
61+
guard let bufferName = bufferName else {
62+
return .dictionary([
63+
CodingKeys.range.stringValue: range.encodeToLSPAny(),
64+
CodingKeys.newText.stringValue: .string(newText),
65+
])
66+
}
67+
5568
return .dictionary([
5669
CodingKeys.range.stringValue: range.encodeToLSPAny(),
5770
CodingKeys.newText.stringValue: .string(newText),

‎Sources/SourceKitLSP/Swift/SemanticRefactoring.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ extension SwiftLanguageService {
6363

6464
/// Handles the `SemanticRefactorCommand`.
6565
///
66-
/// Makes request to sourcekitd and wraps the result into a
66+
/// Sends a request to sourcekitd and wraps the result into a
6767
/// `SemanticRefactoring` and then makes an `ApplyEditRequest` to the client
6868
/// side for the actual refactoring.
6969
///

‎Sources/SourceKitLSP/Swift/SwiftLanguageService.swift

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -819,16 +819,14 @@ extension SwiftLanguageService {
819819

820820
var canInlineMacro = false
821821

822-
let showMacroExpansionsIsEnabled = await self.sourceKitLSPServer?.options.experimentalFeatures.contains(
823-
.showMacroExpansions
824-
)
822+
let showMacroExpansionsIsEnabled =
823+
await self.sourceKitLSPServer?.options.experimentalFeatures
824+
.contains(.showMacroExpansions) ?? false
825825

826826
var refactorActions = cursorInfoResponse.refactorActions.compactMap {
827827
let lspCommand = $0.asCommand()
828-
canInlineMacro = $0.actionString == "source.refactoring.kind.inline.macro"
829-
830-
if !(showMacroExpansionsIsEnabled != nil && showMacroExpansionsIsEnabled!) {
831-
canInlineMacro = false
828+
if !canInlineMacro, showMacroExpansionsIsEnabled {
829+
canInlineMacro = $0.actionString == "source.refactoring.kind.inline.macro"
832830
}
833831

834832
return CodeAction(title: $0.title, kind: .refactor, command: lspCommand)

‎Tests/SourceKitLSPTests/ExecuteCommandTests.swift

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,9 @@ final class ExecuteCommandTests: XCTestCase {
140140
func testFreestandingMacroExpansion() async throws {
141141
try await SkipUnless.canBuildMacroUsingSwiftSyntaxFromSourceKitLSPBuild()
142142

143+
var serverOptions = SourceKitLSPServer.Options.testDefault
144+
serverOptions.experimentalFeatures.insert(.showMacroExpansions)
145+
143146
let project = try await SwiftPMTestProject(
144147
files: [
145148
"MyMacros/MyMacros.swift": #"""
@@ -178,7 +181,7 @@ final class ExecuteCommandTests: XCTestCase {
178181
""",
179182
],
180183
manifest: SwiftPMTestProject.macroPackageManifest,
181-
serverOptions: .init(swiftPublishDiagnosticsDebounceDuration: 0, experimentalFeatures: [.showMacroExpansions])
184+
serverOptions: serverOptions
182185
)
183186
try await SwiftPMTestProject.build(at: project.scratchDirectory)
184187

0 commit comments

Comments
 (0)