Swap BuildSystem to remove build settings method (#183)

Remove `settings(for:)` from the BuildSystem API in favor of the change callback

- With the `settings(for:)` method removed, we instead block on a
  callback from the `BuildSystemManager` which itself handles
  the primary build system callback in conjunction with the fallback
  system

- We queue up notifications / requests that reference documents
  until we receive the callback.

- Remove language from FileBuildSettings as it is no longer needed,
  Swift-specific working dir fixes are now in SwiftCompileCommand

- registerForChangeNotifications should inform its delegate
  asynchronously instead of immediately

Author: DavidGoldman

Sources/LanguageServerProtocol/Message.swift

@@ -67,8 +67,14 @@ extension NotificationType {
   }
 }
 
-/// A `textDocument/*` request, which takes a text document identifier indicating which document it
-/// operates in or on.
+/// A `textDocument/*` notification, which takes a text document identifier
+/// indicating which document it operates in or on.
+public protocol TextDocumentNotification: NotificationType {
+  var textDocument: TextDocumentIdentifier { get }
+}
+
+/// A `textDocument/*` request, which takes a text document identifier
+/// indicating which document it operates in or on.
 public protocol TextDocumentRequest: RequestType {
   var textDocument: TextDocumentIdentifier { get }
 }

Sources/LanguageServerProtocol/Notifications/TextSynchronizationNotifications.swift

@@ -97,7 +97,7 @@ public struct DidChangeTextDocumentNotification: NotificationType, Hashable {
 ///   - reason: Whether this was user-initiated, auto-saved, etc.
 ///
 /// Servers that support willSave should set the `willSave` text document sync option.
-public struct WillSaveTextDocumentNotification: NotificationType, Hashable {
+public struct WillSaveTextDocumentNotification: TextDocumentNotification, Hashable {
   public static let method: String = "textDocument/willSave"
 
   /// The document that will be saved.
@@ -114,7 +114,7 @@ public struct WillSaveTextDocumentNotification: NotificationType, Hashable {
 ///   - text: The content of the document at the time of save.
 ///
 /// Servers that support didSave should set the `save` text document sync option.
-public struct DidSaveTextDocumentNotification: NotificationType, Hashable {
+public struct DidSaveTextDocumentNotification: TextDocumentNotification, Hashable {
   public static let method: String = "textDocument/didSave"
 
   /// The document that was saved.

Sources/LanguageServerProtocol/Requests/ReferencesRequest.swift

@@ -23,7 +23,7 @@
 ///   - includeDeclaration: Whether to include the declaration in the list of symbols.
 ///
 /// - Returns: An array of locations, one for each reference.
-public struct ReferencesRequest: RequestType, Hashable {
+public struct ReferencesRequest: TextDocumentRequest, Hashable {
   public static let method: String = "textDocument/references"
   public typealias Response = [Location]
 

Sources/SKCore/BuildServerBuildSystem.swift

@@ -140,20 +140,36 @@ final class BuildServerHandler: LanguageServerEndpoint {
   }
 
   func handleFileOptionsChanged(_ notification: Notification<FileOptionsChangedNotification>) {
-    // TODO: add delegate method to include the changed settings directly
-    self.delegate?.fileBuildSettingsChanged([notification.params.uri])
+    let result = notification.params.updatedOptions
+    let settings = FileBuildSettings(
+        compilerArguments: result.options, workingDirectory: result.workingDirectory)
+    self.delegate?.fileBuildSettingsChanged([notification.params.uri: .modified(settings)])
+  }
+}
+
+extension BuildServerBuildSystem {
+  /// Exposed for *testing*.
+  public func _settings(for uri: DocumentURI) -> FileBuildSettings? {
+    if let response = try? self.buildServer?.sendSync(SourceKitOptions(uri: uri)) {
+      return FileBuildSettings(
+        compilerArguments: response.options,
+        workingDirectory: response.workingDirectory)
+    }
+    return nil
   }
 }
 
 extension BuildServerBuildSystem: BuildSystem {
 
-  /// Register the given file for build-system level change notifications, such as command
-  /// line flag changes, dependency changes, etc.
   public func registerForChangeNotifications(for uri: DocumentURI, language: Language) {
     let request = RegisterForChanges(uri: uri, action: .register)
     _ = self.buildServer?.send(request, queue: requestQueue, reply: { result in
       if let error = result.failure {
         log("error registering \(uri): \(error)", level: .error)
+
+        // BuildServer registration failed, so tell our delegate that no build
+        // settings are available.
+        self.delegate?.fileBuildSettingsChanged([uri: .removedOrUnavailable])
       }
     })
   }
@@ -169,16 +185,6 @@ extension BuildServerBuildSystem: BuildSystem {
     })
   }
 
-  public func settings(for uri: DocumentURI, _ language: Language) -> FileBuildSettings? {
-    if let response = try? self.buildServer?.sendSync(SourceKitOptions(uri: uri)) {
-      return FileBuildSettings(
-        compilerArguments: response.options,
-        workingDirectory: response.workingDirectory,
-        language: language)
-    }
-    return nil
-  }
-
   public func buildTargets(reply: @escaping (LSPResult<[BuildTarget]>) -> Void) {
     _ = self.buildServer?.send(BuildTargets(), queue: requestQueue) { response in
       switch response {

Sources/SKCore/BuildSystem.swift

@@ -31,15 +31,16 @@ public protocol BuildSystem: AnyObject {
   /// The path to put the index database, if any.
   var indexDatabasePath: AbsolutePath? { get }
 
-  /// Returns the settings for the given url and language mode, if known.
-  func settings(for: DocumentURI, _ language: Language) -> FileBuildSettings?
-
   /// Delegate to handle any build system events such as file build settings
-  /// changing.
+  /// initial reports as well as changes.
   var delegate: BuildSystemDelegate? { get set }
 
   /// Register the given file for build-system level change notifications, such
   /// as command line flag changes, dependency changes, etc.
+  ///
+  /// IMPORTANT: When first receiving a register request, the `BuildSystem` MUST asynchronously
+  /// inform its delegate of any initial settings for the given file via the
+  /// `fileBuildSettingsChanged` method, even if unavailable.
   func registerForChangeNotifications(for: DocumentURI, language: Language)
 
   /// Unregister the given file for build-system level change notifications,

Sources/SKCore/BuildSystemDelegate.swift

@@ -21,16 +21,18 @@ public protocol BuildSystemDelegate: AnyObject {
   /// interest.
   func buildTargetsChanged(_ changes: [BuildTargetEvent])
 
-  /// Notify the delegate that the given files' build settings have changed. If the given set is
-  /// empty, assume that all open files are affected.
+  /// Notify the delegate that the given files' build settings have changed.
   ///
-  /// The callee should request new build settings for any of the given files
-  /// that they are interested in.
-  func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>)
+  /// The delegate should cache the new build settings for any of the given
+  /// files that they are interested in.
+  func fileBuildSettingsChanged(
+    _ changedFiles: [DocumentURI: FileBuildSettingsChange])
 
-  /// Notify the delegate that the dependencies of the given files have changed and that ASTs
-  /// may need to be refreshed. If the given set is empty, assume that all open files are affected.
+  /// Notify the delegate that the dependencies of the given files have changed
+  /// and that ASTs may need to be refreshed. If the given set is empty, assume
+  /// that all watched files are affected.
   ///
-  /// The callee should refresh ASTs unless it is able to determine that a refresh is not necessary.
+  /// The callee should refresh ASTs unless it is able to determine that a
+  /// refresh is not necessary.
   func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>)
 }