From 44accbba019a3c3e91e5f53c66e49e132378fb84 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Mon, 9 Nov 2020 17:53:37 +0100 Subject: [PATCH 01/17] `lsp-rename': check if supported `lsp-rename' doesn't check whether renaming is supported first. Do that, throwing an error otherwise. Fixes #2314. --- lsp-mode.el | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 7d29bd33ac3..2770d1c6eb7 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -1191,6 +1191,13 @@ Deprecated. Use `lsp-repeatable-vector' instead. " "Display lsp error message with FORMAT with ARGS." (message "%s :: %s" (propertize "LSP" 'face 'error) (apply #'format format args))) +(defun lsp--fatal-error (format &rest args) + "Show an lsp error message using `error'. +FORMAT is the `format' format to use, while ARGS provides the +fields to it." + (error "%s :: %s" (propertize "LSP" 'face 'error) + (apply #'format format args))) + (defun lsp--eldoc-message (&optional msg) "Show MSG in eldoc." (setq lsp--eldoc-saved-message msg) @@ -5654,22 +5661,30 @@ perform the request synchronously." (lsp-request "workspace/symbol" `(:query ,pattern)))) (defun lsp--get-symbol-to-rename () - "Get symbol to rename and placeholder at point." - (if (let ((rename-provider (or (lsp--capability :renameProvider) - (-some-> (lsp--registered-capability "textDocument/rename") - (lsp--registered-capability-options))))) - (lsp:rename-options-prepare-provider? rename-provider)) - (-when-let (response (lsp-request "textDocument/prepareRename" + "Get symbol to rename and placeholder at point. +Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER)." + ;; Is there some connected server that supports renaming? + (if-let ((rename-provider + (or (lsp--capability :renameProvider) + (-some-> (lsp--registered-capability "textDocument/rename") + (lsp--registered-capability-options))))) + ;; Do we have prepare rename? Do it in that case. If we fail, don't do a + ;; rename, since it's invalid. + (if (lsp:rename-options-prepare-provider? rename-provider) + (-let* ((prepare (lsp-request "textDocument/prepareRename" (lsp--text-document-position-params))) - (-let* (((start . end) (lsp--range-to-region - (if (lsp-range? response) - response - (lsp:prepare-rename-result-range response)))) - (symbol (buffer-substring-no-properties start end)) - (placeholder (lsp:prepare-rename-result-placeholder response))) - (cons symbol (or placeholder symbol)))) - (let ((symbol (thing-at-point 'symbol t))) - (cons symbol symbol)))) + ((start . end) (lsp--range-to-region + (if (lsp-range? prepare) + prepare + (lsp:prepare-rename-result-range prepare)))) + (symbol (buffer-substring-no-properties start end)) + (placeholder (lsp:prepare-rename-result-placeholder prepare))) + (cons symbol (or placeholder symbol))) + ;; We don't support prepare-rename, so just yield the symbol at point. + (let ((sym (thing-at-point 'symbol))) + (cons sym sym))) + ;; No connected server supports renaming. + (lsp--fatal-error "The connected server(s) doesn't support renaming"))) (defun lsp-rename (newname) "Rename the symbol (and all references to it) under point to NEWNAME." From ef951ec8c533f81ec343cdf2b1dc1ebaac4ea2b8 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Mon, 9 Nov 2020 18:13:45 +0100 Subject: [PATCH 02/17] `lsp-rename': support ommitted :placeholder In `lsp--get-symbol-to-rename', handle the server not providing a placeholder by limiting the `when-let' block to the prepare result. --- lsp-mode.el | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 2770d1c6eb7..2a7ecbe186f 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -1198,6 +1198,12 @@ fields to it." (error "%s :: %s" (propertize "LSP" 'face 'error) (apply #'format format args))) +(defun lsp--user-error (format &rest args) + "Like `lsp--fatal-error', but with `user-error'. +FORMAT and ARGS are the same." + (user-error "%s :: %s" (propertize "LSP" 'face 'error) + (apply #'format format args))) + (defun lsp--eldoc-message (&optional msg) "Show MSG in eldoc." (setq lsp--eldoc-saved-message msg) @@ -5671,15 +5677,16 @@ Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER)." ;; Do we have prepare rename? Do it in that case. If we fail, don't do a ;; rename, since it's invalid. (if (lsp:rename-options-prepare-provider? rename-provider) - (-let* ((prepare (lsp-request "textDocument/prepareRename" - (lsp--text-document-position-params))) - ((start . end) (lsp--range-to-region - (if (lsp-range? prepare) - prepare - (lsp:prepare-rename-result-range prepare)))) - (symbol (buffer-substring-no-properties start end)) - (placeholder (lsp:prepare-rename-result-placeholder prepare))) - (cons symbol (or placeholder symbol))) + (when-let ((prepare + (lsp-request "textDocument/prepareRename" + (lsp--text-document-position-params)))) + (-let* (((start . end) (lsp--range-to-region + (if (lsp-range? prepare) + prepare + (lsp:prepare-rename-result-range prepare)))) + (symbol (buffer-substring start end)) + (placeholder (lsp:prepare-rename-result-placeholder prepare))) + (cons symbol (or placeholder symbol)))) ;; We don't support prepare-rename, so just yield the symbol at point. (let ((sym (thing-at-point 'symbol))) (cons sym sym))) @@ -5691,7 +5698,7 @@ Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER)." (interactive (list (-when-let ((symbol . placeholder) (lsp--get-symbol-to-rename)) (read-string (format "Rename %s to: " symbol) placeholder nil symbol)))) (unless newname - (user-error "A rename is not valid at this position")) + (lsp--user-error "A rename is not valid at this position")) (when-let ((edits (lsp-request "textDocument/rename" `( :textDocument ,(lsp--text-document-identifier) :position ,(lsp--cur-position) From 66adf5517c7df019f02ef07fc00667a2e1a69d92 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Mon, 9 Nov 2020 18:18:20 +0100 Subject: [PATCH 03/17] Address PR review - Use an `unless' form instead of `if-let' to check for "textDocument/rename". The latter was wrong anyway, and should've been an `if'. - Remove useless comments. - Revert the variable name change "response" -> "prepare" - Use `lsp-feature?' instead of reinventing it inline --- lsp-mode.el | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 2a7ecbe186f..d41848f1e36 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5669,29 +5669,21 @@ perform the request synchronously." (defun lsp--get-symbol-to-rename () "Get symbol to rename and placeholder at point. Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER)." - ;; Is there some connected server that supports renaming? - (if-let ((rename-provider - (or (lsp--capability :renameProvider) - (-some-> (lsp--registered-capability "textDocument/rename") - (lsp--registered-capability-options))))) - ;; Do we have prepare rename? Do it in that case. If we fail, don't do a - ;; rename, since it's invalid. - (if (lsp:rename-options-prepare-provider? rename-provider) - (when-let ((prepare - (lsp-request "textDocument/prepareRename" - (lsp--text-document-position-params)))) - (-let* (((start . end) (lsp--range-to-region - (if (lsp-range? prepare) - prepare - (lsp:prepare-rename-result-range prepare)))) - (symbol (buffer-substring start end)) - (placeholder (lsp:prepare-rename-result-placeholder prepare))) - (cons symbol (or placeholder symbol)))) - ;; We don't support prepare-rename, so just yield the symbol at point. - (let ((sym (thing-at-point 'symbol))) - (cons sym sym))) - ;; No connected server supports renaming. - (lsp--fatal-error "The connected server(s) doesn't support renaming"))) + (unless (lsp-feature? "textDocument/rename") + (lsp--fatal-error "The connected server(s) doesn't support renaming")) + (if (lsp-feature? "textDocument/prepareRename") + (when-let ((response + (lsp-request "textDocument/prepareRename" + (lsp--text-document-position-params)))) + (-let* (((start . end) (lsp--range-to-region + (if (lsp-range? response) + response + (lsp:prepare-rename-result-range response)))) + (symbol (buffer-substring start end)) + (placeholder (lsp:prepare-rename-result-placeholder response))) + (cons symbol (or placeholder symbol)))) + (let ((sym (thing-at-point 'symbol))) + (cons sym sym)))) (defun lsp-rename (newname) "Rename the symbol (and all references to it) under point to NEWNAME." From 39acd1f5ae2faa26e761b652a5bfdc239a4d2211 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Mon, 9 Nov 2020 22:36:10 +0100 Subject: [PATCH 04/17] `lsp-rename': use built-in error functions Drop `lsp--fatal-error' and `lsp--user-error', since they cannot display propertized messages anyway. Use `user-error' and `error' in `lsp-rename' and `lsp--get-symbol-to-rename' instead. --- lsp-mode.el | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index d41848f1e36..3d05312f898 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -1191,19 +1191,6 @@ Deprecated. Use `lsp-repeatable-vector' instead. " "Display lsp error message with FORMAT with ARGS." (message "%s :: %s" (propertize "LSP" 'face 'error) (apply #'format format args))) -(defun lsp--fatal-error (format &rest args) - "Show an lsp error message using `error'. -FORMAT is the `format' format to use, while ARGS provides the -fields to it." - (error "%s :: %s" (propertize "LSP" 'face 'error) - (apply #'format format args))) - -(defun lsp--user-error (format &rest args) - "Like `lsp--fatal-error', but with `user-error'. -FORMAT and ARGS are the same." - (user-error "%s :: %s" (propertize "LSP" 'face 'error) - (apply #'format format args))) - (defun lsp--eldoc-message (&optional msg) "Show MSG in eldoc." (setq lsp--eldoc-saved-message msg) @@ -5670,7 +5657,7 @@ perform the request synchronously." "Get symbol to rename and placeholder at point. Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER)." (unless (lsp-feature? "textDocument/rename") - (lsp--fatal-error "The connected server(s) doesn't support renaming")) + (error "The connected server(s) doesn't support renaming")) (if (lsp-feature? "textDocument/prepareRename") (when-let ((response (lsp-request "textDocument/prepareRename" @@ -5689,8 +5676,7 @@ Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER)." "Rename the symbol (and all references to it) under point to NEWNAME." (interactive (list (-when-let ((symbol . placeholder) (lsp--get-symbol-to-rename)) (read-string (format "Rename %s to: " symbol) placeholder nil symbol)))) - (unless newname - (lsp--user-error "A rename is not valid at this position")) + (unless newname (user-error "`lsp-rename' is invalid here")) (when-let ((edits (lsp-request "textDocument/rename" `( :textDocument ,(lsp--text-document-identifier) :position ,(lsp--cur-position) From f353c9fcbf7ab269a2ec03604dd20da6b1d9f4ef Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Mon, 9 Nov 2020 22:43:41 +0100 Subject: [PATCH 05/17] Fix renaming without prepareRename support In `lsp--get-symbol-to-rename', return nil instead of (nil . nil) if there is no symbol at point. --- lsp-mode.el | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 3d05312f898..59e385f2c85 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5655,7 +5655,8 @@ perform the request synchronously." (defun lsp--get-symbol-to-rename () "Get symbol to rename and placeholder at point. -Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER)." +Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER). Returns nil if +renaming is supported but cannot be done at point." (unless (lsp-feature? "textDocument/rename") (error "The connected server(s) doesn't support renaming")) (if (lsp-feature? "textDocument/prepareRename") @@ -5669,7 +5670,7 @@ Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER)." (symbol (buffer-substring start end)) (placeholder (lsp:prepare-rename-result-placeholder response))) (cons symbol (or placeholder symbol)))) - (let ((sym (thing-at-point 'symbol))) + (when-let ((sym (thing-at-point 'symbol))) (cons sym sym)))) (defun lsp-rename (newname) From 0e1193ed962406b99f05accd29f000493429d4e5 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Mon, 9 Nov 2020 22:44:14 +0100 Subject: [PATCH 06/17] `lsp-rename': move `user-error' into `interactive' "rename is invalid here" makes no sense if `lsp-rename' is not called interactively. --- lsp-mode.el | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 59e385f2c85..f77e2e94e05 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5675,9 +5675,11 @@ renaming is supported but cannot be done at point." (defun lsp-rename (newname) "Rename the symbol (and all references to it) under point to NEWNAME." - (interactive (list (-when-let ((symbol . placeholder) (lsp--get-symbol-to-rename)) - (read-string (format "Rename %s to: " symbol) placeholder nil symbol)))) - (unless newname (user-error "`lsp-rename' is invalid here")) + (interactive + (-if-let ((symbol . placeholder) (lsp--get-symbol-to-rename)) + (list (read-string (format "Rename %s to: " symbol) + placeholder nil symbol)) + (user-error "`lsp-rename' is invalid here"))) (when-let ((edits (lsp-request "textDocument/rename" `( :textDocument ,(lsp--text-document-identifier) :position ,(lsp--cur-position) From 0e44da7b0b4dbdc45f5b66eb05edefc943206d91 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Tue, 10 Nov 2020 08:02:22 +0100 Subject: [PATCH 07/17] Revert unrelated changes --- lsp-mode.el | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index f77e2e94e05..99a99be35bf 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5670,16 +5670,15 @@ renaming is supported but cannot be done at point." (symbol (buffer-substring start end)) (placeholder (lsp:prepare-rename-result-placeholder response))) (cons symbol (or placeholder symbol)))) - (when-let ((sym (thing-at-point 'symbol))) - (cons sym sym)))) + (when-let ((symbol (thing-at-point 'symbol))) + (cons symbol symbol)))) (defun lsp-rename (newname) "Rename the symbol (and all references to it) under point to NEWNAME." - (interactive - (-if-let ((symbol . placeholder) (lsp--get-symbol-to-rename)) - (list (read-string (format "Rename %s to: " symbol) - placeholder nil symbol)) - (user-error "`lsp-rename' is invalid here"))) + (interactive (list (-when-let ((symbol . placeholder) (lsp--get-symbol-to-rename)) + (read-string (format "Rename %s to: " symbol) placeholder nil symbol)))) + (unless newname + (user-error "`lsp-rename' is invalid here")) (when-let ((edits (lsp-request "textDocument/rename" `( :textDocument ,(lsp--text-document-identifier) :position ,(lsp--cur-position) From c5ad2f22b7472c2b63867c52d0562dea907e1845 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Tue, 10 Nov 2020 15:05:39 +0100 Subject: [PATCH 08/17] `lsp-rename': highlight renaming `lsp-rename' now highlights the identifier being renamed. To do so, refactor `lsp--get-symbol-to-rename' to return the bounds of the symbol being renamed instead of it, and allow its placeholder to be nil. The latter is necessary to reduce code duplication, as the two code paths of that function (has prepareRename, doesn't have prepareRename) would need to do the same `buffer-substring' with the same bounds. Add a new function, `lsp--read-rename', which takes the result of a call to `lsp--get-symbol-to-rename' and either `user-error's (if the result was nil) or asks the user for a new symbol, highlighting the identifier being renamed. The split was done to make them independently testable. `lsp-rename': no longer `user-error' if NEW-NAME is nil, since that only makes sense if it is called interactively. However, in the latter case, `lsp--read-rename' would do the necessary error handling already. --- lsp-mode.el | 66 ++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 16 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 99a99be35bf..62205dce039 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5654,31 +5654,65 @@ perform the request synchronously." (lsp-request "workspace/symbol" `(:query ,pattern)))) (defun lsp--get-symbol-to-rename () - "Get symbol to rename and placeholder at point. -Returns a cons (SYMBOL-TO-RENAME . PLACEHOLDER). Returns nil if -renaming is supported but cannot be done at point." + "Get a symbol to rename and placeholder at point. +Returns a cons ((START . END) . PLACEHOLDER?), and nil if +renaming is generally supported but cannot be done at point. +START and END are the bounds of the identifiers being renamed, +while PLACEHOLDER?, is either nil or a string suggested by the +language server as the initial input of a new-name prompt." (unless (lsp-feature? "textDocument/rename") (error "The connected server(s) doesn't support renaming")) (if (lsp-feature? "textDocument/prepareRename") (when-let ((response (lsp-request "textDocument/prepareRename" (lsp--text-document-position-params)))) - (-let* (((start . end) (lsp--range-to-region - (if (lsp-range? response) - response - (lsp:prepare-rename-result-range response)))) - (symbol (buffer-substring start end)) - (placeholder (lsp:prepare-rename-result-placeholder response))) - (cons symbol (or placeholder symbol)))) - (when-let ((symbol (thing-at-point 'symbol))) - (cons symbol symbol)))) + (let* ((bounds (lsp--range-to-region + (if (lsp-range? response) + response + (lsp:prepare-rename-result-range response)))) + (placeholder (lsp:prepare-rename-result-placeholder response))) + (cons bounds placeholder))) + (when-let ((bounds (bounds-of-thing-at-point 'symbol))) + (cons bounds nil)))) + +(defface lsp-face-rename '((t :inherit highlight)) + "Face used to highlight the identifier being renamed. +Renaming can be done using `lsp-rename'.") + +(defvar lsp--rename-history '() + "History for `lsp--read-rename'.") + +(defun lsp--read-rename (at-point) + "Read a new name for a `lsp-rename' at `point' from the user. +AT-POINT shall be a structure as returned by +`lsp--get-symbol-to-rename'. + +Returns a string, which should be the new name for the identifier +at point. If renaming cannot be done at point (as determined from +AT-POINT), throw a `user-error'. + +This function is for use in `lsp-rename' only, and shall not be +relied upon." + (unless at-point + (user-error "`lsp-rename' is invalid here")) + (-let* ((((start . end) . placeholder?) at-point) + ;; Do the `buffer-substring' first to not include `lsp-face-rename' + (rename-me (buffer-substring start end)) + (placeholder (or placeholder? rename-me))) + ;; We need unwind protect, as the user might cancel here, causing the + ;; overlay to linger. + (unwind-protect + (progn + (-doto (make-overlay start end) + (overlay-put 'face 'lsp-face-rename) + (overlay-put 'lsp-rename t)) + (read-string (format "Rename %s to: " rename-me) placeholder + 'lsp--rename-history)) + (lsp--remove-overlays 'lsp-rename)))) (defun lsp-rename (newname) "Rename the symbol (and all references to it) under point to NEWNAME." - (interactive (list (-when-let ((symbol . placeholder) (lsp--get-symbol-to-rename)) - (read-string (format "Rename %s to: " symbol) placeholder nil symbol)))) - (unless newname - (user-error "`lsp-rename' is invalid here")) + (interactive (list (lsp--read-rename (lsp--get-symbol-to-rename)))) (when-let ((edits (lsp-request "textDocument/rename" `( :textDocument ,(lsp--text-document-identifier) :position ,(lsp--cur-position) From 98590a05bb004d53284a39ba4b031182c70f2cd4 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Tue, 10 Nov 2020 16:24:39 +0100 Subject: [PATCH 09/17] `lsp-rename': add tests `lsp--read-rename' and `lsp--get-symbol-to-rename' now should really work, since they have a comprehensive test-suite to cover them. It has to rely on mocking a lot, though. Fix a bug in `lsp--get-symbol-to-rename': if RESPONSE is a range, getting its placeholder is wrong. Guard that with an `and' `not' `lsp-range?'. --- lsp-mode.el | 8 +++-- test/lsp-methods-test.el | 75 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 62205dce039..e538aa95c41 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5670,7 +5670,9 @@ language server as the initial input of a new-name prompt." (if (lsp-range? response) response (lsp:prepare-rename-result-range response)))) - (placeholder (lsp:prepare-rename-result-placeholder response))) + (placeholder + (and (not (lsp-range? response)) + (lsp:prepare-rename-result-placeholder response)))) (cons bounds placeholder))) (when-let ((bounds (bounds-of-thing-at-point 'symbol))) (cons bounds nil)))) @@ -5705,10 +5707,10 @@ relied upon." (progn (-doto (make-overlay start end) (overlay-put 'face 'lsp-face-rename) - (overlay-put 'lsp-rename t)) + (overlay-put 'lsp--read-rename t)) (read-string (format "Rename %s to: " rename-me) placeholder 'lsp--rename-history)) - (lsp--remove-overlays 'lsp-rename)))) + (lsp--remove-overlays 'lsp--read-rename)))) (defun lsp-rename (newname) "Rename the symbol (and all references to it) under point to NEWNAME." diff --git a/test/lsp-methods-test.el b/test/lsp-methods-test.el index a17b3d9b508..80584635040 100644 --- a/test/lsp-methods-test.el +++ b/test/lsp-methods-test.el @@ -302,3 +302,78 @@ private void extracted() { :kind "rename")])) (should-not (f-exists? old-file-name)) (should (f-exists? new-file-name)))) + +;;; `lsp-rename' +(defmacro lsp-test--simulated-input (keys &rest body) + "Execute body, while simulating the pressing KEYS. +KEYS is passed to `execute-kbd-macro', after being run trough +`kbd'. Returns the result of the last BODY form." + (declare (indent 1)) + `(let (result) + (execute-kbd-macro (kbd ,keys) 1 (lambda () (setq result (progn ,@body)))) + result)) + +(ert-deftest lsp--read-rename () + "Ensure that `lsp--read-rename' works. +If AT-POINT is nil, it throws a `user-error'. +If a placeholder is given, it shall be the default value, + +otherwise the bounds are to be used. + +Rename overlays are removed afterwards, even if the user presses +C-g." + (should-error (lsp--read-rename nil) :type 'user-error) + (with-temp-buffer + (insert "identifier") + (should (string= "identifier" + (lsp-test--simulated-input "RET" + (lsp--read-rename '((1 . 11) . nil))))) + (should (string= "ident" + (lsp-test--simulated-input "RET" + (lsp--read-rename '((1 . 10) . "ident"))))) + (goto-char 1) + (condition-case nil + (lsp-test--simulated-input "C-g" + (lsp--read-rename '((1 . 10) . "id"))) + (quit)) + (should (equal nil (get-text-property 1 'lsp--read-rename))))) + +(ert-deftest lsp--get-symbol-to-rename () + "Test `lsp--get-symbol-to-rename'. +It should error if renaming cannot be done, make use of +prepareRename as much as possible, with or without bounds, and it +should work without the latter." + ;; We don't support rename + (cl-letf (((symbol-function #'lsp-feature?) #'ignore)) + (should-error (lsp--get-symbol-to-rename) :type 'error)) + (cl-letf (((symbol-function #'lsp--text-document-position-params) #'ignore) + ((symbol-function #'lsp--range-to-region) #'identity)) + (with-temp-buffer + (insert "identifier") + (goto-char 1) + ;; We do support rename, but no prepareRename + (cl-letf (((symbol-function #'lsp-feature?) + (lambda (f) (member f '("textDocument/rename"))))) + (should (equal (cons (bounds-of-thing-at-point 'symbol) nil) + (lsp--get-symbol-to-rename))) + (goto-char (point-max)) + (insert " ") + ;; we are not on an identifier + (should (equal nil (lsp--get-symbol-to-rename)))) + ;; Do the following tests with an identifier at point + (goto-char 1) + (cl-letf (((symbol-function #'lsp-feature?) + (lambda (f) (member f '("textDocument/rename" + "textDocument/prepareRename"))))) + (cl-letf (((symbol-function #'lsp-request) + (lambda (&rest _) (lsp-make-prepare-rename-result + :range '(1 . 12) + :placeholder nil)))) + (should (equal '((1 . 12) . nil) (lsp--get-symbol-to-rename)))) + (cl-letf (((symbol-function #'lsp-request) + (lambda (&rest _) (lsp-make-prepare-rename-result + :range '(1 . 12) + :placeholder "_")))) + (should (equal '((1 . 12) . "_") (lsp--get-symbol-to-rename)))))))) + +;;; lsp-methods-test.el ends here From 87f9313aa3f4ad2f946e4a0ed9d578a84ad69930 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Tue, 10 Nov 2020 16:47:28 +0100 Subject: [PATCH 10/17] `lsp--read-rename': use a variable to hold overlay Using `lsp--remove-overlay' didn't actually bring any additional safety, given that the user could still theoretically C-g between `overlay-put' `lsp-face-rename' and `make-overlay', causing it to stay. Use a local variable to hold it instead, which doesn't decrease safety but reduces overhead. Note that it is still very unlikely (probably impossible) that a user could do that using only C-g, without a debugger. There is also no 100% safe way to do this. --- lsp-mode.el | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index e538aa95c41..5f2e5428429 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5700,17 +5700,18 @@ relied upon." (-let* ((((start . end) . placeholder?) at-point) ;; Do the `buffer-substring' first to not include `lsp-face-rename' (rename-me (buffer-substring start end)) - (placeholder (or placeholder? rename-me))) + (placeholder (or placeholder? rename-me)) + + overlay) ;; We need unwind protect, as the user might cancel here, causing the ;; overlay to linger. (unwind-protect (progn - (-doto (make-overlay start end) - (overlay-put 'face 'lsp-face-rename) - (overlay-put 'lsp--read-rename t)) + (setq overlay (-doto (make-overlay start end) + (overlay-put 'face 'lsp-face-rename))) (read-string (format "Rename %s to: " rename-me) placeholder 'lsp--rename-history)) - (lsp--remove-overlays 'lsp--read-rename)))) + (and overlay (delete-overlay overlay))))) (defun lsp-rename (newname) "Rename the symbol (and all references to it) under point to NEWNAME." From 3f941f47d68cf1aa3be905dfda644562e593869f Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Tue, 10 Nov 2020 17:42:37 +0100 Subject: [PATCH 11/17] `lsp-rename-face': add missing :group --- lsp-mode.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lsp-mode.el b/lsp-mode.el index 5f2e5428429..e456b8ac00d 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5679,7 +5679,8 @@ language server as the initial input of a new-name prompt." (defface lsp-face-rename '((t :inherit highlight)) "Face used to highlight the identifier being renamed. -Renaming can be done using `lsp-rename'.") +Renaming can be done using `lsp-rename'." + :group 'lsp-faces) (defvar lsp--rename-history '() "History for `lsp--read-rename'.") From fb9c150a83a8d0b49495e9b3d982b48903af2d14 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Tue, 10 Nov 2020 18:51:58 +0100 Subject: [PATCH 12/17] `lsp-rename': tests: check overlays The test for `lsp--read-rename' now checks whether the highlighting overlay appears and disappears again, in-between calls to a mocked `read-string', not just whether it disappears again. This is now done by checking the `face' attribute of overlays, not by checking the `lsp--read-rename' property, since that was removed with the last commit. --- test/lsp-methods-test.el | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/test/lsp-methods-test.el b/test/lsp-methods-test.el index 80584635040..9adef94ab80 100644 --- a/test/lsp-methods-test.el +++ b/test/lsp-methods-test.el @@ -313,6 +313,12 @@ KEYS is passed to `execute-kbd-macro', after being run trough (execute-kbd-macro (kbd ,keys) 1 (lambda () (setq result (progn ,@body)))) result)) +(defun lsp-test--rename-overlays? (pos) + "Return non-nil if there are `lsp-rename' overlays at POS. +POS is a point in the current buffer." + (--any? (equal (overlay-get it 'face) 'lsp-face-rename) + (overlays-at pos))) + (ert-deftest lsp--read-rename () "Ensure that `lsp--read-rename' works. If AT-POINT is nil, it throws a `user-error'. @@ -333,10 +339,21 @@ C-g." (lsp--read-rename '((1 . 10) . "ident"))))) (goto-char 1) (condition-case nil - (lsp-test--simulated-input "C-g" + (cl-letf (((symbol-function #'read-string) + (lambda (&rest _) + ;; NOTE: BEGIN and END means a range [BEGIN;END[, so at + ;; point 10, there shouldn't be an overlay anymore. This is + ;; consistent with of `bounds-thing-at-point', and it + ;; worked during manual testing. + (should (lsp-test--rename-overlays? 1)) + (should (lsp-test--rename-overlays? 9)) + (should-not (lsp-test--rename-overlays? 10))))) (lsp--read-rename '((1 . 10) . "id"))) (quit)) - (should (equal nil (get-text-property 1 'lsp--read-rename))))) + ;; but not after `lsp--read-rename' + (should-not (lsp-test--rename-overlays? 1)) + (should-not (lsp-test--rename-overlays? 9)) + (should-not (lsp-test--rename-overlays? 10)))) (ert-deftest lsp--get-symbol-to-rename () "Test `lsp--get-symbol-to-rename'. From 3507a96d09328ab73302794273f53f06675a973e Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Wed, 11 Nov 2020 15:33:40 +0100 Subject: [PATCH 13/17] `lsp-rename': make prepareRename configurable The new defcustom `lsp-rename-use-prepare' now controls whether prepareRename should be used or not for `lsp--get-symbol-to-rename'. --- lsp-mode.el | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lsp-mode.el b/lsp-mode.el index e456b8ac00d..923c7d64f30 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5653,6 +5653,13 @@ perform the request synchronously." (seq-map #'lsp--symbol-information-to-xref (lsp-request "workspace/symbol" `(:query ,pattern)))) +(defcustom lsp-rename-use-prepare t + "Whether `lsp-rename' should do a prepareRename first. +For some language servers, textDocument/prepareRename might be +too slow, in which case this variable may be set to nil. +`lsp-rename' will then use `thing-at-point' `symbol' to determine +the symbol to rename at point.") + (defun lsp--get-symbol-to-rename () "Get a symbol to rename and placeholder at point. Returns a cons ((START . END) . PLACEHOLDER?), and nil if @@ -5662,7 +5669,7 @@ while PLACEHOLDER?, is either nil or a string suggested by the language server as the initial input of a new-name prompt." (unless (lsp-feature? "textDocument/rename") (error "The connected server(s) doesn't support renaming")) - (if (lsp-feature? "textDocument/prepareRename") + (if (and lsp-rename-use-prepare (lsp-feature? "textDocument/prepareRename")) (when-let ((response (lsp-request "textDocument/prepareRename" (lsp--text-document-position-params)))) From 7e920895732c54e832bf059eb1bbd2da88123d09 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Sat, 14 Nov 2020 08:49:02 +0100 Subject: [PATCH 14/17] Address nit: `-doto' The overlay is modified once, and assigned to a variable anyway. `setq' it to `make-overlay' directly and `overlay-put' the face to it afterwards. --- lsp-mode.el | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 923c7d64f30..5f6dff8a610 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5715,8 +5715,9 @@ relied upon." ;; overlay to linger. (unwind-protect (progn - (setq overlay (-doto (make-overlay start end) - (overlay-put 'face 'lsp-face-rename))) + (setq overlay (make-overlay start end)) + (overlay-put overlay 'face 'lsp-face-rename) + (read-string (format "Rename %s to: " rename-me) placeholder 'lsp--rename-history)) (and overlay (delete-overlay overlay))))) From 778c9e9d4f2ce8e5eeaf155a17eb9f35b37e3d69 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Tue, 17 Nov 2020 18:31:25 +0100 Subject: [PATCH 15/17] Use a consistent placeholder face `lsp-rename-placeholder-face' is now used to display the new name instead of reusing the face of the identifier in the buffer. The reasoning here is that reusing the symbols's face directly would yield inconsistent placeholders, while conveying no additional information, given that the thing being renamed is shown in that very same prompt as it appears in the buffer. --- lsp-mode.el | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lsp-mode.el b/lsp-mode.el index 5f6dff8a610..ba443a92c25 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5689,6 +5689,12 @@ language server as the initial input of a new-name prompt." Renaming can be done using `lsp-rename'." :group 'lsp-faces) +(defface lsp-rename-placeholder-face '((t :inherit font-lock-variable-name-face)) + "Face used to display the rename placeholder in. +When calling `lsp-rename' interactively, this will be the face of +the new name." + :group 'lsp-faces) + (defvar lsp--rename-history '() "History for `lsp--read-rename'.") @@ -5709,6 +5715,7 @@ relied upon." ;; Do the `buffer-substring' first to not include `lsp-face-rename' (rename-me (buffer-substring start end)) (placeholder (or placeholder? rename-me)) + (placeholder (propertize placeholder 'face 'lsp-rename-placeholder-face)) overlay) ;; We need unwind protect, as the user might cancel here, causing the From 126f74e65e319a74bc063531f4193f3180d15b67 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Tue, 17 Nov 2020 18:34:46 +0100 Subject: [PATCH 16/17] `lsp-face-rename': underline instead of highlight Because `lsp-face-highlight-*' also derive from `highlight', the symbol being renamed could only have either a documentHighlight or a rename face. This caused inconsistent results with `lsp-idle-delay' = 0, where either would sometimes win. By using :underline instead, the symbol being renamed can be marked as such while having a documentHighlight overlay active. --- lsp-mode.el | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lsp-mode.el b/lsp-mode.el index ba443a92c25..6eb5304bbe3 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5684,7 +5684,7 @@ language server as the initial input of a new-name prompt." (when-let ((bounds (bounds-of-thing-at-point 'symbol))) (cons bounds nil)))) -(defface lsp-face-rename '((t :inherit highlight)) +(defface lsp-face-rename '((t :underline t)) "Face used to highlight the identifier being renamed. Renaming can be done using `lsp-rename'." :group 'lsp-faces) From ae280aeb75297067707428e394cd912537640396 Mon Sep 17 00:00:00 2001 From: Nikita Bloshchanevich Date: Fri, 20 Nov 2020 10:43:05 +0100 Subject: [PATCH 17/17] Make `lsp-rename-history' public `lsp-rename-history' should be private so that it can be excluded from savehist. In addition, from looking at the code of other packages, history variables tend to be public: `magit-revision-history', `evil-ex-history', .... --- lsp-mode.el | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lsp-mode.el b/lsp-mode.el index 6eb5304bbe3..36e40a77f57 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5695,7 +5695,7 @@ When calling `lsp-rename' interactively, this will be the face of the new name." :group 'lsp-faces) -(defvar lsp--rename-history '() +(defvar lsp-rename-history '() "History for `lsp--read-rename'.") (defun lsp--read-rename (at-point) @@ -5726,7 +5726,7 @@ relied upon." (overlay-put overlay 'face 'lsp-face-rename) (read-string (format "Rename %s to: " rename-me) placeholder - 'lsp--rename-history)) + 'lsp-rename-history)) (and overlay (delete-overlay overlay))))) (defun lsp-rename (newname)