Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Mar 1, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
53c7a2b
lsp-rust: support rust-analyzer.showReference lens
nbfalcon Nov 3, 2020
dd4d5b7
lsp-lens.el: fix byte-compile warnings
nbfalcon Nov 3, 2020
1767902
Restore `lsp-execute-command' as an obsolete defun
nbfalcon Nov 7, 2020
00a1a58
Fix byte-compile
nbfalcon Nov 7, 2020
4e766bc
Remove `lsp-execute-command' as an obsolete defun
nbfalcon Nov 20, 2020
dddb162
Support `lsp-execute-command'
nbfalcon Nov 20, 2020
84fadc5
`lsp-execute-command': use `make-obsolete'
nbfalcon Nov 21, 2020
d9a17fa
lsp-rust: remove redundant `lsp-interface' comment
nbfalcon Nov 21, 2020
1c0c2b8
`lsp-lens--create-interactive-command': non-nil
nbfalcon Nov 28, 2020
19bdc11
Fix `byte-compile': `lsp-execute-command'
nbfalcon Nov 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions clients/lsp-rust.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ PARAMS progress report notification data."
(lsp-workspace-status nil workspace)
(lsp-workspace-status (format "%s - %s" title (or message? "")) workspace))))

(cl-defmethod lsp-execute-command (_server (_command (eql rls.run)) params)
(lsp-defun lsp-rust--rls-run ((&Command :arguments? params))
(-let* (((&rls:Cmd :env :binary :args :cwd) (lsp-seq-first params))
(default-directory (or cwd (lsp-workspace-root) default-directory) ))
(compile
Expand Down Expand Up @@ -635,6 +635,11 @@ them with `crate` or the crate name they refer to."
(lsp-defun lsp-rust--analyzer-run-single ((&Command :arguments?))
(lsp-rust-analyzer-run (lsp-seq-first arguments?)))

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@nbfalcon nbfalcon Nov 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yyoncho which do you prefer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(lsp-show-xrefs (lsp--locations-to-xref-items references) nil
(s-contains-p "reference" title)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity what are the expected title values here?

Copy link
Member Author

@nbfalcon nbfalcon Nov 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


(lsp-register-client
(make-lsp-client
:new-connection (lsp-stdio-connection
Expand All @@ -648,7 +653,8 @@ them with `crate` or the crate name they refer to."
:priority (if (eq lsp-rust-server 'rust-analyzer) 1 -1)
:initialization-options 'lsp-rust-analyzer--make-init-options
:notification-handlers (ht<-alist lsp-rust-notification-handlers)
:action-handlers (ht ("rust-analyzer.runSingle" #'lsp-rust--analyzer-run-single))
:action-handlers (ht ("rust-analyzer.runSingle" #'lsp-rust--analyzer-run-single)
("rust-analyzer.showReferences" #'lsp-rust--analyzer-show-references))
:library-folders-fn (lambda (_workspace) lsp-rust-library-directories)
:after-open-fn (lambda ()
(when lsp-rust-analyzer-server-display-inlay-hints
Expand Down
21 changes: 8 additions & 13 deletions lsp-lens.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,19 +144,14 @@ See `lsp-lens--schedule-refresh' for details."
(define-key [mouse-1] (lsp-lens--create-interactive-command command))))

(defun lsp-lens--create-interactive-command (command?)
"Create an interactive COMMAND? for the lens."
(let ((server-id (->> (lsp-workspaces)
(cl-first)
(or lsp--cur-workspace)
(lsp--workspace-client)
(lsp--client-server-id))))
(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?))))))
"Create an interactive COMMAND? for the lens.
COMMAND? shall be an `&Command' (e.g. `&CodeLens' :command?) and
mustn't be nil."
(if (functionp (lsp:command-command command?))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(lsp:command-command command?)
(lambda ()
(interactive)
(lsp--execute-command command?))))

(defun lsp-lens--display (lenses)
"Show LENSES."
Expand Down
20 changes: 15 additions & 5 deletions lsp-mode.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,7 +969,8 @@ calling `remove-overlays'.")
(defvar-local lsp--virtual-buffer-point-max nil)

(cl-defgeneric lsp-execute-command (server command arguments)
"Ask SERVER to execute COMMAND with ARGUMENTS.")
"Ask SERVER to execute COMMAND with ARGUMENTS."
(declare (obsolete "use `make-lsp-client' with :action-handlers instead." "7.1.0")))

(defun lsp-elt (sequence n)
"Return Nth element of SEQUENCE or nil if N is out of range."
Expand Down Expand Up @@ -5208,9 +5209,18 @@ It will filter by KIND if non nil."

(lsp-defun lsp--execute-command ((action &as &Command :command :arguments?))
"Parse and execute a code ACTION represented as a Command LSP type."
(-if-let* ((action-handler (lsp--find-action-handler command)))
(funcall action-handler action)
(lsp--send-execute-command command arguments?)))
(let ((server-id (->> (lsp-workspaces)
(cl-first)
(or lsp--cur-workspace)
(lsp--workspace-client)
(lsp--client-server-id))))
(condition-case nil
(with-no-warnings
(lsp-execute-command server-id (intern command) arguments?))
(cl-no-applicable-method
(if-let ((action-handler (lsp--find-action-handler command)))
(funcall action-handler action)
(lsp--send-execute-command command arguments?))))))

(lsp-defun lsp-execute-code-action ((action &as &CodeAction :command? :edit?))
"Execute code action ACTION.
Expand Down Expand Up @@ -5740,7 +5750,7 @@ REFERENCES? t when METHOD returns references."
(lsp-request "workspace/executeCommand" params)))

(defun lsp--send-execute-command (command &optional args)
"Execute workspace COMMAND with ARGS showing error if command is not mapped client-side."
"Create and send a 'workspace/executeCommand' message having command COMMAND and optional ARGS."
(condition-case-unless-debug err
(lsp-workspace-command-execute command args)
(error
Expand Down