From 4086874cf314bd14bf760bc3acedfec2fe0338d5 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Wed, 18 Oct 2023 13:48:23 -0700 Subject: [PATCH] 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. --- Package.swift | 2 + Sources/LSPTestSupport/Assertions.swift | 2 +- .../TestSourceKitLSPClient.swift | 12 +- Sources/SourceKitLSP/CMakeLists.txt | 6 +- .../SourceKitServer+Options.swift | 12 +- Sources/SourceKitLSP/SourceKitServer.swift | 2 +- Sources/SourceKitLSP/Swift/Diagnostic.swift | 143 ++++--- .../Swift/SwiftLanguageServer.swift | 275 ++++++------- .../SourceKitDTests/CrashRecoveryTests.swift | 4 +- .../SourceKitLSPTests/BuildSystemTests.swift | 39 +- Tests/SourceKitLSPTests/CodeActionTests.swift | 12 +- Tests/SourceKitLSPTests/LocalClangTests.swift | 3 +- Tests/SourceKitLSPTests/LocalSwiftTests.swift | 363 ++++++++---------- .../PublishDiagnosticsTests.swift | 58 +-- Tests/SourceKitLSPTests/SourceKitTests.swift | 18 +- 15 files changed, 439 insertions(+), 512 deletions(-) diff --git a/Package.swift b/Package.swift index a14868d07..12e5e1186 100644 --- a/Package.swift +++ b/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"), ], diff --git a/Sources/LSPTestSupport/Assertions.swift b/Sources/LSPTestSupport/Assertions.swift index 66b310104..d71a41f5f 100644 --- a/Sources/LSPTestSupport/Assertions.swift +++ b/Sources/LSPTestSupport/Assertions.swift @@ -42,7 +42,7 @@ public func assertThrowsError( _ message: @autoclosure () -> String = "", file: StaticString = #filePath, line: UInt = #line, - _ errorHandler: (_ error: Error) -> Void = { _ in } + errorHandler: (_ error: Error) -> Void = { _ in } ) async { let didThrow: Bool do { diff --git a/Sources/SKTestSupport/TestSourceKitLSPClient.swift b/Sources/SKTestSupport/TestSourceKitLSPClient.swift index cbeb6e0e4..3b579fe5b 100644 --- a/Sources/SKTestSupport/TestSourceKitLSPClient.swift +++ b/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)) } diff --git a/Sources/SourceKitLSP/CMakeLists.txt b/Sources/SourceKitLSP/CMakeLists.txt index 27e2de3c2..a77931490 100644 --- a/Sources/SourceKitLSP/CMakeLists.txt +++ b/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 $<$>:FoundationXML>) diff --git a/Sources/SourceKitLSP/SourceKitServer+Options.swift b/Sources/SourceKitLSP/SourceKitServer+Options.swift index ec0c0bca6..c59e0496e 100644 --- a/Sources/SourceKitLSP/SourceKitServer+Options.swift +++ b/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 } } } diff --git a/Sources/SourceKitLSP/SourceKitServer.swift b/Sources/SourceKitLSP/SourceKitServer.swift index 846865aa3..f1e42db5f 100644 --- a/Sources/SourceKitLSP/SourceKitServer.swift +++ b/Sources/SourceKitLSP/SourceKitServer.swift @@ -643,7 +643,7 @@ extension SourceKitServer: MessageHandler { } private func _logResponse(_ result: LSPResult, id: RequestID, method: String) { - logger.debug( + logger.log( """ Sending response: Response<\(method, privacy: .public)(\(id, privacy: .public))>( diff --git a/Sources/SourceKitLSP/Swift/Diagnostic.swift b/Sources/SourceKitLSP/Swift/Diagnostic.swift index cb84f22ed..7495407de 100644 --- a/Sources/SourceKitLSP/Swift/Diagnostic.swift +++ b/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..) -> Diagnostic { - var updated = self - updated.range = newRange - return updated - } -} - -struct CachedDiagnostic { - var diagnostic: Diagnostic - var stage: DiagnosticStage - - func withRange(_ newRange: Range) -> 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.. [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) -> 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 + } + } +} diff --git a/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift b/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift index 8ff07474a..c500a0371 100644 --- a/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift +++ b/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift @@ -18,6 +18,7 @@ import SKCore import SKSupport import SourceKitD import SwiftParser +import SwiftParserDiagnostics import SwiftSyntax #if os(Windows) @@ -106,7 +107,13 @@ public actor SwiftLanguageServer: ToolchainLanguageServer { // FIXME: ideally we wouldn't need separate management from a parent server in the same process. var documentManager: DocumentManager - var currentDiagnostics: [DocumentURI: [CachedDiagnostic]] = [:] + /// For each edited document, the last task that was triggered to send a `PublishDiagnosticsNotification`. + /// + /// This is used to cancel previous publish diagnostics tasks if an edit is made to a document. + /// + /// - Note: We only clear entries from the dictionary when a document is closed. The task that the document maps to + /// might have finished. This isn't an issue since the tasks do not retain `self`. + private var inFlightPublishDiagnosticsTasks: [DocumentURI: Task] = [:] let syntaxTreeManager = SyntaxTreeManager() let semanticTokensManager = SemanticTokensManager() @@ -219,92 +226,10 @@ public actor SwiftLanguageServer: ToolchainLanguageServer { } } - /// Shift the ranges of all current diagnostics in the document with the given `uri` to account for `edit`. - private func adjustDiagnosticRanges(of uri: DocumentURI, for edit: TextDocumentContentChangeEvent) { - guard let rangeAdjuster = RangeAdjuster(edit: edit) else { - return - } - currentDiagnostics[uri] = currentDiagnostics[uri]?.compactMap({ cachedDiag in - if let adjustedRange = rangeAdjuster.adjust(cachedDiag.diagnostic.range) { - return cachedDiag.withRange(adjustedRange) - } else { - return nil - } - }) - } - - /// Register the diagnostics returned from sourcekitd in `currentDiagnostics` - /// and returns the corresponding LSP diagnostics. - /// - /// If `isFromFallbackBuildSettings` is `true`, then only parse diagnostics are - /// stored and any semantic diagnostics are ignored since they are probably - /// incorrect in the absence of build settings. - private func registerDiagnostics( - sourcekitdDiagnostics: SKDResponseArray?, - snapshot: DocumentSnapshot, - stage: DiagnosticStage, - isFromFallbackBuildSettings: Bool - ) -> [Diagnostic] { - let supportsCodeDescription = capabilityRegistry.clientHasDiagnosticsCodeDescriptionSupport - - var newDiags: [CachedDiagnostic] = [] - sourcekitdDiagnostics?.forEach { _, diag in - if let diag = CachedDiagnostic(diag, in: snapshot, useEducationalNoteAsCode: supportsCodeDescription) { - newDiags.append(diag) - } - return true - } - - let result = mergeDiagnostics( - old: currentDiagnostics[snapshot.uri] ?? [], - new: newDiags, - stage: stage, - isFallback: isFromFallbackBuildSettings - ) - currentDiagnostics[snapshot.uri] = result - - return result.map(\.diagnostic) - - } - - /// Publish diagnostics for the given `snapshot`. We withhold semantic diagnostics if we are using - /// fallback arguments. - func publishDiagnostics( - response: SKDResponseDictionary, - for snapshot: DocumentSnapshot, - compileCommand: SwiftCompileCommand? - ) async { - let documentUri = snapshot.uri - guard diagnosticsEnabled(for: documentUri) else { - logger.debug("Ignoring diagnostics for blacklisted file \(documentUri.forLogging)") - return - } - - let stageUID: sourcekitd_uid_t? = response[sourcekitd.keys.diagnostic_stage] - let stage = stageUID.flatMap { DiagnosticStage($0, sourcekitd: sourcekitd) } ?? .sema - - let diagnostics = registerDiagnostics( - sourcekitdDiagnostics: response[keys.diagnostics], - snapshot: snapshot, - stage: stage, - isFromFallbackBuildSettings: compileCommand?.isFallback ?? true - ) - - await sourceKitServer?.sendNotificationToClient( - PublishDiagnosticsNotification( - uri: documentUri, - version: snapshot.version, - diagnostics: diagnostics - ) - ) - } - func handleDocumentUpdate(uri: DocumentURI) async { guard let snapshot = documentManager.latestSnapshot(uri) else { return } - let compileCommand = await self.buildSettings(for: uri) - // Make the magic 0,0 replacetext request to update diagnostics and semantic tokens. let req = SKDRequestDictionary(sourcekitd: sourcekitd) @@ -320,9 +245,6 @@ public actor SwiftLanguageServer: ToolchainLanguageServer { // Only update semantic tokens if the 0,0 replacetext request returned semantic information. await semanticTokensManager.setSemanticTokens(for: snapshot.id, semanticTokens: semanticTokens) } - if enablePublishDiagnostics { - await publishDiagnostics(response: dict, for: snapshot, compileCommand: compileCommand) - } if isSemaStage { await requestTokensRefresh() } @@ -404,6 +326,8 @@ extension SwiftLanguageServer { // MARK: - Build System Integration private func reopenDocument(_ snapshot: DocumentSnapshot, _ compileCmd: SwiftCompileCommand?) async { + cancelInFlightPublishDiagnosticsTask(for: snapshot.uri) + let keys = self.keys let path = snapshot.uri.pseudoPath @@ -420,15 +344,9 @@ extension SwiftLanguageServer { openReq[keys.compilerargs] = compileCmd.compilerArgs } - guard let dict = try? self.sourcekitd.sendSync(openReq) else { - // Already logged failure. - return - } - await self.publishDiagnostics( - response: dict, - for: snapshot, - compileCommand: compileCmd - ) + _ = try? self.sourcekitd.sendSync(openReq) + + publishDiagnosticsIfNeeded(for: snapshot.uri) } public func documentUpdatedBuildSettings(_ uri: DocumentURI) async { @@ -455,6 +373,8 @@ extension SwiftLanguageServer { // MARK: - Text synchronization public func openDocument(_ note: DidOpenTextDocumentNotification) async { + cancelInFlightPublishDiagnosticsTask(for: note.textDocument.uri) + let keys = self.keys guard let snapshot = self.documentManager.open(note) else { @@ -473,14 +393,14 @@ extension SwiftLanguageServer { req[keys.compilerargs] = compilerArgs } - guard let dict = try? self.sourcekitd.sendSync(req) else { - // Already logged failure. - return - } - await self.publishDiagnostics(response: dict, for: snapshot, compileCommand: compileCommand) + _ = try? self.sourcekitd.sendSync(req) + publishDiagnosticsIfNeeded(for: note.textDocument.uri) } public func closeDocument(_ note: DidCloseTextDocumentNotification) async { + cancelInFlightPublishDiagnosticsTask(for: note.textDocument.uri) + inFlightPublishDiagnosticsTasks[note.textDocument.uri] = nil + let keys = self.keys self.documentManager.close(note) @@ -491,20 +411,65 @@ extension SwiftLanguageServer { req[keys.request] = self.requests.editor_close req[keys.name] = uri.pseudoPath - // Clear settings that should not be cached for closed documents. - self.currentDiagnostics[uri] = nil - _ = try? self.sourcekitd.sendSync(req) await semanticTokensManager.discardSemanticTokens(for: note.textDocument.uri) } + /// Cancels any in-flight tasks to send a `PublishedDiagnosticsNotification` after edits. + private func cancelInFlightPublishDiagnosticsTask(for document: DocumentURI) { + if let inFlightTask = inFlightPublishDiagnosticsTasks[document] { + inFlightTask.cancel() + } + } + + /// If the client doesn't support pull diagnostics, compute diagnostics for the latest version of the given document + /// and send a `PublishDiagnosticsNotification` to the client for it. + private func publishDiagnosticsIfNeeded(for document: DocumentURI) { + guard enablePublishDiagnostics else { + return + } + guard diagnosticsEnabled(for: document) else { + return + } + cancelInFlightPublishDiagnosticsTask(for: document) + inFlightPublishDiagnosticsTasks[document] = Task(priority: .medium) { [weak self] in + guard let self, let sourceKitServer = await self.sourceKitServer else { + logger.fault("Cannot produce PublishDiagnosticsNotification because sourceKitServer was deallocated") + return + } + do { + // Sleep for a little bit until triggering the diagnostic generation. This effectively de-bounces diagnostic + // generation since any later edit will cancel the previous in-flight task, which will thus never go on to send + // the `DocumentDiagnosticsRequest`. + try await Task.sleep(nanoseconds: UInt64(sourceKitServer.options.swiftPublishDiagnosticsDebounceDuration * 1_000_000_000)) + } catch { + return + } + do { + let diagnosticReport = try await self.fullDocumentDiagnosticReport(DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(document))) + + await sourceKitServer.sendNotificationToClient( + PublishDiagnosticsNotification( + uri: document, + diagnostics: diagnosticReport.items + ) + ) + } catch { + logger.fault(""" + Failed to get diagnostics + \(error.forLogging) + """) + } + } + } + public func changeDocument(_ note: DidChangeTextDocumentNotification) async { + cancelInFlightPublishDiagnosticsTask(for: note.textDocument.uri) + let keys = self.keys var edits: [IncrementalEdit] = [] - var lastResponse: SKDResponseDictionary? = nil - let editResult = self.documentManager.edit(note) { (before: LineTable, edit: TextDocumentContentChangeEvent) in let req = SKDRequestDictionary(sourcekitd: self.sourcekitd) @@ -533,9 +498,7 @@ extension SwiftLanguageServer { } req[keys.sourcetext] = edit.text - lastResponse = try? self.sourcekitd.sendSync(req) - - self.adjustDiagnosticRanges(of: note.textDocument.uri, for: edit) + _ = try? self.sourcekitd.sendSync(req) } guard let (preEditSnapshot, postEditSnapshot) = editResult else { return @@ -551,10 +514,7 @@ extension SwiftLanguageServer { edits: note.contentChanges ) - if let dict = lastResponse { - let compileCommand = await self.buildSettings(for: note.textDocument.uri) - await self.publishDiagnostics(response: dict, for: postEditSnapshot, compileCommand: compileCommand) - } + publishDiagnosticsIfNeeded(for: note.textDocument.uri) } public func willSaveDocument(_ note: WillSaveTextDocumentNotification) { @@ -1176,14 +1136,14 @@ extension SwiftLanguageServer { return codeActions } - func retrieveQuickFixCodeActions(_ params: CodeActionRequest) -> [CodeAction] { - guard let cachedDiags = currentDiagnostics[params.textDocument.uri] else { - return [] - } - - let codeActions = cachedDiags.flatMap { (cachedDiag) -> [CodeAction] in - let diag = cachedDiag.diagnostic + func retrieveQuickFixCodeActions(_ params: CodeActionRequest) async throws -> [CodeAction] { + let diagnosticReport = try await self.fullDocumentDiagnosticReport( + DocumentDiagnosticsRequest( + textDocument: params.textDocument + ) + ) + let codeActions = diagnosticReport.items.flatMap { (diag) -> [CodeAction] in let codeActions: [CodeAction] = (diag.codeActions ?? []) + (diag.relatedInformation?.flatMap { $0.codeActions ?? [] } ?? []) @@ -1255,10 +1215,41 @@ extension SwiftLanguageServer { return Array(hints) } + public func syntacticDiagnosticFromBuiltInSwiftSyntax( + for snapshot: DocumentSnapshot + ) async throws -> RelatedFullDocumentDiagnosticReport { + let syntaxTree = await syntaxTreeManager.syntaxTree(for: snapshot) + let swiftSyntaxDiagnostics = ParseDiagnosticsGenerator.diagnostics(for: syntaxTree) + let diagnostics = swiftSyntaxDiagnostics.compactMap { (diag) -> Diagnostic? in + if diag.diagnosticID == StaticTokenError.editorPlaceholder.diagnosticID { + // Ignore errors about editor placeholders in the source file, similar to how sourcekitd ignores them. + return nil + } + return Diagnostic(diag, in: snapshot) + } + return RelatedFullDocumentDiagnosticReport(items: diagnostics) + } + public func documentDiagnostic(_ req: DocumentDiagnosticsRequest) async throws -> DocumentDiagnosticReport { + return try await .full(fullDocumentDiagnosticReport(req)) + } + + private func fullDocumentDiagnosticReport(_ req: DocumentDiagnosticsRequest) async throws -> RelatedFullDocumentDiagnosticReport { guard let snapshot = documentManager.latestSnapshot(req.textDocument.uri) else { throw ResponseError.unknown("failed to find snapshot for url \(req.textDocument.uri.forLogging)") } + guard let buildSettings = await self.buildSettings(for: req.textDocument.uri), !buildSettings.isFallback else { + logger.log( + "Producing syntactic diagnostics from the built-in swift-syntax because we have fallback arguments" + ) + // If we don't have build settings or we only have fallback build settings, + // sourcekitd won't be able to give us accurate semantic diagnostics. + // Fall back to providing syntactic diagnostics from the built-in + // swift-syntax. That's the best we can do for now. + return try await syntacticDiagnosticFromBuiltInSwiftSyntax(for: snapshot) + } + + try Task.checkCancellation() let keys = self.keys @@ -1267,23 +1258,28 @@ extension SwiftLanguageServer { skreq[keys.sourcefile] = snapshot.uri.pseudoPath // FIXME: SourceKit should probably cache this for us. - let areFallbackBuildSettings: Bool - if let buildSettings = await self.buildSettings(for: req.textDocument.uri) { - skreq[keys.compilerargs] = buildSettings.compilerArgs - areFallbackBuildSettings = buildSettings.isFallback - } else { - areFallbackBuildSettings = true - } + skreq[keys.compilerargs] = buildSettings.compilerArgs let dict = try await self.sourcekitd.send(skreq) - let diagnostics = self.registerDiagnostics( - sourcekitdDiagnostics: dict[keys.diagnostics], - snapshot: snapshot, - stage: .sema, - isFromFallbackBuildSettings: areFallbackBuildSettings - ) - return .full(RelatedFullDocumentDiagnosticReport(items: diagnostics)) + try Task.checkCancellation() + guard documentManager.latestSnapshot(req.textDocument.uri)?.id == snapshot.id else { + // Check that the document wasn't modified while we were getting diagnostics. This could happen because we are + // calling `fullDocumentDiagnosticReport` from `publishDiagnosticsIfNeeded` outside of `messageHandlingQueue` + // and thus a concurrent edit is possible while we are waiting for the sourcekitd request to return a result. + throw ResponseError.unknown("Document was modified while loading document") + } + + let supportsCodeDescription = capabilityRegistry.clientHasDiagnosticsCodeDescriptionSupport + var diagnostics: [Diagnostic] = [] + dict[keys.diagnostics]?.forEach { _, diag in + if let diag = Diagnostic(diag, in: snapshot, useEducationalNoteAsCode: supportsCodeDescription) { + diagnostics.append(diag) + } + return true + } + + return RelatedFullDocumentDiagnosticReport(items: diagnostics) } public func executeCommand(_ req: ExecuteCommandRequest) async throws -> LSPAny? { @@ -1425,6 +1421,19 @@ extension DocumentSnapshot { } } + func position(of position: AbsolutePosition) -> Position? { + return positionOf(utf8Offset: position.utf8Offset) + } + + func range(of range: Range) -> Range? { + guard let lowerBound = self.position(of: range.lowerBound), + let upperBound = self.position(of: range.upperBound) + else { + return nil + } + return lowerBound.. String.Index? { return text.utf8.index(text.startIndex, offsetBy: utf8Offset, limitedBy: text.endIndex) } diff --git a/Tests/SourceKitDTests/CrashRecoveryTests.swift b/Tests/SourceKitDTests/CrashRecoveryTests.swift index 2e2213a68..bed71394c 100644 --- a/Tests/SourceKitDTests/CrashRecoveryTests.swift +++ b/Tests/SourceKitDTests/CrashRecoveryTests.swift @@ -55,9 +55,7 @@ final class CrashRecoveryTests: XCTestCase { try ws.openDocument(loc.url, language: .swift) - // Wait for syntactic and semantic diagnsotics to be produced to make sure the - // document open got handled by sourcekitd - _ = try await ws.testClient.nextDiagnosticsNotification() + // Wait for diagnostics to be produced to make sure the document open got handled by sourcekitd. _ = try await ws.testClient.nextDiagnosticsNotification() // Make a change to the file that's not saved to disk. This way we can check that we re-open the correct in-memory state. diff --git a/Tests/SourceKitLSPTests/BuildSystemTests.swift b/Tests/SourceKitLSPTests/BuildSystemTests.swift index aeb6ae08c..9fa010369 100644 --- a/Tests/SourceKitLSPTests/BuildSystemTests.swift +++ b/Tests/SourceKitLSPTests/BuildSystemTests.swift @@ -217,13 +217,10 @@ final class BuildSystemTests: XCTestCase { ) ) ) - let syntacticDiags1 = try await testClient.nextDiagnosticsNotification() - XCTAssertEqual(syntacticDiags1.diagnostics.count, 0) + let diags1 = try await testClient.nextDiagnosticsNotification() + XCTAssertEqual(diags1.diagnostics.count, 1) XCTAssertEqual(text, documentManager.latestSnapshot(doc)!.text) - let semanticDiags1 = try await testClient.nextDiagnosticsNotification() - XCTAssertEqual(semanticDiags1.diagnostics.count, 1) - // Modify the build settings and inform the delegate. // This should trigger a new publish diagnostics and we should no longer have errors. let newSettings = FileBuildSettings(compilerArguments: args + ["-DFOO"]) @@ -231,13 +228,9 @@ final class BuildSystemTests: XCTestCase { await buildSystem.delegate?.fileBuildSettingsChanged([doc]) - let syntacticDiags2 = try await testClient.nextDiagnosticsNotification() - // Semantic analysis - SourceKit currently caches diagnostics so we still see an error. - XCTAssertEqual(syntacticDiags2.diagnostics.count, 1) - - let semanticDiags2 = try await testClient.nextDiagnosticsNotification() - // Semantic analysis - no expected errors here because we fixed the settings. - XCTAssertEqual(semanticDiags2.diagnostics.count, 0) + // No expected errors here because we fixed the settings. + let diags2 = try await testClient.nextDiagnosticsNotification() + XCTAssertEqual(diags2.diagnostics.count, 0) } func testClangdDocumentFallbackWithholdsDiagnostics() async throws { @@ -319,26 +312,18 @@ final class BuildSystemTests: XCTestCase { ) ) ) - let openSyntacticDiags = try await testClient.nextDiagnosticsNotification() - // Syntactic analysis - one expected errors here (for `func`). - XCTAssertEqual(openSyntacticDiags.diagnostics.count, 1) + let openDiags = try await testClient.nextDiagnosticsNotification() + XCTAssertEqual(openDiags.diagnostics.count, 1) XCTAssertEqual(text, documentManager.latestSnapshot(doc)!.text) - let openSemanticDiags = try await testClient.nextDiagnosticsNotification() - // Should be the same syntactic analysis since we are using fallback arguments - XCTAssertEqual(openSemanticDiags.diagnostics.count, 1) // Swap from fallback settings to primary build system settings. buildSystem.buildSettingsByFile[doc] = primarySettings await buildSystem.delegate?.fileBuildSettingsChanged([doc]) - let refreshedSyntacticDiags = try await testClient.nextDiagnosticsNotification() - // Syntactic analysis with new args - one expected errors here (for `func`). - XCTAssertEqual(refreshedSyntacticDiags.diagnostics.count, 1) - - let refreshedSemanticDiags = try await testClient.nextDiagnosticsNotification() - // Semantic analysis - two errors since `-DFOO` was not passed. - XCTAssertEqual(refreshedSemanticDiags.diagnostics.count, 2) + // Two errors since `-DFOO` was not passed. + let refreshedDiags = try await testClient.nextDiagnosticsNotification() + XCTAssertEqual(refreshedDiags.diagnostics.count, 2) } func testMainFilesChanged() async throws { @@ -349,8 +334,8 @@ final class BuildSystemTests: XCTestCase { try ws.openDocument(unique_h.fileURL!, language: .cpp) - let openSyntacticDiags = try await testClient.nextDiagnosticsNotification() - XCTAssertEqual(openSyntacticDiags.diagnostics.count, 0) + let openDiags = try await testClient.nextDiagnosticsNotification() + XCTAssertEqual(openDiags.diagnostics.count, 0) try ws.buildAndIndex() let diagsFromD = try await testClient.nextDiagnosticsNotification() diff --git a/Tests/SourceKitLSPTests/CodeActionTests.swift b/Tests/SourceKitLSPTests/CodeActionTests.swift index 55f12dd5e..d8aa66393 100644 --- a/Tests/SourceKitLSPTests/CodeActionTests.swift +++ b/Tests/SourceKitLSPTests/CodeActionTests.swift @@ -294,18 +294,14 @@ final class CodeActionTests: XCTestCase { try ws.openDocument(def.url, language: .swift) - let syntacticDiags = try await ws.testClient.nextDiagnosticsNotification() - XCTAssertEqual(syntacticDiags.uri, def.docUri) - XCTAssertEqual(syntacticDiags.diagnostics, []) - - let semanticDiags = try await ws.testClient.nextDiagnosticsNotification() - XCTAssertEqual(semanticDiags.uri, def.docUri) - XCTAssertEqual(semanticDiags.diagnostics.count, 1) + let diags = try await ws.testClient.nextDiagnosticsNotification() + XCTAssertEqual(diags.uri, def.docUri) + XCTAssertEqual(diags.diagnostics.count, 1) let textDocument = TextDocumentIdentifier(def.url) let actionsRequest = CodeActionRequest( range: def.position..