Merge pull request #1350 from ahoppen/no-side-effects-when-searching-for-workspace

Don’t cause any file system file effects when trying to find an implicit workspace for a file

Author: ahoppen

Sources/SKCore/BuildServerBuildSystem.swift

@@ -279,7 +279,7 @@ extension BuildServerBuildSystem: BuildSystem {
     return [ConfiguredTarget(targetID: "dummy", runDestinationID: "dummy")]
   }
 
-  public func generateBuildGraph() {}
+  public func generateBuildGraph(allowFileSystemWrites: Bool) {}
 
   public func topologicalSort(of targets: [ConfiguredTarget]) async -> [ConfiguredTarget]? {
     return nil

Sources/SKCore/BuildSystem.swift

@@ -135,9 +135,14 @@ public protocol BuildSystem: AnyObject, Sendable {
   /// Return the list of targets and run destinations that the given document can be built for.
   func configuredTargets(for document: DocumentURI) async -> [ConfiguredTarget]
 
-  /// Re-generate the build graph including all the tasks that are necessary for building the entire build graph, like
-  /// resolving package versions.
-  func generateBuildGraph() async throws
+  /// Re-generate the build graph.
+  ///
+  /// If `allowFileSystemWrites` is `true`, this should include all the tasks that are necessary for building the entire
+  /// build graph, like resolving package versions.
+  ///
+  /// If `allowFileSystemWrites` is `false`, no files must be written to disk. This mode is used to determine whether
+  /// the build system can handle a source file, and decide whether a workspace should be opened with this build system
+  func generateBuildGraph(allowFileSystemWrites: Bool) async throws
 
   /// Sort the targets so that low-level targets occur before high-level targets.
   ///

Sources/SKCore/BuildSystemManager.swift

@@ -219,8 +219,8 @@ extension BuildSystemManager {
     return settings
   }
 
-  public func generateBuildGraph() async throws {
-    try await self.buildSystem?.generateBuildGraph()
+  public func generateBuildGraph(allowFileSystemWrites: Bool) async throws {
+    try await self.buildSystem?.generateBuildGraph(allowFileSystemWrites: allowFileSystemWrites)
   }
 
   public func topologicalSort(of targets: [ConfiguredTarget]) async throws -> [ConfiguredTarget]? {

Sources/SKCore/CompilationDatabaseBuildSystem.swift

@@ -132,7 +132,7 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
     throw PrepareNotSupportedError()
   }
 
-  public func generateBuildGraph() {}
+  public func generateBuildGraph(allowFileSystemWrites: Bool) {}
 
   public func topologicalSort(of targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
     return nil

Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift

@@ -178,7 +178,9 @@ public actor SwiftPMBuildSystem {
       forRootPackage: AbsolutePath(packageRoot),
       fileSystem: fileSystem
     )
-    if let scratchDirectory = buildSetup.path {
+    if isForIndexBuild {
+      location.scratchDirectory = AbsolutePath(packageRoot.appending(component: ".index-build"))
+    } else if let scratchDirectory = buildSetup.path {
       location.scratchDirectory = AbsolutePath(scratchDirectory)
     }
 
@@ -227,7 +229,6 @@ public actor SwiftPMBuildSystem {
       }
       await delegate.filesDependenciesUpdated(filesWithUpdatedDependencies)
     }
-    try await reloadPackage()
   }
 
   /// Creates a build system using the Swift Package Manager, if this workspace is a package.
@@ -261,13 +262,9 @@ public actor SwiftPMBuildSystem {
 }
 
 extension SwiftPMBuildSystem {
-  public func generateBuildGraph() async throws {
-    try await self.reloadPackage()
-  }
-
   /// (Re-)load the package settings by parsing the manifest and resolving all the targets and
   /// dependencies.
-  func reloadPackage() async throws {
+  public func reloadPackage(forceResolvedVersions: Bool) async throws {
     await reloadPackageStatusCallback(.start)
     defer {
       Task {
@@ -277,7 +274,7 @@ extension SwiftPMBuildSystem {
 
     let modulesGraph = try self.workspace.loadPackageGraph(
       rootInput: PackageGraphRootInput(packages: [AbsolutePath(projectRoot)]),
-      forceResolvedVersions: !isForIndexBuild,
+      forceResolvedVersions: forceResolvedVersions,
       observabilityScope: observabilitySystem.topScope
     )
 
@@ -430,6 +427,10 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
     return []
   }
 
+  public func generateBuildGraph(allowFileSystemWrites: Bool) async throws {
+    try await self.reloadPackage(forceResolvedVersions: !isForIndexBuild || !allowFileSystemWrites)
+  }
+
   public func topologicalSort(of targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
     return targets.sorted { (lhs: ConfiguredTarget, rhs: ConfiguredTarget) -> Bool in
       let lhsIndex = self.targets[lhs]?.index ?? self.targets.count
@@ -590,7 +591,7 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
       logger.log("Reloading package because of file change")
       await orLog("Reloading package") {
         // TODO: It should not be necessary to reload the entire package just to get build settings for one file.
-        try await self.reloadPackage()
+        try await self.reloadPackage(forceResolvedVersions: !isForIndexBuild)
       }
     }
 

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -234,7 +234,9 @@ public final actor SemanticIndexManager {
           signposter.endInterval("Preparing", state)
         }
         await testHooks.buildGraphGenerationDidStart?()
-        await orLog("Generating build graph") { try await self.buildSystemManager.generateBuildGraph() }
+        await orLog("Generating build graph") {
+          try await self.buildSystemManager.generateBuildGraph(allowFileSystemWrites: true)
+        }
         // Ensure that we have an up-to-date indexstore-db. Waiting for the indexstore-db to be updated is cheaper than
         // potentially not knowing about unit files, which causes the corresponding source files to be re-indexed.
         index.pollForUnitChangesAndWait()

Sources/SourceKitLSP/CMakeLists.txt

@@ -1,6 +1,7 @@
 
 add_library(SourceKitLSP STATIC
   CapabilityRegistry.swift
+  CreateBuildSystem.swift
   DocumentManager.swift
   DocumentSnapshot+FromFileContents.swift
   IndexProgressManager.swift

Sources/SourceKitLSP/CreateBuildSystem.swift

@@ -0,0 +1,76 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import LSPLogging
+import LanguageServerProtocol
+import SKCore
+import SKSwiftPMWorkspace
+
+import struct TSCBasic.AbsolutePath
+import struct TSCBasic.RelativePath
+
+/// Tries to create a build system for a workspace at the given location, with the given parameters.
+func createBuildSystem(
+  rootUri: DocumentURI,
+  options: SourceKitLSPServer.Options,
+  toolchainRegistry: ToolchainRegistry,
+  reloadPackageStatusCallback: @Sendable @escaping (ReloadPackageStatus) async -> Void
+) async -> BuildSystem? {
+  guard let rootUrl = rootUri.fileURL, let rootPath = try? AbsolutePath(validating: rootUrl.path) else {
+    // We assume that workspaces are directories. This is only true for URLs not for URIs in general.
+    // Simply skip setting up the build integration in this case.
+    logger.error(
+      "cannot setup build integration at URI \(rootUri.forLogging) because the URI it is not a valid file URL"
+    )
+    return nil
+  }
+  func createSwiftPMBuildSystem(rootUrl: URL) async -> SwiftPMBuildSystem? {
+    return await SwiftPMBuildSystem(
+      url: rootUrl,
+      toolchainRegistry: toolchainRegistry,
+      buildSetup: options.buildSetup,
+      isForIndexBuild: options.indexOptions.enableBackgroundIndexing,
+      reloadPackageStatusCallback: reloadPackageStatusCallback
+    )
+  }
+
+  func createCompilationDatabaseBuildSystem(rootPath: AbsolutePath) -> CompilationDatabaseBuildSystem? {
+    return CompilationDatabaseBuildSystem(
+      projectRoot: rootPath,
+      searchPaths: options.compilationDatabaseSearchPaths
+    )
+  }
+
+  func createBuildServerBuildSystem(rootPath: AbsolutePath) async -> BuildServerBuildSystem? {
+    return await BuildServerBuildSystem(projectRoot: rootPath, buildSetup: options.buildSetup)
+  }
+
+  let defaultBuildSystem: BuildSystem? =
+    switch options.buildSetup.defaultWorkspaceType {
+    case .buildServer: await createBuildServerBuildSystem(rootPath: rootPath)
+    case .compilationDatabase: createCompilationDatabaseBuildSystem(rootPath: rootPath)
+    case .swiftPM: await createSwiftPMBuildSystem(rootUrl: rootUrl)
+    case nil: nil
+    }
+  if let defaultBuildSystem {
+    return defaultBuildSystem
+  } else if let buildServer = await createBuildServerBuildSystem(rootPath: rootPath) {
+    return buildServer
+  } else if let swiftpm = await createSwiftPMBuildSystem(rootUrl: rootUrl) {
+    return swiftpm
+  } else if let compdb = createCompilationDatabaseBuildSystem(rootPath: rootPath) {
+    return compdb
+  } else {
+    logger.error("Could not set up a build system at '\(rootUri.forLogging)'")
+    return nil
+  }
+}

Sources/SourceKitLSP/SourceKitLSPServer.swift

@@ -234,11 +234,20 @@ public actor SourceKitLSPServer {
       // The latter might happen if there is an existing SwiftPM workspace that hasn't been reloaded after a new file
       // was added to it and thus currently doesn't know that it can handle that file. In that case, we shouldn't open
       // a new workspace for the same root. Instead, the existing workspace's build system needs to be reloaded.
-      if let workspace = await self.createWorkspace(WorkspaceFolder(uri: DocumentURI(url))),
-        await workspace.buildSystemManager.fileHandlingCapability(for: uri) == .handled,
-        let projectRoot = await workspace.buildSystemManager.projectRoot,
-        !projectRoots.contains(projectRoot)
-      {
+      let workspace = await self.createWorkspace(WorkspaceFolder(uri: DocumentURI(url))) { buildSystem in
+        guard let buildSystem, !projectRoots.contains(await buildSystem.projectRoot) else {
+          // If we didn't create a build system, `url` is not capable of handling the document.
+          // If we already have a workspace at the same project root, don't create another one.
+          return false
+        }
+        do {
+          try await buildSystem.generateBuildGraph(allowFileSystemWrites: false)
+        } catch {
+          return false
+        }
+        return await buildSystem.fileHandlingCapability(for: uri) == .handled
+      }
+      if let workspace {
         return workspace
       }
       url.deleteLastPathComponent()
@@ -866,25 +875,22 @@ extension SourceKitLSPServer {
   }
 
   /// Creates a workspace at the given `uri`.
-  private func createWorkspace(_ workspaceFolder: WorkspaceFolder) async -> Workspace? {
+  ///
+  /// If the build system that was determined for the workspace does not satisfy `condition`, `nil` is returned.
+  private func createWorkspace(
+    _ workspaceFolder: WorkspaceFolder,
+    condition: (BuildSystem?) async -> Bool = { _ in true }
+  ) async -> Workspace? {
     guard let capabilityRegistry = capabilityRegistry else {
       logger.log("Cannot open workspace before server is initialized")
       return nil
     }
     var options = self.options
     options.buildSetup = self.options.buildSetup.merging(buildSetup(for: workspaceFolder))
-    return try? await Workspace(
-      documentManager: self.documentManager,
+    let buildSystem = await createBuildSystem(
       rootUri: workspaceFolder.uri,
-      capabilityRegistry: capabilityRegistry,
-      toolchainRegistry: self.toolchainRegistry,
       options: options,
-      compilationDatabaseSearchPaths: self.options.compilationDatabaseSearchPaths,
-      indexOptions: self.options.indexOptions,
-      indexTaskScheduler: indexTaskScheduler,
-      indexProcessDidProduceResult: { [weak self] in
-        self?.indexTaskDidProduceResult($0)
-      },
+      toolchainRegistry: toolchainRegistry,
       reloadPackageStatusCallback: { [weak self] status in
         guard let self else { return }
         switch status {
@@ -893,6 +899,34 @@ extension SourceKitLSPServer {
         case .end:
           await self.packageLoadingWorkDoneProgress.endProgress(server: self)
         }
+      }
+    )
+    guard await condition(buildSystem) else {
+      return nil
+    }
+    do {
+      try await buildSystem?.generateBuildGraph(allowFileSystemWrites: true)
+    } catch {
+      logger.error("failed to generate build graph at \(workspaceFolder.uri.forLogging): \(error.forLogging)")
+      return nil
+    }
+
+    let projectRoot = await buildSystem?.projectRoot.pathString
+    logger.log(
+      "Created workspace at \(workspaceFolder.uri.forLogging) as \(type(of: buildSystem)) with project root \(projectRoot ?? "<nil>")"
+    )
+
+    return try? await Workspace(
+      documentManager: self.documentManager,
+      rootUri: workspaceFolder.uri,
+      capabilityRegistry: capabilityRegistry,
+      buildSystem: buildSystem,
+      toolchainRegistry: self.toolchainRegistry,
+      options: options,
+      indexOptions: self.options.indexOptions,
+      indexTaskScheduler: indexTaskScheduler,
+      indexProcessDidProduceResult: { [weak self] in
+        self?.indexTaskDidProduceResult($0)
       },
       indexTasksWereScheduled: { [weak self] count in
         self?.indexProgressManager.indexTasksWereScheduled(count: count)

Sources/SourceKitLSP/Workspace.swift

@@ -15,7 +15,6 @@ import LSPLogging
 import LanguageServerProtocol
 import SKCore
 import SKSupport
-import SKSwiftPMWorkspace
 import SemanticIndex
 
 import struct TSCBasic.AbsolutePath
@@ -148,82 +147,15 @@ public final class Workspace: Sendable {
     documentManager: DocumentManager,
     rootUri: DocumentURI,
     capabilityRegistry: CapabilityRegistry,
+    buildSystem: BuildSystem?,
     toolchainRegistry: ToolchainRegistry,
     options: SourceKitLSPServer.Options,
-    compilationDatabaseSearchPaths: [RelativePath],
     indexOptions: IndexOptions = IndexOptions(),
     indexTaskScheduler: TaskScheduler<AnyIndexTaskDescription>,
     indexProcessDidProduceResult: @escaping @Sendable (IndexProcessResult) -> Void,
-    reloadPackageStatusCallback: @Sendable @escaping (ReloadPackageStatus) async -> Void,
     indexTasksWereScheduled: @Sendable @escaping (Int) -> Void,
     indexProgressStatusDidChange: @Sendable @escaping () -> Void
   ) async throws {
-    var buildSystem: BuildSystem? = nil
-
-    if let rootUrl = rootUri.fileURL, let rootPath = try? AbsolutePath(validating: rootUrl.path) {
-      var options = options
-      var isForIndexBuild = false
-      if options.indexOptions.enableBackgroundIndexing, options.buildSetup.path == nil {
-        options.buildSetup.path = rootPath.appending(component: ".index-build")
-        isForIndexBuild = true
-      }
-      func createSwiftPMBuildSystem(rootUrl: URL) async -> SwiftPMBuildSystem? {
-        return await SwiftPMBuildSystem(
-          url: rootUrl,
-          toolchainRegistry: toolchainRegistry,
-          buildSetup: options.buildSetup,
-          isForIndexBuild: isForIndexBuild,
-          reloadPackageStatusCallback: reloadPackageStatusCallback
-        )
-      }
-
-      func createCompilationDatabaseBuildSystem(rootPath: AbsolutePath) -> CompilationDatabaseBuildSystem? {
-        return CompilationDatabaseBuildSystem(
-          projectRoot: rootPath,
-          searchPaths: compilationDatabaseSearchPaths
-        )
-      }
-
-      func createBuildServerBuildSystem(rootPath: AbsolutePath) async -> BuildServerBuildSystem? {
-        return await BuildServerBuildSystem(projectRoot: rootPath, buildSetup: options.buildSetup)
-      }
-
-      let defaultBuildSystem: BuildSystem? =
-        switch options.buildSetup.defaultWorkspaceType {
-        case .buildServer: await createBuildServerBuildSystem(rootPath: rootPath)
-        case .compilationDatabase: createCompilationDatabaseBuildSystem(rootPath: rootPath)
-        case .swiftPM: await createSwiftPMBuildSystem(rootUrl: rootUrl)
-        case nil: nil
-        }
-      if let defaultBuildSystem {
-        buildSystem = defaultBuildSystem
-      } else if let buildServer = await createBuildServerBuildSystem(rootPath: rootPath) {
-        buildSystem = buildServer
-      } else if let swiftpm = await createSwiftPMBuildSystem(rootUrl: rootUrl) {
-        buildSystem = swiftpm
-      } else if let compdb = createCompilationDatabaseBuildSystem(rootPath: rootPath) {
-        buildSystem = compdb
-      } else {
-        buildSystem = nil
-      }
-      if let buildSystem {
-        let projectRoot = await buildSystem.projectRoot
-        logger.log(
-          "Opening workspace at \(rootUrl) as \(type(of: buildSystem)) with project root \(projectRoot.pathString)"
-        )
-      } else {
-        logger.error(
-          "Could not set up a build system for workspace at '\(rootUri.forLogging)'"
-        )
-      }
-    } else {
-      // We assume that workspaces are directories. This is only true for URLs not for URIs in general.
-      // Simply skip setting up the build integration in this case.
-      logger.error(
-        "cannot setup build integration for workspace at URI \(rootUri.forLogging) because the URI it is not a valid file URL"
-      )
-    }
-
     var index: IndexStoreDB? = nil
     var indexDelegate: SourceKitIndexDelegate? = nil