Merge pull request #1138 from ahoppen/ahoppen/rename-with-multiple-index-entries

Don’t fail rename if there are multiple index entries for the same USR

Author: ahoppen

Sources/SourceKitLSP/Rename.swift

@@ -554,22 +554,50 @@ extension SourceKitServer {
     index: IndexStoreDB
   ) async throws -> CrossLanguageName? {
     let definitions = index.occurrences(ofUSR: usr, roles: [.definition])
-    guard let definitionSymbol = definitions.only else {
-      if definitions.isEmpty {
-        logger.error("no definitions for \(usr) found")
-      } else {
-        logger.error("Multiple definitions for \(usr) found")
-      }
+    if definitions.isEmpty {
+      logger.error("no definitions for \(usr) found")
       return nil
     }
+    if definitions.count > 1 {
+      logger.log("Multiple definitions for \(usr) found")
+    }
+    // There might be multiple definitions of the same symbol eg. in different `#if` branches. In this case pick any of
+    // them because with very high likelihood they all translate to the same clang and Swift name. Sort the entries to
+    // ensure that we deterministically pick the same entry every time.
+    for definitionOccurrence in definitions.sorted() {
+      do {
+        return try await getCrossLanguageName(
+          forDefinitionOccurrence: definitionOccurrence,
+          overrideName: overrideName,
+          workspace: workspace,
+          index: index
+        )
+      } catch {
+        // If getting the cross-language name fails for this occurrence, try the next definition, if there are multiple.
+        logger.log(
+          "Getting cross-language name for occurrence at \(definitionOccurrence.location) failed. \(error.forLogging)"
+        )
+      }
+    }
+    return nil
+  }
+
+  private func getCrossLanguageName(
+    forDefinitionOccurrence definitionOccurrence: SymbolOccurrence,
+    overrideName: String? = nil,
+    workspace: Workspace,
+    index: IndexStoreDB
+  ) async throws -> CrossLanguageName {
+    let definitionSymbol = definitionOccurrence.symbol
+    let usr = definitionSymbol.usr
     let definitionLanguage: Language =
-      switch definitionSymbol.symbol.language {
+      switch definitionSymbol.language {
       case .c: .c
       case .cxx: .cpp
       case .objc: .objective_c
       case .swift: .swift
       }
-    let definitionDocumentUri = DocumentURI(URL(fileURLWithPath: definitionSymbol.location.path))
+    let definitionDocumentUri = DocumentURI(URL(fileURLWithPath: definitionOccurrence.location.path))
 
     guard
       let definitionLanguageService = await self.languageService(
@@ -578,25 +606,24 @@ extension SourceKitServer {
         in: workspace
       )
     else {
-      logger.fault("Failed to get language service for the document defining \(usr)")
-      return nil
+      throw ResponseError.unknown("Failed to get language service for the document defining \(usr)")
     }
 
-    let definitionName = overrideName ?? definitionSymbol.symbol.name
+    let definitionName = overrideName ?? definitionSymbol.name
 
     switch definitionLanguageService {
     case is ClangLanguageServerShim:
       let swiftName: String?
       if let swiftReference = await getReferenceFromSwift(usr: usr, index: index, workspace: workspace) {
-        let isObjectiveCSelector = definitionLanguage == .objective_c && definitionSymbol.symbol.kind.isMethod
+        let isObjectiveCSelector = definitionLanguage == .objective_c && definitionSymbol.kind.isMethod
         swiftName = try await swiftReference.languageServer.translateClangNameToSwift(
           at: swiftReference.location,
           in: swiftReference.snapshot,
           isObjectiveCSelector: isObjectiveCSelector,
           name: definitionName
         )
       } else {
-        logger.debug("Not translating \(usr) to Swift because it is not referenced from Swift")
+        logger.debug("Not translating \(definitionSymbol) to Swift because it is not referenced from Swift")
         swiftName = nil
       }
       return CrossLanguageName(clangName: definitionName, swiftName: swiftName, definitionLanguage: definitionLanguage)
@@ -610,7 +637,7 @@ extension SourceKitServer {
       let clangName: String?
       if hasReferenceFromClang {
         clangName = try await swiftLanguageServer.translateSwiftNameToClang(
-          at: definitionSymbol.location,
+          at: definitionOccurrence.location,
           in: definitionDocumentUri,
           name: CompoundDeclName(definitionName)
         )

Tests/SourceKitLSPTests/RenameTests.swift

@@ -1081,4 +1081,77 @@ final class RenameTests: XCTestCase {
         """
     )
   }
+
+  func testRenameAfterFileMove() async throws {
+    let ws = try await SwiftPMTestWorkspace(
+      files: [
+        "definition.swift": """
+        func 1️⃣foo2️⃣() {}
+        """,
+        "caller.swift": """
+        func test() {
+          3️⃣foo4️⃣()
+        }
+        """,
+      ],
+      build: true
+    )
+
+    let definitionUri = try ws.uri(for: "definition.swift")
+    let (callerUri, callerPositions) = try ws.openDocument("caller.swift")
+
+    // Validate that we get correct rename results before moving the definition file.
+    let resultBeforeFileMove = try await ws.testClient.send(
+      RenameRequest(textDocument: TextDocumentIdentifier(callerUri), position: callerPositions["3️⃣"], newName: "bar")
+    )
+    XCTAssertEqual(
+      resultBeforeFileMove,
+      WorkspaceEdit(
+        changes: [
+          definitionUri: [
+            TextEdit(
+              range: try ws.position(of: "1️⃣", in: "definition.swift")..<ws.position(of: "2️⃣", in: "definition.swift"),
+              newText: "bar"
+            )
+          ],
+          callerUri: [TextEdit(range: callerPositions["3️⃣"]..<callerPositions["4️⃣"], newText: "bar")],
+        ]
+      )
+    )
+
+    let movedDefinitionUri =
+      DocumentURI(
+        definitionUri.fileURL!
+          .deletingLastPathComponent()
+          .appendingPathComponent("movedDefinition.swift")
+      )
+
+    try FileManager.default.moveItem(at: XCTUnwrap(definitionUri.fileURL), to: XCTUnwrap(movedDefinitionUri.fileURL))
+
+    ws.testClient.send(
+      DidChangeWatchedFilesNotification(changes: [
+        FileEvent(uri: definitionUri, type: .deleted), FileEvent(uri: movedDefinitionUri, type: .created),
+      ])
+    )
+
+    try await SwiftPMTestWorkspace.build(at: ws.scratchDirectory)
+
+    let resultAfterFileMove = try await ws.testClient.send(
+      RenameRequest(textDocument: TextDocumentIdentifier(callerUri), position: callerPositions["3️⃣"], newName: "bar")
+    )
+    XCTAssertEqual(
+      resultAfterFileMove,
+      WorkspaceEdit(
+        changes: [
+          movedDefinitionUri: [
+            TextEdit(
+              range: try ws.position(of: "1️⃣", in: "definition.swift")..<ws.position(of: "2️⃣", in: "definition.swift"),
+              newText: "bar"
+            )
+          ],
+          callerUri: [TextEdit(range: callerPositions["3️⃣"]..<callerPositions["4️⃣"], newText: "bar")],
+        ]
+      )
+    )
+  }
 }