Update request and notification definitions to LSP 3.17 #669
Conversation
|
@swift-ci Please test |
| @@ -431,7 +431,7 @@ extension ClangLanguageServerShim { | |||
| // with `forceRebuild` set in case any missing header files have been added. | |||
| // This works well for us as the moment since clangd ignores the document version. | |||
| let note = DidChangeTextDocumentNotification( | |||
| textDocument: VersionedTextDocumentIdentifier(uri, version: nil), | |||
| textDocument: VersionedTextDocumentIdentifier(uri, version: 0), | |||
There was a problem hiding this comment.
The version number is supposed to always increment, so reseting to 0 seems odd. Should we use OptionalVersioned... instead and keep nil?
There was a problem hiding this comment.
I was wondering about this one as well. DidChangeTextDocumentNotification is supposed to take VersionedTextDocumentIdentifier and not OptionalVersionedTextDocumentIdentifier but apparently clangd has been handling this just fine so far… So maybe you’re right that the cleanest solution is if we just relax our LSP definition here… I still don’t like it though…
There was a problem hiding this comment.
Well that's fun. In theory this means the 3.16+ spec is not backwards compatible with 3.15 and earlier for DidChangeTextDocument. But the documentation for null version is a bit vague in earlier specs and the authors say the intent is that in client to server communication it is never null (ie only allowed null for a server to client message), so what we were doing before is a bug I guess since we're the client in this scenario. I'm okay with using 0 since clangd handles it 🤷
There was a problem hiding this comment.
I ended up adding a test case: fa9b5f9. It appears to work fine.
| /// The server cancelled the request. This error code should | ||
| /// only be used for requests that explicitly support being | ||
| /// server cancellable. | ||
| public static let ServerCancelled = ErrorCode(rawValue: -32802) |
There was a problem hiding this comment.
We return cancelled in places that are not client-canceled, so we should probably update those. Doesn't have to be for this PR though.
Sources/LanguageServerProtocol/Requests/SelectionRangeRequest.swift
Outdated
Show resolved
Hide resolved
Sources/LanguageServerProtocol/Requests/SelectionRangeRequest.swift
Outdated
Show resolved
Hide resolved
Sources/LanguageServerProtocol/Requests/SignatureHelpRequest.swift
Outdated
Show resolved
Hide resolved
|
All the new request/notification types need to be added to the builtin message lists in Messages.swift or they won't decode correctly. |
ahoppen
force-pushed
the
ahopppen/update-lsp
branch
from
e6293e2 to
7a723d0
Compare
|
@swift-ci Please test |
| case end(WorkDoneProgressEnd) | ||
| } | ||
|
|
||
| public var state: State |
There was a problem hiding this comment.
Just curious: could WorkDoneProgress itself be the enum, or does that not work? I'm fine with it either way.
There was a problem hiding this comment.
Oh, good idea. I didn’t think of that.
ahoppen
force-pushed
the
ahopppen/update-lsp
branch
from
7a723d0 to
93a8f91
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
ahoppen
force-pushed
the
ahopppen/update-lsp
branch
from
2fe1928 to
cf65abd
Compare
…lang We weren’t sure if it works because it always sends 0 as the document’s version number, but everythig appears to be fine.
ahoppen
force-pushed
the
ahopppen/update-lsp
branch
from
cf65abd to
fa9b5f9
Compare
|
@swift-ci Please test |
This updates the definitions of the requests and notifications to match version 3.17 of the LSP spec
rdar://83833293