Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tolerate other keys in textDocument/documentSymbol #138

Closed
wants to merge 1 commit into from
Closed

Tolerate other keys in textDocument/documentSymbol #138

wants to merge 1 commit into from

Conversation

meqif
Copy link
Contributor

@meqif meqif commented Oct 29, 2018

Some LSP servers such as solargraph may send additional keys in the response to textDocument/documentSymbol requests, causing errors such as the following:

Keyword argument :deprecated not one of (:name :kind :location :containerName)

The corresponding code in solargraph is the following: https://github.com/castwide/solargraph/blob/db06853eff05bb597a40babc78844524bea29139/lib/solargraph/language_server/message/text_document/document_symbol.rb

Thus, the extraneous keys must be ignored.

Some LSP servers such as solargraph send additional keys in the response to `textDocument/documentSymbol` requests. Thus, the extraneous keys must be ignored.
@joaotavora
Copy link
Owner

I don't know what the standard says about this bit, but you could also convince Solargraph's devs to not break the spec (in case it is breaking the specs). That said, perhaps some macros based on destructuring-bind could be devised that respect some global parameter that indicates eglot's leniency towards these cases, and perhaps warns insteads of erroring.

In case Solargraph isn't breaking the spec, ignore this whole rant :-)

@MaskRay
Copy link
Contributor

MaskRay commented Oct 29, 2018

textDocument/documentSymbol can return either SymbolInformation[] or DocumentSymbol[]. Both interfaces contain deprecated?.

Don't know TypeScript much, but I feel even if they do not, it is reasonable to allow excess properties not specified in the interfaces.
https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#strict-object-literal-assignment-checking

@meqif
Copy link
Contributor Author

meqif commented Oct 30, 2018

As @MaskRay mentions, deprecated is a valid, although optional, field in either response to textDocument/documentSymbol, as can be read in the spec.

In any case, ignoring unknown fields would be a pragmatic decision in a world of decidedly less than ideal implementations of the protocol, especially if the alternative is breaking altogether (as happened in this case — the imenu integration doesn't work at all with solargraph without this fix or similar).

Do feel free to close this PR in favor of a better solution. This seemed like the least invasive and most pragmatic solution to me.

@joaotavora
Copy link
Owner

@MaskRay, @meqif I'm not saying it isn't possible. Applications should be strict about what they send, and lenient about what they receive. I'm just saying we should focus on both aspects, otherwise we'll soon be breaking the usefulness of having a spec in the first place.

@joaotavora
Copy link
Owner

Should be fixed, please test it and report here.

For the record, I think the best way to address this is to make eglot--lambda and eglot--dbind macros that accept an extra argument, which is the actual object type as defined in the spec. These macros should replace uses of jsonrpc-lambda and destructuring-bind. A global switch could then easily switch between strict/lenient.

joaotavora added a commit that referenced this pull request Oct 30, 2018
@meqif
Copy link
Contributor Author

meqif commented Oct 30, 2018

I confirm it's fixed now, thank you @joaotavora!

For the record, I think the best way to address this is to make eglot--lambda and eglot--dbind macros that accept an extra argument, which is the actual object type as defined in the spec. These macros should replace uses of jsonrpc-lambda and destructuring-bind.

I agree, that does sound like a better solution.

@meqif meqif deleted the patch-1 branch October 30, 2018 16:57
@mkcms mkcms mentioned this pull request Nov 22, 2018
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
* eglot.el (xref-backend-identifier-completion-table)
(eglot-imenu): Accept and ignore "deprecated"
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (xref-backend-identifier-completion-table)
(eglot-imenu): Accept and ignore "deprecated"
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (xref-backend-identifier-completion-table)
(eglot-imenu): Accept and ignore "deprecated"

#138: joaotavora/eglot#138
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
* eglot.el (xref-backend-identifier-completion-table)
(eglot-imenu): Accept and ignore "deprecated"

GitHub-reference: fix joaotavora/eglot#138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants