`lsp-rename`: improve UI #2317

nbfalcon · 2020-11-09T16:56:54Z

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

yyoncho · 2020-11-09T17:09:24Z

I am not sure we need anything more than...

(unless (lsp-feature? "textDocument/rename")
  (user-error "Rename not..." ))

in the lsp-rename interactive form.

nbfalcon · 2020-11-09T17:10:43Z

Don't merge yet - there is a bug that I need to fix. As to your comment, if we use that new function, there will be a nice LSP :: message instead. Do you consider that undesirable? If so, I'll revert.

nbfalcon · 2020-11-09T17:13:10Z

Also, the prompt is now a bit nicer, because I don't use buffer-substring-no-properties.

nbfalcon · 2020-11-09T17:20:13Z

The bug is fixed; rename now works
I use lsp-feature? now, thanks for the tip. It is really much simpler, and probably less fragile.

yyoncho · 2020-11-09T17:25:10Z

lsp-mode.el

+  "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 (lsp-feature? "textDocument/rename")


(-if-let (lsp-feature? "xxx") lsp-feature? ...)

returns "xxx".

Can you use the unless form as suggested?

The if-let was wrong, and was a refactoring leftover. I use unless instead as you suggested now.

yyoncho · 2020-11-09T17:26:18Z

lsp-mode.el

+        ;; 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.


can you avoid putting comments like that? This should be evident from the content of the next line.

I usually don't do that - this one was an accident. Fixed it.

nbfalcon · 2020-11-09T17:44:33Z

Note that the error messages aren't propertized, which may require further thought.

nbfalcon · 2020-11-09T17:47:33Z

Because of the unpropertized error messages, I'd advise you to wait before merging.

yyoncho · 2020-11-09T17:45:33Z

lsp-mode.el

+  (if (lsp-feature? "textDocument/prepareRename")
+      (when-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)))
+                                (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))))
-    (let ((symbol (thing-at-point 'symbol t)))
-      (cons symbol symbol))))
+    ;; We don't support prepare-rename, so just yield the symbol at point.
+    (let ((sym (thing-at-point 'symbol)))
+      (cons sym sym))))


prepareRename does not work with lsp-feature?

Can you revert the renamed symbols?

Can you remove irrelevant comments?

What should I use instead? The old code?
2, 3. Will do.

The old code should be fine.

@yyoncho Are you sure that it doesn't though? From stepping trough that with edebug, lsp-feature? returned a complex result for "textDocument/prepareRename", while consistently returning nil if I evaluate it somewhere without lsp-mode enabled (a scratch buffer, for instance).

I checked and lsp-method-requirements are actually implemented so lsp-feature? will work. It is odd that we haven't replaced the lsp--get-symbol-to-rename

yyoncho · 2020-11-09T17:46:15Z

lsp-mode.el

  "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)


seems like this is redundant: there is lsp--error.

Ignore... I missed the fact that it is using error not message. But still, is this function used in this PR? Are we going to replace all error calls with that?

lsp--error logs a message, which doesn't cause evaluation to abort.

This function is used by this PR, which is why I added it. However, replacing all error calls might be a bit much. From looking at the code, the (error (lsp-error ...)) idiom seems common, however it doesn't solve the text-properties issue.

yyoncho · 2020-11-09T17:49:21Z

lsp-mode.el

+  (error "%s :: %s" (propertize "LSP" 'face 'error)
+         (apply #'format format args)))
+
+(defun lsp--user-error (format &rest args)


I don't see any effect from this propertize - the message is displayed with default font.

Yes, that is the issue currently.

nbfalcon · 2020-11-09T21:54:50Z

I cleaned up the history a bit. Aside from that, I eliminated the new error functions, since they don't work they way I wanted anyway, and use user-error and normal error instead. This is probably also more idiomatic. Additionally, if prepareRename is unsupported, nil is returned for thing-at-point = nil in lsp--get-symbol-to-rename (instead of (nil . nil)).

nbfalcon · 2020-11-09T21:56:13Z

This PR is now ready.

yyoncho · 2020-11-09T22:02:12Z

lsp-mode.el

+  (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")))


please revert that.

yyoncho · 2020-11-09T22:46:12Z

lsp-mode.el

          (cons symbol (or placeholder symbol))))
-    (let ((symbol (thing-at-point 'symbol t)))
-      (cons symbol symbol))))
+    (when-let ((sym (thing-at-point 'symbol)))


why do you rename symbol? Why do you introduce when-let?

Let's clarify that for future reference - do not introduce random renames, restructuring, etc - nobody is doing this PR in this repo. You are making the life of the reviewers harder than it should be. I have to go and test each that you have changed to verify that each line of all lines that you have changed does not introduce a regression. I cannot spend that much time reviewing trivial changes chasing for regressions.

Ok, I won't do that in the future. The when-let is because that function should return nil if there is nothing to rename at point. Without that when-let, if (thing-at-point symbol) were to yield nil, the function would return (cons nil nil), instead of nil as it should. This, unlike I had assumed, didn't cause any real problems, because -if-let checks the bindings, not the input against nil.

(eq nil nil) ; => true (eq nil (cons nil nil)) ; => false (equal nil (cons nil nil)) ; => false

You will still rename all local variables, introduce random changes without documenting/testing them based on assumptions and you will expect the rest of the team to do that for you?!

Sorry, I missspelled won't...

kiennq · 2020-11-10T05:27:36Z

lsp-mode.el

                                    response
                                  (lsp:prepare-rename-result-range response))))
-                (symbol (buffer-substring-no-properties start end))
+                (symbol (buffer-substring start end))


Why we need to use buffer-substring instead of -no-properties version?
Do we need the highlighting when prompting?

Compare:

VS with highlighting:

Do you prefer the former?

kiennq · 2020-11-10T05:45:43Z

lsp-mode.el

+    (when-let ((sym (thing-at-point 'symbol)))
+      (cons sym sym))))

 (defun lsp-rename (newname)


Why don't we just check the capability in this command (public) instead of changing internal one?
I don't think guarding every internal one is desirable/good from perf POV. Just guard once in public one should be enough, shouldn't it?

If we guard only in lsp-rename, the user would be prompted to enter a new name even though renaming is unsupported.

Also, it's fast enough:

(with-current-buffer "main.c" (benchmark-run 1000 (ignore (lsp-feature? "textDocument/rename"))))

yields 0.037 seconds.

Calling it only twice gives me 2ms timings, which is less than a 120FPS monitor's refresh latency.

If we guard only in lsp-rename, the user would be prompted to enter a new name even though renaming is unsupported.

see #2317 (comment)
yyoncho@443f801 .

Is it better to move the check outside of lsp--get-symbol-to-rename though?

I think it would be better to do that. lsp--get-symbol-to-rename is internal function, which should only be called once the condition (has capability for rename) has been cleared.
That would be a good convention to follow, else we will have to add the check for every function, which can be overwhelming.

It's actually called only once though, in lsp--get-symbol-to-rename. We needn't add checks in every function, because the server will respond with a proper "not implemented" error if it isn't supported. Here, we cannot rely on that though, since we mustn't prompt the user for a name if the rename wouldn't work anyway.

I think you don't get my point. Let say if the lsp--get-symbol-to-rename is gone/refactored, which can happen since it's internal function, now we will need to find a new place to put the capacity checking.
On other hand, lsp-rename is public function, and is unlikely to change without a breaking change, put the capacity guard there is the best, since user has to use that function to actually go through rename.
Is there any specific concern why the capacity guard has to be put in lsp--get-symbol-to-rename?

We could add an additional lsp-feature? check to lsp-rename. However, the check in lsp--get-symbol-to-rename must stay: the interactive block is executed before the body, meaning the user would get prompted for the symbol to rename even though rename might not be possible at all or at point.

You can put the check in (interactive) form too. Like what @yyoncho suggested.
If you hate that, you can even make (interactive '(nil)) and check for newName outside of interactive block and call to lsp--get-symbol-to-rename.

nbfalcon · 2020-11-10T14:04:29Z

I'll extend this to highlight the symbol being renamed; with the refactoring necessary do that, I can add tests ensuring that it will work in all cases.

nbfalcon · 2020-11-10T15:27:35Z

The various `lsp-rename' helper functions are now thoroughly tested, and the symbol being renamed is highlighted.

nbfalcon · 2020-11-10T17:56:48Z

In summary:

lsp-rename now checks whether it is supported in the interactive form, trough a new function
lsp-rename no longer user errors if the input is nil
Renamed identifiers are marked in the buffer using lsp-face-rename
The symbol to be renamed is propertized in the prompt, as is the initial input
Lots of tests for the two functions. I'll need to investigate the CI failures; the tests work in isolation on my machine just fine.

nbfalcon · 2020-11-10T18:25:26Z

Here is a screenshot:

kiennq · 2020-11-14T01:56:52Z

lsp-mode.el

+(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'.


It would be better to specify what the structure look like instead of just saying it will be the return value from lsp--get-symbol-to-rename

The structure is defined in lsp--get-symbol-to-rename's docstring.

kiennq · 2020-11-14T02:07:15Z

lsp-mode.el

+    ;; overlay to linger.
+    (unwind-protect
+        (progn
+          (setq overlay (-doto (make-overlay start end)


nit: is there a need for -doto in this case, since there's only on form?

Fixed, thanks.

nbfalcon · 2020-11-14T07:56:12Z

There are two issues with this as of now (hence the "Draft" status):

lsp-rename sometimes override symbol-highlight overlays, and sometimes don't, which leads to incosistency (fixed in 06e8631 by using :underline instead)
For some language servers, prepareRename can take too long or time out (rust-analyzer). Maybe somehow waiting until C-g would be better here, or perhaps setting a lower timeout, notifying the user somehow. Not using prepareRename is also a possibility, but something I'd prefer to avoid. (not a regression caused by this PR, this issue was already present before).
Fix CI
Upcoming 3.16 spec: handle { defaultBehaviour: true } from prepareRename

And perhaps some ToDos:

Support rename preview, like VSCode (out of scope, danielmartin is working on it)

danielmartin · 2020-11-15T15:53:10Z

And perhaps some ToDos:

Support rename preview, like VSCode

I'm working on this feature at the moment. It'll be orthogonal to the changes proposed here (if accepted). You'll enter the rename preview mode if you invoke lsp-rename with C-u.

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

In `lsp--get-symbol-to-rename', handle the server not providing a placeholder by limiting the `when-let' block to the prepare result.

- 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

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.

In `lsp--get-symbol-to-rename', return nil instead of (nil . nil) if there is no symbol at point.

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

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

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.

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.

The new defcustom `lsp-rename-use-prepare' now controls whether prepareRename should be used or not for `lsp--get-symbol-to-rename'.

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

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.

yyoncho

Looks good. Please drop a note which servers you have tested with.

Few minor nits suggestions:

yyoncho · 2020-11-19T17:07:28Z

lsp-mode.el

+the new name."
+  :group 'lsp-faces)
+
+(defvar lsp--rename-history '()


Shouldn't this variable be public to avoid persisting it?

You make a good point, and from looking at other packages (magit, evil (evil-ex)), they also seem to be using public history variables. I'm making this public.

yyoncho · 2020-11-19T17:08:02Z

lsp-mode.el

+          ;; 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))


I would vote for using different faces for placeholder and for the thing to rename. Using bold face seems to be a good candidate for one of them (to avoid being too colorful).

Do you propose overriding the face of the thing to be renamed the same way as is done for the placeholder? What we might also want to explore is adding a face to it, using add-face-text-property. I'll post some screenshots of various approaches soon.

I am proposing using 2 different faces like this:

(let ((placeholder "foo") (rename-me "foo")) (read-string (format "Rename %s to: " (propertize rename-me 'face 'bold)) (propertize placeholder 'face 'font-lock-variable-name-face) 'lsp-rename-history))

lsp-mode.el

nbfalcon · 2020-11-20T09:41:28Z

I tested with jdtls, rust-analyzer, ccls and pyright.

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

nbfalcon · 2020-11-20T14:28:35Z

rust-analyzer seems to send incorrect ranges in certain cases (sometimes with an extra :uri), which causes the rename overlays and placeholder to be borked (not a regression, master also has this). This is a spec violation though, as the result shall be a Range, not something containing a :uri.

nbfalcon · 2020-11-20T15:04:59Z

Digging trough RA's code, it seems that the strange results are fixed, at least in master, because there is no code returning a uri in handle_prepare_rename.

yyoncho · 2020-11-20T20:13:34Z

Thank you.

I merged it as it is - the cosmetics about the faces can be discussed separatelly.

yyoncho reviewed Nov 9, 2020

View reviewed changes

nbfalcon force-pushed the bugfix/lsp-rename-check branch 2 times, most recently from c80fee1 to e4fbe8b Compare November 9, 2020 21:52

yyoncho reviewed Nov 9, 2020

View reviewed changes

kiennq reviewed Nov 10, 2020

View reviewed changes

nbfalcon marked this pull request as draft November 10, 2020 14:03

nbfalcon marked this pull request as ready for review November 10, 2020 15:27

nbfalcon force-pushed the bugfix/lsp-rename-check branch from fc5cc8b to 42d522f Compare November 10, 2020 18:03

nbfalcon changed the title ~~lsp-rename: check if supported~~ lsp-rename: check if supported, improve UI Nov 11, 2020

nbfalcon force-pushed the bugfix/lsp-rename-check branch from 42d522f to 280b1e1 Compare November 11, 2020 12:42

nbfalcon marked this pull request as draft November 11, 2020 14:47

nbfalcon changed the title ~~lsp-rename: check if supported, improve UI~~ lsp-rename: improve UI Nov 11, 2020

kiennq reviewed Nov 14, 2020

View reviewed changes

nbfalcon added 16 commits November 18, 2020 13:12

`lsp-rename': check if supported

44accbb

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

`lsp-rename': support ommitted :placeholder

ef951ec

In `lsp--get-symbol-to-rename', handle the server not providing a placeholder by limiting the `when-let' block to the prepare result.

`lsp-rename': use built-in error functions

39acd1f

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

f353c9f

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'

0e1193e

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

Revert unrelated changes

0e44da7

`lsp-rename-face': add missing :group

3f941f4

`lsp-rename': make prepareRename configurable

3507a96

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'

7e92089

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.

yyoncho reviewed Nov 19, 2020

View reviewed changes

Make `lsp-rename-history' public

ae280ae

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

nbfalcon force-pushed the bugfix/lsp-rename-check branch from 06e8631 to ae280ae Compare November 20, 2020 10:04

yyoncho approved these changes Nov 20, 2020

View reviewed changes

yyoncho marked this pull request as ready for review November 20, 2020 20:12

yyoncho merged commit 57ea180 into master Nov 20, 2020

nbfalcon mentioned this pull request Nov 29, 2020

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

Merged

Uh oh!

Uh oh!

lsp-rename: improve UI #2317

lsp-rename: improve UI #2317

Uh oh!

Conversation

nbfalcon commented Nov 9, 2020

Uh oh!

yyoncho commented Nov 9, 2020

Uh oh!

nbfalcon commented Nov 9, 2020

Uh oh!

nbfalcon commented Nov 9, 2020

Uh oh!

nbfalcon commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbfalcon commented Nov 9, 2020

Uh oh!

nbfalcon commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbfalcon Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbfalcon commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbfalcon commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbfalcon Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

`lsp-rename`: improve UI #2317

`lsp-rename`: improve UI #2317

nbfalcon Nov 9, 2020 •

edited

Loading

nbfalcon commented Nov 9, 2020 •

edited

Loading

nbfalcon Nov 10, 2020 •

edited

Loading

nbfalcon commented Nov 14, 2020 •

edited

Loading