Remove document manager in `SwiftLanguageService`

[louisunlimited]

• Fixes: #1423

[louisunlimited]

@ahoppen I found a lot of places where we reference documentManager directly, either it be within SwiftLanguageService or it's extensions elsewhere, so I ended up adding it as a computed property.

/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Sources/SourceKitLSP/DocumentManager.swift:98:27: warning: stored property 'documents' of 'Sendable'-conforming class 'DocumentManager' is mutable
  nonisolated(unsafe) var documents: [DocumentURI: Document] = [:]
                          ^
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Sources/SourceKitLSP/SourceKitLSPServer.swift:44:35: warning: stored property 'replied' of 'Sendable'-conforming generic class 'RequestAndReply' is mutable
  private nonisolated(unsafe) var replied: AtomicBool = AtomicBool(initialValue: false)
                                  ^
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Sources/SourceKitLSP/SourceKitLSPServer.swift:218:31: warning: cannot access property 'indexProgressManager' here in non-isolated initializer; this is an error in Swift 6
    self.indexProgressManager = IndexProgressManager(sourceKitLSPServer: self)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Sources/SourceKitLSP/SourceKitLSPServer.swift:218:74: note: after making a copy of 'self', only non-isolated properties of 'self' can be accessed from this init
    self.indexProgressManager = IndexProgressManager(sourceKitLSPServer: self)
                                                                         ^~~~
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift:508:9: warning: no 'async' operations occur within 'await' expression
        await sourceKitLSPServer.sendNotificationToClient(
        ^
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift:740:8: warning: converting non-sendable function value to '@Sendable (CodeActionRequest) async throws -> [CodeAction]' may introduce data races
      (retrieveSyntaxCodeActions, nil),
       ^
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift:741:8: warning: converting non-sendable function value to '@Sendable (CodeActionRequest) async throws -> [CodeAction]' may introduce data races
      (retrieveRefactorCodeActions, .refactor),
       ^
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift:742:8: warning: converting non-sendable function value to '@Sendable (CodeActionRequest) async throws -> [CodeAction]' may introduce data races
      (retrieveQuickFixCodeActions, .quickFix),
       ^
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Tests/SourceKitLSPTests/ExpectedIndexTaskTracker.swift:85:20: warning: cannot access property 'testHooks' here in non-isolated initializer; this is an error in Swift 6
    self.testHooks = IndexTestHooks(
    ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
/Users/louissssss/Desktop/CS_Projects/sourcekit-lsp/Tests/SourceKitLSPTests/ExpectedIndexTaskTracker.swift:86:40: note: after making a copy of 'self', only non-isolated properties of 'self' can be accessed from this init
      preparationTaskDidStart: { [weak self] in

Doing so introduces the above new warnings, I'm currently trying to figure out how to resolve these. Also some tests are failing, trying to address those as well.

[louisunlimited]

Hey, sorry if this is taking a while, never had a good chance to sit down and dig into this. I'm still experiencing the same issues locally when I run swift test. All the notification tests are timing out for some reason that I don't quite understand yet. Also the warnings are still there. I probably need some help on this to pin point the issue if you have time

[ahoppen]

Ah, looks like a merge conflict has crept up. Could you rebase your PR onto main? Also, could you squash your commits int a single one. It makes for a nicer Git history. https://github.com/apple/sourcekit-lsp/blob/main/CONTRIBUTING.md#authoring-commits

[louisunlimited]

Ah, looks like a merge conflict has crept up. Could you rebase your PR onto main? Also, could you squash your commits int a single one. It makes for a nicer Git history. https://github.com/apple/sourcekit-lsp/blob/main/CONTRIBUTING.md#authoring-commits

Ah shoot I just did merge instead of rebase. I can redo that

[louisunlimited]

@ahoppen Just made these changes and squashed my commits, I'm not no my personal computer now so I can't test till later today, let me now if you need anything else! Thanks

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows

[louisunlimited]

Yeah these failed test are something I probably need some help with

[ahoppen]

The issue was that SourceKitLSPServer opened the document in the document manager and then SwiftLanguageService tried opening the document in the same document manager again, which failed. Thus snapshot in openDocument was nil, we never opened the document in sourcekitd and thus couldn’t provide any semantic functionality for the document. I noticed this by looking at the logs generated during test execution and saw an error being logged that said failed to open document: alreadyOpen(…

The solution is that only SourceKitLSPServer should be responsible for updating the document manager state.

The following diff fixes the issue and also does a few clean-up, inlining methods that are now only called from one location.

Diff

```diff diff --git a/Sources/SourceKitLSP/Clang/ClangLanguageService.swift b/Sources/SourceKitLSP/Clang/ClangLanguageService.swift index 0fcd6b61..9d2f5faf 100644 --- a/Sources/SourceKitLSP/Clang/ClangLanguageService.swift +++ b/Sources/SourceKitLSP/Clang/ClangLanguageService.swift @@ -17,6 +17,7 @@ import LanguageServerProtocolJSONRPC import SKCore import SKSupport import SwiftExtensions +import SwiftSyntax import struct TSCBasic.AbsolutePath @@ -443,7 +444,7 @@ extension ClangLanguageService { // MARK: - Text synchronization - public func openDocument(_ notification: DidOpenTextDocumentNotification) async { + public func openDocument(_ notification: DidOpenTextDocumentNotification, snapshot: DocumentSnapshot) async { openDocuments[notification.textDocument.uri] = notification.textDocument.language // Send clangd the build settings for the new file. We need to do this before // sending the open notification, so that the initial diagnostics already @@ -459,7 +460,12 @@ extension ClangLanguageService { func reopenDocument(_ notification: ReopenTextDocumentNotification) {} - public func changeDocument(_ notification: DidChangeTextDocumentNotification) { + public func changeDocument( + _ notification: DidChangeTextDocumentNotification, + preEditSnapshot: DocumentSnapshot, + postEditSnapshot: DocumentSnapshot, + edits: [SourceEdit] + ) { clangd.send(notification) } diff --git a/Sources/SourceKitLSP/DocumentManager.swift b/Sources/SourceKitLSP/DocumentManager.swift index 97cfdc62..3d0aaa61 100644 --- a/Sources/SourceKitLSP/DocumentManager.swift +++ b/Sources/SourceKitLSP/DocumentManager.swift @@ -204,42 +204,6 @@ public final class DocumentManager: InMemoryDocumentManager, Sendable { } } -extension DocumentManager { - - // MARK: - LSP notification handling - - /// Convenience wrapper for `open(_:language:version:text:)` that logs on failure. - @discardableResult - func open(_ notification: DidOpenTextDocumentNotification) -> DocumentSnapshot? { - let doc = notification.textDocument - return orLog("failed to open document", level: .error) { - try open(doc.uri, language: doc.language, version: doc.version, text: doc.text) - } - } - - /// Convenience wrapper for `close(_:)` that logs on failure. - func close(_ notification: DidCloseTextDocumentNotification) { - orLog("failed to close document", level: .error) { - try close(notification.textDocument.uri) - } - } - - /// Convenience wrapper for `edit(_:newVersion:edits:updateDocumentTokens:)` - /// that logs on failure. - @discardableResult - func edit( - _ notification: DidChangeTextDocumentNotification - ) -> (preEditSnapshot: DocumentSnapshot, postEditSnapshot: DocumentSnapshot, edits: [SourceEdit])? { - return orLog("failed to edit document", level: .error) { - return try edit( - notification.textDocument.uri, - newVersion: notification.textDocument.version, - edits: notification.contentChanges - ) - } - } -} - fileprivate extension SourceEdit { /// Constructs a `SourceEdit` from the given `TextDocumentContentChangeEvent`. /// diff --git a/Sources/SourceKitLSP/LanguageService.swift b/Sources/SourceKitLSP/LanguageService.swift index 1b2429e3..638c0d0a 100644 --- a/Sources/SourceKitLSP/LanguageService.swift +++ b/Sources/SourceKitLSP/LanguageService.swift @@ -13,6 +13,7 @@ import Foundation import LanguageServerProtocol import SKCore +import SwiftSyntax /// The state of a `ToolchainLanguageServer` public enum LanguageServerState { @@ -113,9 +114,9 @@ public protocol LanguageService: AnyObject, Sendable { // MARK: - Text synchronization /// Sent to open up a document on the Language Server. - /// This may be called before or after a corresponding - /// `documentUpdatedBuildSettings` call for the same document. - func openDocument(_ notification: DidOpenTextDocumentNotification) async + /// + /// This may be called before or after a corresponding `documentUpdatedBuildSettings` call for the same document. + func openDocument(_ notification: DidOpenTextDocumentNotification, snapshot: DocumentSnapshot) async /// Sent to close a document on the Language Server. func closeDocument(_ notification: DidCloseTextDocumentNotification) async @@ -127,7 +128,12 @@ public protocol LanguageService: AnyObject, Sendable { /// Only intended for `SwiftLanguageService`. func reopenDocument(_ notification: ReopenTextDocumentNotification) async - func changeDocument(_ notification: DidChangeTextDocumentNotification) async + func changeDocument( + _ notification: DidChangeTextDocumentNotification, + preEditSnapshot: DocumentSnapshot, + postEditSnapshot: DocumentSnapshot, + edits: [SourceEdit] + ) async func willSaveDocument(_ notification: WillSaveTextDocumentNotification) async func didSaveDocument(_ notification: DidSaveTextDocumentNotification) async diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index fe41305f..ab176ebe 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -1251,7 +1251,18 @@ extension SourceKitLSPServer { private func openDocument(_ notification: DidOpenTextDocumentNotification, workspace: Workspace) async { // Immediately open the document even if the build system isn't ready. This is important since // we check that the document is open when we receive messages from the build system. - documentManager.open(notification) + let snapshot = orLog("Opening document") { + try documentManager.open( + notification.textDocument.uri, + language: notification.textDocument.language, + version: notification.textDocument.version, + text: notification.textDocument.text + ) + } + guard let snapshot else { + // Already logged failure + return + } let textDocument = notification.textDocument let uri = textDocument.uri @@ -1265,7 +1276,7 @@ extension SourceKitLSPServer { await workspace.buildSystemManager.registerForChangeNotifications(for: uri, language: language) // If the document is ready, we can immediately send the notification. - await service.openDocument(notification) + await service.openDocument(notification, snapshot: snapshot) } func closeDocument(_ notification: DidCloseTextDocumentNotification) async { @@ -1293,7 +1304,9 @@ extension SourceKitLSPServer { func closeDocument(_ notification: DidCloseTextDocumentNotification, workspace: Workspace) async { // Immediately close the document. We need to be sure to clear our pending work queue in case // the build system still isn't ready. - documentManager.close(notification) + orLog("failed to close document", level: .error) { + try documentManager.close(notification.textDocument.uri) + } let uri = notification.textDocument.uri @@ -1314,8 +1327,23 @@ extension SourceKitLSPServer { await workspace.semanticIndexManager?.schedulePreparationForEditorFunctionality(of: uri) // If the document is ready, we can handle the change right now. - documentManager.edit(notification) - await workspace.documentService.value[uri]?.changeDocument(notification) + let editResult = orLog("Editing document") { + try documentManager.edit( + notification.textDocument.uri, + newVersion: notification.textDocument.version, + edits: notification.contentChanges + ) + } + guard let (preEditSnapshot, postEditSnapshot, edits) = editResult else { + // Already logged failure + return + } + await workspace.documentService.value[uri]?.changeDocument( + notification, + preEditSnapshot: preEditSnapshot, + postEditSnapshot: postEditSnapshot, + edits: edits + ) } func willSaveDocument( diff --git a/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift index bf137e15..119e9840 100644 --- a/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift +++ b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift @@ -430,15 +430,10 @@ extension SwiftLanguageService { ]) } - public func openDocument(_ notification: DidOpenTextDocumentNotification) async { + public func openDocument(_ notification: DidOpenTextDocumentNotification, snapshot: DocumentSnapshot) async { cancelInFlightPublishDiagnosticsTask(for: notification.textDocument.uri) await diagnosticReportManager.removeItemsFromCache(with: notification.textDocument.uri) - guard let snapshot = orLog("Getting document manager", { try self.documentManager.open(notification) }) else { - // Already logged failure. - return - } - let buildSettings = await self.buildSettings(for: snapshot.uri) let req = openDocumentSourcekitdRequest(snapshot: snapshot, compileCommand: buildSettings) @@ -451,10 +446,6 @@ extension SwiftLanguageService { inFlightPublishDiagnosticsTasks[notification.textDocument.uri] = nil await diagnosticReportManager.removeItemsFromCache(with: notification.textDocument.uri) - orLog("Getting document manager") { - try self.documentManager.close(notification) - } - let req = closeDocumentSourcekitdRequest(uri: notification.textDocument.uri) _ = try? await self.sourcekitd.send(req, fileContents: nil) } @@ -537,7 +528,12 @@ extension SwiftLanguageService { } } - public func changeDocument(_ notification: DidChangeTextDocumentNotification) async { + public func changeDocument( + _ notification: DidChangeTextDocumentNotification, + preEditSnapshot: DocumentSnapshot, + postEditSnapshot: DocumentSnapshot, + edits: [SourceEdit] + ) async { cancelInFlightPublishDiagnosticsTask(for: notification.textDocument.uri) let keys = self.keys @@ -547,15 +543,6 @@ extension SwiftLanguageService { let replacement: String } - guard - let (preEditSnapshot, postEditSnapshot, edits) = orLog( - "Getting document manager", - { try self.documentManager.edit(notification) } - ) - else { - return - } - for edit in edits { let req = sourcekitd.dictionary([ keys.request: self.requests.editorReplaceText, @@ -1003,7 +990,6 @@ extension SwiftLanguageService: SKDNotificationHandler { } if notification.error == .connectionInterrupted { - self.state = .connectionInterrupted await self.setState(.connectionInterrupted) } } ```

[louisunlimited]

Oh thank you so much for this, I just did a quick fix but that introduced some other error; I'll look into this later tomorrow. Appreciate the help!

[ahoppen]

Let me know when it’s ready for review again.

[louisunlimited]

Hi @ahoppen, I just found out it's due to some of my dependencies being outdated, after I pulled all the external deps it builds fine. I was also able to run test and lint, let me know if CI somehow fails again and I can work on that tomorrow!

I was actually facing an error in SourceKitLSPTests.ExecuteCommandTests testFreestandingMacroExpansion saying that

 error: -[SourceKitLSPTests.ExecuteCommandTests testFreestandingMacroExpansion] : failed: caught error: "ResponseError(code: LanguageServerProtocol.ErrorCode(rawValue: -32001), message: "Unknown error: no edits reported for semantic refactoring action for url file:///private/var/folders/_9/4xjxjjbj0qg2tm33d82jh2qw0000gn/T/sourcekit-lsp-test-scratch/testFreestandingMacroExpansion-2D573E51/Sources/MyMacroClient/MyMacroClient.swift")"

sourcekit-lsp/Tests/SourceKitLSPTests/ExecuteCommandTests.swift:210: error: -[SourceKitLSPTests.ExecuteCommandTests testFreestandingMacroExpansion] : Failed due to unwaited expectation 'Handle Show Document Request'.

Interestingly this particular test also fails on a fresh clone of the repo's main branch so I wasn't sure if it's something related to my changes.

[ahoppen]

I was actually facing an error in SourceKitLSPTests.ExecuteCommandTests testFreestandingMacroExpansion

Thanks for letting me know. That test was missing a SkipUnless check and should have been skipped if you’re running tests using a Swift 5 toolchain. https://github.com/swiftlang/sourcekit-lsp/pull/1548

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows

[louisunlimited]

Uhm why did this CI fail when it says no identified problem. I'll rebase main and see what happens from there

[ahoppen]

CI failed with

+ /Users/ec2-user/jenkins/workspace/swift-sourcekit-lsp-PR-macOS/branch-main/build/buildbot_incremental/unified-swiftpm-build-macosx-x86_64/release/swift-format lint --parallel --strict --recursive /Users/ec2-user/jenkins/workspace/swift-sourcekit-lsp-PR-macOS/branch-main/sourcekit-lsp
branch-main/sourcekit-lsp/Sources/SourceKitLSP/LanguageService.swift:130:103: warning: [RemoveLine] remove line break
branch-main/sourcekit-lsp/Sources/SourceKitLSP/LanguageService.swift:131:1: warning: [TrailingWhitespace] remove trailing whitespace
branch-main/sourcekit-lsp/Sources/SourceKitLSP/LanguageService.swift:132:1: warning: [TrailingWhitespace] remove trailing whitespace

Could you run swift-format on your changes? https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#formatting

[louisunlimited]

Yeah that's my bad, some how forgot to commit that file. Should be updated now!

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows