Allow multiple Xcode toolchains in the registry

1) Toolchain ID is no longer unique; XcodeDefault toolchains may be
   registered multiple times as long as their path is different
   (e.g. different Xcode versions). At the moment no other toolchain
   may be duplicated.
2) `BuildSystem` specifies a `Toolchain` directly instead of a
   Toolchain identifier.
3) New methods to access Toolchains in the registry:
   - All toolchain(s) with the given identifier
   - Toolchain (if any) for the given path

Author: DavidGoldman

Sources/SKCore/FileBuildSettings.swift

@@ -17,8 +17,8 @@
 /// of a BuildSystem query.
 public struct FileBuildSettings {
 
-  /// The identifier of the toolchain that is preferred for compiling this file, if any.
-  public var preferredToolchain: String? = nil
+  /// The Toolchain that is preferred for compiling this file, if any.
+  public var preferredToolchain: Toolchain? = nil
 
   /// The compiler arguments to use for this file.
   public var compilerArguments: [String]
@@ -27,7 +27,7 @@ public struct FileBuildSettings {
   public var workingDirectory: String? = nil
 
   public init(
-    preferredToolchain: String? = nil,
+    preferredToolchain: Toolchain? = nil,
     compilerArguments: [String],
     workingDirectory: String? = nil)
   {

Sources/SKCore/ToolchainRegistry.swift

@@ -28,7 +28,12 @@ public final class ToolchainRegistry {
   var _toolchains: [Toolchain] = []
 
   /// The toolchains indexed by their identifier. **Must be accessed on `queue`**.
-  var toolchainIdentifiers: [String: Toolchain] = [:]
+  /// Multiple toolchains may exist for the XcodeDefault toolchain identifier.
+  var toolchainsByIdentifier: [String: [Toolchain]] = [:]
+
+  /// The toolchains indexed by their path. **Must be accessed on `queue`**.
+  /// Note: Not all toolchains have a path.
+  var toolchainsByPath: [AbsolutePath: Toolchain] = [:]
 
   /// The default toolchain. **Must be accessed on `queue`**.
   var _default: Toolchain? = nil
@@ -92,7 +97,7 @@ extension ToolchainRegistry {
     get {
       return queue.sync {
         if _default == nil {
-          if let tc = toolchainIdentifiers[darwinToolchainIdentifier] {
+          if let tc = toolchainsByIdentifier[darwinToolchainIdentifier]?.first {
             _default = tc
           } else {
             _default = _toolchains.first
@@ -108,8 +113,8 @@ extension ToolchainRegistry {
           _default = nil
           return
         }
-        precondition(toolchainIdentifiers[toolchain.identifier] === toolchain,
-          "default toolchain must be registered first")
+        precondition(_toolchains.contains { $0 === toolchain },
+                     "default toolchain must be registered first")
         _default = toolchain
       }
     }
@@ -132,8 +137,16 @@ extension ToolchainRegistry {
     return queue.sync { _toolchains }
   }
 
+  public func toolchains(identifier: String) -> [Toolchain] {
+    return queue.sync { toolchainsByIdentifier[identifier] ?? [] }
+  }
+
   public func toolchain(identifier: String) -> Toolchain? {
-    return queue.sync { toolchainIdentifiers[identifier] }
+    return toolchains(identifier: identifier).first
+  }
+
+  public func toolchain(path: AbsolutePath) -> Toolchain? {
+    return queue.sync { toolchainsByPath[path] }
   }
 }
 
@@ -144,6 +157,9 @@ extension ToolchainRegistry {
     /// There is already a toolchain with the given identifier.
     case duplicateToolchainIdentifier
 
+    /// There is already a toolchain with the given path.
+    case duplicateToolchainPath
+
     /// The toolchain does not exist, or has no tools.
     case invalidToolchain
   }
@@ -157,10 +173,24 @@ extension ToolchainRegistry {
   }
 
   func _registerToolchain(_ toolchain: Toolchain) throws {
-    guard toolchainIdentifiers[toolchain.identifier] == nil else {
-      throw Error.duplicateToolchainIdentifier
+    // Non-XcodeDefault toolchain: disallow all duplicates.
+    if toolchain.identifier != ToolchainRegistry.darwinDefaultToolchainIdentifier {
+      guard toolchainsByIdentifier[toolchain.identifier] == nil else {
+        throw Error.duplicateToolchainIdentifier
+      }
     }
-    toolchainIdentifiers[toolchain.identifier] = toolchain
+
+    // Toolchain should always be unique by path if it is present.
+    if let path = toolchain.path {
+      guard toolchainsByPath[path] == nil else {
+        throw Error.duplicateToolchainPath
+      }
+      toolchainsByPath[path] = toolchain
+    }
+
+    var toolchains = toolchainsByIdentifier[toolchain.identifier] ?? []
+    toolchains.append(toolchain)
+    toolchainsByIdentifier[toolchain.identifier] = toolchains
     _toolchains.append(toolchain)
   }
 

Sources/SourceKit/SourceKitServer.swift

@@ -121,9 +121,7 @@ public final class SourceKitServer: LanguageServer {
   }
 
   func toolchain(for url: URL, _ language: Language) -> Toolchain? {
-    if let id = workspace?.buildSettings.settings(for: url, language)?.preferredToolchain,
-       let toolchain = toolchainRegistry.toolchain(identifier:id)
-    {
+    if let toolchain = workspace?.buildSettings.settings(for: url, language)?.preferredToolchain {
       return toolchain
     }
 

Tests/SKCoreTests/ToolchainRegistryTests.swift

@@ -349,6 +349,70 @@ final class ToolchainRegistryTests: XCTestCase {
     makeToolchain(binPath: AbsolutePath("/t3/bin"), fs, sourcekitd: true)
     XCTAssertNotNil(Toolchain(AbsolutePath("/t3"), fs))
   }
+
+  func testDuplicateError() {
+    let tr = ToolchainRegistry()
+    let toolchain = Toolchain(identifier: "a", displayName: "a", path: nil)
+    XCTAssertNoThrow(try tr.registerToolchain(toolchain), "Error registering toolchain")
+    XCTAssertThrowsError(try tr.registerToolchain(toolchain),
+                         "Expected error registering toolchain twice") { e in
+      guard let error = e as? ToolchainRegistry.Error, error == .duplicateToolchainIdentifier else {
+        XCTFail("Expected .duplicateToolchainIdentifier not \(e)")
+        return
+      }
+    }
+  }
+
+  func testDuplicatePathError() {
+    let tr = ToolchainRegistry()
+    let path = AbsolutePath("/foo/bar")
+    let first = Toolchain(identifier: "a", displayName: "a", path: path)
+    let second = Toolchain(identifier: "b", displayName: "b", path: path)
+    XCTAssertNoThrow(try tr.registerToolchain(first), "Error registering toolchain")
+    XCTAssertThrowsError(try tr.registerToolchain(second),
+                         "Expected error registering toolchain twice") { e in
+      guard let error = e as? ToolchainRegistry.Error, error == .duplicateToolchainPath else {
+        XCTFail("Error mismatch: expected duplicateToolchainPath not \(e)")
+        return
+      }
+    }
+  }
+
+  func testDuplicateXcodeError() {
+    let tr = ToolchainRegistry()
+    let xcodeToolchain = Toolchain(identifier: ToolchainRegistry.darwinDefaultToolchainIdentifier,
+                                   displayName: "a",
+                                   path: AbsolutePath("/versionA"))
+    XCTAssertNoThrow(try tr.registerToolchain(xcodeToolchain), "Error registering toolchain")
+    XCTAssertThrowsError(try tr.registerToolchain(xcodeToolchain),
+                         "Expected error registering toolchain twice") { e in
+      guard let error = e as? ToolchainRegistry.Error, error == .duplicateToolchainPath else {
+        XCTFail("Error mismatch: expected duplicateToolchainPath not \(e)")
+        return
+      }
+    }
+  }
+
+  func testMultipleXcodes() {
+    let tr = ToolchainRegistry()
+    let pathA = AbsolutePath("/versionA")
+    let xcodeA = Toolchain(identifier: ToolchainRegistry.darwinDefaultToolchainIdentifier,
+                           displayName: "a",
+                           path: pathA)
+    let pathB = AbsolutePath("/versionB")
+    let xcodeB = Toolchain(identifier: ToolchainRegistry.darwinDefaultToolchainIdentifier,
+                           displayName: "b",
+                           path: pathB)
+    XCTAssertNoThrow(try tr.registerToolchain(xcodeA))
+    XCTAssertNoThrow(try tr.registerToolchain(xcodeB))
+    XCTAssert(tr.toolchain(path: pathA) === xcodeA)
+    XCTAssert(tr.toolchain(path: pathB) === xcodeB)
+
+    let toolchains = tr.toolchains(identifier: xcodeA.identifier)
+    XCTAssert(toolchains.count == 2)
+    XCTAssert(toolchains[0] === xcodeA)
+    XCTAssert(toolchains[1] === xcodeB)
+  }
 }
 
 #if os(macOS)

Tests/SKCoreTests/XCTestManifests.swift

@@ -37,8 +37,12 @@ extension ToolchainRegistryTests {
     static let __allTests__ToolchainRegistryTests = [
         ("testDefaultBasic", testDefaultBasic),
         ("testDefaultDarwin", testDefaultDarwin),
+        ("testDuplicateError", testDuplicateError),
+        ("testDuplicatePathError", testDuplicatePathError),
+        ("testDuplicateXcodeError", testDuplicateXcodeError),
         ("testDylibNames", testDylibNames),
         ("testFromDirectory", testFromDirectory),
+        ("testMultipleXcodes", testMultipleXcodes),
         ("testSearchDarwin", testSearchDarwin),
         ("testSearchExplicitEnv", testSearchExplicitEnv),
         ("testSearchExplicitEnvBuiltin", testSearchExplicitEnvBuiltin),