lsp-rust: support rust-analyzer.showReference lens #2299

nbfalcon · 2020-11-03T21:05:50Z

Add a rust-analyzer specific :action-handler for the showReference lens,
which leverages lsp-show-xrefs'. Refactor: eliminate lsp-execute-command', because it caused the :title to be lost, which is
needed to distinguish between the "references" or "implementations"
variants of that lens.

Defining a new `lsp-interface' to destructure
"rust-analyzer.showReference" was impossible, as the former doesn't
support destructuring lists (the :arguments? paramter is a special,
3-element list.).

yyoncho · 2020-11-03T21:43:09Z

Please revert the lsp-execute-command elimination because it is used multiple times in different projects (e. g. lsp-java, ccls, etc).

kiennq · 2020-11-03T22:13:14Z

clients/lsp-rust.el

+;; `lsp-interface' is unsuitable for ShowReference destructuring, as that code
+;; action yields an array of three elements, not a map.
+(lsp-defun lsp-rust--analyzer-show-references
+  ((&Command :title :arguments? [_uri _filepos references]))


This should have different indent or should be on the same line with the function name

Then the line would be > 80 columns. Is that preferred?

You can put multi-line for the parameters right?
That would make it easier to distinguished between params and body of a function

The options are:

(lsp-defun lsp-rust--analyzer-show-references ((&Command :title :arguments? [_uri _filepos references]))

or

(lsp-defun lsp-rust--analyzer-show-references ((&Command :title :arguments? [_uri _filepos references]))

, both of which require one more line. What do you think? Do you prefer either of them over the current solution?

Sorry for late reply, both look fine to me.
The current solution has same indent between args and body and that make it quite hard to distinguish them.

@yyoncho which do you prefer?

For me all three options are acceptable... Just want to mention that we do not enforce 80 symbols or at least there is no agreement on that ATM.

Thank you, I'll keep your latter point in mind. Not changing the call here is the best option then, because it means no new changes have to made and reviewed.

clients/lsp-rust.el

nbfalcon · 2020-11-04T08:30:33Z

@yyoncho can I perhaps instead refactor the other projects to not use it? That function needs to be at least be refactored, as it removes the :title field (without it, it is impossible to know whether to use xref--show-defs' or xref--show-references'), and it would become a liability in the future, if more fields would be added to the Lens lsp structure, yielding confusing errors because fields become nil out of nowhere.

yyoncho · 2020-11-04T08:41:19Z

@yyoncho can I perhaps instead refactor the other projects to not use it? That function needs to be at least be refactored, as it removes the :title field (without it, it is impossible to know whether to use xref--show-defs' or xref--show-references'), and it would become a liability in the future, if more fields would be added to the Lens lsp structure, yielding confusing errors because fields become nil out of nowhere.

I am fine with this approach.

nbfalcon · 2020-11-04T08:42:11Z

@yyoncho can you give me a list of projects that use it?

nbfalcon · 2020-11-04T09:02:46Z

lsp-java is done.

nbfalcon · 2020-11-04T09:15:10Z

ccls.el is done.

yyoncho · 2020-11-05T18:00:54Z

@yyoncho can you give me a list of projects that use it?

IDK - you have to check all packages that depend on lsp-mode.

kiennq · 2020-11-07T14:17:50Z

Instead of just deleting code, I think we should probably make that function become obsoleted, and promote lsp--execute-command to public function, that ways we will not break any package that depends on lsp-mode.
The parameter for the old function lsp-execute-command is clear, it will just passing command and arguments, and not react to anything else, so the user of that API should aware that already.

nbfalcon · 2020-11-07T16:02:20Z

@kiennq actually, lsp-execute-command isn't a function that the user calls, it's the other way around: that function is a cl-defgeneric, provided by some language server clients. The problem here is that lens resolution goes trough that function, causing the :title? field to be dropped. This makes it impossible to implement this lens properly, but much worse than that might become a huge blocker in the future. I could theoretically extend lsp--execute-command to try to see if a cl-defmethod exists, but doing so would involve horrible hacks, and I'd much rather fix all downstream packages instead. We also aren't losing functionality here, as :action-handlers is a 100% replacement for overriding that function.

nbfalcon · 2020-11-07T16:07:33Z

Also, from the packages I checked, there are no more uses of it. We'll need to wait for ccls and lsp-java, and then this can be merged.

kiennq · 2020-11-07T17:03:23Z

actually, lsp-execute-command isn't a function that the user calls, it's the other way around: that function is a cl-defgeneric, provided by some language server clients. The problem here is that lens resolution goes trough that function, causing the :title? field to be dropped.

Lens resolution is not using :title information until now, and for this feature we better switch to another function that also passing the :title information, I'm totally agree with that.
But, that's not the reason to delete this function and its functionality.

The cl-defgeneric of this function provides an easy way to add new handlers for :action-handlers without modifying the lsp-mode source code that user can easily extend.

A quick search of lsp-execute-command shows over 1K results, I assume all will be broken (not including private repo) when this change is merged.

nbfalcon · 2020-11-07T20:17:58Z

We cannot use a new function for this. Also, my problem isn't only the :title field, even though it hints the problems that will follow; lsp-execute-command was misdesigned: it should pass the entire action trough, not only specific fields of it. Not doing that will become a problem when new fields are added, perhaps even server-specific ones. This API is a liability and needs to be eliminated.

Regarding your github search, most of them are elpa directories containing lsp-java-boot. I already made a PR to fix that there. I'll investigate the interesting ones. Your search will be a good starting point, thank you.

nbfalcon · 2020-11-07T20:22:19Z

Nevermind, it's still too many. I propose an alternative: lsp-execute-command will still be available as a function, but cl-defmethod overrides of it will no longer be possible. This means nothing should be broken.

nbfalcon · 2020-11-07T20:43:44Z

I'll re-add it as a normal defun. It is not overriden using cl-defmethod anywhere I didn't provide a PR to fix that though, see [this query]
(https://github.com/search?p=3&q=cl-defmethod+lsp-execute-command+-filename%3Alsp-java-boot.el+-filename%3Accls.el+-filename%3Alsp-java.el+-filename%3Alsp-java.elc+-filename%3Alsp-java-boot.elc+-filename%3Accls.elc+-filename%3A*.elc+-filename%3Alsp-rust.el+-filename%3Alsp-mode.el&type=Code)

Note the exclusions: I have provided PRs for lsp-java-* and ccls.el. All the results are just copies of lsp-mode, ccls.el and lsp-java.el, also in compiled form.

The following PRs need to be merged before this can be, though:

Also, should we make lsp--execute-command public?

lsp-mode.el

nbfalcon · 2020-11-09T21:27:51Z

We may want to make a new release for this though, as it would break ccls.el and lsp-java-* for anyone who decides to update lsp-mode independently of these other packages.

nbfalcon · 2020-11-20T12:23:11Z

lsp-execute-command now works as it did previously (albeit with a deprecation warning).

I tested this with ccls before my PR; however, to get it working I had to do two things: use lsp-show-xrefs instead of xref--show-xrefs (because the latter takes a function, not a list of xrefs) and to handle symbols in lsp--send-execute-command because I was getting errors related to json-serialize (wrong type argument json-value-p for a symbol).

These seem unrelated to the changes though (I'll investigate with master).

nbfalcon · 2020-11-20T12:30:19Z

I tested with master, ccls' xref lenses are broken independently of this PR (I have made a PR there which fixes these issues, along with not using lsp-execute-command), so I believe that this can be merged.

kiennq · 2020-11-20T13:28:05Z

lsp-mode.el

+                        (lsp--client-server-id))))
+    (condition-case nil
+        (prog1 (lsp-execute-command server-id (intern command) arguments?)
+          (lsp--warn "`lsp-execute-command' is deprecated"))


Why don't you just make lsp-execute-command obsolete instead? We will have compile warning about that, and that would be standard way to alert package maintainer about obsolete function.

That somehow slipped my mind; concluded from testing, calling that function doesn't cause warnings, but overriding it using cl-defmethod does. I couldn't find anything in the Emacs docs about the obsoletion of generics, though. Fixed.

yyoncho

The code looks good to me. I will proceed with the merge after doing manual testing.

yyoncho · 2020-11-21T18:21:39Z

clients/lsp-rust.el

+(lsp-defun lsp-rust--analyzer-show-references
+  ((&Command :title :arguments? [_uri _filepos references]))
+  (lsp-show-xrefs (lsp--locations-to-xref-items references) nil
+                  (s-contains-p "reference" title)))


out of curiosity what are the expected title values here?

reference(s)
definition(s)
E.g. 2 references, 2 definitions. I'm unsure whether the singular forms can actually be encountered, though.

As a side note, do we prefer scheme-style ? or common-lisp-style p predicate functions? I.e. s-contains-p or s-contains?.

As a side note, do we prefer scheme-style ? or common-lisp-style p predicate functions? I.e. s-contains-p or s-contains?.

Most of the code is written using scheme-style although this is not enforced 100% during reviews.

nbfalcon · 2020-11-22T08:02:05Z

@yyoncho you may want to hold off merging until my ccls.el PR gets merged, as otherwise users will get inevitable byte-compiler warnings.

yyoncho · 2020-11-27T19:43:28Z

@yyoncho you may want to hold off merging until my ccls.el PR gets merged, as otherwise users will get inevitable byte-compiler warnings.

Is the ccls pr mandatory?

kiennq · 2020-11-28T03:01:16Z

lsp-lens.el

-        (lsp-execute-command server-id
-                             (intern (lsp:command-command command?))
-                             (lsp:command-arguments? command?))))))
+  (if (functionp (lsp:command-command command?))


Can command? be nil here?
If so, don't you need to check before passing it to lsp--execute-command?

I'd guess no, because it wasn't possible previously (this wouldn't be a regression caused by this PR):

(if (functionp (lsp:command-command command?)) (lsp:command-command command?) (lambda () (interactive) (lsp-execute-command server-id (intern (lsp:command-command command?)) (lsp:command-arguments? command?)))) ; (if (functionp (lsp:command-command nil)) ...) -> (functionp nil) -> nil

This means that the control flow passed to the other branch, creating a lambda that would try to intern nil, which errors.

(lsp:command-command nil) ; => nil (intern nil) ; => error

This function is called on line 190 in master (without this PR), and only lenses that have a command are passed to it (-filter #'lsp:command-command), so I guess command? cannot be nil. I'll note that in the docstring.

The reason I'm asking is because of parameter name command? which indicates it can be nil, we should change the parameter name too.

I wouldn't do that, since it is an Elisp-level change, meaning that this entire thing has to be manually tested again, for something this trivial. See this comment by @yyoncho. Noting that in the docstring is good enough, IMHO.

Also, the name isn't that bad: command? also corresponds to the naming of the CodeLens and CodeAction :command? fields, indicating that this function expects them to be passed in.

nbfalcon · 2020-11-28T20:10:57Z

@yyoncho I'd argue yes, because ccls users would get annoying depreciation warnings if they chose to upgrade lsp-mode. We could also not deprecate that interface, but it kind-of is a second class citizen here, so it should probably be removed at some point.

nbfalcon · 2020-12-09T22:50:53Z

I would like to propose not deprecating lsp-execute-command, given that my ccls PR apparently won't be merged too soon. @yyoncho what do you think?

yyoncho · 2021-02-20T19:24:09Z

Can you rebase?

Add a rust-analyzer specific :action-handler for the showReference lens, which leverages `lsp-show-xrefs'. Refactor: eliminate `lsp-execute-command', because it caused the :title to be lost, which is needed to distinguish between the "references" or "implementations" variants of that lens. Defining a new `lsp-interface' to destructure "rust-analyzer.showReference" was impossible, as the former doesn't support destructuring lists (the :arguments? paramter is a special, 3-element list.).

Its original function, being a `cl-defgeneric' that can be overriden per language server is gone; however, PRs were provided to eliminate all uses of it in that context. To remain backwards-compatible, it will still remain as an obsolete `defun' that calls `lsp--execute-command' internally.

Since it wouldn't do what it did previously (being an extension point), restoring that function doesn't actually improve backwards compatibility. Reverts 957a056.

`lsp-execute-command' now works as an extension point again, but a deprecation warning is shown if it is used to handle a command. To implement this, leverage `cl-no-applicable-method': call `lsp-execute-command' first, and then, if there is no implementation, go trough the handler hash tables as usual.

The proper way to mark a function as deprecated is to use `make-obsolete'; use that instead of `lsp--warn'.

`lsp-interface' wasn't designed to destructure lists; stating that fact before `lsp-rust--analyzer-show-references' is as such redundant, since that is normal and not noteworthy.

@kiennq

The COMMAND? argument of that function mustn't be nil, which is unclear from its name (?). Note that fact in its docstring. The name of that argument is still good though, because it refers to a field name in `CodeAction' and `CodeLens'. Based on a discussion with @kiennq.

Unlike previously assumed, calling `lsp-execute-command' causes byte-compile warnings, not just using it in `cl-defmethod'. Wrap the call in `with-no-warnings', since that should be the only use of that function.

github-actions bot added the client One or more of lsp-mode language clients label Nov 3, 2020

kiennq reviewed Nov 3, 2020

View reviewed changes

yyoncho reviewed Nov 4, 2020

View reviewed changes

clients/lsp-rust.el Outdated Show resolved Hide resolved

nbfalcon mentioned this pull request Nov 4, 2020

Don't use `lsp-execute-command' emacs-lsp/emacs-ccls#104

Open

nbfalcon force-pushed the lens/rust-analyzer.showReference branch from 2f4a74d to 957a056 Compare November 7, 2020 21:23

yyoncho reviewed Nov 9, 2020

View reviewed changes

lsp-mode.el Outdated Show resolved Hide resolved

nbfalcon force-pushed the lens/rust-analyzer.showReference branch from 8c2e6a1 to 44e5986 Compare November 20, 2020 12:20

kiennq reviewed Nov 20, 2020

View reviewed changes

yyoncho reviewed Nov 21, 2020

View reviewed changes

brotzeit mentioned this pull request Nov 27, 2020

Doctest syntax highlighting and run test brotzeit/rustic#187

Closed

kiennq reviewed Nov 28, 2020

View reviewed changes

nbfalcon added 10 commits March 1, 2021 20:59

lsp-lens.el: fix byte-compile warnings

dd4d5b7

Fix byte-compile

00a1a58

Remove `lsp-execute-command' as an obsolete defun

4e766bc

Since it wouldn't do what it did previously (being an extension point), restoring that function doesn't actually improve backwards compatibility. Reverts 957a056.

lsp-execute-command': use make-obsolete'

84fadc5

The proper way to mark a function as deprecated is to use `make-obsolete'; use that instead of `lsp--warn'.

lsp-rust: remove redundant `lsp-interface' comment

d9a17fa

`lsp-interface' wasn't designed to destructure lists; stating that fact before `lsp-rust--analyzer-show-references' is as such redundant, since that is normal and not noteworthy.

Fix byte-compile': lsp-execute-command'

19bdc11

Unlike previously assumed, calling `lsp-execute-command' causes byte-compile warnings, not just using it in `cl-defmethod'. Wrap the call in `with-no-warnings', since that should be the only use of that function.

yyoncho force-pushed the lens/rust-analyzer.showReference branch from 5ea5db1 to 19bdc11 Compare March 1, 2021 19:00

yyoncho approved these changes Mar 1, 2021

View reviewed changes

yyoncho merged commit b8a7455 into master Mar 1, 2021

lsp-rust: support rust-analyzer.showReference lens #2299

lsp-rust: support rust-analyzer.showReference lens #2299

Conversation

nbfalcon commented Nov 3, 2020

yyoncho commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbfalcon Nov 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbfalcon commented Nov 4, 2020

yyoncho commented Nov 4, 2020

nbfalcon commented Nov 4, 2020

nbfalcon commented Nov 4, 2020

nbfalcon commented Nov 4, 2020

yyoncho commented Nov 5, 2020

kiennq commented Nov 7, 2020

nbfalcon commented Nov 7, 2020 • edited Loading

nbfalcon commented Nov 7, 2020

kiennq commented Nov 7, 2020

nbfalcon commented Nov 7, 2020

nbfalcon commented Nov 7, 2020

nbfalcon commented Nov 7, 2020 • edited Loading

nbfalcon commented Nov 9, 2020

nbfalcon commented Nov 20, 2020 • edited Loading

nbfalcon commented Nov 20, 2020

kiennq Nov 20, 2020 • edited by nbfalcon Loading

Choose a reason for hiding this comment

nbfalcon Nov 21, 2020 • edited Loading

Choose a reason for hiding this comment

yyoncho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbfalcon Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbfalcon commented Nov 22, 2020

yyoncho commented Nov 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbfalcon commented Nov 28, 2020

nbfalcon commented Dec 9, 2020

yyoncho commented Feb 20, 2021

nbfalcon Nov 21, 2020 •

edited

Loading

nbfalcon commented Nov 7, 2020 •

edited

Loading

nbfalcon commented Nov 7, 2020 •

edited

Loading

nbfalcon commented Nov 20, 2020 •

edited

Loading

kiennq Nov 20, 2020 •

edited by nbfalcon

Loading

nbfalcon Nov 21, 2020 •

edited

Loading

nbfalcon Nov 22, 2020 •

edited

Loading