Skip to content

Feature/handle disabled code actions #2402

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

Closed
wants to merge 19 commits into from
Closed

Feature/handle disabled code actions #2402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2a261d8
lsp-protocol: expose CodeAction `disabled?'
nbfalcon Dec 8, 2020
046ffd1
`lsp--select-action': don't show disabled actions
nbfalcon Dec 8, 2020
4865379
`lsp--select-code-action`: highlight preferred
nbfalcon Dec 8, 2020
7286bca
`lsp-execute-code-action': C-u -> show all actions
nbfalcon Dec 8, 2020
22d46fd
`lsp-execute-code-action-by-kind': refactor
nbfalcon Dec 11, 2020
0f48af3
Add new code action commands
nbfalcon Dec 11, 2020
e6342b4
Review: `lsp--select-action': use `pcase'
nbfalcon Dec 11, 2020
d9d26c3
Refactor: drop `lsp--select-action-enabled-or-die'
nbfalcon Dec 11, 2020
9b3602f
`lsp--select-action': preselect preferred actions
nbfalcon Dec 11, 2020
cb1897a
Refactor: remove `lsp--select-enabled-action'
nbfalcon Dec 11, 2020
801d014
`lsp-code-actions-at-point': fix disabled? check
nbfalcon Dec 11, 2020
ace8758
`lsp-execute-code-action-by-kind': choose enabled
nbfalcon Dec 11, 2020
74a828e
`lsp-execute-code-action-by-type': ENABLED filter
nbfalcon Dec 11, 2020
abedf27
`lsp--code-action-title': support omitted reason
nbfalcon Dec 12, 2020
8367f49
`lsp--select-action': pre-select non-disabled
nbfalcon Dec 12, 2020
7f65d1c
Add `lsp-quickfix' and `lsp-refactor' commands
nbfalcon Dec 23, 2020
7b55270
`lsp--code-action-title': add docstring
nbfalcon Jan 2, 2021
980144d
`lsp--code-action-title': preferred + disabled
nbfalcon Jan 2, 2021
05d5271
Add a macro to expose base kinds as commands
nbfalcon Jan 7, 2021
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
133 changes: 107 additions & 26 deletions lsp-mode.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1272,9 +1272,15 @@ the lists according to METHOD."

TRANSFORM-FN will be used to transform each of the items before displaying.

PROMPT COLLECTION PREDICATE REQUIRE-MATCH INITIAL-INPUT HIST DEF
INHERIT-INPUT-METHOD will be proxied to `completing-read' without changes."
IF DEF is a number, it specifies the zero-based index into the
COLLECTION that is to be used as the default result and that is
to be preselected.

PROMPT COLLECTION PREDICATE REQUIRE-MATCH INITIAL-INPUT HIST
INHERIT-INPUT-METHOD will be proxied to `completing-read' without
changes."
(let* ((col (--map (cons (funcall transform-fn it) it) collection))
(def (if (numberp def) (nth def col) def))
(completion (completing-read prompt col
predicate require-match initial-input hist
def inherit-input-method)))
Expand Down Expand Up @@ -3245,6 +3251,7 @@ disappearing, unset all the variables related to it."
(rename . ((dynamicRegistration . t) (prepareSupport . t)))
(codeAction . ((dynamicRegistration . t)
(isPreferredSupport . t)
(disabledSupport . t)
(codeActionLiteralSupport . ((codeActionKind . ((valueSet . [""
"quickfix"
"refactor"
Expand Down Expand Up @@ -4834,18 +4841,46 @@ When language is nil render as markup if `markdown-mode' is loaded."
(format "%s (%s)" element count))
(push element elements))))))

(defface lsp-preferred-code-action-face '((t :underline t))
"Face used to highlight preferred code actions.
Used by `lsp-execute-code-action' and
`lsp-execute-code-action-by-kind' if they have to prompt for an
action.")

(defface lsp-disabled-code-action-face '((t :inherit shadow))
"Face used to highlight disabled code actions.
See `lsp-preferred-code-action-face' for details.")

(defface lsp-disabled-code-action-reason-face '((t :inherit lsp-details-face))
Copy link
Member

Choose a reason for hiding this comment

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

This is not used?

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. I wanted to add a help-echo to disabled code actions at some point, but then realized that it wouldn't work with completing-read.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, this is a regression from the commit to support both preferred and disabled code actions at the same time. It was supposed to show the reason. I'll need to add it back.

"Face used to highlight the reason a code action is disabled.
Shown after the code action in `lsp-execute-code-action',
... (see `lsp-preferred-code-action-face').")

(lsp-defun lsp--code-action-title ((action &as &CodeAction :title :is-preferred? :disabled?))
"Render an `&CodeAction' as a propertized string."
(--doto (copy-sequence title)
(let ((len (length it)))
(when is-preferred?
(add-face-text-property 0 len 'lsp-preferred-code-action-face nil it))
(when disabled?
(add-face-text-property 0 len 'lsp-disabled-code-action-face nil it)
(-when-let ((&CodeActionDisabled :reason) disabled?)
(cl-callf concat it " " (propertize reason 'face 'lsp-disabled-code-action-reason-face)))))))

(defun lsp--select-action (actions)
"Select an action to execute from ACTIONS."
(cond
((seq-empty-p actions) (signal 'lsp-no-code-actions nil))
((and (eq (seq-length actions) 1) lsp-auto-execute-action)
(lsp-seq-first actions))
(t (let ((completion-ignore-case t))
(lsp--completing-read "Select code action: "
(seq-into actions 'list)
(-compose (lsp--create-unique-string-fn)
#'lsp:code-action-title)
nil t)))))
(let ((actions (seq-into actions 'list)))
(pcase actions
(`nil (signal 'lsp-no-code-actions nil))
((and `(,action) (guard lsp-auto-execute-action)) action)
(_ (let ((completion-ignore-case t))
(lsp--completing-read "Select code action: "
actions
(-compose (lsp--create-unique-string-fn)
#'lsp--code-action-title)
nil t nil nil
(or (-find-index #'lsp:code-action-is-preferred? actions)
Copy link
Member

Choose a reason for hiding this comment

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

These should be -find instead of -find-index, the index passing is only working in ivy-mode

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 have implemented that for lsp--completing-read.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, I missed that.
Also, why don't just pass using -find instead of -find-index?
It seems that you're taking a round trip of finding index and taking the list element by index here.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the COLLECTION is a list of strings that is to be transformed, there would be an ambiguitiy: should DEF be used to look up the corresponding string in COLLECTION, or be the DEF argument to completing read?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that ambiguity be resolved if we document it? For example, DEF should be the corresponding string in COLLECTION (since it's what's returned). That would also be aligned with original completing-read which is what wrapped in lsp--completing-read.

(-find-index (-not #'lsp:code-action-disabled?) actions))))))))

(defun lsp--workspace-server-id (workspace)
"Return the server ID of WORKSPACE."
Expand Down Expand Up @@ -5211,19 +5246,57 @@ It will show up only if current point has signature help."
:context `( :diagnostics ,(lsp-cur-line-diagnostics)
,@(when kind (list :only (vector kind))))))

(defun lsp-code-actions-at-point (&optional kind)
(defun lsp-code-actions-at-point (&optional kind enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here... could we make this a list of kinds? The protocol supports that.

"Retrieve the code actions for the active region or the current line.
It will filter by KIND if non nil."
(lsp-request "textDocument/codeAction" (lsp--text-document-code-action-params kind)))
If ENABLED is specified, only return code actions that are not
disabled."
(--doto (lsp-request "textDocument/codeAction" (lsp--text-document-code-action-params kind))
Copy link
Member

Choose a reason for hiding this comment

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

I guess this section is on the edge of being too trickier.

Copy link
Member Author

Choose a reason for hiding this comment

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

What I had in mind here was the following: currently there is a single filter: ENABLED. In the future, more filters could be added, and using --doto with cl-callf allows adding them easily (e.g. is-preferred). I could rewrite this in terms of if if you consider that to be a large enough readability gain:

  (let ((actions (lsp-request "textDocument/codeAction" (lsp--text-document-code-action-params kind))))
    (if enabled
        (seq-remove #'lsp:code-action-disabled? actions)
      acitons))

I just realized that the code has a bug: lsp-code-action-disabled? should be lsp:code-action-disabled?.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it could stay as it is, just it is a bit unusual. Once -cond->> lands in dash we can use it here(and few other places).

Copy link
Member Author

Choose a reason for hiding this comment

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

-cond->> would certainly be awesome, and I remember that I wanted to use it somewhere but couldn't. However, I don't see how this would help here? The point of this extensible function style is to allow filters to be chained: e.g. first filter out all disabled actions, then those that don't match the type base, .... (we don't have that yet, but might in the future).

Copy link
Member

Choose a reason for hiding this comment

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

Something like that (clj)


(let [even true
      small true]
  (cond->> (range 20)
    even (filter even?)
    small (filter #(< 10 %))))

(when enabled
(cl-callf2 seq-remove #'lsp:code-action-disabled? it))))

(defun lsp-execute-code-action-by-kind (command-kind)
"Execute code action by COMMAND-KIND."
(if-let ((action (->> (lsp-get-or-calculate-code-actions command-kind)
(-filter (-lambda ((&CodeAction :kind?))
(and kind? (equal command-kind kind?))))
lsp--select-action)))
(lsp-execute-code-action action)
(signal 'lsp-no-code-actions '(command-kind))))
"Execute code action by name."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Execute code action by name."
"Execute code action by kind."

(let* ((all-actions (->> (lsp-get-or-calculate-code-actions command-kind)
(-filter (-lambda ((&CodeAction :kind?))
(and kind? (equal command-kind kind?))))))
(enabled-actions (-remove #'lsp:code-action-disabled? all-actions))
(action (lsp--select-action (or enabled-actions all-actions))))
Copy link
Member

Choose a reason for hiding this comment

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

Is this ok? In case of all actions are disabled, we show all actions instead of empty list?
Also, shouldn't we need guard of action as the old code, in case of user not select anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to show an error message in case there is no enabled code action, if the user has enabled lsp-auto-execute-action as t (discussion). However, I agree that this might be suboptimal. Do you have an idea for a better UI in this case?

As for guarding, we needn't do that, since the user cannot exit from lsp--select-action without either selecting an action or quitting (causing a non-local exit), due to REQUIRE-MATCH being set as t.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't lsp-auto-execute-action is only for single code action?
The case of lsp-execute-code-action-by-kind is falling into the bellow in the spec, should we just take the first code action and let-bind lsp-auto-execute-action to t so the error will always be shown?

	 * - If the user has a keybinding that auto applies a code action and only
	 *   a disabled code actions are returned, the client should show the user
	 *   an error message with `reason` in the editor.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could let-bind lsp-auto-execute-action to t if there are no enabled ones, however I am unsure if that is the right thing: lsp-execute-code-action-by-kind doesn't auto-execute a single code action if available by default, either. If there is only one disabled code action, a prompt containing only it will show up, and along the code action title the disabled-reason will be shown:
20210102-153855

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't lsp--select-action automatically choose the only one code action available when lsp-auto-execute-action is t?
Is there special case for disable code action that make the lsp--select-action behaves otherwise?

(lsp-execute-code-action action)))

(defun lsp-execute-code-action-by-type (kind &optional enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

The difference between this and lsp-execute-code-action-by-kind isn't clear to me (or from the docs).

Copy link
Member Author

Choose a reason for hiding this comment

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

lsp-execute-code-action-by-type matches base-types, i.e. lsp-execute-code-action-by-type "source" would match "source.organizeImports", while -by-kind matches exactly, i.e. the same example would yield nothing, and it would have to be (lsp-execute-code-action-by-kind "source.organizeImports").

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is a distinction the spec really knows about - certainly "base type" doesn't appear. They do talk about "base kinds", but I'm not sure they're meaningfully distinct from "kinds", it just indicates that they should be hierarchical parents of a set of kinds.

What about just having an exact-match argument to lsp-execute-code-action-by-kind? Or maybe we shouldn't even provide exact matching. The intention for spec methods that allow you to do kind-based filtering is to include kinds "under" the selected kinds (see e.g. microsoft/language-server-protocol#970 (comment)). Do we have a usecase for exact matching?

"Execute a code action with a given base KIND.
When ENABLED is given, filter out all disabled code actions. This
is in contrast with the spec's recommended behavior."
(->> (lsp-get-or-calculate-code-actions kind enabled)
(-filter (-lambda ((&CodeAction :kind?))
(and kind? (string-match-p (format "\\`%s\\(\\.\\|\\'\\)"
(regexp-quote kind))
kind?))))
lsp--select-action
lsp-execute-code-action))

(defmacro lsp-make-interactive-code-action-by-type (name kind)
"Make interactive function NAME to execute a KIND code action.
Like `lsp-make-interactive-code-action' but using
`lsp-execute-code-action-by-type'."
`(defun ,name ()
,(format "Execute a %S code action at point." kind)
(interactive)
(condition-case nil
(lsp-execute-code-action-by-kind ,kind)
(lsp-no-code-actions
(user-error "No %S actions available")))))

(lsp-make-interactive-code-action-by-type lsp-quickfix "quickfix")
(lsp-make-interactive-code-action-by-type lsp-refactor "refactor")

(defun lsp-auto-fix ()
"Execute a preferred code action at point."
(interactive)
(->> (lsp-get-or-calculate-code-actions)
(-filter #'lsp:code-action-is-preferred?)
lsp--select-action
lsp-execute-code-action))

(defalias 'lsp-get-or-calculate-code-actions 'lsp-code-actions-at-point)

Expand All @@ -5242,11 +5315,19 @@ It will filter by KIND if non nil."
(funcall action-handler action)
(lsp-send-execute-command command arguments?))))))

(lsp-defun lsp-execute-code-action ((action &as &CodeAction :command? :edit?))
(lsp-defun lsp-execute-code-action ((action &as &CodeAction :command? :edit? :disabled?))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have an optional kinds argument? Generally, it seems like almost all the functions that operate on code actions could take an optional kinds argument, perhaps that would let us get away with fewer functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in this case. lsp-execute-code-action should execute whatever action it is given. It is only additionally interactive.

"Execute code action ACTION.
If ACTION is not set it will be selected from `lsp-code-actions-at-point'.
Request codeAction/resolve for more info if server supports."
(interactive (list (lsp--select-action (lsp-code-actions-at-point))))
Interactively, ask for a code action. With the prefix argument,
filter those that are not disabled. Request codeAction/resolve
for more info if server supports."
(interactive
(list (lsp--select-action
(lsp-code-actions-at-point nil (not current-prefix-arg)))))
(when disabled?
(error "%s is disabled%s"
(lsp:code-action-title action)
(or (-some->> (lsp:code-action-disabled-reason disabled?) (concat ": ")) "")))

(if (and (lsp-feature? "codeAction/resolve")
(not command?)
(not edit?))
Expand Down
3 changes: 2 additions & 1 deletion lsp-protocol.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,8 @@ See `-let' for a description of the destructuring mechanism."
(CallHierarchyOutgoingCall (:to :fromRanges) nil)
(CallHierarchyOutgoingCallsParams (:item) nil)
(CallHierarchyPrepareParams (:textDocument :position) (:uri))
(CodeAction (:title) (:command :diagnostics :edit :isPreferred :kind :data))
(CodeActionDisabled (:reason))
(CodeAction (:title) (:command :diagnostics :edit :isPreferred :kind :data :disabled?))
(CodeActionKind nil nil)
(CodeActionParams (:textDocument :context :range) nil)
(CodeLens (:range) (:command :data))
Expand Down