Migrate SwiftPMWorkspace to be an actor

ahoppen · ahoppen · commit fccbc3973ce7 · 2023-09-29T16:55:51.000-07:00
diff --git a/Sources/LSPTestSupport/Assertions.swift b/Sources/LSPTestSupport/Assertions.swift
@@ -22,6 +22,27 @@ public func assertNoThrow<T>(
   XCTAssertNoThrow(try expression(), message(), file: file, line: line)
 }
 
+/// Same as `XCTAssertThrows` but executes the trailing closure.
+public func assertThrowsError<T>(
+  _ expression: @autoclosure () async throws -> T,
+  _ message: @autoclosure () -> String = "",
+  file: StaticString = #filePath,
+  line: UInt = #line,
+  _ errorHandler: (_ error: Error) -> Void = { _ in }
+) async {
+  let didThrow: Bool
+  do {
+    _ = try await expression()
+    didThrow = false
+  } catch {
+    errorHandler(error)
+    didThrow = true
+  }
+  if !didThrow {
+    XCTFail("Expression was expected to throw but did not throw", file: file, line: line)
+  }
+}
+
 /// Same as `XCTAssertEqual` but doesn't take autoclosures and thus `expression1`
 /// and `expression2` can contain `await`.
 public func assertEqual<T: Equatable>(
@@ -34,6 +55,28 @@ public func assertEqual<T: Equatable>(
   XCTAssertEqual(expression1, expression2, message(), file: file, line: line)
 }
 
+/// Same as `XCTAssertNil` but doesn't take autoclosures and thus `expression`
+/// can contain `await`.
+public func assertNil<T: Equatable>(
+  _ expression: T?,
+  _ message: @autoclosure () -> String = "",
+  file: StaticString = #filePath,
+  line: UInt = #line
+) {
+  XCTAssertNil(expression, message(), file: file, line: line)
+}
+
+/// Same as `XCTAssertNotNil` but doesn't take autoclosures and thus `expression`
+/// can contain `await`.
+public func assertNotNil<T: Equatable>(
+  _ expression: T?,
+  _ message: @autoclosure () -> String = "",
+  file: StaticString = #filePath,
+  line: UInt = #line
+) {
+  XCTAssertNotNil(expression, message(), file: file, line: line)
+}
+
 extension XCTestCase {
   private struct ExpectationNotFulfilledError: Error, CustomStringConvertible {
     var expecatations: [XCTestExpectation]
diff --git a/Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift b/Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift
@@ -48,7 +48,7 @@ public enum ReloadPackageStatus {
 /// This class implements the `BuildSystem` interface to provide the build settings for a Swift
 /// Package Manager (SwiftPM) package. The settings are determined by loading the Package.swift
 /// manifest using `libSwiftPM` and constructing a build plan using the default (debug) parameters.
-public final class SwiftPMWorkspace {
+public actor SwiftPMWorkspace {
 
   public enum Error: Swift.Error {
 
@@ -82,14 +82,6 @@ public final class SwiftPMWorkspace {
   /// mapped to the language the delegate specified when registering for change notifications.
   var watchedFiles: [DocumentURI: Language] = [:]
 
-  /// Queue guarding the following properties:
-  /// - `delegate`
-  /// - `watchedFiles`
-  /// - `packageGraph`
-  /// - `fileToTarget`
-  /// - `sourceDirToTarget`
-  let queue: DispatchQueue = .init(label: "SwiftPMWorkspace.queue", qos: .utility)
-
   /// This callback is informed when `reloadPackage` starts and ends executing.
   var reloadPackageStatusCallback: (ReloadPackageStatus) -> Void
 
@@ -108,8 +100,7 @@ public final class SwiftPMWorkspace {
     fileSystem: FileSystem = localFileSystem,
     buildSetup: BuildSetup,
     reloadPackageStatusCallback: @escaping (ReloadPackageStatus) -> Void = { _ in }
-  ) throws
-  {
+  ) async throws {
     self.workspacePath = workspacePath
     self.fileSystem = fileSystem
 
@@ -169,15 +160,14 @@ public final class SwiftPMWorkspace {
   /// - Parameters:
   ///   - reloadPackageStatusCallback: Will be informed when `reloadPackage` starts and ends executing.
   /// - Returns: nil if `workspacePath` is not part of a package or there is an error.
-  public convenience init?(
+  public init?(
     url: URL,
     toolchainRegistry: ToolchainRegistry,
     buildSetup: BuildSetup,
     reloadPackageStatusCallback: @escaping (ReloadPackageStatus) -> Void
-  )
-  {
+  ) async {
     do {
-      try self.init(
+      try await self.init(
         workspacePath: try TSCAbsolutePath(validating: url.path),
         toolchainRegistry: toolchainRegistry,
         fileSystem: localFileSystem,
@@ -198,7 +188,6 @@ extension SwiftPMWorkspace {
 
   /// (Re-)load the package settings by parsing the manifest and resolving all the targets and
   /// dependencies.
-  /// Must only be called on `queue` or from the initializer.
   func reloadPackage() throws {
     reloadPackageStatusCallback(.start)
     defer {
@@ -256,7 +245,7 @@ extension SwiftPMWorkspace {
     var changedFiles: [DocumentURI: FileBuildSettingsChange] = [:]
     for (uri, language) in self.watchedFiles {
       orLog {
-        if let settings = try self.settings(for: uri, language) {
+        if let settings = try self.buildSettings(for: uri, language: language) {
           changedFiles[uri] = FileBuildSettingsChange(settings)
         } else {
           changedFiles[uri] = .removedOrUnavailable
@@ -289,29 +278,10 @@ extension SwiftPMWorkspace: SKCore.BuildSystem {
     for uri: DocumentURI,
     _ language: Language) throws -> FileBuildSettings?
   {
-    return try queue.sync {
-      try self.settings(for: uri, language)
-    }
+    try self.buildSettings(for: uri, language: language)
   }
 
-  public func buildSettings(for document: DocumentURI, language: Language) async throws -> FileBuildSettings? {
-    return try await withCheckedThrowingContinuation { continuation in
-      queue.async {
-        do {
-          continuation.resume(returning: try self.settings(for: document, language))
-        } catch {
-          continuation.resume(throwing: error)
-        }
-      }
-    }
-  }
-
-  /// Must only be called on `queue`.
-  private func settings(
-    for uri: DocumentURI,
-    _ language: Language) throws -> FileBuildSettings?
-  {
-    dispatchPrecondition(condition: .onQueue(queue))
+  public func buildSettings(for uri: DocumentURI, language: Language) throws -> FileBuildSettings? {
     guard let url = uri.fileURL else {
       // We can't determine build settings for non-file URIs.
       return nil
@@ -336,37 +306,31 @@ extension SwiftPMWorkspace: SKCore.BuildSystem {
   }
 
   public func registerForChangeNotifications(for uri: DocumentURI, language: Language) {
-    queue.async {
-      assert(self.watchedFiles[uri] == nil, "Registered twice for change notifications of the same URI")
-      guard let delegate = self.delegate else { return }
-      self.watchedFiles[uri] = language
-
-      var settings: FileBuildSettings? = nil
-      do {
-        settings = try self.settings(for: uri, language)
-      } catch {
-        log("error computing settings: \(error)")
-      }
-      if let settings = settings {
-        delegate.fileBuildSettingsChanged([uri: FileBuildSettingsChange(settings)])
-      } else {
-        delegate.fileBuildSettingsChanged([uri: .removedOrUnavailable])
-      }
+    assert(self.watchedFiles[uri] == nil, "Registered twice for change notifications of the same URI")
+    guard let delegate = self.delegate else { return }
+    self.watchedFiles[uri] = language
+
+    var settings: FileBuildSettings? = nil
+    do {
+      settings = try self.buildSettings(for: uri, language: language)
+    } catch {
+      log("error computing settings: \(error)")
+    }
+    if let settings = settings {
+      delegate.fileBuildSettingsChanged([uri: FileBuildSettingsChange(settings)])
+    } else {
+      delegate.fileBuildSettingsChanged([uri: .removedOrUnavailable])
     }
   }
 
   /// Unregister the given file for build-system level change notifications, such as command
   /// line flag changes, dependency changes, etc.
   public func unregisterForChangeNotifications(for uri: DocumentURI) {
-    queue.async {
-      self.watchedFiles[uri] = nil
-    }
+    self.watchedFiles[uri] = nil
   }
 
   /// Returns the resolved target description for the given file, if one is known.
-  /// Must only be called on `queue`.
   private func targetDescription(for file: AbsolutePath) throws -> TargetBuildDescription? {
-    dispatchPrecondition(condition: .onQueue(queue))
     if let td = fileToTarget[file] {
       return td
     }
@@ -403,12 +367,10 @@ extension SwiftPMWorkspace: SKCore.BuildSystem {
   }
 
   public func filesDidChange(_ events: [FileEvent]) {
-    queue.async {
-      if events.contains(where: { self.fileEventShouldTriggerPackageReload(event: $0) }) {
-        orLog {
-          // TODO: It should not be necessary to reload the entire package just to get build settings for one file.
-          try self.reloadPackage()
-        }
+    if events.contains(where: { self.fileEventShouldTriggerPackageReload(event: $0) }) {
+      orLog {
+        // TODO: It should not be necessary to reload the entire package just to get build settings for one file.
+        try self.reloadPackage()
       }
     }
   }
@@ -417,12 +379,10 @@ extension SwiftPMWorkspace: SKCore.BuildSystem {
     guard let fileUrl = uri.fileURL else {
       return .unhandled
     }
-    return self.queue.sync {
-      if (try? targetDescription(for: AbsolutePath(validating: fileUrl.path))) != nil {
-        return .handled
-      } else {
-        return .unhandled
-      }
+    if (try? targetDescription(for: AbsolutePath(validating: fileUrl.path))) != nil {
+      return .handled
+    } else {
+      return .unhandled
     }
   }
 }
@@ -450,9 +410,7 @@ extension SwiftPMWorkspace {
   }
 
   /// Retrieve settings for a package manifest (Package.swift).
-  /// Must only be called on `queue`.
   private func settings(forPackageManifest path: AbsolutePath) throws -> FileBuildSettings? {
-    dispatchPrecondition(condition: .onQueue(queue))
     func impl(_ path: AbsolutePath) -> FileBuildSettings? {
       for package in packageGraph.packages where path == package.manifest.path {
         let compilerArgs = workspace.interpreterFlags(for: package.path) + [path.pathString]
@@ -470,9 +428,7 @@ extension SwiftPMWorkspace {
   }
 
   /// Retrieve settings for a given header file.
-  /// Must only be called on `queue`.
   private func settings(forHeader path: AbsolutePath, _ language: Language) throws -> FileBuildSettings? {
-    dispatchPrecondition(condition: .onQueue(queue))
     func impl(_ path: AbsolutePath) throws -> FileBuildSettings? {
       var dir = path.parentDirectory
       while !dir.isRoot {
diff --git a/Sources/SKTestSupport/SKSwiftPMTestWorkspace.swift b/Sources/SKTestSupport/SKSwiftPMTestWorkspace.swift
@@ -85,19 +85,19 @@ public final class SKSwiftPMTestWorkspace {
     let buildPath = try AbsolutePath(validating: buildDir.path)
     let buildSetup = BuildSetup(configuration: .debug, path: buildPath, flags: BuildFlags())
 
-    let swiftpm = try SwiftPMWorkspace(
+    let swiftpm = try await SwiftPMWorkspace(
       workspacePath: sourcePath,
       toolchainRegistry: ToolchainRegistry.shared,
       buildSetup: buildSetup)
 
     let libIndexStore = try IndexStoreLibrary(dylibPath: toolchain.libIndexStore!.pathString)
 
-    try fm.createDirectory(atPath: swiftpm.indexStorePath!.pathString, withIntermediateDirectories: true)
+    try fm.createDirectory(atPath: await swiftpm.indexStorePath!.pathString, withIntermediateDirectories: true)
 
     let indexDelegate = SourceKitIndexDelegate()
 
     self.index = try IndexStoreDB(
-      storePath: swiftpm.indexStorePath!.pathString,
+      storePath: await swiftpm.indexStorePath!.pathString,
       databasePath: tmpDir.path,
       library: libIndexStore,
       delegate: indexDelegate,
diff --git a/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift b/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift
@@ -987,14 +987,28 @@ extension SwiftLanguageServer {
     _ = handle
   }
 
-  public func foldingRange(_ req: Request<FoldingRangeRequest>) {
+  public func foldingRange(_ req: Request<FoldingRangeRequest>) async {
     let foldingRangeCapabilities = capabilityRegistry.clientCapabilities.textDocument?.foldingRange
-    guard let snapshot = self.documentManager.latestSnapshot(req.params.textDocument.uri) else {
+    let uri = req.params.textDocument.uri
+    guard var snapshot = self.documentManager.latestSnapshot(uri) else {
       log("failed to find snapshot for url \(req.params.textDocument.uri)")
       req.reply(nil)
       return
     }
 
+    // FIXME: (async) We might not have computed the syntax tree yet. Wait until we have a syntax tree.
+    // Really, getting the syntax tree should be an async operation.
+    while snapshot.tokens.syntaxTree == nil {
+      try? await Task.sleep(nanoseconds: 1_000_000)
+      if let newSnapshot = documentManager.latestSnapshot(uri) {
+        snapshot = newSnapshot
+      } else {
+        log("failed to find snapshot for url \(req.params.textDocument.uri)")
+        req.reply(nil)
+        return
+      }
+    }
+
     guard let sourceFile = snapshot.tokens.syntaxTree else {
       log("no lexical structure available for url \(req.params.textDocument.uri)")
       req.reply(nil)
diff --git a/Sources/SourceKitLSP/Swift/VariableTypeInfo.swift b/Sources/SourceKitLSP/Swift/VariableTypeInfo.swift
@@ -94,10 +94,21 @@ extension SwiftLanguageServer {
     _ range: Range<Position>? = nil,
     _ completion: @escaping (Swift.Result<[VariableTypeInfo], VariableTypeInfoError>) -> Void
   ) async {
-    guard let snapshot = documentManager.latestSnapshot(uri) else {
+    guard var snapshot = documentManager.latestSnapshot(uri) else {
       return completion(.failure(.unknownDocument(uri)))
     }
 
+    // FIXME: (async) We might not have computed the syntax tree yet. Wait until we have a syntax tree.
+    // Really, getting the syntax tree should be an async operation.
+    while snapshot.tokens.syntaxTree == nil {
+      try? await Task.sleep(nanoseconds: 1_000_000)
+      if let newSnapshot = documentManager.latestSnapshot(uri) {
+        snapshot = newSnapshot
+      } else {
+        return completion(.failure(.unknownDocument(uri)))
+      }
+    }
+
     let keys = self.keys
 
     let skreq = SKDRequestDictionary(sourcekitd: sourcekitd)
diff --git a/Sources/SourceKitLSP/Workspace.swift b/Sources/SourceKitLSP/Workspace.swift
@@ -99,7 +99,7 @@ public final class Workspace {
     if let rootUrl = rootUri.fileURL, let rootPath = try? AbsolutePath(validating: rootUrl.path) {
       if let buildServer = BuildServerBuildSystem(projectRoot: rootPath, buildSetup: buildSetup) {
         buildSystem = buildServer
-      } else if let swiftpm = SwiftPMWorkspace(
+      } else if let swiftpm = await SwiftPMWorkspace(
         url: rootUrl,
         toolchainRegistry: toolchainRegistry,
         buildSetup: buildSetup,
diff --git a/Tests/SKSwiftPMWorkspaceTests/SwiftPMWorkspaceTests.swift b/Tests/SKSwiftPMWorkspaceTests/SwiftPMWorkspaceTests.swift
