Merge pull request #1404 from ahoppen/cpp-to-swift-rename-tests

Add tests to rename C++ symbols exposed to Swift

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
+    )
+  }
 }