Implement push diagnostics in terms of DocumentDiagnosticsRequest

Instead of listening for document updates sent from `sourcekitd` and sending a `PublishDiagnosticsNotification` based on the `sourcekitd` notification, wait for a little while and then execute an internal `DocumentDiagnosticsRequest` to load diagnostics and send them to the client.

This has two advantages:
- It unifies the two diagnostic implementations
- It removes the need to keep track of semantic diagnostics in `SwiftLanguageServer`
- It gets us one step closed to opening and editing documents in `syntactic_only` mode. The only thing that is left now are semantic tokens.

Author: ahoppen

Package.swift

@@ -291,8 +291,10 @@ let package = Package(
         "SKSwiftPMWorkspace",
         "SourceKitD",
         .product(name: "IndexStoreDB", package: "indexstore-db"),
+        .product(name: "SwiftDiagnostics", package: "swift-syntax"),
         .product(name: "SwiftIDEUtils", package: "swift-syntax"),
         .product(name: "SwiftParser", package: "swift-syntax"),
+        .product(name: "SwiftParserDiagnostics", package: "swift-syntax"),
         .product(name: "SwiftSyntax", package: "swift-syntax"),
         .product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"),
       ],

Sources/LSPTestSupport/Assertions.swift

@@ -42,7 +42,7 @@ public func assertThrowsError<T>(
   _ message: @autoclosure () -> String = "",
   file: StaticString = #filePath,
   line: UInt = #line,
-  _ errorHandler: (_ error: Error) -> Void = { _ in }
+  errorHandler: (_ error: Error) -> Void = { _ in }
 ) async {
   let didThrow: Bool
   do {

Sources/SKTestSupport/TestSourceKitLSPClient.swift

@@ -21,7 +21,7 @@ import XCTest
 
 extension SourceKitServer.Options {
   /// The default SourceKitServer options for testing.
-  public static var testDefault = Self()
+  public static var testDefault = Self(swiftPublishDiagnosticsDebounceDuration: 0)
 }
 
 /// A mock SourceKit-LSP client (aka. a mock editor) that behaves like an editor
@@ -63,13 +63,13 @@ public final class TestSourceKitLSPClient: MessageHandler {
   /// - Parameters:
   ///   - useGlobalModuleCache: If `false`, the server will use its own module
   ///     cache in an empty temporary directory instead of the global module cache.
-  public init(useGlobalModuleCache: Bool = true) {
+  public init(serverOptions: SourceKitServer.Options = .testDefault, useGlobalModuleCache: Bool = true) {
     if !useGlobalModuleCache {
       moduleCache = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent(UUID().uuidString)
     } else {
       moduleCache = nil
     }
-    var serverOptions = SourceKitServer.Options.testDefault
+    var serverOptions = serverOptions
     if let moduleCache {
       serverOptions.buildSetup.flags.swiftCompilerFlags += ["-module-cache-path", moduleCache.path]
     }
@@ -158,14 +158,16 @@ public final class TestSourceKitLSPClient: MessageHandler {
   ///
   /// If the next notification is not a `PublishDiagnosticsNotification`, this
   /// methods throws.
-  public func nextDiagnosticsNotification() async throws -> PublishDiagnosticsNotification {
+  public func nextDiagnosticsNotification(
+    timeout: TimeInterval = defaultTimeout
+  ) async throws -> PublishDiagnosticsNotification {
     struct CastError: Error, CustomStringConvertible {
       let actualType: any NotificationType.Type
 
       var description: String { "Expected a publish diagnostics notification but got '\(actualType)'" }
     }
 
-    let nextNotification = try await nextNotification()
+    let nextNotification = try await nextNotification(timeout: timeout)
     guard let diagnostics = nextNotification as? PublishDiagnosticsNotification else {
       throw CastError(actualType: type(of: nextNotification))
     }

Sources/SourceKitLSP/CMakeLists.txt

@@ -46,10 +46,12 @@ target_link_libraries(SourceKitLSP PUBLIC
   SKSwiftPMWorkspace
   SourceKitD
   SwiftSyntax::SwiftBasicFormat
-  SwiftSyntax::SwiftParser
   SwiftSyntax::SwiftDiagnostics
+  SwiftSyntax::SwiftIDEUtils
+  SwiftSyntax::SwiftParser
+  SwiftSyntax::SwiftParserDiagnostics
   SwiftSyntax::SwiftSyntax
-  SwiftSyntax::SwiftIDEUtils)
+)
 target_link_libraries(SourceKitLSP PRIVATE
   $<$<NOT:$<PLATFORM_ID:Darwin>>:FoundationXML>)
 

Sources/SourceKitLSP/SourceKitServer+Options.swift

@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+import Foundation
 import LanguageServerProtocol
 import SKCore
 import SKSupport
@@ -37,18 +38,27 @@ extension SourceKitServer {
     /// Override the default directory where generated interfaces will be stored
     public var generatedInterfacesPath: AbsolutePath
 
+    /// The time that `SwiftLanguageServer` should wait after an edit before starting to compute diagnostics and sending
+    /// a `PublishDiagnosticsNotification`.
+    ///
+    /// This is mostly intended for testing purposes so we don't need to wait the debouncing time to get a diagnostics
+    /// notification when running unit tests.
+    public var swiftPublishDiagnosticsDebounceDuration: TimeInterval
+
     public init(
       buildSetup: BuildSetup = .default,
       clangdOptions: [String] = [],
       indexOptions: IndexOptions = .init(),
       completionOptions: SKCompletionOptions = .init(),
-      generatedInterfacesPath: AbsolutePath = defaultDirectoryForGeneratedInterfaces
+      generatedInterfacesPath: AbsolutePath = defaultDirectoryForGeneratedInterfaces,
+      swiftPublishDiagnosticsDebounceDuration: TimeInterval = 2 /* 2s */
     ) {
       self.buildSetup = buildSetup
       self.clangdOptions = clangdOptions
       self.indexOptions = indexOptions
       self.completionOptions = completionOptions
       self.generatedInterfacesPath = generatedInterfacesPath
+      self.swiftPublishDiagnosticsDebounceDuration = swiftPublishDiagnosticsDebounceDuration
     }
   }
 }

Sources/SourceKitLSP/SourceKitServer.swift

@@ -643,7 +643,7 @@ extension SourceKitServer: MessageHandler {
   }
 
   private func _logResponse<Response>(_ result: LSPResult<Response>, id: RequestID, method: String) {
-    logger.debug(
+    logger.log(
       """
       Sending response:
       Response<\(method, privacy: .public)(\(id, privacy: .public))>(

Sources/SourceKitLSP/Swift/Diagnostic.swift

@@ -14,6 +14,7 @@ import LSPLogging
 import LanguageServerProtocol
 import SKSupport
 import SourceKitD
+import SwiftDiagnostics
 
 extension CodeAction {
 
@@ -69,6 +70,13 @@ extension CodeAction {
     )
   }
 
+  init?(_ fixIt: FixIt, in snapshot: DocumentSnapshot) {
+    // FIXME: Once https://github.com/apple/swift-syntax/pull/2226 is merged and
+    // FixItApplier is public, use it to compute the edits that should be
+    // applied to the source.
+    return nil
+  }
+
   /// Describe a fixit's edit briefly.
   ///
   /// For example, "Replace 'x' with 'y'", or "Remove 'z'".
@@ -161,7 +169,7 @@ extension Diagnostic {
       return nil
     }
 
-    var severity: DiagnosticSeverity? = nil
+    var severity: LanguageServerProtocol.DiagnosticSeverity? = nil
     if let uid: sourcekitd_uid_t = diag[keys.severity] {
       switch uid {
       case values.diag_error:
@@ -234,6 +242,51 @@ extension Diagnostic {
       codeActions: actions
     )
   }
+
+  init?(
+    _ diag: SwiftDiagnostics.Diagnostic,
+    in snapshot: DocumentSnapshot
+  ) {
+    guard let position = snapshot.position(of: diag.position) else {
+      logger.error(
+        """
+        Cannot construct Diagnostic from SwiftSyntax diagnostic because its UTF-8 offset \(diag.position.utf8Offset) \
+        is out of range of the source file \(snapshot.uri.forLogging)
+        """
+      )
+      return nil
+    }
+    // Start with a zero-length range based on the position.
+    // If the diagnostic has highlights associated with it that start at the
+    // position, use that as the diagnostic's range.
+    var range = Range(position)
+    for highlight in diag.highlights {
+      let swiftSyntaxRange = highlight.positionAfterSkippingLeadingTrivia..<highlight.endPositionBeforeTrailingTrivia
+      guard let highlightRange = snapshot.range(of: swiftSyntaxRange) else {
+        break
+      }
+      if range.upperBound == highlightRange.lowerBound {
+        range = range.lowerBound..<highlightRange.upperBound
+      } else {
+        break
+      }
+    }
+
+    let relatedInformation = diag.notes.compactMap { DiagnosticRelatedInformation($0, in: snapshot) }
+    let codeActions = diag.fixIts.compactMap { CodeAction($0, in: snapshot) }
+
+    self.init(
+      range: range,
+      severity: diag.diagMessage.severity.lspSeverity,
+      code: nil,
+      codeDescription: nil,
+      source: "SwiftSyntax",
+      message: diag.message,
+      tags: nil,
+      relatedInformation: relatedInformation,
+      codeActions: codeActions
+    )
+  }
 }
 
 extension DiagnosticRelatedInformation {
@@ -271,74 +324,31 @@ extension DiagnosticRelatedInformation {
       codeActions: actions
     )
   }
-}
-
-extension Diagnostic {
-  func withRange(_ newRange: Range<Position>) -> Diagnostic {
-    var updated = self
-    updated.range = newRange
-    return updated
-  }
-}
-
-struct CachedDiagnostic {
-  var diagnostic: Diagnostic
-  var stage: DiagnosticStage
-
-  func withRange(_ newRange: Range<Position>) -> CachedDiagnostic {
-    return CachedDiagnostic(
-      diagnostic: self.diagnostic.withRange(newRange),
-      stage: self.stage
-    )
-  }
-}
 
-extension CachedDiagnostic {
-  init?(
-    _ diag: SKDResponseDictionary,
-    in snapshot: DocumentSnapshot,
-    useEducationalNoteAsCode: Bool
-  ) {
-    let sk = diag.sourcekitd
-    guard
-      let diagnostic = Diagnostic(
-        diag,
-        in: snapshot,
-        useEducationalNoteAsCode: useEducationalNoteAsCode
+  init?(_ note: Note, in snapshot: DocumentSnapshot) {
+    let nodeRange = note.node.positionAfterSkippingLeadingTrivia..<note.node.endPositionBeforeTrailingTrivia
+    guard let range = snapshot.range(of: nodeRange) else {
+      logger.error(
+        """
+        Cannot construct DiagnosticRelatedInformation because the range \(nodeRange, privacy: .public) \
+        is out of range of the source file \(snapshot.uri.forLogging).
+        """
       )
-    else {
       return nil
     }
-    self.diagnostic = diagnostic
-    let stageUID: sourcekitd_uid_t? = diag[sk.keys.diagnostic_stage]
-    self.stage = stageUID.flatMap { DiagnosticStage($0, sourcekitd: sk) } ?? .parse
+    self.init(
+      location: Location(uri: snapshot.uri, range: range),
+      message: note.message
+    )
   }
 }
 
-/// Returns the new diagnostics after merging in any existing diagnostics from a higher diagnostic
-/// stage that should not be cleared yet.
-///
-/// Sourcekitd returns parse diagnostics immediately after edits, but we do not want to clear the
-/// semantic diagnostics until we have semantic level diagnostics from after the edit.
-///
-/// However, if fallback arguments are being used, we withhold semantic diagnostics in favor of only
-/// emitting syntactic diagnostics.
-func mergeDiagnostics(
-  old: [CachedDiagnostic],
-  new: [CachedDiagnostic],
-  stage: DiagnosticStage,
-  isFallback: Bool
-) -> [CachedDiagnostic] {
-  if stage == .sema {
-    return isFallback ? old.filter { $0.stage == .parse } : new
-  }
-
-  #if DEBUG
-  if let sema = new.first(where: { $0.stage == .sema }) {
-    logger.fault("unexpected semantic diagnostic in parse diagnostics \(String(reflecting: sema.diagnostic))")
+extension Diagnostic {
+  func withRange(_ newRange: Range<Position>) -> Diagnostic {
+    var updated = self
+    updated.range = newRange
+    return updated
   }
-  #endif
-  return new.filter { $0.stage == .parse } + old.filter { $0.stage == .sema }
 }
 
 /// Whether a diagostic is semantic or syntatic (parse).
@@ -361,3 +371,14 @@ extension DiagnosticStage {
     }
   }
 }
+
+fileprivate extension SwiftDiagnostics.DiagnosticSeverity {
+  var lspSeverity: LanguageServerProtocol.DiagnosticSeverity {
+    switch self {
+    case .error: return .error
+    case .warning: return .warning
+    case .note: return .information
+    case .remark: return .hint
+    }
+  }
+}