Merge pull request #848 from ahoppen/ahoppen/minor-cleanups

Fix a couple `FIXME: (async)`

Author: ahoppen

Sources/SKCore/BuildSystemManager.swift

@@ -254,14 +254,7 @@ extension BuildSystemManager {
 }
 
 extension BuildSystemManager: BuildSystemDelegate {
-  // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has ben asyncified
-  public nonisolated func fileBuildSettingsChanged(_ changes: Set<DocumentURI>) {
-    Task {
-      await fileBuildSettingsChangedImpl(changes)
-    }
-  }
-
-  public func fileBuildSettingsChangedImpl(_ changedFiles: Set<DocumentURI>) async {
+  public func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) async {
     let changedWatchedFiles = changedFiles.flatMap({ mainFile in
       self.watchedFiles.filter { $1.mainFile == mainFile }.keys
     })
@@ -271,14 +264,7 @@ extension BuildSystemManager: BuildSystemDelegate {
     }
   }
 
-  // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has ben asyncified
-  public nonisolated func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) {
-    Task {
-      await filesDependenciesUpdatedImpl(changedFiles)
-    }
-  }
-
-  public func filesDependenciesUpdatedImpl(_ changedFiles: Set<DocumentURI>) async {
+  public func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) async {
     // Empty changes --> assume everything has changed.
     guard !changedFiles.isEmpty else {
       if let delegate = self._delegate {
@@ -295,43 +281,22 @@ extension BuildSystemManager: BuildSystemDelegate {
     }
   }
 
-  // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has ben asyncified
-  public nonisolated func buildTargetsChanged(_ changes: [BuildTargetEvent]) {
-    Task {
-      await buildTargetsChangedImpl(changes)
-    }
-  }
-
-  public func buildTargetsChangedImpl(_ changes: [BuildTargetEvent]) async {
+  public func buildTargetsChanged(_ changes: [BuildTargetEvent]) async {
     if let delegate = self._delegate {
       await delegate.buildTargetsChanged(changes)
     }
   }
 
-  // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has ben asyncified
-  public nonisolated func fileHandlingCapabilityChanged() {
-    Task {
-      await fileHandlingCapabilityChangedImpl()
-    }
-  }
-
-  public func fileHandlingCapabilityChangedImpl() async {
+  public func fileHandlingCapabilityChanged() async {
     if let delegate = self._delegate {
       await delegate.fileHandlingCapabilityChanged()
     }
   }
 }
 
 extension BuildSystemManager: MainFilesDelegate {
-  // FIXME: (async) Make this method isolated once `MainFilesDelegate` has ben asyncified
-  public nonisolated func mainFilesChanged() {
-    Task {
-      await mainFilesChangedImpl()
-    }
-  }
-
   // FIXME: Consider debouncing/limiting this, seems to trigger often during a build.
-  public func mainFilesChangedImpl() async {
+  public func mainFilesChanged() async {
     let origWatched = self.watchedFiles
     self.watchedFiles = [:]
     var buildSettingsChanges = Set<DocumentURI>()

Sources/SKCore/CompilationDatabaseBuildSystem.swift

@@ -87,8 +87,16 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
 
   public var indexPrefixMappings: [PathPrefixMapping] { return [] }
 
-  public func buildSettings(for document: DocumentURI, language: Language) async throws -> FileBuildSettings? {
-    return self.settings(for: document)
+  public func buildSettings(for document: DocumentURI, language: Language) async -> FileBuildSettings? {
+    guard let url = document.fileURL else {
+      // We can't determine build settings for non-file URIs.
+      return nil
+    }
+    guard let db = database(for: url),
+          let cmd = db[url].first else { return nil }
+    return FileBuildSettings(
+      compilerArguments: Array(cmd.commandLine.dropFirst()),
+      workingDirectory: cmd.directory)
   }
 
   public func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
@@ -168,22 +176,3 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
     }
   }
 }
-
-extension CompilationDatabaseBuildSystem {
-  private func settings(for uri: DocumentURI) -> FileBuildSettings? {
-    guard let url = uri.fileURL else {
-      // We can't determine build settings for non-file URIs.
-      return nil
-    }
-    guard let db = database(for: url),
-          let cmd = db[url].first else { return nil }
-    return FileBuildSettings(
-      compilerArguments: Array(cmd.commandLine.dropFirst()),
-      workingDirectory: cmd.directory)
-  }
-
-  /// Exposed for *testing*.
-  public func _settings(for uri: DocumentURI) -> FileBuildSettings? {
-    return self.settings(for: uri)
-  }
-}

Sources/SKCore/MainFilesProvider.swift

@@ -30,5 +30,5 @@ public protocol MainFilesProvider: AnyObject {
 public protocol MainFilesDelegate: AnyObject {
 
   /// The mapping from files to main files (may have) changed.
-  func mainFilesChanged()
+  func mainFilesChanged() async
 }

Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift

@@ -83,7 +83,7 @@ public actor SwiftPMWorkspace {
   var watchedFiles: [DocumentURI: Language] = [:]
 
   /// This callback is informed when `reloadPackage` starts and ends executing.
-  var reloadPackageStatusCallback: (ReloadPackageStatus) -> Void
+  var reloadPackageStatusCallback: (ReloadPackageStatus) async -> Void
 
 
   /// Creates a build system using the Swift Package Manager, if this workspace is a package.
@@ -99,7 +99,7 @@ public actor SwiftPMWorkspace {
     toolchainRegistry: ToolchainRegistry,
     fileSystem: FileSystem = localFileSystem,
     buildSetup: BuildSetup,
-    reloadPackageStatusCallback: @escaping (ReloadPackageStatus) -> Void = { _ in }
+    reloadPackageStatusCallback: @escaping (ReloadPackageStatus) async -> Void = { _ in }
   ) async throws {
     self.workspacePath = workspacePath
     self.fileSystem = fileSystem
@@ -164,7 +164,7 @@ public actor SwiftPMWorkspace {
     url: URL,
     toolchainRegistry: ToolchainRegistry,
     buildSetup: BuildSetup,
-    reloadPackageStatusCallback: @escaping (ReloadPackageStatus) -> Void
+    reloadPackageStatusCallback: @escaping (ReloadPackageStatus) async -> Void
   ) async {
     do {
       try await self.init(
@@ -189,11 +189,7 @@ extension SwiftPMWorkspace {
   /// (Re-)load the package settings by parsing the manifest and resolving all the targets and
   /// dependencies.
   func reloadPackage() async throws {
-    reloadPackageStatusCallback(.start)
-    defer {
-      reloadPackageStatusCallback(.end)
-    }
-
+    await reloadPackageStatusCallback(.start)
 
     let observabilitySystem = ObservabilitySystem({ scope, diagnostic in
         log(diagnostic.description, level: diagnostic.severity.asLogLevel)
@@ -241,10 +237,14 @@ extension SwiftPMWorkspace {
         return td
     })
 
-    guard let delegate = self.delegate else { return }
+    guard let delegate = self.delegate else {
+      await reloadPackageStatusCallback(.end)
+      return
+    }
     let changedFiles = Set<DocumentURI>(self.watchedFiles.keys)
     await delegate.fileBuildSettingsChanged(changedFiles)
     await delegate.fileHandlingCapabilityChanged()
+    await reloadPackageStatusCallback(.end)
   }
 }
 

Sources/SourceKitLSP/SourceKitIndexDelegate.swift

@@ -56,7 +56,9 @@ public final class SourceKitIndexDelegate: IndexDelegate {
   /// *Must be called on queue*.
   func _indexChanged() {
     for delegate in mainFilesDelegates {
-      delegate.mainFilesChanged()
+      Task {
+        await delegate.mainFilesChanged()
+      }
     }
   }
 

Sources/SourceKitLSP/SourceKitServer.swift

@@ -648,8 +648,7 @@ extension SourceKitServer: MessageHandler {
 // MARK: - Build System Delegate
 
 extension SourceKitServer: BuildSystemDelegate {
-  // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has been asyncified
-  public nonisolated func buildTargetsChanged(_ changes: [BuildTargetEvent]) {
+  public func buildTargetsChanged(_ changes: [BuildTargetEvent]) {
     // TODO: do something with these changes once build target support is in place
   }
 
@@ -664,19 +663,11 @@ extension SourceKitServer: BuildSystemDelegate {
     return documentManager.openDocuments.intersection(changes)
   }
 
-  // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has been asyncified
-  /// Non-async variant that executes `fileBuildSettingsChangedImpl` in a new task.
-  public nonisolated func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) {
-    Task {
-      await self.fileBuildSettingsChangedImpl(changedFiles)
-    }
-  }
-
   /// Handle a build settings change notification from the `BuildSystem`.
   /// This has two primary cases:
   /// - Initial settings reported for a given file, now we can fully open it
   /// - Changed settings for an already open file
-  public func fileBuildSettingsChangedImpl(_ changedFiles: Set<DocumentURI>) async {
+  public func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) async {
     for uri in changedFiles {
       guard self.documentManager.openDocuments.contains(uri) else {
         continue
@@ -690,17 +681,10 @@ extension SourceKitServer: BuildSystemDelegate {
     }
   }
 
-  // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has been asyncified
-  public nonisolated func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) {
-    Task {
-      await filesDependenciesUpdatedImpl(changedFiles)
-    }
-  }
-
   /// Handle a dependencies updated notification from the `BuildSystem`.
   /// We inform the respective language services as long as the given file is open
   /// (not queued for opening).
-  public func filesDependenciesUpdatedImpl(_ changedFiles: Set<DocumentURI>) async {
+  public func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) async {
     // Split the changedFiles into the workspaces they belong to.
     // Then invoke affectedOpenDocumentsForChangeSet for each workspace with its affected files.
     let changedFilesAndWorkspace = await changedFiles.asyncMap {
@@ -720,14 +704,7 @@ extension SourceKitServer: BuildSystemDelegate {
     }
   }
 
-  // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has been asyncified
-  public nonisolated func fileHandlingCapabilityChanged() {
-    Task {
-      await fileHandlingCapabilityChangedImpl()
-    }
-  }
-
-  public func fileHandlingCapabilityChangedImpl() {
+  public func fileHandlingCapabilityChanged() {
     self.uriToWorkspaceCache = [:]
   }
 }
@@ -756,18 +733,11 @@ extension SourceKitServer {
           // Client doesn’t support work done progress
           return
         }
-        // FIXME: (async) This can cause out-of-order notifications to be sent to the editor
-        // if the scheduled tasks change order.
-        // Make `reloadPackageStatusCallback` async and shift the responsibility for
-        // guaranteeing in-order calls to `reloadPackageStatusCallback` to
-        // `SwiftPMWorkspace.reloadPackage` once that method is async.
-        Task {
-          switch status {
-          case .start:
-            await self.packageLoadingWorkDoneProgress.startProgress(server: self)
-          case .end:
-            await self.packageLoadingWorkDoneProgress.endProgress(server: self)
-          }
+        switch status {
+        case .start:
+          await self.packageLoadingWorkDoneProgress.startProgress(server: self)
+        case .end:
+          await self.packageLoadingWorkDoneProgress.endProgress(server: self)
         }
       }
     )

Sources/SourceKitLSP/Workspace.swift

@@ -99,7 +99,7 @@ public final class Workspace {
     toolchainRegistry: ToolchainRegistry,
     buildSetup: BuildSetup,
     indexOptions: IndexOptions = IndexOptions(),
-    reloadPackageStatusCallback: @escaping (ReloadPackageStatus) -> Void
+    reloadPackageStatusCallback: @escaping (ReloadPackageStatus) async -> Void
   ) async throws {
     var buildSystem: BuildSystem? = nil
     if let rootUrl = rootUri.fileURL, let rootPath = try? AbsolutePath(validating: rootUrl.path) {