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

A couple minor fixes for Flymake diagnostics #4435

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

A couple minor fixes for Flymake diagnostics #4435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a20661
Fix Flymake diagnostic column off-by-one error.
brownts Apr 20, 2024
a686a77
Add a defcustom to control default diagnostic severity.
brownts Apr 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 25 additions & 28 deletions lsp-diagnostics.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,9 @@
(define-obsolete-variable-alias 'lsp-flycheck-default-level
'lsp-diagnostics-flycheck-default-level "lsp-mode 7.0.1")

(defcustom lsp-diagnostics-flycheck-default-level 'error
"Error level to use when the server does not report back a diagnostic level."
:type '(choice
(const error)
(const warning)
(const info))
:group 'lsp-diagnostics)
;;;###autoload
(define-obsolete-variable-alias 'lsp-diagnostics-flycheck-default-level
'lsp-diagnostics-default-severity "lsp-mode 9.0.1")

(defcustom lsp-diagnostics-attributes
`((unnecessary :foreground "gray")
Expand Down Expand Up @@ -131,18 +127,19 @@ g. `error', `warning') and list of LSP TAGS."

(defun lsp-diagnostics--flycheck-calculate-level (severity tags)
"Calculate flycheck level by SEVERITY and TAGS."
(let ((level (pcase severity
(1 'error)
(2 'warning)
(3 'info)
(4 'info)
(_ lsp-flycheck-default-level)))
;; materialize only first tag.
(tags (seq-map (lambda (tag)
(cond
((= tag lsp/diagnostic-tag-unnecessary) 'unnecessary)
((= tag lsp/diagnostic-tag-deprecated) 'deprecated)))
tags)))
(let* ((severity (or severity
(lsp-diagnostics-severity->numeric
lsp-diagnostics-default-severity)))
(level (cond ((= severity lsp/diagnostic-severity-error) 'error)
((= severity lsp/diagnostic-severity-warning) 'warning)
((= severity lsp/diagnostic-severity-information) 'info)
((= severity lsp/diagnostic-severity-hint) 'info)))
Copy link
Member

Choose a reason for hiding this comment

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

Using pcase is probably cleaner.

And the one below, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that at first, but was having trouble with pcase not evaluating the lsp/diagnostic variables. If you have an example of how to transform this, I'd appreciate it.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this isn't any cleaner. 🤔

(pcase severity
        ((pred (= lsp/diagnostic-severity-error))       'error)
        ((pred (= lsp/diagnostic-severity-warning))     'warning)
        ((pred (= lsp/diagnostic-severity-information)) 'info)
        ((pred (= lsp/diagnostic-severity-hint))        'info)
        (_ 'info))

It's okay to leave it as it is.

;; materialize only first tag.
(tags (seq-map (lambda (tag)
(cond
((= tag lsp/diagnostic-tag-unnecessary) 'unnecessary)
((= tag lsp/diagnostic-tag-deprecated) 'deprecated)))
tags)))
(if tags
(lsp-diagnostics--flycheck-level level tags)
level)))
Expand Down Expand Up @@ -298,21 +295,21 @@ See https://github.com/emacs-lsp/lsp-mode."
(when (= start end)
(if-let ((region (flymake-diag-region (current-buffer)
(1+ start-line)
character)))
(1+ character))))
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate why +1? 🤔 Thanks!

Copy link
Contributor Author

@brownts brownts Apr 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

flymake-diag-region expects the line and column values to start at 1, not start at 0 like LSP specifies. Thus, both the line and column need incrementing when calling flymake-diag-region. The line number was already incremented but the column was not.

Note that this logic is only executed when the LSP server doesn't support an end position (i.e., the start and end position are the same), so flymake-diag-region is used to compute it.

flymake-diag-region is typically used to turn a normal compiler diagnostic position (1-based) into a region. Since LSP specifies position as 0-based, we need to adjust the line and column numbers before using the function.

(setq start (car region)
end (cdr region))
(lsp-save-restriction-and-excursion
(goto-char (point-min))
(setq start (line-beginning-position (1+ start-line))
end (line-end-position (1+ end-line))))))
(flymake-make-diagnostic (current-buffer)
start
end
(cl-case severity?
(1 :error)
(2 :warning)
(t :note))
message))))
(let* ((severity (or severity?
(lsp-diagnostics-severity->numeric
lsp-diagnostics-default-severity)))
(type (cond ((= severity lsp/diagnostic-severity-error) :error)
((= severity lsp/diagnostic-severity-warning) :warning)
((= severity lsp/diagnostic-severity-information) :note)
((= severity lsp/diagnostic-severity-hint) :note))))
(flymake-make-diagnostic (current-buffer) start end type message)))))
;; This :region keyword forces flymake to delete old diagnostics in
;; case the buffer hasn't changed since the last call to the report
;; function. See https://github.com/joaotavora/eglot/issues/159
Expand Down
6 changes: 5 additions & 1 deletion lsp-headerline.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,11 @@ PATH is the current folder to be checked."
(let ((range-severity 10))
(mapc (-lambda ((&Diagnostic :range (&Range :start) :severity?))
(when (lsp-point-in-range? start range)
(setq range-severity (min range-severity severity?))))
(setq range-severity
(min range-severity
(or severity?
(lsp-diagnostics-severity->numeric
lsp-diagnostics-default-severity))))))
(lsp--get-buffer-diagnostics))
range-severity))

Expand Down
25 changes: 23 additions & 2 deletions lsp-mode.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,15 @@ diagnostics have changed."
:type 'hook
:group 'lsp-mode)

(defcustom lsp-diagnostics-default-severity 'error
"Error level to use when the server does not report one."
:type '(choice (const :tag "Error" error)
(const :tag "Warning" warning)
(const :tag "Information" info)
(const :tag "Hint" hint))
:group 'lsp-mode
:package-version '(lsp-mode . "9.0.1"))
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep this in lsp-diagnostics.el? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jcs090218, the reason I put it in lsp-mode.el is because there are multiple users, not just lsp-diagnostics.el. lsp-modeline, lsp-headerline, lsp-mode and lsp-diagnostics all need this information, so it seemed more general and likely belongs in lsp-mode.el. It answers the general question of what the user wants to do in the absence of LSP severity information. lsp-diagnostics.el is just one such user of this information.

I placed it in lsp-mode.el near a couple other diagnostic-related user options (i.e., lsp-after-diagnostics-hook and lsp-diagnostics-updated-hook) to be consistent there. It appears that lsp-diagnostics.el is strictly the diagnostics provider (i.e., Flymake/Flycheck) interfacing, but other LSP diagnostics related information is kept in lsp-mode.el.


(define-obsolete-variable-alias 'lsp-workspace-folders-changed-hook
'lsp-workspace-folders-changed-functions "lsp-mode 6.3")

Expand Down Expand Up @@ -2277,6 +2286,14 @@ Common usecase are:
result)))
(ht)))

(defun lsp-diagnostics-severity->numeric (severity)
"Determine numeric severity from symbolic SEVERITY."
(pcase severity
('error lsp/diagnostic-severity-error)
('warning lsp/diagnostic-severity-warning)
('info lsp/diagnostic-severity-information)
('hint lsp/diagnostic-severity-hint)))

(defun lsp-diagnostics-stats-for (path)
"Get diagnostics statistics for PATH.
The result format is vector [_ errors warnings infos hints] or nil."
Expand All @@ -2296,11 +2313,15 @@ The result format is vector [_ errors warnings infos hints] or nil."
(let ((path (lsp--fix-path-casing (lsp--uri-to-path uri)))
(new-stats (make-vector 5 0)))
(mapc (-lambda ((&Diagnostic :severity?))
(cl-incf (aref new-stats (or severity? 1))))
(cl-incf (aref new-stats (or severity?
(lsp-diagnostics-severity->numeric
lsp-diagnostics-default-severity)))))
diagnostics)
(when-let ((old-diags (gethash path (lsp--workspace-diagnostics workspace))))
(mapc (-lambda ((&Diagnostic :severity?))
(cl-decf (aref new-stats (or severity? 1))))
(cl-decf (aref new-stats (or severity?
(lsp-diagnostics-severity->numeric
lsp-diagnostics-default-severity)))))
old-diags))
(lsp-diagnostics--update-path path new-stats)
(while (not (string= path (setf path (file-name-directory
Expand Down
6 changes: 4 additions & 2 deletions lsp-modeline.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,10 @@ The `:global' workspace is global one.")
(mapc (lambda (buf-diags)
(mapc (lambda (diag)
(-let [(&Diagnostic? :severity?) diag]
(when severity?
(cl-incf (aref stats severity?)))))
(cl-incf (aref stats
(or severity?
(lsp-diagnostics-severity->numeric
lsp-diagnostics-default-severity))))))
buf-diags))
diagnostics)
(while (< i lsp/diagnostic-severity-max)
Expand Down
Loading