Close #52: Use entire line as xref summary when possible

[cmm]

• eglot.el (eglot--xref-make): Make the item with summary being the
  whole line at given location, when possible
  (eglot--locations-to-xrefs,eglot--path-xrefs,eglot--buf-xrefs):
  Convert location list to xref list, trying to open each file just once.
  (xref-backend-identifier-at-point):
  (xref-backend-definitions):
  (xref-backend-references): Pass the list of locations to
  eglot--locations-to-xrefs.

[cmm]

This is an enhanced version of #103, my github-fu is probably lacking...

[mkcms]

Some of these lines are really long, you should truncate them to 80 characters.

[cmm]

eglot.el already has lines over 80 characters long, so I don't know if this style fix is worth the noise of an additional commit (but I'll be happy to do it if the author says so, of course).

[mkcms]

I don't know if this style fix is worth the noise of an additional commit

You can git commit --amend to amend changes to the last commit, then push with --force.

Line 1384 is very long (142 characters) and definitely needs to be fixed. You can see the problem with M-x toggle-truncate-lines.

[cmm]

Wasn't sure GitHub processes force-pushes rewritten branches correctly :)

[mkcms]

@cmm Have you assigned copyright to the FSF?

[cmm]

totally missed the requirement :/.
will do, in a couple of weeks (not near a proper computer right now).

[cmm]

@cmm Have you assigned copyright to the FSF?

OK, I give up. Is there a simple (or not simple, but at least clear) step-by-step process for this anywhere?

(I did the assignment dance once, a long time ago, when I was working on Guile, but don't remember anything; quick googling only gives me explanations as to why I should assign my copyright to FSF, blog entries vaguely detailing changes to the procedure (without referencing said procedure), etc. Life's too short for this shit)

[joaotavora]

Just send email to emacs-devel@gnu.org and explain that you want to assign copyright. You'll get instructions off-list. João On Nov 2, 2018 09:43, "Michael Livshin" <notifications@github.com> wrote: @cmm <https://github.com/cmm> Have you assigned copyright to the FSF? OK, I give up. is there a simple (or not simple, but at least clear) step-by-step process for this? (I did the assignment dance once, a long time ago, when I was working on Guile, but don't remember anything; quick googling only gives me explanations as to why I should assign my copyright to FSF, blog entries vaguely detailing changes to the procedure (without referencing said procedure), etc. Life's too short for this shit) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#127 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXnw5v-OeuPXcoQSc9MSYfRBjfi4pDXks5urBPcgaJpZM4XK_jb> .

[cmm]

(in case anyone wonders about the status of this PR: assign at gnu dot org has been ignoring me for 2 weeks now, no clue why)

[joaotavora]

(in case anyone wonders about the status of this PR: assign at gnu dot org has been ignoring me for 2 weeks now, no clue why)

Have you contacted emacs-devel@gnu.org? If so, two weeks is a reasonable delay to ping them there again

[cmm]

I did, yes, and also pinged the assign address a week ago. Eli says there's a new person handling assignments, so I'm not alarmed yet.

[joaotavora]

I did, yes, and also pinged the assign address a week ago. Eli says there's a new person handling assignments, so I'm not alarmed yet.

Yeah, well if you're serious about doing the assingment, I don't see any problem in accepting your contribution before you get it done.

But this doesn't mean I've accepted the contribution yet :-), because I haven't read it properly yet.

[cmm]

assignment done

[joaotavora]

Hi @cmm,

So I've finally had a look at this and have the following comments:

1. Understandably, you don't seem to be taking into account the column-counting problems of Fix #124: Treat :character offsets as UTF-16 code units #125. These need to be addressed.
2. I'd rather have a less intrusive solution with fewer helper functions and contained in eglot--xref-make. Can you find anything wrong with this approach?

diff --git a/eglot.el b/eglot.el
index 4996f5b..2b90277 100644
--- a/eglot.el
+++ b/eglot.el
@@ -1478,13 +1478,37 @@ DUMMY is ignored."
 (advice-add 'xref-find-definitions :after #'eglot--xref-reset-known-symbols)
 (advice-add 'xref-find-references :after #'eglot--xref-reset-known-symbols)
 
-(defun eglot--xref-make (name uri position)
-  "Like `xref-make' but with LSP's NAME, URI and POSITION."
-  (cl-destructuring-bind (&key line character) position
-    (xref-make name (xref-make-file-location
-                     (eglot--uri-to-path uri)
-                     ;; F!@(#*&#$)CKING OFF-BY-ONE again
-                     (1+ line) character))))
+(defun eglot--xref-make (name uri range)
+  "Like `xref-make' but with LSP's NAME, URI and RANGE.
+To provide more information, try to visit the target file and
+extract the full line of the reference as the reference's
+summary."
+  (cl-destructuring-bind (&key start end) range
+    (let* ((file (eglot--uri-to-path uri))
+           (visiting (find-buffer-visiting file))
+           (buffer (or visiting
+                       (and (file-readable-p file)
+                            (find-file-noselect file)))))
+      (pcase-let
+          ((`(,summary ,line ,column)
+            (if buffer
+                (unwind-protect
+                    (with-current-buffer buffer
+                      (eglot--widening
+                       (goto-char (eglot--lsp-position-to-point start))
+                       (let* ((bol (point-at-bol))
+                              (summary
+                               (buffer-substring-no-properties bol
+                                                               (point-at-eol))))
+                         (add-face-text-property
+                          (- (point) bol)
+                          (- (eglot--lsp-position-to-point end) bol)
+                          'highlight t summary)
+                         (list summary (current-line) (current-column)))))
+                  (unless visiting (kill-buffer buffer)))
+              ;; Fall back to the "dumb" strategy
+              (list name (1+ (cl-getf start :line)) (cl-getf start :character)))))
+        (xref-make summary (xref-make-file-location file line column))))))
 
 (defun eglot--sort-xrefs (xrefs)
   (sort xrefs
@@ -1537,7 +1561,7 @@ DUMMY is ignored."
                (if (vectorp definitions) definitions (vector definitions)))))
     (eglot--sort-xrefs
      (mapcar (jsonrpc-lambda (&key uri range)
-               (eglot--xref-make identifier uri (plist-get range :start)))
+               (eglot--xref-make identifier uri range))
              locations))))
 
 (cl-defmethod xref-backend-references ((_backend (eql eglot)) identifier)
@@ -1552,7 +1576,7 @@ DUMMY is ignored."
     (eglot--sort-xrefs
      (mapcar
       (jsonrpc-lambda (&key uri range)
-        (eglot--xref-make identifier uri (plist-get range :start)))
+        (eglot--xref-make identifier uri range))
       (jsonrpc-request (eglot--current-server-or-lose)
                        :textDocument/references
                        (append
@@ -1566,7 +1590,7 @@ DUMMY is ignored."
      (mapcar
       (jsonrpc-lambda (&key name location &allow-other-keys)
         (cl-destructuring-bind (&key uri range) location
-          (eglot--xref-make name uri (plist-get range :start))))
+          (eglot--xref-make name uri range)))
       (jsonrpc-request (eglot--current-server-or-lose)
                        :workspace/symbol
                        `(:query ,pattern))))))

[cmm]

@joaotavora looks great (also TIL about pcase-let).

#125 is dealt with here by using find-file-noselect instead of insert-file-contents-literally, right? Character encodings make my head hurt.

(Helper functions vs painful indentation vs code going to outer space rightwards is a matter of taste, and yours is more important.)

[joaotavora]

#125 is dealt with here by using find-file-noselect instead of insert-file-contents-literally, right? Character encodings make my head hurt.

Yes, but you could also have used insert-file-contents and you would probably be OK.

(Helper functions vs painful indentation vs code going to outer space rightwards is a matter of taste, and yours is more important.)

Not 100% what those things mean, but I like to keep the ability to reason about a feature locally. Given this can be coded in about half the loc with no abstractions to learn, it's an easy decision.

But I'll give you credit for the idea and original working implementation. :-)

[cmm]

The "idea" being the fact that lsp-mode works this way, FWIW :)

[joaotavora]

The "idea" being the fact that lsp-mode works this way, FWIW :)

Oh, good to know.

[mkcms]

@joaotavora Won't your solution be doing more IO work when there are many xrefs in a file which is not visited by a buffer? Currently you're killing the visited buffer after each call to eglot--xref-make. Maybe you can set it up to kill the buffer in a local post-command-hook, for example (without extra functions or variables)

[joaotavora]

@joaotavora Won't your solution be doing more IO work when there are many xrefs in a file which is not visited by a buffer? Currently you're killing the visited buffer after each call to eglot--xref-make. Maybe you can set it up to kill the buffer in a local post-command-hook, for example (without extra functions or variables)

Good catch. This is untested, this is why I put it in a scratch branch.

[joaotavora]

Maybe you can set it up to kill the buffer in a local post-command-hook, for example (without extra functions or variables)

Don't know if this would work, but it would be gross, right? In this situation I'd rather have a helper function do the grouping.

[mkcms]

Don't know if this would work, but it would be gross, right? In this situation I'd rather have a helper function do the grouping.

Yes, it would be pretty hacky.

[joaotavora]

@mkcms have a look at the latest one. It's still completely untested (don't have any LS's right now).

[mkcms]

@mkcms have a look at the latest one.

Works well, but sometimes I get the error below using ccls in emacs repo (src/eval.c:90:19)
I'll test it a more later when I get to a proper machine.

Debugger entered--Lisp error: (args-out-of-range 22 41)
  add-face-text-property(22 41 highlight t "  if (!NILP (Vcommand_error_function))")
  #f(compiled-function (end beg) #<bytecode 0xffda59>)(30601 30582)
  eglot--xref-make(#("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))) "file:///mnt/f/devel/repos/emacs/src/keyboard.c" (:start (:line 1004 :character 4) :end (:line 1004 :character 23)))
  #f(compiled-function (&rest rest) #<bytecode 0x3ee4d9>)(:uri "file:///mnt/f/devel/repos/emacs/src/keyboard.c" :range (:start (:line 1004 :character 4) :end (:line 1004 :character 23)))
  apply(#f(compiled-function (&rest rest) #<bytecode 0x3ee4d9>) (:uri "file:///mnt/f/devel/repos/emacs/src/keyboard.c" :range (:start (:line 1004 :character 4) :end (:line 1004 :character 23))))
  #f(compiled-function (jsonrpc-lambda-elem8) #<bytecode 0x1291671>)((:uri "file:///mnt/f/devel/repos/emacs/src/keyboard.c" :range (:start (:line 1004 :character 4) :end (:line 1004 :character 23))))
  mapcar(#f(compiled-function (jsonrpc-lambda-elem8) #<bytecode 0x1291671>) [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 1583 :character 2) :end (:line 1583 :character 21))) (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 1590 :character 1) :end (:line 1590 :character 20))) (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 4073 :character 14) :end (:line 4073 :character 33))) (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 4074 :character 2) :end (:line 4074 :character 21))) (:uri "file:///mnt/f/devel/repos/emacs/src/keyboard.c" :range (:start (:line 995 :character 4) :end (:line 995 :character 23))) (:uri "file:///mnt/f/devel/repos/emacs/src/keyboard.c" :range (:start (:line 1004 :character 4) :end (:line 1004 :character 23))) (:uri "file:///mnt/f/devel/repos/emacs/src/keyboard.c" :range (:start (:line 1006 :character 2) :end (:line 1006 :character 21))) (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31))) (:uri "file:///mnt/f/devel/repos/emacs/src/lisp.h" :range (:start (:line 3834 :character 19) :end (:line 3834 :character 38)))])
  #f(compiled-function (backend identifier) #<bytecode 0x10499e5>)(eglot #("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))))
  apply(#f(compiled-function (backend identifier) #<bytecode 0x10499e5>) eglot #("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))))
  xref-backend-references(eglot #("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))))
  xref--find-xrefs(#("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))) references #("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))) nil)
  #f(compiled-function (identifier) "Find references to the identifier at point.\nThis command might prompt for the identifier as needed, perhaps\noffering the symbol at point as the default.\nWith prefix argument, or if `xref-prompt-for-identifier' is t,\nalways prompt for the identifier.  If `xref-prompt-for-identifier'\nis nil, prompt only if there's no usable symbol at point." (interactive #f(compiled-function () #<bytecode 0x1277fc1>)) #<bytecode 0xbe802d>)(#("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))))
  apply(#f(compiled-function (identifier) "Find references to the identifier at point.\nThis command might prompt for the identifier as needed, perhaps\noffering the symbol at point as the default.\nWith prefix argument, or if `xref-prompt-for-identifier' is t,\nalways prompt for the identifier.  If `xref-prompt-for-identifier'\nis nil, prompt only if there's no usable symbol at point." (interactive #f(compiled-function () #<bytecode 0x9f57ed>)) #<bytecode 0xbe802d>) #("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))))
  xref-find-references(#("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))))
  funcall-interactively(xref-find-references #("Vsignaling_function" 0 19 (:containerName nil :kind 13 :locations [(:uri "file:///mnt/f/devel/repos/emacs/src/eval.c" :range (:start (:line 89 :character 12) :end (:line 89 :character 31)))] :textDocumentPositionParams (:textDocument (:uri "file:///mnt/f/devel/repos/emacs/src/eval.c") :position (:line 89 :character 12)))))
  call-interactively(xref-find-references nil nil)
  command-execute(xref-find-references)

[joaotavora]

Works well, but sometimes I get the error below using ccls in emacs repo (src/eval.c:90:19)

Probably if the LSP range exceeds the line end and we need a check there.

[mkcms]

Probably if the LSP range exceeds the line end and we need a check there.

Hmm, I just looked at your commit, I believe the problem is that you call eglot--range-region when binding beg . end in the buffer which is not visiting the target buffer. You should calculate beg and end in the collect lambda.

[mkcms]

You should calculate beg and end in the collect lambda.

Like this

diff --git a/eglot.el b/eglot.el
index 1eb88be..8069563 100644
--- a/eglot.el
+++ b/eglot.el
@@ -1495,13 +1495,13 @@ DUMMY is ignored."
   "Like `xref-make' but with LSP's NAME, URI and RANGE.
 Try to visit the target file for a richer summary line."
   (pcase-let*
-      ((`(,beg . ,end) (eglot--range-region range))
-       (file (eglot--uri-to-path uri))
+      ((file (eglot--uri-to-path uri))
        (visiting (or (find-buffer-visiting file)
                      (gethash uri eglot--temp-location-buffers)))
        (collect (lambda ()
-                  (let* ((bol (progn (goto-char beg) (point-at-bol)))
-                         (substring (buffer-substring bol (point-at-eol))))
+                  (pcase-let* ((`(,beg . ,end) (eglot--range-region range))
+                               (bol (progn (goto-char beg) (point-at-bol)))
+                               (substring (buffer-substring bol (point-at-eol))))
                     (add-face-text-property (- beg bol) (- end bol) 'highlight
                                             t substring)
                     (list substring (current-line) (current-column)))))

[joaotavora]

Like this

Yeah looks good. You can force-push this to my scratch branch, if you wish.

[mkcms]

You can force-push this to my scratch branch, if you wish.

Alright, I also made these changes:

• save-excursion in collect
• Add 1 to (current-line), because this function counts lines starting from 0.

[mkcms]

* Add 1 to `(current-line)`, because this function counts lines starting from 0.

We should also either bind tab-width to 1 around call to current-column or use some other function for current column, otherwise point is misplaced after following an xref if the location contains control characters or tabs (like in #158)

[joaotavora]

Add 1 to (current-line), because this function counts lines starting from 0.

This would seems to be a bug in xref.el. Shouldn't it be the one to add 1, given line-numbers in Emacs are 0-indexed? (though display-line-numbers-mode starts at 1, linum starts at 0). Paging @dgutov for another boring detail :-)

We should also either bind tab-width to 1 around call to current-column or use some other function for current column, otherwise point is misplaced after following an xref if the location contains control characters or tabs (like in #158)

ACK. Perhaps it's time for an eglot--current-column helper.

[mkcms]

This would seems to be a bug in xref.el. Shouldn't it be the one to add 1, given line-numbers in Emacs are 0-indexed?

I always thought Emacs lines were 1-based? Many other built-ins apparently start from 1, including line-number-at-pos, and (format-mode-line "%l")

[joaotavora]

I always thought Emacs lines were 1-based? Many other built-ins apparently start from 1, including line-number-at-pos, and (format-mode-line "%l")

Guess it depends where you look. current-line is 0 based.

[mkcms]

@joaotavora Thanks for this feature, it's quite useful.

ACK. Perhaps it's time for an eglot--current-column helper.

Should I just push this to master? Or do you want to use current-column inside?

(defun eglot--current-column ()
  (- (point) (point-at-bol)))

[dgutov]

Shouldn't it be the one to add 1, given line-numbers in Emacs are 0-indexed?

Guess we could say there are two standards, and the choice is arbitrary (but documented in xref-file-location). Didn't this particular choice originate in SLIME, where the basic idea and some code were extracted from?

Anyway, it's probably too late to change now.

[joaotavora]

Anyway, it's probably too late to change now.

Yeah I figure, but though I'd ask anyway.

[joaotavora]

Should I just push this to master?

Go ahead.

Or do you want to use current-column inside?

No need. Just add a docstring. Or better yet, make it a one-liner (it's pretty self-documenting). Thanks