Move isFallback to FileBuildSettings

Remove isFallback from func buildSettings for FallbackBuildSystem

Finetune a comment

Finetune the code

Author: joehsieh

Sources/SKCore/BuildSystemManager.swift

@@ -115,27 +115,32 @@ extension BuildSystemManager {
   private func buildSettings(
     for document: DocumentURI,
     language: Language
-  ) async -> (buildSettings: FileBuildSettings, isFallback: Bool)? {
+  ) 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 (buildSettings: settings, isFallback: false)
+        return settings
       }
     } catch {
       logger.error("Getting build settings failed: \(error.forLogging)")
     }
-    if let settings = fallbackBuildSystem?.buildSettings(for: document, language: language) {
+    // If there is no build system and we only have the fallback build system,
+    // we will never get real build settings. Consider the build settings
+    // non-fallback.
+    guard var settings = fallbackBuildSystem?.buildSettings(for: document, language: language) else {
+      return nil
+    }
+    if buildSystem == nil {
       // If there is no build system and we only have the fallback build system,
       // we will never get real build settings. Consider the build settings
       // non-fallback.
-      return (buildSettings: settings, isFallback: buildSystem != nil)
-    } else {
-      return nil
+      settings.isFallback = false
     }
+    return settings
   }
 
   /// Returns the build settings for the given document.
@@ -148,34 +153,34 @@ extension BuildSystemManager {
   public func buildSettingsInferredFromMainFile(
     for document: DocumentURI,
     language: Language
-  ) async -> (buildSettings: FileBuildSettings, isFallback: Bool)? {
+  ) async -> FileBuildSettings? {
     let mainFile = mainFile(for: document)
-    var buildSettings = await buildSettings(for: mainFile, language: language)
-    if mainFile != document, let settings = buildSettings?.buildSettings {
+    guard var settings = await buildSettings(for: mainFile, language: language) else {
+      return nil
+    }
+    if mainFile != document {
       // If the main file isn't the file itself, we need to patch the build settings
       // to reference `document` instead of `mainFile`.
-      buildSettings?.buildSettings = settings.patching(newFile: document.pseudoPath, originalFile: mainFile.pseudoPath)
+      settings = settings.patching(newFile: document.pseudoPath, originalFile: mainFile.pseudoPath)
     }
-    if let buildSettings {
-      let log = """
-        Compiler Arguments:
-        \(buildSettings.buildSettings.compilerArguments.joined(separator: "\n"))
+    let log = """
+      Compiler Arguments:
+      \(settings.compilerArguments.joined(separator: "\n"))
 
-        Working directory:
-        \(buildSettings.buildSettings.workingDirectory ?? "<nil>")
-        """
+      Working directory:
+      \(settings.workingDirectory ?? "<nil>")
+      """
 
-      let chunks = splitLongMultilineMessage(message: log, maxChunkSize: 800)
-      for (index, chunk) in chunks.enumerated() {
-        logger.log(
-          """
-          Build settings for \(document.forLogging) (\(index + 1)/\(chunks.count))
-          \(chunk)
-          """
-        )
-      }
+    let chunks = splitLongMultilineMessage(message: log, maxChunkSize: 800)
+    for (index, chunk) in chunks.enumerated() {
+      logger.log(
+        """
+        Build settings for \(document.forLogging) (\(index + 1)/\(chunks.count))
+        \(chunk)
+        """
+      )
     }
-    return buildSettings
+    return settings
   }
 
   public func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {

Sources/SKCore/FallbackBuildSystem.swift

@@ -52,14 +52,17 @@ public final class FallbackBuildSystem {
   public var indexPrefixMappings: [PathPrefixMapping] { return [] }
 
   public func buildSettings(for uri: DocumentURI, language: Language) -> FileBuildSettings? {
+    var fileBuildSettings: FileBuildSettings?
     switch language {
     case .swift:
-      return settingsSwift(uri.pseudoPath)
+      fileBuildSettings = settingsSwift(uri.pseudoPath)
     case .c, .cpp, .objective_c, .objective_cpp:
-      return settingsClang(uri.pseudoPath, language)
+      fileBuildSettings = settingsClang(uri.pseudoPath, language)
     default:
-      return nil
+      fileBuildSettings = nil
     }
+    fileBuildSettings?.isFallback = true
+    return fileBuildSettings
   }
 
   func settingsSwift(_ file: String) -> FileBuildSettings {

Sources/SKCore/FileBuildSettings.swift

@@ -25,9 +25,13 @@ public struct FileBuildSettings: Equatable {
   /// The working directory to resolve any relative paths in `compilerArguments`.
   public var workingDirectory: String? = nil
 
-  public init(compilerArguments: [String], workingDirectory: String? = nil) {
+  /// Whether the build settings were computed from a real build system or whether they are synthesized fallback arguments while the build system is still busy computing build settings.
+  public var isFallback: Bool
+
+  public init(compilerArguments: [String], workingDirectory: String? = nil, isFallback: Bool = false) {
     self.compilerArguments = compilerArguments
     self.workingDirectory = workingDirectory
+    self.isFallback = isFallback
   }
 }
 
@@ -74,6 +78,6 @@ public extension FileBuildSettings {
         arguments.insert("-xobjective-c++", at: 0)
       }
     }
-    return FileBuildSettings(compilerArguments: arguments, workingDirectory: self.workingDirectory)
+    return FileBuildSettings(compilerArguments: arguments, workingDirectory: self.workingDirectory, isFallback: self.isFallback)
   }
 }

Sources/SourceKitLSP/Clang/ClangLanguageServer.swift

@@ -142,7 +142,7 @@ actor ClangLanguageServerShim: ToolchainLanguageServer, MessageHandler {
     else {
       return nil
     }
-    return ClangBuildSettings(settings.buildSettings, clangPath: clangdPath, isFallback: settings.isFallback)
+    return ClangBuildSettings(settings, clangPath: clangdPath)
   }
 
   nonisolated func canHandle(workspace: Workspace) -> Bool {
@@ -600,7 +600,7 @@ private struct ClangBuildSettings: Equatable {
   /// fallback arguments and represent the file state differently.
   public let isFallback: Bool
 
-  public init(_ settings: FileBuildSettings, clangPath: AbsolutePath?, isFallback: Bool = false) {
+  public init(_ settings: FileBuildSettings, clangPath: AbsolutePath?) {
     var arguments = [clangPath?.pathString ?? "clang"] + settings.compilerArguments
     if arguments.contains("-fmodules") {
       // Clangd is not built with support for the 'obj' format.
@@ -619,7 +619,7 @@ private struct ClangBuildSettings: Equatable {
 
     self.compilerArgs = arguments
     self.workingDirectory = settings.workingDirectory ?? ""
-    self.isFallback = isFallback
+    self.isFallback = settings.isFallback
   }
 
   public var compileCommand: ClangCompileCommand {

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

@@ -75,15 +75,15 @@ public struct SwiftCompileCommand: Equatable {
   /// fallback arguments and represent the file state differently.
   public let isFallback: Bool
 
-  public init(_ settings: FileBuildSettings, isFallback: Bool = false) {
+  public init(_ settings: FileBuildSettings) {
     let baseArgs = settings.compilerArguments
     // Add working directory arguments if needed.
     if let workingDirectory = settings.workingDirectory, !baseArgs.contains("-working-directory") {
       self.compilerArgs = baseArgs + ["-working-directory", workingDirectory]
     } else {
       self.compilerArgs = baseArgs
     }
-    self.isFallback = isFallback
+    self.isFallback = settings.isFallback
   }
 }
 
@@ -177,7 +177,7 @@ public actor SwiftLanguageServer: ToolchainLanguageServer {
       for: document,
       language: .swift
     ) {
-      return SwiftCompileCommand(settings.buildSettings, isFallback: settings.isFallback)
+      return SwiftCompileCommand(settings)
     } else {
       return nil
     }

Tests/SKCoreTests/BuildSystemManagerTests.swift

@@ -105,7 +105,7 @@ final class BuildSystemManagerTests: XCTestCase {
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["x"])
     await bsm.registerForChangeNotifications(for: a, language: .swift)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift)?.buildSettings, bs.map[a]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift), bs.map[a]!)
 
     bs.map[a] = nil
     let changed = expectation(description: "changed settings")
@@ -127,7 +127,7 @@ final class BuildSystemManagerTests: XCTestCase {
     defer { withExtendedLifetime(bsm) {} }  // Keep BSM alive for callbacks.
     let del = await BSMDelegate(bsm)
     await bsm.registerForChangeNotifications(for: a, language: .swift)
-    assertNil(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift)?.buildSettings)
+    assertNil(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift))
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["x"])
     let changed = expectation(description: "changed settings")
@@ -151,7 +151,7 @@ final class BuildSystemManagerTests: XCTestCase {
     let del = await BSMDelegate(bsm)
     let fallbackSettings = fallback.buildSettings(for: a, language: .swift)
     await bsm.registerForChangeNotifications(for: a, language: .swift)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift)?.buildSettings, fallbackSettings)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift), fallbackSettings)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["non-fallback", "args"])
     let changed = expectation(description: "changed settings")
@@ -183,9 +183,9 @@ final class BuildSystemManagerTests: XCTestCase {
     bs.map[a] = FileBuildSettings(compilerArguments: ["x"])
     bs.map[b] = FileBuildSettings(compilerArguments: ["y"])
     await bsm.registerForChangeNotifications(for: a, language: .swift)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift)?.buildSettings, bs.map[a]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift), bs.map[a]!)
     await bsm.registerForChangeNotifications(for: b, language: .swift)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: b, language: .swift)?.buildSettings, bs.map[b]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: b, language: .swift), bs.map[b]!)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["xx"])
     bs.map[b] = FileBuildSettings(compilerArguments: ["yy"])
@@ -225,10 +225,10 @@ final class BuildSystemManagerTests: XCTestCase {
     bs.map[b] = FileBuildSettings(compilerArguments: ["b"])
 
     await bsm.registerForChangeNotifications(for: a, language: .swift)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift)?.buildSettings, bs.map[a]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift), bs.map[a]!)
 
     await bsm.registerForChangeNotifications(for: b, language: .swift)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: b, language: .swift)?.buildSettings, bs.map[b]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: b, language: .swift), bs.map[b]!)
 
     bs.map[a] = nil
     bs.map[b] = nil
@@ -262,7 +262,7 @@ final class BuildSystemManagerTests: XCTestCase {
     bs.map[cpp2] = FileBuildSettings(compilerArguments: ["C++ 2"])
 
     await bsm.registerForChangeNotifications(for: h, language: .c)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: h, language: .c)?.buildSettings, bs.map[cpp1]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: h, language: .c), bs.map[cpp1]!)
 
     mainFiles.mainFiles[h] = Set([cpp2])
 
@@ -320,8 +320,8 @@ final class BuildSystemManagerTests: XCTestCase {
 
     let expectedArgsH1 = FileBuildSettings(compilerArguments: ["-xc++", cppArg, h1.pseudoPath])
     let expectedArgsH2 = FileBuildSettings(compilerArguments: ["-xc++", cppArg, h2.pseudoPath])
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: h1, language: .c)?.buildSettings, expectedArgsH1)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: h2, language: .c)?.buildSettings, expectedArgsH2)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: h1, language: .c), expectedArgsH1)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: h2, language: .c), expectedArgsH2)
 
     let newCppArg = "New C++ Main File"
     bs.map[cpp] = FileBuildSettings(compilerArguments: [newCppArg, cpp.pseudoPath])
@@ -360,9 +360,9 @@ final class BuildSystemManagerTests: XCTestCase {
     await bsm.registerForChangeNotifications(for: a, language: .swift)
     await bsm.registerForChangeNotifications(for: b, language: .swift)
     await bsm.registerForChangeNotifications(for: c, language: .swift)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift)?.buildSettings, bs.map[a]!)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: b, language: .swift)?.buildSettings, bs.map[b]!)
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: c, language: .swift)?.buildSettings, bs.map[c]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift), bs.map[a]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: b, language: .swift), bs.map[b]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: c, language: .swift), bs.map[c]!)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["new-a"])
     bs.map[b] = FileBuildSettings(compilerArguments: ["new-b"])
@@ -397,7 +397,7 @@ final class BuildSystemManagerTests: XCTestCase {
     let del = await BSMDelegate(bsm)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["x"])
-    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift)?.buildSettings, bs.map[a]!)
+    assertEqual(await bsm.buildSettingsInferredFromMainFile(for: a, language: .swift), bs.map[a]!)
 
     await bsm.registerForChangeNotifications(for: a, language: .swift)
 
@@ -507,7 +507,7 @@ private actor BSMDelegate: BuildSystemDelegate {
       }
 
       XCTAssertEqual(uri, expected.uri, file: expected.file, line: expected.line)
-      let settings = await bsm.buildSettingsInferredFromMainFile(for: uri, language: expected.language)?.buildSettings
+      let settings = await bsm.buildSettingsInferredFromMainFile(for: uri, language: expected.language)
       XCTAssertEqual(settings, expected.settings, file: expected.file, line: expected.line)
       expected.expectation.fulfill()
     }

Tests/SourceKitLSPTests/BuildSystemTests.swift

@@ -251,6 +251,7 @@ final class BuildSystemTests: XCTestCase {
 
     // Primary settings must be different than the fallback settings.
     var primarySettings = FallbackBuildSystem(buildSetup: .default).buildSettings(for: doc, language: .swift)!
+    primarySettings.isFallback = false
     primarySettings.compilerArguments.append("-DPRIMARY")
 
     let text = """