Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix wrong-type-argument when MarkedString is a plist #72

Merged
merged 1 commit into from
Aug 17, 2018
Merged

Fix wrong-type-argument when MarkedString is a plist #72

merged 1 commit into from
Aug 17, 2018

Conversation

mkcms
Copy link
Collaborator

@mkcms mkcms commented Aug 12, 2018

In eglot--hover-info we weren't handling the case when MarkedString is a plist. This leads to wrong-type-argument, e.g when a class has no documentation and we call eglot-help-at-point when using eclipse.jdt.ls.

Debugger entered--Lisp error: (wrong-type-argument char-or-string-p nil)
  insert(nil)
  (save-current-buffer (set-buffer standard-output) (insert blurb))
  (progn (save-current-buffer (set-buffer standard-output) (insert blurb)))
  (progn (progn (save-current-buffer (set-buffer standard-output) (insert blurb))))
  (setq #:value (progn (progn (save-current-buffer (set-buffer standard-output) (insert blurb)))))
  (let* ((#:buffer (temp-buffer-window-setup "*eglot help*")) (standard-output #:buffer) #:window #:value) (setq #:value (progn (progn (save-current-buffer (set-buffer standard-output) (insert blurb))))) (save-current-buffer (set-buffer #:buffer) (setq #:window (temp-buffer-window-show #:buffer nil))) (if (functionp (quote help-window-setup)) (funcall (quote help-window-setup) #:window #:value) #:value))
  (let ((temp-buffer-window-setup-hook (cons (quote help-mode-setup) temp-buffer-window-setup-hook)) (temp-buffer-window-show-hook (cons (quote help-mode-finish) temp-buffer-window-show-hook))) (setq help-window-old-frame (selected-frame)) (let* ((#:buffer (temp-buffer-window-setup "*eglot help*")) (standard-output #:buffer) #:window #:value) (setq #:value (progn (progn (save-current-buffer (set-buffer standard-output) (insert blurb))))) (save-current-buffer (set-buffer #:buffer) (setq #:window (temp-buffer-window-show #:buffer nil))) (if (functionp (quote help-window-setup)) (funcall (quote help-window-setup) #:window #:value) #:value)))
  (progn (set-marker help-window-point-marker nil) (let ((temp-buffer-window-setup-hook (cons (quote help-mode-setup) temp-buffer-window-setup-hook)) (temp-buffer-window-show-hook (cons (quote help-mode-finish) temp-buffer-window-show-hook))) (setq help-window-old-frame (selected-frame)) (let* ((#:buffer (temp-buffer-window-setup "*eglot help*")) (standard-output #:buffer) #:window #:value) (setq #:value (progn (progn (save-current-buffer (set-buffer standard-output) (insert blurb))))) (save-current-buffer (set-buffer #:buffer) (setq #:window (temp-buffer-window-show #:buffer nil))) (if (functionp (quote help-window-setup)) (funcall (quote help-window-setup) #:window #:value) #:value))))
  (let ((blurb (eglot--hover-info contents range))) (progn (set-marker help-window-point-marker nil) (let ((temp-buffer-window-setup-hook (cons (quote help-mode-setup) temp-buffer-window-setup-hook)) (temp-buffer-window-show-hook (cons (quote help-mode-finish) temp-buffer-window-show-hook))) (setq help-window-old-frame (selected-frame)) (let* ((#:buffer (temp-buffer-window-setup "*eglot help*")) (standard-output #:buffer) #:window #:value) (setq #:value (progn (progn (save-current-buffer ... ...)))) (save-current-buffer (set-buffer #:buffer) (setq #:window (temp-buffer-window-show #:buffer nil))) (if (functionp (quote help-window-setup)) (funcall (quote help-window-setup) #:window #:value) #:value)))))
  (progn (let ((#:--cl-keys-- #:--cl-rest--)) (while #:--cl-keys-- (cond ((memq (car #:--cl-keys--) (quote (:contents :range :allow-other-keys))) (setq #:--cl-keys-- (cdr (cdr #:--cl-keys--)))) ((car (cdr (memq ... #:--cl-rest--))) (setq #:--cl-keys-- nil)) (t (error "Keyword argument %s not one of (:contents :range)" (car #:--cl-keys--)))))) (if (seq-empty-p contents) (progn (eglot--error "No hover info here"))) (let ((blurb (eglot--hover-info contents range))) (progn (set-marker help-window-point-marker nil) (let ((temp-buffer-window-setup-hook (cons (quote help-mode-setup) temp-buffer-window-setup-hook)) (temp-buffer-window-show-hook (cons (quote help-mode-finish) temp-buffer-window-show-hook))) (setq help-window-old-frame (selected-frame)) (let* ((#:buffer (temp-buffer-window-setup "*eglot help*")) (standard-output #:buffer) #:window #:value) (setq #:value (progn (progn ...))) (save-current-buffer (set-buffer #:buffer) (setq #:window (temp-buffer-window-show #:buffer nil))) (if (functionp (quote help-window-setup)) (funcall (quote help-window-setup) #:window #:value) #:value))))))
  (let* ((#:--cl-rest-- (jsonrpc-request (eglot--current-server-or-lose) :textDocument/hover (eglot--TextDocumentPositionParams))) (contents (car (cdr (plist-member #:--cl-rest-- (quote :contents))))) (range (car (cdr (plist-member #:--cl-rest-- (quote :range)))))) (progn (let ((#:--cl-keys-- #:--cl-rest--)) (while #:--cl-keys-- (cond ((memq (car #:--cl-keys--) (quote ...)) (setq #:--cl-keys-- (cdr ...))) ((car (cdr ...)) (setq #:--cl-keys-- nil)) (t (error "Keyword argument %s not one of (:contents :range)" (car #:--cl-keys--)))))) (if (seq-empty-p contents) (progn (eglot--error "No hover info here"))) (let ((blurb (eglot--hover-info contents range))) (progn (set-marker help-window-point-marker nil) (let ((temp-buffer-window-setup-hook (cons ... temp-buffer-window-setup-hook)) (temp-buffer-window-show-hook (cons ... temp-buffer-window-show-hook))) (setq help-window-old-frame (selected-frame)) (let* ((#:buffer ...) (standard-output #:buffer) #:window #:value) (setq #:value (progn ...)) (save-current-buffer (set-buffer #:buffer) (setq #:window ...)) (if (functionp ...) (funcall ... #:window #:value) #:value)))))))
  eglot-help-at-point()```

@mkcms
Fix wrong-type-argument when MarkedString is a plist
40477f1 
* eglot.el (eglot--hover-info): Handle the case when contents is
  neither a vector nor a string.
amol-mandhane
amol-mandhane reviewed Aug 13, 2018
View reviewed changes
eglot.el
@@ -1353,8 +1353,7 @@ is not active."
(let ((heading (and range (pcase-let ((`(,beg . ,end) (eglot--range-region range)))
(concat (buffer-substring beg end) ": "))))
(body (mapconcat #'eglot--format-markup
(append (cond ((vectorp contents) contents)
Copy link

Choose a reason for hiding this comment

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

This doesn't solve the issue completely as contents can be strings, vectors or plist. The plist itself can be of two types: MarkedString with language and MarkupContent. Also, according to the LSP docs, MarkedString is deprecated for hover response.

I think a correct solution would be something like

(defun eglot--parse-hover-contents (contents)
  "Doc."
  (cond
   ;; string
   ((stringp contents) (eglot--format-markup contents))
   ;; MarkedString[]
   ((vectorp contents) (mapconcat #'eglot--parse-hover-contents contents "\n"))
   ((listp contents)
    (if (plist-get contents :language)
        ;; MarkedString
        (eglot--format-markup contents)
      ;; MarkupContent
      (eglot--format-markup (plist-get contents :value))))))

(let ((heading ...)
       (body (eglot--parse-hover-contents contents))

This is untested, but in concept it should work.

Copy link
Collaborator Author

@mkcms mkcms Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I think it will work just because MarkedString (when it's a plist) and MarkupContent both have a :value key and eglot--format-buffer will silently handle an error in case it's argument is a MarkupContent and has no :language key. We should also handle MarkupContent's :kind though, but that's another issue.

Copy link

Choose a reason for hiding this comment

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

I think you can also merge it with eglot--format-markup to simplify the implementation and support all kinds of documentation easily.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with @mkcms. eglot--format-markup could see a clarification of the accepted datatypes in the docstring though, and indeed is the place where full support for MarkupContent should be implemented.

@joaotavora joaotavora merged commit 8e0cf60 into joaotavora:master Aug 17, 2018
@joaotavora
Copy link
Owner

Pushed with a tweak to the commit message

bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
@mkcms @joaotavora
Fix textDocument/hover responses where MarkedString is a plist (#72)
2e0c3d8 
* eglot.el (eglot--hover-info): Forward all non-vector content to 
eglot--format-markup.
(#72: joaotavora/eglot#72
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
@mkcms @joaotavora
Fix textdocument/hover responses where markedstring is a plist ()
7704fba 
* eglot.el (eglot--hover-info): Forward all non-vector content to 
eglot--format-markup.

GitHub-reference: joaotavora/eglot#72
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amol-mandhane amol-mandhane amol-mandhane left review comments

@joaotavora joaotavora joaotavora left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkcms @joaotavora @amol-mandhane