Merge pull request #2289 from bnbarham/infinite-root-2

Standardize file paths when attempting to find toolchains

Author: bnbarham

Sources/SwiftExtensions/URLExtensions.swift

@@ -87,6 +87,8 @@ extension URL {
     }
   }
 
+  /// Assuming this URL is a file URL, checks if it looks like a root path. This is a string check, ie. the return
+  /// value for a path of `"/foo/.."` would be `false`. An error will be thrown is this is a non-file URL.
   package var isRoot: Bool {
     get throws {
       let checkPath = try filePath

Sources/ToolchainRegistry/Toolchain.swift

@@ -417,10 +417,23 @@ public final class Toolchain: Sendable {
 }
 
 /// Find a containing xctoolchain with plist, if available.
-func containingXCToolchain(
+private func containingXCToolchain(
   _ path: URL
 ) -> (XCToolchainPlist, URL)? {
-  var path = path
+  // `deletingLastPathComponent` only makes sense on resolved paths (ie. those without symlinks or `..`). Any given
+  // toolchain path should have already been realpathed, but since this can turn into an infinite loop otherwise, it's
+  // better to be safe than sorry.
+  let resolvedPath = orLog("Toolchain realpath") {
+    try path.realpath
+  }
+  guard let resolvedPath else {
+    return nil
+  }
+  if path != resolvedPath {
+    logger.fault("\(path) was not realpathed")
+  }
+
+  var path = resolvedPath
   while !((try? path.isRoot) ?? true) {
     if path.pathExtension == "xctoolchain" {
       if let infoPlist = orLog("Loading information from xctoolchain", { try XCToolchainPlist(fromDirectory: path) }) {

Sources/ToolchainRegistry/ToolchainRegistry.swift

@@ -60,13 +60,13 @@ package final actor ToolchainRegistry {
   private let toolchainsByIdentifier: [String: [Toolchain]]
 
   /// The toolchains indexed by their path.
-  private let toolchainsByPath: [URL: Toolchain]
+  private var toolchainsByPath: [URL: Toolchain]
 
   /// Map from compiler paths (`clang`, `swift`, `swiftc`) mapping to the toolchain that contained them.
   ///
   /// This allows us to find the toolchain that should be used for semantic functionality based on which compiler it is
   /// built with in the `compile_commands.json`.
-  private let toolchainsByCompiler: [URL: Toolchain]
+  private var toolchainsByCompiler: [URL: Toolchain]
 
   /// The currently selected toolchain identifier on Darwin.
   package let darwinToolchainOverride: String?
@@ -96,6 +96,13 @@ package final actor ToolchainRegistry {
     var toolchainsByPath: [URL: Toolchain] = [:]
     var toolchainsByCompiler: [URL: Toolchain] = [:]
     for (toolchain, reason) in toolchainsAndReasonsParam {
+      // Toolchain should always be unique by path. It isn't particularly useful to log if we already have a toolchain
+      // though, as we could have just found toolchains through symlinks (this is actually quite normal - eg. OSS
+      // toolchains add a `swift-latest.xctoolchain` symlink on macOS).
+      if toolchainsByPath[toolchain.path] != nil {
+        continue
+      }
+
       // Non-XcodeDefault toolchain: disallow all duplicates.
       if toolchainsByIdentifier[toolchain.identifier] != nil,
         toolchain.identifier != ToolchainRegistry.darwinDefaultToolchainIdentifier
@@ -104,12 +111,6 @@ package final actor ToolchainRegistry {
         continue
       }
 
-      // Toolchain should always be unique by path.
-      if toolchainsByPath[toolchain.path] != nil {
-        logger.fault("Found two toolchains with the same path: \(toolchain.path)")
-        continue
-      }
-
       toolchainsByPath[toolchain.path] = toolchain
       toolchainsByIdentifier[toolchain.identifier, default: []].append(toolchain)
 
@@ -218,9 +219,14 @@ package final actor ToolchainRegistry {
       }
     }
 
-    let toolchainsAndReasons = toolchainPaths.compactMap {
-      if let toolchain = Toolchain($0.path) {
-        return (toolchain, $0.reason)
+    let toolchainsAndReasons = toolchainPaths.compactMap { toolchainAndReason in
+      let resolvedPath = orLog("Toolchain realpath") {
+        try toolchainAndReason.path.realpath
+      }
+      if let resolvedPath,
+        let toolchain = Toolchain(resolvedPath)
+      {
+        return (toolchain, toolchainAndReason.reason)
       }
       return nil
     }
@@ -283,7 +289,43 @@ package final actor ToolchainRegistry {
   /// If we have a toolchain in the toolchain registry that contains the compiler with the given URL, return it.
   /// Otherwise, return `nil`.
   package func toolchain(withCompiler compiler: URL) -> Toolchain? {
-    return toolchainsByCompiler[compiler]
+    if let toolchain = toolchainsByCompiler[compiler] {
+      return toolchain
+    }
+
+    // Only canonicalize the folder path, as we don't want to resolve symlinks to eg. `swift-driver`.
+    let resolvedPath = orLog("Compiler realpath") {
+      try compiler.deletingLastPathComponent().realpath
+    }?.appending(component: compiler.lastPathComponent)
+    guard let resolvedPath,
+      let toolchain = toolchainsByCompiler[resolvedPath]
+    else {
+      return nil
+    }
+
+    // Cache mapping of non-realpath to the realpath toolchain for faster subsequent lookups
+    toolchainsByCompiler[compiler] = toolchain
+    return toolchain
+  }
+
+  /// If we have a toolchain in the toolchain registry with the given URL, return it. Otherwise, return `nil`.
+  package func toolchain(withPath path: URL) -> Toolchain? {
+    if let toolchain = toolchainsByPath[path] {
+      return toolchain
+    }
+
+    let resolvedPath = orLog("Toolchain realpath") {
+      try path.realpath
+    }
+    guard let resolvedPath,
+      let toolchain = toolchainsByPath[resolvedPath]
+    else {
+      return nil
+    }
+
+    // Cache mapping of non-realpath to the realpath toolchain for faster subsequent lookups
+    toolchainsByPath[path] = toolchain
+    return toolchain
   }
 }
 
@@ -292,10 +334,6 @@ extension ToolchainRegistry {
   package func toolchains(withIdentifier identifier: String) -> [Toolchain] {
     return toolchainsByIdentifier[identifier] ?? []
   }
-
-  package func toolchain(withPath path: URL) -> Toolchain? {
-    return toolchainsByPath[path]
-  }
 }
 
 extension ToolchainRegistry {

Tests/ToolchainRegistryTests/ToolchainRegistryTests.swift

@@ -325,8 +325,12 @@ final class ToolchainRegistryTests: XCTestCase {
 
   func testSearchPATH() async throws {
     try await withTestScratchDir { tempDir in
-      let binPath = tempDir.appendingPathComponent("bin", isDirectory: true)
-      try makeToolchain(binPath: binPath, sourcekitd: true)
+      let binPath = tempDir.appending(component: "bin", directoryHint: .isDirectory)
+      try makeToolchain(binPath: binPath, clang: true, sourcekitd: true)
+
+      let compilerPath = binPath.appending(component: "clang" + (Platform.current?.executableExtension ?? ""))
+      let linkPath = tempDir.appending(component: "link")
+      try FileManager.default.createSymbolicLink(at: linkPath, withDestinationURL: compilerPath)
 
       #if os(Windows)
       let separator: String = ";"
@@ -336,7 +340,12 @@ final class ToolchainRegistryTests: XCTestCase {
 
       try ProcessEnv.setVar(
         "SOURCEKIT_PATH_FOR_TEST",
-        value: ["/bogus", binPath.filePath, "/bogus2"].joined(separator: separator)
+        value: [
+          "/bogus/../parent",
+          "/bogus",
+          binPath.appending(components: "..", "bin", directoryHint: .isDirectory).filePath,
+          "/bogus2",
+        ].joined(separator: separator)
       )
       defer { try! ProcessEnv.setVar("SOURCEKIT_PATH_FOR_TEST", value: "") }
 
@@ -349,15 +358,21 @@ final class ToolchainRegistryTests: XCTestCase {
         darwinToolchainOverride: nil
       )
 
-      let tc = try unwrap(await tr.toolchains.first(where: { $0.path == binPath }))
+      let pathTC = try unwrap(await tr.toolchain(withPath: binPath))
 
-      await assertEqual(tr.default?.identifier, tc.identifier)
-      XCTAssertEqual(tc.identifier, try binPath.filePath)
-      XCTAssertNil(tc.clang)
-      XCTAssertNil(tc.clangd)
-      XCTAssertNil(tc.swiftc)
-      XCTAssertNotNil(tc.sourcekitd)
-      XCTAssertNil(tc.libIndexStore)
+      await assertEqual(tr.default?.identifier, pathTC.identifier)
+      XCTAssertEqual(pathTC.identifier, try binPath.filePath)
+      XCTAssertNotNil(pathTC.clang)
+      XCTAssertNil(pathTC.clangd)
+      XCTAssertNil(pathTC.swiftc)
+      XCTAssertNotNil(pathTC.sourcekitd)
+      XCTAssertNil(pathTC.libIndexStore)
+
+      let compilerTC = try unwrap(await tr.toolchain(withCompiler: compilerPath))
+      XCTAssertEqual(compilerTC.identifier, try binPath.filePath)
+
+      let linkTC = await tr.toolchain(withCompiler: linkPath)
+      XCTAssertNil(linkTC)
     }
   }