Address Review Comments

Author: lokesh-tr

Sources/SourceKitLSP/Swift/MacroExpansion.swift

@@ -15,8 +15,6 @@ import LSPLogging
 import LanguageServerProtocol
 import SourceKitD
 
-@_spi(Testing) public typealias MacroExpansionEdit = RefactoringEdit
-
 /// Detailed information about the result of a macro expansion operation.
 ///
 /// Wraps the information returned by sourcekitd's `semantic_refactoring`
@@ -28,15 +26,15 @@ struct MacroExpansion: RefactoringResponse {
   /// The URI of the file where the macro is used
   var uri: DocumentURI
 
-  /// The resulting array of `MacroExpansionEdit` of a semantic refactoring request
-  var edits: [MacroExpansionEdit]
+  /// The resulting array of `RefactoringEdit` of a semantic refactoring request
+  var edits: [RefactoringEdit]
 
   init(title: String, uri: DocumentURI, refactoringEdits: [RefactoringEdit]) {
     self.title = title
     self.uri = uri
     self.edits = refactoringEdits.compactMap { refactoringEdit in
       if refactoringEdit.bufferName.isEmpty {
-        logger.error("Unable to retrieve some parts of the expansion")
+        logger.fault("Unable to retrieve some parts of the expansion")
         return nil
       }
 
@@ -53,12 +51,9 @@ extension SwiftLanguageService {
   /// expansion to be displayed.
   ///
   /// - Parameters:
-  ///   - expandMacroCommand: The `ExpandMacroCommand` that triggered this
-  /// request.
+  ///   - expandMacroCommand: The `ExpandMacroCommand` that triggered this request.
   ///
-  /// - Returns:
-  ///   - an `[MacroExpansionEdit]` with the necessary edits and buffer name as
-  /// a `LSPAny`
+  /// - Returns: An `[RefactoringEdit]` with the necessary edits and buffer name as a `LSPAny`
   func expandMacro(
     _ expandMacroCommand: ExpandMacroCommand
   ) async throws -> LSPAny {
@@ -69,17 +64,20 @@ extension SwiftLanguageService {
     }
 
     guard let sourceFileURL = expandMacroCommand.textDocument.uri.fileURL else {
-      throw ResponseError.unknown("Given URL is not a file URL")
+      throw ResponseError.unknown("Given URI is not a file URL")
     }
 
     let expansion = try await self.refactoring(expandMacroCommand)
 
     for macroEdit in expansion.edits {
-      let macroExpansionBufferDirectoryName = macroEdit.bufferName.dropLast(6)  // buffer name without ".swift"
-
-      let macroExpansionBufferDirectoryURL = self.generatedMacroExpansionsPath.appendingPathComponent(
-        String(macroExpansionBufferDirectoryName)
-      )
+      // buffer name without ".swift"
+      let macroExpansionBufferDirectoryName =
+        macroEdit.bufferName.hasSuffix(".swift")
+        ? String(macroEdit.bufferName.dropLast(6))
+        : macroEdit.bufferName
+
+      let macroExpansionBufferDirectoryURL = self.generatedMacroExpansionsPath
+        .appendingPathComponent(macroExpansionBufferDirectoryName)
       do {
         try FileManager.default.createDirectory(at: macroExpansionBufferDirectoryURL, withIntermediateDirectories: true)
       } catch {
@@ -96,22 +94,20 @@ extension SwiftLanguageService {
         "\(macroExpansionFileName)_\(macroExpansionPositionRangeIndicator).\(sourceFileURL.pathExtension)"
       )
 
-      let macroExpansionDocURI = DocumentURI(macroExpansionFilePath)
-
       do {
-        try macroEdit.newText.write(to: macroExpansionFilePath, atomically: true, encoding: String.Encoding.utf8)
+        try macroEdit.newText.write(to: macroExpansionFilePath, atomically: true, encoding: .utf8)
       } catch {
         throw ResponseError.unknown("Unable to write macro expansion to file path: \"\(macroExpansionFilePath.path)\"")
       }
 
       Task {
-        let req = ShowDocumentRequest(uri: macroExpansionDocURI, selection: macroEdit.range)
+        let req = ShowDocumentRequest(uri: DocumentURI(macroExpansionFilePath), selection: macroEdit.range)
 
         let response = await orLog("Sending ShowDocumentRequest to Client") {
           try await sourceKitLSPServer.sendRequestToClient(req)
         }
 
-        if !(response?.success ?? true) {
+        if let response, !response.success {
           logger.error("client refused to show document for \(expansion.title, privacy: .public)")
         }
       }

Sources/SourceKitLSP/Swift/Refactoring.swift

@@ -29,29 +29,29 @@ extension RefactoringResponse {
   ///   - keys: The sourcekitd key set to use for looking up into `dict`.
   init?(_ title: String, _ dict: SKDResponseDictionary, _ snapshot: DocumentSnapshot, _ keys: sourcekitd_api_keys) {
     guard let categorizedEdits: SKDResponseArray = dict[keys.categorizedEdits] else {
-      logger.error("categorizedEdits doesn't exist in response dictionary")
+      logger.fault("categorizedEdits doesn't exist in response dictionary")
       return nil
     }
 
     var refactoringEdits = [RefactoringEdit]()
 
     categorizedEdits.forEach { _, categorizedEdit in
       guard let edits: SKDResponseArray = categorizedEdit[keys.edits] else {
-        logger.error("edits doesn't exist in categorizedEdit dictionary")
+        logger.fault("edits doesn't exist in categorizedEdit dictionary")
         return true
       }
       edits.forEach { _, edit in
-        // The LSP is zero based, but semantic_refactoring is one based.
         guard let startLine: Int = edit[keys.line],
           let startColumn: Int = edit[keys.column],
           let endLine: Int = edit[keys.endLine],
           let endColumn: Int = edit[keys.endColumn],
           let text: String = edit[keys.text]
         else {
-          logger.error("Failed to deserialise edit dictionary containing values: \(edit)")
+          logger.fault("Failed to deserialise edit dictionary containing values: \(edit)")
           return true  // continue
         }
 
+        // The LSP is zero based, but semantic_refactoring is one based.
         let startPosition = snapshot.positionOf(
           zeroBasedLine: startLine - 1,
           utf8Column: startColumn - 1
@@ -77,7 +77,7 @@ extension RefactoringResponse {
     }
 
     guard !refactoringEdits.isEmpty else {
-      logger.error("No refactors found")
+      logger.error("No refactoring edits found")
       return nil
     }
 
@@ -94,8 +94,7 @@ extension SwiftLanguageService {
   ///
   /// - Parameters:
   ///   - refactorCommand: The semantic `RefactorCommand` that triggered this request.
-  /// - Returns:
-  ///   - The `RefactoringResponse` as `T`
+  /// - Returns: The response of the refactoring
   func refactoring<T: RefactorCommand>(
     _ refactorCommand: T
   ) async throws -> T.Response {

Sources/SourceKitLSP/Swift/RefactoringEdit.swift

@@ -13,7 +13,7 @@
 import LanguageServerProtocol
 import SourceKitD
 
-/// Represents an edit from semantic refactor response. Notionally, a subclass /// of `TextEdit`
+/// Represents an edit from semantic refactor response. Notionally, a subclass of `TextEdit`
 @_spi(Testing) public struct RefactoringEdit: Hashable, Sendable, Codable {
   /// The range of text to be replaced.
   @CustomCodable<PositionRange>
@@ -22,6 +22,10 @@ import SourceKitD
   /// The new text.
   public var newText: String
 
+  /// If the new text of the edit should not be applied to the original source
+  /// file but to a separate buffer, a fake name for that buffer. For example
+  /// for expansion of macros, this is @ followed by the mangled name of the
+  /// macro expansion, followed by .swift.
   public var bufferName: String
 
   public init(range: Range<Position>, newText: String, bufferName: String) {

Sources/SourceKitLSP/Swift/SemanticRefactoring.swift

@@ -70,8 +70,7 @@ extension SwiftLanguageService {
   /// - Parameters:
   ///   - semanticRefactorCommand: The `SemanticRefactorCommand` that triggered this request.
   ///
-  /// - Returns:
-  ///   - a `WorkspaceEdit` with the necessary refactors as a `LSPAny`
+  /// - Returns: A `WorkspaceEdit` with the necessary refactors as a `LSPAny`
   func semanticRefactoring(
     _ semanticRefactorCommand: SemanticRefactorCommand
   ) async throws -> LSPAny {

Tests/SourceKitLSPTests/ExecuteCommandTests.swift

@@ -203,7 +203,7 @@ final class ExecuteCommandTests: XCTestCase {
 
       let request = ExecuteCommandRequest(command: command.command, arguments: command.arguments)
 
-      let expectation = XCTestExpectation(description: "Handle Show Document Request")
+      let expectation = self.expectation(description: "Handle Show Document Request")
       let showDocumentRequestURI = ThreadSafeBox<DocumentURI?>(initialValue: nil)
 
       project.testClient.handleSingleRequest { (req: ShowDocumentRequest) in
@@ -214,7 +214,7 @@ final class ExecuteCommandTests: XCTestCase {
 
       let result = try await project.testClient.send(request)
 
-      guard let resultArray: [MacroExpansionEdit] = Array(fromLSPArray: result ?? .null) else {
+      guard let resultArray: [RefactoringEdit] = Array(fromLSPArray: result ?? .null) else {
         XCTFail(
           "Result is not an array. Failed for position range between \(positionMarker.start) and \(positionMarker.end)"
         )
@@ -232,7 +232,7 @@ final class ExecuteCommandTests: XCTestCase {
         "Wrong macro expansion. Failed for position range between \(positionMarker.start) and \(positionMarker.end)"
       )
 
-      await fulfillment(of: [expectation], timeout: 10.0)
+      try await fulfillmentOfOrThrow([expectation])
 
       let url = try XCTUnwrap(
         showDocumentRequestURI.value?.fileURL,