lsp-rename: improve UI (#2317)

* `lsp-rename': check if supported

`lsp-rename' doesn't check whether renaming is supported first. Do that,
throwing an error otherwise.

Fixes #2314.

* `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.

* 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-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.

* 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-rename': move `user-error' into `interactive'

"rename is invalid here" makes no sense if `lsp-rename' is not called
interactively.

* Revert unrelated changes

* `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-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--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-rename-face': add missing :group

* `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.

* `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'.

* 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.

* 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-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.

* 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', ....

Author: nbfalcon

lsp-mode.el

@@ -5653,30 +5653,85 @@ 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 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"
-                                        (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))))
+  "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 (and lsp-rename-use-prepare (lsp-feature? "textDocument/prepareRename"))
+      (when-let ((response
+                  (lsp-request "textDocument/prepareRename"
+                               (lsp--text-document-position-params))))
+        (let* ((bounds (lsp--range-to-region
+                        (if (lsp-range? response)
+                            response
+                          (lsp:prepare-rename-result-range 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))))
+
+(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)
+
+(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'.")
+
+(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))
+          (placeholder (propertize placeholder 'face 'lsp-rename-placeholder-face))
+
+          overlay)
+    ;; We need unwind protect, as the user might cancel here, causing the
+    ;; overlay to linger.
+    (unwind-protect
+        (progn
+          (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)))))
 
 (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 "A rename is not valid at this position"))
+  (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)

test/lsp-methods-test.el

@@ -302,3 +302,95 @@ 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))
+
+(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'.
+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
+        (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))
+    ;; 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'.
+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