Add tests to rename C++ symbols exposed to Swift

This exposes an issue where we wouldn’t rename a symbol on the clang side f a symbol was only defined in a header and not a C/C++/... file. In that case we failed to determine the language of the header file because there was no unit file that contained the header file. The cleaner solution here is to ask for the symbol provider of each occurrence directly.

rdar://127391127

Author: ahoppen

Sources/SKTestSupport/SwiftPMTestProject.swift

@@ -177,7 +177,7 @@ public class SwiftPMTestProject: MultiFileTestProject {
     if !manifest.contains("swift-tools-version") {
       // Tests specify a shorthand package manifest that doesn't contain the tools version boilerplate.
       manifest = """
-        // swift-tools-version: 5.7
+        // swift-tools-version: 5.10
 
         import PackageDescription
 

Sources/SemanticIndex/CheckedIndex.swift

@@ -176,10 +176,6 @@ public struct UncheckedIndex: Sendable {
     return CheckedIndex(index: underlyingIndexStoreDB, checkLevel: checkLevel)
   }
 
-  public func symbolProvider(for sourceFilePath: String) -> SymbolProviderKind? {
-    return underlyingIndexStoreDB.symbolProvider(for: sourceFilePath)
-  }
-
   /// Wait for IndexStoreDB to be updated based on new unit files written to disk.
   public func pollForUnitChangesAndWait() {
     self.underlyingIndexStoreDB.pollForUnitChangesAndWait()

Sources/SourceKitLSP/Rename.swift

@@ -500,7 +500,7 @@ extension SourceKitLSPServer {
   ) async -> (swiftLanguageService: SwiftLanguageService, snapshot: DocumentSnapshot, location: SymbolLocation)? {
     var reference: SymbolOccurrence? = nil
     index.forEachSymbolOccurrence(byUSR: usr, roles: renameRoles) {
-      if index.unchecked.symbolProvider(for: $0.location.path) == .swift {
+      if $0.symbolProvider == .swift {
         reference = $0
         // We have found a reference from Swift. Stop iteration.
         return false
@@ -631,7 +631,7 @@ extension SourceKitLSPServer {
       // If we terminate early by returning `false` from the closure, `forEachSymbolOccurrence` returns `true`,
       // indicating that we have found a reference from clang.
       let hasReferenceFromClang = !index.forEachSymbolOccurrence(byUSR: usr, roles: renameRoles) {
-        return index.unchecked.symbolProvider(for: $0.location.path) != .clang
+        return $0.symbolProvider != .clang
       }
       let clangName: String?
       if hasReferenceFromClang {
@@ -730,32 +730,7 @@ extension SourceKitLSPServer {
 
     // If we have a USR + old name, perform an index lookup to find workspace-wide symbols to rename.
     // First, group all occurrences of that USR by the files they occur in.
-    var locationsByFile: [DocumentURI: [RenameLocation]] = [:]
-
-    actor LanguageServerTypesCache {
-      let index: UncheckedIndex
-      var languageServerTypesCache: [DocumentURI: LanguageServerType?] = [:]
-
-      init(index: UncheckedIndex) {
-        self.index = index
-      }
-
-      func languageServerType(for uri: DocumentURI) -> LanguageServerType? {
-        if let cachedValue = languageServerTypesCache[uri] {
-          return cachedValue
-        }
-        let serverType: LanguageServerType? =
-          if let fileURL = uri.fileURL {
-            LanguageServerType(symbolProvider: index.symbolProvider(for: fileURL.path))
-          } else {
-            nil
-          }
-        languageServerTypesCache[uri] = serverType
-        return serverType
-      }
-    }
-
-    let languageServerTypesCache = LanguageServerTypesCache(index: index.unchecked)
+    var locationsByFile: [DocumentURI: (renameLocations: [RenameLocation], symbolProvider: SymbolProviderKind)] = [:]
 
     let usrsToRename = overridingAndOverriddenUsrs(of: usr, index: index)
     let occurrencesToRename = usrsToRename.flatMap { index.occurrences(ofUSR: $0, roles: renameRoles) }
@@ -770,19 +745,16 @@ extension SourceKitLSPServer {
           // perform an indexed rename for it.
           continue
         }
-        switch await languageServerTypesCache.languageServerType(for: uri) {
+        switch occurrence.symbolProvider {
         case .swift:
           // sourcekitd only produces AST-based results for the direct calls to this USR. This is because the Swift
           // AST only has upwards references to superclasses and overridden methods, not the other way round. It is
           // thus not possible to (easily) compute an up-down closure like described in `overridingAndOverriddenUsrs`.
           // We thus need to perform an indexed rename for other, related USRs.
           break
-        case .clangd:
+        case .clang:
           // clangd produces AST-based results for the entire class hierarchy, so nothing to do.
           continue
-        case nil:
-          // Unknown symbol provider
-          continue
         }
       }
 
@@ -791,25 +763,39 @@ extension SourceKitLSPServer {
         utf8Column: occurrence.location.utf8Column,
         usage: RenameLocation.Usage(roles: occurrence.roles)
       )
-      locationsByFile[uri, default: []].append(renameLocation)
+      if let existingLocations = locationsByFile[uri] {
+        if existingLocations.symbolProvider != occurrence.symbolProvider {
+          logger.fault(
+            """
+            Found mismatching symbol providers for \(uri.forLogging): \
+            \(String(describing: existingLocations.symbolProvider), privacy: .public) vs \
+            \(String(describing: occurrence.symbolProvider), privacy: .public)
+            """
+          )
+        }
+        locationsByFile[uri] = (existingLocations.renameLocations + [renameLocation], occurrence.symbolProvider)
+      } else {
+        locationsByFile[uri] = ([renameLocation], occurrence.symbolProvider)
+      }
     }
 
     // Now, call `editsToRename(locations:in:oldName:newName:)` on the language service to convert these ranges into
     // edits.
     let urisAndEdits =
       await locationsByFile
-      .concurrentMap { (uri: DocumentURI, renameLocations: [RenameLocation]) -> (DocumentURI, [TextEdit])? in
+      .concurrentMap {
+        (
+          uri: DocumentURI,
+          value: (renameLocations: [RenameLocation], symbolProvider: SymbolProviderKind)
+        ) -> (DocumentURI, [TextEdit])? in
         let language: Language
-        switch await languageServerTypesCache.languageServerType(for: uri) {
-        case .clangd:
+        switch value.symbolProvider {
+        case .clang:
           // Technically, we still don't know the language of the source file but defaulting to C is sufficient to
           // ensure we get the clang toolchain language server, which is all we care about.
           language = .c
         case .swift:
           language = .swift
-        case nil:
-          logger.error("Failed to determine symbol provider for \(uri.forLogging)")
-          return nil
         }
         // Create a document snapshot to operate on. If the document is open, load it from the document manager,
         // otherwise conjure one from the file on disk. We need the file in memory to perform UTF-8 to UTF-16 column
@@ -825,13 +811,13 @@ extension SourceKitLSPServer {
         var edits: [TextEdit] =
           await orLog("Getting edits for rename location") {
             return try await languageService.editsToRename(
-              locations: renameLocations,
+              locations: value.renameLocations,
               in: snapshot,
               oldName: oldName,
               newName: newName
             )
           } ?? []
-        for location in renameLocations where location.usage == .definition {
+        for location in value.renameLocations where location.usage == .definition {
           edits += await languageService.editsToRenameParametersInFunctionBody(
             snapshot: snapshot,
             renameLocation: location,

Tests/SourceKitLSPTests/CrossLanguageRenameTests.swift

@@ -23,6 +23,16 @@ private let libAlibBPackageManifest = """
   )
   """
 
+private let libAlibBCxxInteropPackageManifest = """
+  let package = Package(
+    name: "MyLibrary",
+    targets: [
+    .target(name: "LibA"),
+    .target(name: "LibB", dependencies: ["LibA"], swiftSettings: [.interoperabilityMode(.Cxx)]),
+    ]
+  )
+  """
+
 final class CrossLanguageRenameTests: XCTestCase {
   func testZeroArgCFunction() async throws {
     try await SkipUnless.clangdSupportsIndexBasedRename()
@@ -733,4 +743,140 @@ final class CrossLanguageRenameTests: XCTestCase {
       manifest: libAlibBPackageManifest
     )
   }
+
+  func testRenameCxxClassExposedToSwift() async throws {
+    try await SkipUnless.clangdSupportsIndexBasedRename()
+    try await assertMultiFileRename(
+      files: [
+        "LibA/include/LibA.h": """
+        struct 1️⃣Foo {};
+        """,
+        "LibA/LibA.cpp": "",
+        "LibB/LibB.swift": """
+        import LibA
+
+        func test(foo: 2️⃣Foo) {}
+        """,
+      ],
+      headerFileLanguage: .cpp,
+      newName: "Bar",
+      expectedPrepareRenamePlaceholder: "Foo",
+      expected: [
+        "LibA/include/LibA.h": """
+        struct Bar {};
+        """,
+        "LibA/LibA.cpp": "",
+        "LibB/LibB.swift": """
+        import LibA
+
+        func test(foo: Bar) {}
+        """,
+      ],
+      manifest: libAlibBCxxInteropPackageManifest
+    )
+  }
+
+  func testRenameCxxMethodExposedToSwift() async throws {
+    try await SkipUnless.clangdSupportsIndexBasedRename()
+    try await assertMultiFileRename(
+      files: [
+        "LibA/include/LibA.h": """
+        struct Foo {
+          int 1️⃣doStuff() const;
+        };
+        """,
+        "LibA/LibA.cpp": """
+        #include "LibA.h"
+
+        int Foo::2️⃣doStuff() const {
+          return 42;
+        }
+        """,
+        "LibB/LibB.swift": """
+        import LibA
+
+        func test(foo: Foo) {
+          foo.3️⃣doStuff()
+        }
+        """,
+      ],
+      headerFileLanguage: .cpp,
+      newName: "doNewStuff",
+      expectedPrepareRenamePlaceholder: "doStuff",
+      expected: [
+        "LibA/include/LibA.h": """
+        struct Foo {
+          int doNewStuff() const;
+        };
+        """,
+        "LibA/LibA.cpp": """
+        #include "LibA.h"
+
+        int Foo::doNewStuff() const {
+          return 42;
+        }
+        """,
+        "LibB/LibB.swift": """
+        import LibA
+
+        func test(foo: Foo) {
+          foo.doNewStuff()
+        }
+        """,
+      ],
+      manifest: libAlibBCxxInteropPackageManifest
+    )
+  }
+
+  func testRenameSwiftMethodExposedToSwift() async throws {
+    try await SkipUnless.clangdSupportsIndexBasedRename()
+    try await assertMultiFileRename(
+      files: [
+        "LibA/include/LibA.h": """
+        struct Foo {
+          int 1️⃣doStuff() const;
+        };
+        """,
+        "LibA/LibA.cpp": """
+        #include "LibA.h"
+
+        int Foo::2️⃣doStuff() const {
+          return 42;
+        }
+        """,
+        "LibB/LibB.swift": """
+        import LibA
+
+        func test(foo: Foo) {
+          foo.3️⃣doStuff()
+        }
+        """,
+      ],
+      headerFileLanguage: .cpp,
+      newName: "doNewStuff",
+      expectedPrepareRenamePlaceholder: "doStuff",
+      expected: [
+        "LibA/include/LibA.h": """
+        struct Foo {
+          int doNewStuff() const;
+        };
+        """,
+        "LibA/LibA.cpp": """
+        #include "LibA.h"
+
+        int Foo::doNewStuff() const {
+          return 42;
+        }
+        """,
+        "LibB/LibB.swift": """
+        import LibA
+
+        func test(foo: Foo) {
+          foo.doNewStuff()
+        }
+        """,
+      ],
+      manifest: libAlibBCxxInteropPackageManifest
+    )
+  }
 }