fix: handle empty capability options in plist mode#4941
Conversation
When LSP_USE_PLISTS is enabled, empty capability objects like
`DefinitionOptions {}` are parsed as `nil` by json-parse-string,
causing lsp--capability to incorrectly report the capability as
unsupported.
This fix uses lsp-member? to check if the capability key exists
in the server capabilities, and returns t if the key exists but
has an empty value. This ensures that servers returning empty
option objects (e.g., buf lsp's `DefinitionProvider: &protocol.DefinitionOptions{}`)
are correctly recognized as supporting the capability.
Add tests to verify lsp--capability correctly handles:
- Empty capability objects (key exists with nil value) returning t
- Capability keys with truthy values returning the actual value
- Capability keys with structured values returning the structure
- Non-existent capability keys returning nil
Tests work in both plist mode (LSP_USE_PLISTS) and hash-table mode,
ensuring the fix for empty DefinitionOptions {} is properly covered.
There was a problem hiding this comment.
Pull request overview
This PR fixes the lsp--capability function to correctly detect LSP server capabilities when using LSP_USE_PLISTS=true mode, where empty JSON objects {} are parsed as nil by Emacs's json-parse-string. The fix uses lsp-member? to check if a capability key exists before evaluating its value.
Key changes:
- Modified
lsp--capabilityto uselsp-member?to distinguish between missing capabilities and capabilities with empty/nil values - Added comprehensive unit tests covering empty objects, boolean values, structured values, and non-existent capabilities
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lsp-mode.el | Updated lsp--capability function to check for key existence using lsp-member? and return t for empty-valued capabilities |
| test/lsp-mode-test.el | Added two new test functions to verify correct behavior for various capability states in both plist and hash-table modes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (ert-deftest lsp--capability-test () | ||
| "Test lsp--capability returns correct values for various capability states. | ||
| This tests the fix for empty capability objects like DefinitionOptions {}." | ||
| ;; Create capabilities structure that works in both plist and hash-table modes | ||
| (let ((capabilities (if lsp-use-plists | ||
| ;; plist mode: empty objects are parsed as nil | ||
| (list :definitionProvider nil | ||
| :hoverProvider t | ||
| :completionProvider (list :triggerCharacters ["." ":"])) | ||
| ;; hash-table mode | ||
| (let ((ht (make-hash-table :test 'equal))) | ||
| (puthash "definitionProvider" nil ht) | ||
| (puthash "hoverProvider" t ht) | ||
| (let ((completion-ht (make-hash-table :test 'equal))) | ||
| (puthash "triggerCharacters" ["." ":"] completion-ht) | ||
| (puthash "completionProvider" completion-ht ht)) | ||
| ht)))) | ||
| ;; Test 1: Capability exists with nil value (empty object like DefinitionOptions {}) | ||
| ;; Should return t, not nil | ||
| (should (eq t (lsp--capability :definitionProvider capabilities))) | ||
|
|
||
| ;; Test 2: Capability exists with truthy value | ||
| ;; Should return the actual value | ||
| (should (eq t (lsp--capability :hoverProvider capabilities))) | ||
|
|
||
| ;; Test 3: Capability exists with a structured value | ||
| ;; Should return the actual value | ||
| (let ((completion-cap (lsp--capability :completionProvider capabilities))) | ||
| (should completion-cap) | ||
| (should-not (eq t completion-cap))) | ||
|
|
||
| ;; Test 4: Capability does not exist | ||
| ;; Should return nil | ||
| (should-not (lsp--capability :nonExistentProvider capabilities)))) |
There was a problem hiding this comment.
The test suite is missing a critical test case for when a server explicitly returns false for a capability (which would be represented as :json-false in Emacs). This is different from an empty object and should return nil to indicate the capability is not supported. Add a test case where a capability is set to :json-false and verify that lsp--capability returns nil.
| "Get the value of capability CAP. If CAPABILITIES is non-nil, use them instead. | ||
| Returns the capability value, or t if the capability exists but has an empty value | ||
| \(e.g., an empty DefinitionOptions object)." |
There was a problem hiding this comment.
The documentation says the function returns t for capabilities with empty values, but doesn't mention the behavior for :json-false (explicit false). The documentation should clarify that :json-false values should return nil because they indicate the capability is explicitly not supported by the server.
| (let ((caps (or capabilities (lsp--server-capabilities)))) | ||
| (when (lsp-member? caps cap) | ||
| (or (lsp-get caps cap) t)))) |
There was a problem hiding this comment.
The implementation incorrectly treats :json-false (explicit false from server) the same as nil (empty object). According to LSP spec, when a server sends "definitionProvider": false, it explicitly indicates the capability is NOT supported. The current logic would return t for this case because lsp-member? returns true and (or :json-false t) evaluates to t. This should be changed to distinguish between nil (empty object) and :json-false (explicit false).
| (let ((caps (or capabilities (lsp--server-capabilities)))) | |
| (when (lsp-member? caps cap) | |
| (or (lsp-get caps cap) t)))) | |
| (let* ((caps (or capabilities (lsp--server-capabilities))) | |
| (val (and (lsp-member? caps cap) | |
| (lsp-get caps cap)))) | |
| (cond | |
| ;; Explicit JSON false: capability is explicitly not supported. | |
| ((eq val :json-false) nil) | |
| ;; Capability key exists but has an empty object / nil value: treat as supported. | |
| ((null val) (and (lsp-member? caps cap) t)) | |
| ;; Non-nil, non-:json-false value: return the capability value itself. | |
| (t val)))) |
| (lsp-get (or capabilities | ||
| (lsp--server-capabilities)) | ||
| cap)) | ||
| (let ((caps (or capabilities (lsp--server-capabilities)))) |
There was a problem hiding this comment.
Can we rewrite this as
(let ((caps (or capabilities (lsp--server-capabilities))))
(or (lsp-get caps cap)
(lsp-member? caps cap)))So, no need to double check for lsp-member?
Use `(or (lsp-get caps cap) (lsp-member? caps cap))` instead of `(when (lsp-member? caps cap) (or (lsp-get caps cap) t))` to avoid redundant lsp-member? check. Suggested by @kiennq in PR review.
|
@suzuki, thanks for your changes, I hope it would resolve a large array of bugs we have regarding to deserialize json in plist. Do you think it's possible to properly check for empty list using combination of |
In plist mode, lsp-member? returns a list (from plist-member), not t. Changed test assertions from (eq t ...) to (should ...) to work correctly in both plist and hash-table modes.
Add lsp-null? function and use :json-null as the null marker in plist
mode. This allows lsp--capability to correctly distinguish between:
- {} (empty object) -> capability supported
- null -> capability explicitly not supported
- key not present -> capability not supported
Changes:
- Add lsp--json-null constant and lsp-null? function in lsp-protocol.el
- Change :null-object from nil to :json-null in JSON parsing (plist mode)
- Update lsp--capability to check for explicit null values
- Add tests for lsp-null? and explicit null capability handling
This complements the lsp-member? fix for empty objects by also handling
the case where a server explicitly sets a capability to null.
In hash-table mode, nil represents JSON null, not an empty object. Empty objects should be represented as empty hash-tables.
|
@kiennq Sorry for the late reply. I've introduced |
Summary
Fix
lsp--capabilityto correctly detect capabilities whenLSP_USE_PLISTSis enabled and the server returns empty option objects like"definitionProvider": {}.Problem
When
LSP_USE_PLISTS=true, Emacs'sjson-parse-stringparses empty JSON objects{}asnil:This causes
lsp--capabilityto incorrectly report that the capability is not supported, because it only checks if the value is truthy.Affected servers
Any LSP server that returns empty option objects, for example:
buf lsp serve) returns:{ "definitionProvider": {}, "typeDefinitionProvider": {}, "referencesProvider": {} }With the current implementation, "Go to Definition", "Find References", etc. are broken when using these servers with
LSP_USE_PLISTS=true.Solution
Use
lsp-member?to check if the capability key exists in the server capabilities, rather than just checking if the value is truthy:This correctly distinguishes between:
{}→nilin plist) → capability supported ✓Test plan
lsp--capabilitycovering:completionProviderwith options)