Merge pull request #1249 from ahoppen/configured-targets

Introduce a notion of `ConfiguredTargets` into the build system

Author: ahoppen

Sources/SKCore/BuildServerBuildSystem.swift

@@ -263,14 +263,22 @@ extension BuildServerBuildSystem: BuildSystem {
   ///
   /// Returns `nil` if no build settings have been received from the build
   /// server yet or if no build settings are available for this file.
-  public func buildSettings(for document: DocumentURI, language: Language) async -> FileBuildSettings? {
+  public func buildSettings(
+    for document: DocumentURI,
+    in target: ConfiguredTarget,
+    language: Language
+  ) async -> FileBuildSettings? {
     return buildSettings[document]
   }
 
   public func defaultLanguage(for document: DocumentURI) async -> Language? {
     return nil
   }
 
+  public func configuredTargets(for document: DocumentURI) async -> [ConfiguredTarget] {
+    return [ConfiguredTarget(targetID: "dummy", runDestinationID: "dummy")]
+  }
+
   public func registerForChangeNotifications(for uri: DocumentURI) {
     let request = RegisterForChanges(uri: uri, action: .register)
     _ = self.buildServer?.send(request) { result in

Sources/SKCore/BuildSystem.swift

@@ -43,6 +43,27 @@ public struct SourceFileInfo: Sendable {
   }
 }
 
+/// A target / run destination combination. For example, a configured target can represent building the target
+/// `MyLibrary` for iOS.
+public struct ConfiguredTarget: Hashable, Sendable {
+  /// An opaque string that represents the target.
+  ///
+  /// The target's ID should be generated by the build system that handles the target and only interpreted by that
+  /// build system.
+  public let targetID: String
+
+  /// An opaque string that represents the run destination.
+  ///
+  /// The run destination's ID should be generated by the build system that handles the target and only interpreted by
+  /// that build system.
+  public let runDestinationID: String
+
+  public init(targetID: String, runDestinationID: String) {
+    self.targetID = targetID
+    self.runDestinationID = runDestinationID
+  }
+}
+
 /// Provider of FileBuildSettings and other build-related information.
 ///
 /// The primary role of the build system is to answer queries for
@@ -53,7 +74,6 @@ public struct SourceFileInfo: Sendable {
 /// For example, a SwiftPMWorkspace provides compiler arguments for the files
 /// contained in a SwiftPM package root directory.
 public protocol BuildSystem: AnyObject, Sendable {
-
   /// The root of the project that this build system manages. For example, for SwiftPM packages, this is the folder
   /// containing Package.swift. For compilation databases it is the root folder based on which the compilation database
   /// was found.
@@ -85,7 +105,14 @@ public protocol BuildSystem: AnyObject, Sendable {
   ///
   /// Returns `nil` if the build system can't provide build settings for this
   /// file or if it hasn't computed build settings for the file yet.
-  func buildSettings(for document: DocumentURI, language: Language) async throws -> FileBuildSettings?
+  func buildSettings(
+    for document: DocumentURI,
+    in target: ConfiguredTarget,
+    language: Language
+  ) async throws -> FileBuildSettings?
+
+  /// Return the list of targets and run destinations that the given document can be built for.
+  func configuredTargets(for document: DocumentURI) async -> [ConfiguredTarget]
 
   /// If the build system has knowledge about the language that this document should be compiled in, return it.
   ///

Sources/SKCore/BuildSystemManager.swift

@@ -122,22 +122,53 @@ extension BuildSystemManager {
     }
   }
 
+  /// Returns the `ConfiguredTarget` that should be used for semantic functionality of the given document.
+  public func canonicalConfiguredTarget(for document: DocumentURI) async -> ConfiguredTarget? {
+    // Sort the configured targets to deterministically pick the same `ConfiguredTarget` every time.
+    // We could allow the user to specify a preference of one target over another. For now this is not necessary because
+    // no build system currently returns multiple targets for a source file.
+    return await buildSystem?.configuredTargets(for: document)
+      .sorted { ($0.targetID, $0.runDestinationID) < ($1.targetID, $1.runDestinationID) }
+      .first
+  }
+
+  /// Returns the build settings for `document` from `buildSystem`.
+  ///
+  /// Implementation detail of `buildSettings(for:language:)`.
+  private func buildSettingsFromPrimaryBuildSystem(
+    for document: DocumentURI,
+    language: Language
+  ) async throws -> FileBuildSettings? {
+    guard let buildSystem else {
+      return nil
+    }
+    guard let target = await canonicalConfiguredTarget(for: document) else {
+      logger.error("Failed to get target for \(document.forLogging)")
+      return nil
+    }
+    // FIXME: (async) We should only wait `fallbackSettingsTimeout` for build
+    // settings and return fallback afterwards. I am not sure yet, how best to
+    // implement that with Swift concurrency.
+    // For now, this should be fine because all build systems return
+    // very quickly from `settings(for:language:)`.
+    guard let settings = try await buildSystem.buildSettings(for: document, in: target, language: language) else {
+      return nil
+    }
+    return settings
+  }
+
   private func buildSettings(
     for document: DocumentURI,
     language: Language
   ) async -> FileBuildSettings? {
     do {
-      // FIXME: (async) We should only wait `fallbackSettingsTimeout` for build
-      // settings and return fallback afterwards. I am not sure yet, how best to
-      // implement that with Swift concurrency.
-      // For now, this should be fine because all build systems return
-      // very quickly from `settings(for:language:)`.
-      if let settings = try await buildSystem?.buildSettings(for: document, language: language) {
-        return settings
+      if let buildSettings = try await buildSettingsFromPrimaryBuildSystem(for: document, language: language) {
+        return buildSettings
       }
     } catch {
       logger.error("Getting build settings failed: \(error.forLogging)")
     }
+
     guard var settings = fallbackBuildSystem?.buildSettings(for: document, language: language) else {
       return nil
     }

Sources/SKCore/CompilationDatabaseBuildSystem.swift

@@ -93,14 +93,17 @@ public actor CompilationDatabaseBuildSystem {
 }
 
 extension CompilationDatabaseBuildSystem: BuildSystem {
-
   public var indexDatabasePath: AbsolutePath? {
     indexStorePath?.parentDirectory.appending(component: "IndexDatabase")
   }
 
   public var indexPrefixMappings: [PathPrefixMapping] { return [] }
 
-  public func buildSettings(for document: DocumentURI, language: Language) async -> FileBuildSettings? {
+  public func buildSettings(
+    for document: DocumentURI,
+    in buildTarget: ConfiguredTarget,
+    language: Language
+  ) async -> FileBuildSettings? {
     guard let url = document.fileURL else {
       // We can't determine build settings for non-file URIs.
       return nil
@@ -118,6 +121,10 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
     return nil
   }
 
+  public func configuredTargets(for document: DocumentURI) async -> [ConfiguredTarget] {
+    return [ConfiguredTarget(targetID: "dummy", runDestinationID: "dummy")]
+  }
+
   public func registerForChangeNotifications(for uri: DocumentURI) async {
     self.watchedFiles.insert(uri)
   }

Sources/SKSupport/Process+WaitUntilExitWithCancellation.swift

@@ -15,9 +15,10 @@ import Foundation
 import class TSCBasic.Process
 import struct TSCBasic.ProcessResult
 
-public extension Process {
+extension Process {
   /// Wait for the process to exit. If the task gets cancelled, during this time, send a `SIGINT` to the process.
-  func waitUntilExitSendingSigIntOnTaskCancellation() async throws -> ProcessResult {
+  @discardableResult
+  public func waitUntilExitSendingSigIntOnTaskCancellation() async throws -> ProcessResult {
     return try await withTaskCancellationHandler {
       try await waitUntilExit()
     } onCancel: {

Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift

@@ -29,10 +29,14 @@ import struct Basics.AbsolutePath
 import struct Basics.IdentifiableSet
 import struct Basics.TSCAbsolutePath
 import struct Foundation.URL
+import struct TSCBasic.AbsolutePath
 import protocol TSCBasic.FileSystem
+import class TSCBasic.Process
 import var TSCBasic.localFileSystem
 import func TSCBasic.resolveSymlinks
 
+typealias AbsolutePath = Basics.AbsolutePath
+
 #if canImport(SPMBuildCore)
 import SPMBuildCore
 #endif
@@ -92,9 +96,11 @@ public actor SwiftPMBuildSystem {
   let workspace: Workspace
   public let buildParameters: BuildParameters
   let fileSystem: FileSystem
+  private let toolchainRegistry: ToolchainRegistry
 
   var fileToTarget: [AbsolutePath: SwiftBuildTarget] = [:]
   var sourceDirToTarget: [AbsolutePath: SwiftBuildTarget] = [:]
+  var targets: [SwiftBuildTarget] = []
 
   /// The URIs for which the delegate has registered for change notifications,
   /// mapped to the language the delegate specified when registering for change notifications.
@@ -130,6 +136,7 @@ public actor SwiftPMBuildSystem {
   ) async throws {
     self.workspacePath = workspacePath
     self.fileSystem = fileSystem
+    self.toolchainRegistry = toolchainRegistry
 
     guard let packageRoot = findPackageDirectory(containing: workspacePath, fileSystem) else {
       throw Error.noManifest(workspacePath: workspacePath)
@@ -265,6 +272,8 @@ extension SwiftPMBuildSystem {
     /// with only some properties modified.
     self.modulesGraph = modulesGraph
 
+    self.targets = try buildDescription.allTargetsInTopologicalOrder(in: modulesGraph)
+
     self.fileToTarget = [AbsolutePath: SwiftBuildTarget](
       modulesGraph.allTargets.flatMap { target in
         return target.sources.paths.compactMap {
@@ -320,43 +329,72 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
 
   public var indexPrefixMappings: [PathPrefixMapping] { return [] }
 
-  public func buildSettings(for uri: DocumentURI, language: Language) throws -> FileBuildSettings? {
-    // SwiftPMBuildSystem doesn't respect the langue specified by the editor.
-    return try buildSettings(for: uri)
-  }
-
-  private func buildSettings(for uri: DocumentURI) throws -> FileBuildSettings? {
-    guard let url = uri.fileURL else {
+  public func buildSettings(
+    for uri: DocumentURI,
+    in configuredTarget: ConfiguredTarget,
+    language: Language
+  ) throws -> FileBuildSettings? {
+    guard let url = uri.fileURL, let path = try? AbsolutePath(validating: url.path) else {
       // We can't determine build settings for non-file URIs.
       return nil
     }
-    guard let path = try? AbsolutePath(validating: url.path) else {
-      return nil
-    }
 
-    if let buildTarget = try buildTarget(for: path) {
-      return FileBuildSettings(
-        compilerArguments: try buildTarget.compileArguments(for: path.asURL),
-        workingDirectory: workspacePath.pathString
-      )
+    if configuredTarget.targetID == "" {
+      return try settings(forPackageManifest: path)
     }
 
-    if path.basename == "Package.swift" {
-      return try settings(forPackageManifest: path)
+    let buildTargets = self.targets.filter({ $0.name == configuredTarget.targetID })
+    if buildTargets.count > 1 {
+      logger.error("Found multiple targets with name \(configuredTarget.targetID). Picking the first one")
+    }
+    guard let buildTarget = buildTargets.first else {
+      if buildTargets.isEmpty {
+        logger.error("Did not find target with name \(configuredTarget.targetID)")
+      }
+      return nil
     }
 
-    if path.extension == "h" {
-      return try settings(forHeader: path)
+    if url.pathExtension == "h", let substituteFile = buildTarget.sources.first {
+      return FileBuildSettings(
+        compilerArguments: try buildTarget.compileArguments(for: substituteFile),
+        workingDirectory: workspacePath.pathString
+      ).patching(newFile: try resolveSymlinks(path).pathString, originalFile: substituteFile.absoluteString)
     }
 
-    return nil
+    return FileBuildSettings(
+      compilerArguments: try buildTarget.compileArguments(for: url),
+      workingDirectory: workspacePath.pathString
+    )
   }
 
   public func defaultLanguage(for document: DocumentURI) async -> Language? {
     // TODO (indexing): Query The SwiftPM build system for the document's language
     return nil
   }
 
+  public func configuredTargets(for uri: DocumentURI) -> [ConfiguredTarget] {
+    guard let url = uri.fileURL, let path = try? AbsolutePath(validating: url.path) else {
+      // We can't determine targets for non-file URIs.
+      return []
+    }
+
+    if let target = try? buildTarget(for: path) {
+      return [ConfiguredTarget(targetID: target.name, runDestinationID: "dummy")]
+    }
+
+    if path.basename == "Package.swift" {
+      // We use an empty target name to represent the package manifest since an empty target name is not valid for any
+      // user-defined target.
+      return [ConfiguredTarget(targetID: "", runDestinationID: "dummy")]
+    }
+
+    if url.pathExtension == "h", let target = try? target(forHeader: path) {
+      return [target]
+    }
+
+    return []
+  }
+
   public func registerForChangeNotifications(for uri: DocumentURI) async {
     self.watchedFiles.insert(uri)
   }
@@ -443,10 +481,10 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
   }
 
   public func fileHandlingCapability(for uri: DocumentURI) -> FileHandlingCapability {
-    if (try? buildSettings(for: uri)) != nil {
-      return .handled
+    if configuredTargets(for: uri).isEmpty {
+      return .unhandled
     }
-    return .unhandled
+    return .handled
   }
 
   public func sourceFiles() -> [SourceFileInfo] {
@@ -491,25 +529,13 @@ extension SwiftPMBuildSystem {
     return canonicalPath == path ? nil : impl(canonicalPath)
   }
 
-  /// Retrieve settings for a given header file.
-  ///
-  /// This finds the target the header belongs to based on its location in the file system, retrieves the build settings
-  /// for any file within that target and generates compiler arguments by replacing that picked file with the header
-  /// file.
-  /// This is safe because all files within one target have the same build settings except for reference to the file
-  /// itself, which we are replacing.
-  private func settings(forHeader path: AbsolutePath) throws -> FileBuildSettings? {
-    func impl(_ path: AbsolutePath) throws -> FileBuildSettings? {
+  /// This finds the target the header belongs to based on its location in the file system.
+  private func target(forHeader path: AbsolutePath) throws -> ConfiguredTarget? {
+    func impl(_ path: AbsolutePath) throws -> ConfiguredTarget? {
       var dir = path.parentDirectory
       while !dir.isRoot {
         if let buildTarget = sourceDirToTarget[dir] {
-          if let sourceFile = buildTarget.sources.first {
-            return FileBuildSettings(
-              compilerArguments: try buildTarget.compileArguments(for: sourceFile),
-              workingDirectory: workspacePath.pathString
-            ).patching(newFile: path.pathString, originalFile: sourceFile.absoluteString)
-          }
-          return nil
+          return ConfiguredTarget(targetID: buildTarget.name, runDestinationID: "dummy")
         }
         dir = dir.parentDirectory
       }

Sources/SourceKitLSP/SourceKitLSPServer.swift

@@ -2471,7 +2471,11 @@ fileprivate extension CheckedIndex {
   /// If the USR has an ambiguous definition, the most important role of this function is to deterministically return
   /// the same result every time.
   func primaryDefinitionOrDeclarationOccurrence(ofUSR usr: String) -> SymbolOccurrence? {
-    return definitionOrDeclarationOccurrences(ofUSR: usr).sorted().first
+    let result = definitionOrDeclarationOccurrences(ofUSR: usr).sorted().first
+    if result == nil {
+      logger.error("Failed to find definition of \(usr) in index")
+    }
+    return result
   }
 }
 

Tests/SKCoreTests/BuildSystemManagerTests.swift

@@ -445,14 +445,18 @@ class ManualBuildSystem: BuildSystem {
     self.delegate = delegate
   }
 
-  func buildSettings(for uri: DocumentURI, language: Language) -> FileBuildSettings? {
+  func buildSettings(for uri: DocumentURI, in buildTarget: ConfiguredTarget, language: Language) -> FileBuildSettings? {
     return map[uri]
   }
 
   public func defaultLanguage(for document: DocumentURI) async -> Language? {
     return nil
   }
 
+  public func configuredTargets(for document: DocumentURI) async -> [ConfiguredTarget] {
+    return [ConfiguredTarget(targetID: "dummy", runDestinationID: "dummy")]
+  }
+
   func registerForChangeNotifications(for uri: DocumentURI) async {
   }