Address Review Comments

Author: lokesh-tr

Sources/SKSupport/FileSystem.swift

@@ -36,15 +36,3 @@ extension AbsolutePath {
 public var defaultDirectoryForGeneratedFiles: AbsolutePath {
   try! AbsolutePath(validating: NSTemporaryDirectory()).appending(component: "sourcekit-lsp")
 }
-
-/// The default directory to write generated module interfaces
-/// `<TEMPORARY_DIRECTORY>/sourcekit-lsp/GeneratedInterfaces/`
-public var defaultDirectoryForGeneratedInterfaces: AbsolutePath {
-  defaultDirectoryForGeneratedFiles.appending(component: "GeneratedInterfaces")
-}
-
-/// The default directory to write generated macro expansions
-/// `<TEMPORARY_DIRECTORY>/sourcekit-lsp/GeneratedMacroExpansions/`
-public var defaultDirectoryForGeneratedMacroExpansions: AbsolutePath {
-  defaultDirectoryForGeneratedFiles.appending(component: "GeneratedMacroExpansions")
-}

Sources/SourceKitLSP/Swift/MacroExpansion.swift

@@ -14,6 +14,8 @@ 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`
@@ -32,17 +34,12 @@ struct MacroExpansion: RefactoringResponse {
     self.title = title
     self.uri = uri
     self.edits = refactoringEdits.compactMap { refactoringEdit in
-
-      guard let bufferName = refactoringEdit.bufferName else {
+      if refactoringEdit.bufferName.isEmpty {
         logger.error("Unable to retrieve some parts of the expansion")
         return nil
       }
 
-      return MacroExpansionEdit(
-        range: refactoringEdit.startPosition..<refactoringEdit.endPosition,
-        newText: refactoringEdit.newText,
-        bufferName: bufferName
-      )
+      return refactoringEdit
     }
   }
 }
@@ -86,9 +83,13 @@ extension SwiftLanguageService {
 
       Task {
         let req = ShowDocumentRequest(uri: macroExpansionDocURI, selection: macroEdit.range)
-        let response = try await sourceKitLSPServer.sendRequestToClient(req)
-        if !response.success {
-          logger.error("client refused to show document for \(expansion.title, privacy: .public)!")
+
+        let response = await orLog("Sending ShowDocumentRequest to Client") {
+          try await sourceKitLSPServer.sendRequestToClient(req)
+        }
+
+        if !(response?.success ?? true) {
+          logger.error("client refused to show document for \(expansion.title, privacy: .public)")
         }
       }
     }

Sources/SourceKitLSP/Swift/RefactorCommand.swift

@@ -13,10 +13,8 @@
 import LanguageServerProtocol
 import SourceKitD
 
-/// A protocol to be utilised by all commands where underlies sourcekitd calls
-/// to perform semantic refactoring.
+/// A protocol to be utilised by all commands that are served by sourcekitd refactorings.
 protocol RefactorCommand: SwiftCommand {
-
   /// The response type of the refactor command
   associatedtype Response: RefactoringResponse
 

Sources/SourceKitLSP/Swift/Refactoring.swift

@@ -12,52 +12,43 @@
 
 import LanguageServerProtocol
 import SourceKitD
+import LSPLogging
 
 protocol RefactoringResponse {
-
   init(title: String, uri: DocumentURI, refactoringEdits: [RefactoringEdit])
-
-  /// Create an instance of `RefactoringResponse` from a sourcekitd semantic
-  /// refactoring response dictionary, if possible. Passes the response to
-  /// `init(title: String, uri: DocumentURI, refactoringEdits:
-  /// [RefactoringEdit])` in a neat manner
-  init?(_ title: String, _ dict: SKDResponseDictionary, _ snapshot: DocumentSnapshot, _ keys: sourcekitd_api_keys)
 }
 
-typealias RefactoringEdit = (startPosition: Position, endPosition: Position, newText: String, bufferName: String?)
-
 extension RefactoringResponse {
-
-  /// Create an instance of `RefactoringResponse` from a sourcekitd semantic
-  /// refactoring response dictionary, if possible. Passes the response to
-  /// `init(title: String, uri: DocumentURI, refactoringEdits:
-  /// [RefactoringEdit])` in a neat manner
+  /// Create an instance of `RefactoringResponse` from a sourcekitd semantic  
+  /// refactoring response dictionary, if possible. 
   ///
   /// - Parameters:
   ///   - title: The title of the refactoring action.
   ///   - dict: Response dictionary to extract information from.
-  ///   - snapshot: The snapshot that triggered the `semantic_refactoring`
-  /// request.
+  ///   - snapshot: The snapshot that triggered the `semantic_refactoring` request.
   ///   - 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")
       return nil
     }
 
     var refactoringEdits = [RefactoringEdit]()
 
-    categorizedEdits.forEach { _, value in
-      guard let edits: SKDResponseArray = value[keys.edits] else {
-        return false
+    categorizedEdits.forEach { _, categorizedEdit in
+      guard let edits: SKDResponseArray = categorizedEdit[keys.edits] else {
+        logger.error("edits doesn't exist in categorizedEdit dictionary")
+        return true
       }
-      edits.forEach { _, value in
+      edits.forEach { _, edit in
         // The LSP is zero based, but semantic_refactoring is one based.
-        guard let startLine: Int = value[keys.line],
-          let startColumn: Int = value[keys.column],
-          let endLine: Int = value[keys.endLine],
-          let endColumn: Int = value[keys.endColumn],
-          let text: String = value[keys.text]
+        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)")
           return true  // continue
         }
 
@@ -74,17 +65,15 @@ extension RefactoringResponse {
         // can't be represented in the editor properly.
         let textWithSnippets = rewriteSourceKitPlaceholders(in: text, clientSupportsSnippets: false)
         refactoringEdits.append(
-          (
-            startPosition: startPosition, endPosition: endPosition, newText: textWithSnippets,
-            bufferName: value[keys.bufferName]
-          )
+          RefactoringEdit(range: startPosition..<endPosition, newText: textWithSnippets, bufferName: edit[keys.bufferName] ?? "")
         )
         return true
       }
       return true
     }
 
-    guard refactoringEdits.isEmpty == false else {
+    guard !refactoringEdits.isEmpty else {
+      logger.error("No refactors found")
       return nil
     }
 
@@ -100,13 +89,12 @@ extension SwiftLanguageService {
   /// request, such as the necessary edits and placeholder locations.
   ///
   /// - Parameters:
-  ///   - command: The semantic `RefactorCommand` that triggered this request.
-  ///   - responseType: The response type `T.Type` of the particular command
+  ///   - refactorCommand: The semantic `RefactorCommand` that triggered this request.
   /// - Returns:
   ///   - The `RefactoringResponse` as `T`
-  func refactoring<T: RefactorCommand>(_ refactorCommand: T) async throws
-    -> T.Response
-  {
+  func refactoring<T: RefactorCommand>(
+    _ refactorCommand: T
+  ) async throws -> T.Response {
     let keys = self.keys
 
     let uri = refactorCommand.textDocument.uri

Sources/SourceKitLSP/Swift/MacroExpansionEdit.swift renamed to Sources/SourceKitLSP/Swift/RefactoringEdit.swift

@@ -13,9 +13,8 @@
 import LanguageServerProtocol
 import SourceKitD
 
-/// Represents a macro expansion as an edit. Notionally, a subclass of
-/// `TextEdit`
-@_spi(Testing) public struct MacroExpansionEdit: Hashable, Sendable, Codable {
+/// 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>
   public var range: Range<Position>
@@ -32,7 +31,7 @@ import SourceKitD
   }
 }
 
-extension MacroExpansionEdit: LSPAnyCodable {
+extension RefactoringEdit: LSPAnyCodable {
   public init?(fromLSPDictionary dictionary: [String: LSPAny]) {
     guard case .dictionary(let rangeDict) = dictionary[CodingKeys.range.stringValue],
       case .string(let newText) = dictionary[CodingKeys.newText.stringValue],

Sources/SourceKitLSP/Swift/SemanticRefactoring.swift

@@ -34,7 +34,7 @@ struct SemanticRefactoring: RefactoringResponse {
   init(title: String, uri: DocumentURI, refactoringEdits: [RefactoringEdit]) {
     self.title = title
     self.edit = WorkspaceEdit(changes: [
-      uri: refactoringEdits.map { TextEdit(range: $0.startPosition..<$0.endPosition, newText: $0.newText) }
+      uri: refactoringEdits.map { TextEdit(range: $0.range, newText: $0.newText) }
     ])
   }
 }
@@ -68,8 +68,7 @@ extension SwiftLanguageService {
   /// side for the actual refactoring.
   ///
   /// - Parameters:
-  ///   - semanticRefactorCommand: The `SemanticRefactorCommand` that triggered
-  /// this request.
+  ///   - semanticRefactorCommand: The `SemanticRefactorCommand` that triggered this request.
   ///
   /// - Returns:
   ///   - a `WorkspaceEdit` with the necessary refactors as a `LSPAny`
@@ -94,7 +93,7 @@ extension SwiftLanguageService {
       } else {
         reason = ""
       }
-      logger.error("client refused to apply edit for \(semanticRefactor.title, privacy: .public)!\(reason)")
+      logger.error("client refused to apply edit for \(semanticRefactor.title, privacy: .public) \(reason)")
     }
 
     return edit.encodeToLSPAny()

Tests/SourceKitLSPTests/ExecuteCommandTests.swift

@@ -14,6 +14,7 @@ import LSPTestSupport
 import LanguageServerProtocol
 import SKTestSupport
 import XCTest
+import SwiftExtensions
 @_spi(Testing) import SourceKitLSP
 
 final class ExecuteCommandTests: XCTestCase {
@@ -182,18 +183,16 @@ final class ExecuteCommandTests: XCTestCase {
 
     let (uri, positions) = try project.openDocument("MyMacroClient.swift")
 
-    let argsToBeTested = [
-      ExpandMacroCommand(
-        positionRange: positions["1️⃣"]..<positions["3️⃣"],
-        textDocument: TextDocumentIdentifier(uri)
-      ),
-      ExpandMacroCommand(
-        positionRange: positions["2️⃣"]..<positions["3️⃣"],
-        textDocument: TextDocumentIdentifier(uri)
-      ),
+    let positionMarkersToBeTested = [
+      (start: "1️⃣", end: "1️⃣"),
+      (start: "2️⃣", end: "2️⃣"),
+      (start: "1️⃣", end: "3️⃣"),
+      (start: "2️⃣", end: "3️⃣"),
     ]
 
-    for args in argsToBeTested {
+    for positionMarker in positionMarkersToBeTested {
+      let args = ExpandMacroCommand(positionRange: positions[positionMarker.start]..<positions[positionMarker.end], textDocument: TextDocumentIdentifier(uri))
+
       let metadata = SourceKitLSPCommandMetadata(textDocument: TextDocumentIdentifier(uri))
 
       var command = args.asCommand()
@@ -202,35 +201,31 @@ final class ExecuteCommandTests: XCTestCase {
       let request = ExecuteCommandRequest(command: command.command, arguments: command.arguments)
 
       let expectation = XCTestExpectation(description: "Handle Show Document Request")
-      var showDocumentRequestURI: DocumentURI? = nil
+      let showDocumentRequestURI = ThreadSafeBox<DocumentURI?>(initialValue: nil)
 
       project.testClient.handleSingleRequest { (req: ShowDocumentRequest) in
-        showDocumentRequestURI = req.uri
+        showDocumentRequestURI.value = req.uri
         expectation.fulfill()
         return ShowDocumentResponse(success: true)
       }
 
       let result = try await project.testClient.send(request)
 
       guard let resultArray: [MacroExpansionEdit] = Array(fromLSPArray: result ?? .null) else {
-        XCTFail("Result is not an array.")
+        XCTFail("Result is not an array. Failed for position range between \(positionMarker.start) and \(positionMarker.end)")
         return
       }
 
-      XCTAssertEqual(resultArray.count, 1)
-      XCTAssertEqual(resultArray.only?.newText, "(1 + 2, \"1 + 2\")")
+      XCTAssertEqual(resultArray.count, 1, "resultArray is empty. Failed for position range between \(positionMarker.start) and \(positionMarker.end)")
+      XCTAssertEqual(resultArray.only?.newText, "(1 + 2, \"1 + 2\")", "Wrong macro expansion. Failed for position range between \(positionMarker.start) and \(positionMarker.end)")
 
       await fulfillment(of: [expectation], timeout: 10.0)
 
-      let url = try XCTUnwrap(showDocumentRequestURI?.fileURL)
-
-      XCTAssert(
-        (try? url.checkResourceIsReachable()) ?? false
-      )
+      let url = try XCTUnwrap(showDocumentRequestURI.value?.fileURL, "Failed for position range between \(positionMarker.start) and \(positionMarker.end)")
 
-      let fileContents = try XCTUnwrap(try? String(contentsOf: url, encoding: .utf8))
+      let fileContents = try String(contentsOf: url, encoding: .utf8)
 
-      XCTAssert(fileContents.contains("(1 + 2, \"1 + 2\")"))
+      XCTAssert(fileContents.contains("(1 + 2, \"1 + 2\")"), "File doesn't contain macro expansion. Failed for position range between \(positionMarker.start) and \(positionMarker.end)")
     }
   }