Remove document manager in `SwiftLanguageService`

[ahoppen]

SwiftLanguageService has a reference to SourceKitLSPServer, so it should be able to use the DocumentManager from SourceKitLSPServer instead of maintaining its own.

[ahoppen]

Synced to Apple’s issue tracker as rdar://129223534

[louisunlimited]

Hi @ahoppen I'd like to work on this issue, do you mind assigning it to me and provide a bit more explanation for it?

[ahoppen]

Great to hear, @louisunlimited.

SwiftLanguageService has a documentManager property here

https://github.com/apple/sourcekit-lsp/blob/f825d9c57e4a20f85bd008b9c9a6d7bb41f1714b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift#L115-L116

And it’s keeping track of all the document contents separately from SourceKitLSPServer, which has its own DocumentManager. That poses a potential for the two getting out of sync and is wasting memory because we keep every document in memory twice (once in SourceKitLSPServer and once in SwiftLanguageService).

The goal would be to remove the documentManager property in SwiftLanguageService and where we need it, access the document manager from SourceKitLSPServer via sourceKitLSPServer.documentManager.

[louisunlimited]

Thanks, I'll take a look over the weekend and see what can be done :)

[ahoppen]

Great, thank you! Let me know if you have questions.

[louisunlimited]

Hi, since sourceKitLSPServer is a weak var that's marked optional, does that mean we'd have a optional guard let in every member function that requires documentManager? I'm assuming that sourceKitLSPServer owns SwiftLanguageService so it might be safe to guard let and log out errors?

[ahoppen]

Yes, we already have patterns like this where we unwrap sourceKitLSPServer in a few places. If you search for guard let sourceKitLSPServer else you should find a few of them.

If we have a few places where we need this, it might make sense to define documentManager as a computed property that throws an error if the sourceKitLSPServer is nil. That way we could avoid the guard let dance at every position that needs the document manager.

[louisunlimited]

Cool, I'll try that, thanks