emacs-lsp / lsp-ui

UI integrations for lsp-mode
https://emacs-lsp.github.io/lsp-ui
GNU General Public License v3.0
1.03k stars 141 forks source link

After fixing an error and save buffer, lsp-ui still displays the error message #347

Open hisnawi opened 4 years ago

hisnawi commented 4 years ago

I am enabling flycheck syntax checking only on save with :

 flycheck-check-syntax-automatically  '(mode-enabled save)

And disabling lsp ui live reporting:

      (setq lsp-ui-flycheck-live-reporting nil)

however, if I follow the steps: edit a line and cause an error -> save buffer -> lsp-ui shows error message -> fix error -> save buffer -> lsp-ui still shows same error message

If I check flycheck list of errors the error is not listed there anymore which tells me it is a lsp-ui issue.

melton1968 commented 4 years ago

I was able to work around this issue with the following one line patch to lsp-ui-flycheck.el:

diff --git a/lsp-ui-flycheck.el b/lsp-ui-flycheck.el
index 3c48710..c5d5cb4 100644
--- a/lsp-ui-flycheck.el
+++ b/lsp-ui-flycheck.el
@@ -216,7 +216,6 @@ See https://github.com/emacs-lsp/lsp-mode."

 (defun lsp-ui-flycheck--report nil
   (and flycheck-mode
-       lsp-ui-flycheck-live-reporting
        (flycheck-buffer)))

 ;; FIXME: Provide a way to disable lsp-ui-flycheck

I am unfamiliar with the code-base, so I am uncertain if this is the correct fix, but it is working for me with the configuration listed above. Without this change, there is no way that flycheck can keep the errors messages in sync with the last result from the language server.

If you have installed lsp-ui from elpa, you can simply delete the line from the file and remove the corresponding .elc file and then restart emacs.

hisnawi commented 4 years ago

Hmm. it dosen't seem that worked for me. I removed the line and reloaded emacs, but I still see live updates by flycheck.

melton1968 commented 4 years ago

After a little more testing, I see that in general you are correct. At this point, this is what I observe:

At global scope, I get live error reporting.

In inner scopes (e.g. object abc { … }), I get the desired behavior.

So, the fix I suggested is clearly not correct, although it does makes the syntax checking much more usable for me since I am typing in some inner scope much more often that at global scope. I will continue with it until the real fix is in place.

ps. I presume you removed the .elc file?

On Dec 28, 2019, at 11:21 PM, hisnawi notifications@github.com wrote:

Hmm. it dosen't seem that worked for me. I removed the line and reloaded emacs, but I still see live updates by flycheck.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/emacs-lsp/lsp-ui/issues/347?email_source=notifications&email_token=AAHONJA7XINM3OWLSHBEAL3Q3AQSZA5CNFSM4JVBGZJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHYXSFA#issuecomment-569473300, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHONJC3M75EVXQBWPKUGE3Q3AQSZANCNFSM4JVBGZJA.

hisnawi commented 4 years ago

There was no .elc file. Did you mean in the elpa/lsp-ui(version-number) directory?

melton1968 commented 4 years ago

Yes, here is what I see in my emacs.d directory for lsp-ui (MacOSX):

$ ls -al .emacs.d/elpa/lsp-ui-20191228.1151/ total 496 drwxr-xr-x 17 mmelton staff 544 Dec 28 22:33 . drwxr-xr-x 29 mmelton staff 928 Dec 28 09:07 .. -rw-r--r-- 1 mmelton staff 2220 Dec 28 09:06 lsp-ui-autoloads.el -rw-r--r-- 1 mmelton staff 30760 Dec 28 09:06 lsp-ui-doc.el -rw-r--r-- 1 mmelton staff 28389 Dec 28 09:06 lsp-ui-doc.elc -rw-r--r-- 1 mmelton staff 1504 Dec 28 09:06 lsp-ui-doc.html -rw-r--r-- 1 mmelton staff 9335 Dec 28 22:31 lsp-ui-flycheck.el -rw-r--r-- 1 mmelton staff 12616 Dec 28 09:06 lsp-ui-imenu.el -rw-r--r-- 1 mmelton staff 11405 Dec 28 09:06 lsp-ui-imenu.elc -rw-r--r-- 1 mmelton staff 29922 Dec 28 09:06 lsp-ui-peek.el -rw-r--r-- 1 mmelton staff 28452 Dec 28 09:06 lsp-ui-peek.elc -rw-r--r-- 1 mmelton staff 482 Dec 28 09:06 lsp-ui-pkg.el -rw-r--r-- 1 mmelton staff 24396 Dec 28 09:06 lsp-ui-sideline.el -rw-r--r-- 1 mmelton staff 21438 Dec 28 09:06 lsp-ui-sideline.elc -rw-r--r-- 1 mmelton staff 6105 Dec 28 09:06 lsp-ui.el -rw-r--r-- 1 mmelton staff 5709 Dec 28 09:06 lsp-ui.elc

I manually removed the lsp-ui-flycheck.elc file and didn’t bother to re-compile it.

On Dec 29, 2019, at 12:05 AM, hisnawi notifications@github.com wrote:

There was no .elc file. Did you mean in the elpa/lsp-ui(version-number) directory?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/emacs-lsp/lsp-ui/issues/347?email_source=notifications&email_token=AAHONJDTUQM3AR7PVK4PCODQ3AV2RA5CNFSM4JVBGZJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHYYCSY#issuecomment-569475403, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHONJHHL2UYT5NXM4UNBQDQ3AV2RANCNFSM4JVBGZJA.

hisnawi commented 4 years ago

Not for me. No .elc files.

melton1968 commented 4 years ago

After poking around the code a bit, here is my current understanding:

Diagnostic reports are received from the language server asynchronously and notifications can be received by using the 'lsp-after-diagnostics-hook. We would like to update the fly-check errors only when the report from the language server was induced by saving the buffer. As far as I can tell, there is no direct way to determine this.

Trying to use 'after-save-hook creates a race condition between 'after-save-hook and 'lsp-after-diagnostics-hook.

As an improved work-around for the time being, I made the following changes to lsp-ui-flycheck.el which uses the simple heuristic that if a diagnostics report arrives and the buffer is unmodified, then call flycheck-buffer.

Seems to work reasonably well.

diff --git a/lsp-ui-flycheck.el b/lsp-ui-flycheck.el
index 3c48710..eb6d91e 100644
--- a/lsp-ui-flycheck.el
+++ b/lsp-ui-flycheck.el
@@ -216,14 +216,14 @@ See https://github.com/emacs-lsp/lsp-mode."

 (defun lsp-ui-flycheck--report nil
   (and flycheck-mode
-       lsp-ui-flycheck-live-reporting
+       (or lsp-ui-flycheck-live-reporting
+      (not (buffer-modified-p)))
        (flycheck-buffer)))

 ;; FIXME: Provide a way to disable lsp-ui-flycheck
 (defun lsp-ui-flycheck-enable (_)
   "Enable flycheck integration for the current buffer."
-  (when lsp-ui-flycheck-live-reporting
-    (setq-local flycheck-check-syntax-automatically nil))
+  (setq-local flycheck-check-syntax-automatically nil)
   (setq-local flycheck-checker 'lsp-ui)
   (lsp-ui-flycheck-add-mode major-mode)
   (add-to-list 'flycheck-checkers 'lsp-ui)
yyoncho commented 4 years ago

@melton1968 I was about to propose the same workaround. Would you like to create a PR?

(not (buffer-modified-p))

Here you should also check whether the flycheck mode is on saved because if it is manual the errors should be rendered only when the user calls flycheck-buffer.

melton1968 commented 4 years ago

Yes, the current patch does not respect the value of flycheck-check-syntax-automatically and in fact sets it to nil to keep flycheck from checking the buffer before the diagnostics are received.

I can see how to support the following configurations with minimal code changes:

  1. lsp-ui-flycheck-live-reporting is not nil lsp-ui-flycheck—report invokes (flycheck-buffer) every time new diagnostics are received

  2. lsp-ui-flycheck-live-reporting is nil and flycheck-check-syntax-automatically contains ’save lsp-ui-flycheck—report invokes (flycheck-buffer) when new diagnostics are received and the buffer is unmodified

  3. lsp-ui-flycheck-live-reporting is nil and flycheck-check-syntax-automatically does not contain ’save lsp-ui-flycheck—report does not invoke (flycheck-buffer)

If that makes sense, I will make the changes and submit a PR.

On Dec 29, 2019, at 2:48 PM, Ivan Yonchovski notifications@github.com wrote:

@melton1968 https://github.com/melton1968 I was about to propose the same workaround. Would you like to create a PR?

(not (buffer-modified-p))

Here you should also check whether the flycheck mode is on saved because if it is manual the errors should be rendered only when the user calls flycheck-buffer.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/emacs-lsp/lsp-ui/issues/347?email_source=notifications&email_token=AAHONJBGWTQ5R5AKLBOL6BDQ3D5ITA5CNFSM4JVBGZJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHZG5JQ#issuecomment-569536166, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHONJFBT7LBIAIAQT2TCUTQ3D5ITANCNFSM4JVBGZJA.

yyoncho commented 4 years ago

If that makes sense, I will make the changes and submit a PR.

It makes sense.

hisnawi commented 4 years ago

@melton1968 thank you for looking into this

deviantfero commented 4 years ago

Hi, I'm still seeing this issue, has this changed reached the melpa repository yet?

melton1968 commented 4 years ago

I updated my packages from ELPA stable and that version had the fix.

cheers, mark

On Jan 12, 2020, at 1:19 PM, Fernando Vásquez notifications@github.com wrote:

Hi, I'm still seeing this issue, has this changed reached the melpa repository yet?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/emacs-lsp/lsp-ui/issues/347?email_source=notifications&email_token=AAHONJHWXQWJATX2UR2FHXLQ5NUK5A5CNFSM4JVBGZJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXB4QI#issuecomment-573447745, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHONJHLKYYB6QUZBDVTNJDQ5NUK5ANCNFSM4JVBGZJA.

hisnawi commented 4 years ago

Melton, could you share the package version number?

melton1968 commented 4 years ago

lsp-ui 20200106.1152

cheers, mark

On Jan 13, 2020, at 10:19 AM, hisnawi notifications@github.com wrote:

Melton, could you share the package version number?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/emacs-lsp/lsp-ui/issues/347?email_source=notifications&email_token=AAHONJGVWMHXPFYOJGJZH6TQ5SIBJA5CNFSM4JVBGZJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIZKPTI#issuecomment-573745101, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHONJH6MU4LVKCMKS7MQCDQ5SIBJANCNFSM4JVBGZJA.

melton1968 commented 4 years ago

I just loaded up a c++ file to verify and I get syntax checking. Here is the relevant part of my .emacs

;; Enable nice rendering of diagnostics like compile errors. (use-package flycheck :init (global-flycheck-mode) :config (setq flycheck-check-syntax-automatically '(save)) (setq flycheck-clang-args (list "-extra-arg-before=-xc++")))

(use-package lsp-mode ;; Optional - enable lsp-mode automatically in scala files :hook ((c++-mode . lsp) (scala-mode . lsp)) :config (setq lsp-enable-snippet nil) (setq lsp-prefer-flymake nil) (setq lsp-clients-clangd-args (list "--query-driver=/opt/local/bin/clang*9.0")) (setq lsp-clients-clangd-executable "/opt/local/bin/clangd-mp-9.0"))

(use-package lsp-ui :config (setq lsp-ui-flycheck-live-reporting nil) )

;; Add snippets and company-lsp backend for metals (use-package yasnippet) (use-package company-lsp)

cheers, mark

On Jan 13, 2020, at 12:32 PM, hisnawi notifications@github.com wrote:

Interesting...now flycheck doesn't work at all. Whether I save the file or I don't. It gives the no error symbol no matter what I code. This is my setup:

(use-package lsp-ui :ensure t :after lsp-mode :diminish :commands lsp-ui-mode :bind (("s-c" . lsp-ui-imenu)) :custom-face (lsp-ui-doc-background ((t (:background nil)))) (lsp-ui-doc-header ((t (:inherit (font-lock-string-face italic))))) :custom (lsp-ui-doc-enable nil) ;; Disable since it is distracting (all following apply when enabled) (lsp-ui-doc-include-signature t) (lsp-ui-doc-position 'top) (lsp-ui-doc-border (face-foreground 'default)) ;;(lsp-ui-sideline-enable nil) (lsp-ui-sideline-ignore-duplicate t) (lsp-ui-sideline-show-code-actions nil) :config ;; Use lsp-ui-doc-webkit only in GUI (setq lsp-ui-doc-use-webkit t) ;; WORKAROUND Hide mode-line of the lsp-ui-imenu buffer ;; https://github.com/emacs-lsp/lsp-ui/issues/243 (defadvice lsp-ui-imenu (after hide-lsp-ui-imenu-mode-line activate) (setq mode-line-format nil)) ) (use-package flycheck :ensure t :init (add-hook 'prog-mode-hook #'flycheck-mode) :config (setq-default flycheck-display-errors-delay 0.3 flycheck-check-syntax-automatically '(mode-enabled save) flycheck-disabled-checkers '(emacs-lisp-checkdoc c/c++-clang c/c++-cppcheck c/c++-gcc) ) (setq-default flycheck-gcc-language-standard "c++17") (setq-default flycheck-clang-language-standard "c++17") )

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/emacs-lsp/lsp-ui/issues/347?email_source=notifications&email_token=AAHONJGFUVNVUXCO4CMBPFDQ5SXU7A5CNFSM4JVBGZJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIZZIQI#issuecomment-573805633, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHONJC77JZYGFSADT4NP4DQ5SXU7ANCNFSM4JVBGZJA.

melton1968 commented 4 years ago

And, here is what I get when I run flycheck-verify-setup:

Syntax checkers for buffer rest_t0.cpp in c++-mode:

First checker to run:

lsp-ui (explicitly selected)

Checkers that could run if selected:

c/c++-clang select

Checkers that are compatible with this mode, but will not run until properly configured:

c/c++-cppcheck (disabled)

Flycheck Mode is enabled. Use C-u C-c ! x to enable disabled checkers.


Flycheck version: 32snapshot (package: 20191126.2142) Emacs version: 26.3 System: x86_64-apple-darwin19.0.0 Window system: nil

cheers, mark

On Jan 13, 2020, at 12:32 PM, hisnawi notifications@github.com wrote:

Interesting...now flycheck doesn't work at all. Whether I save the file or I don't. It gives the no error symbol no matter what I code. This is my setup:

(use-package lsp-ui :ensure t :after lsp-mode :diminish :commands lsp-ui-mode :bind (("s-c" . lsp-ui-imenu)) :custom-face (lsp-ui-doc-background ((t (:background nil)))) (lsp-ui-doc-header ((t (:inherit (font-lock-string-face italic))))) :custom (lsp-ui-doc-enable nil) ;; Disable since it is distracting (all following apply when enabled) (lsp-ui-doc-include-signature t) (lsp-ui-doc-position 'top) (lsp-ui-doc-border (face-foreground 'default)) ;;(lsp-ui-sideline-enable nil) (lsp-ui-sideline-ignore-duplicate t) (lsp-ui-sideline-show-code-actions nil) :config ;; Use lsp-ui-doc-webkit only in GUI (setq lsp-ui-doc-use-webkit t) ;; WORKAROUND Hide mode-line of the lsp-ui-imenu buffer ;; https://github.com/emacs-lsp/lsp-ui/issues/243 (defadvice lsp-ui-imenu (after hide-lsp-ui-imenu-mode-line activate) (setq mode-line-format nil)) ) (use-package flycheck :ensure t :init (add-hook 'prog-mode-hook #'flycheck-mode) :config (setq-default flycheck-display-errors-delay 0.3 flycheck-check-syntax-automatically '(mode-enabled save) flycheck-disabled-checkers '(emacs-lisp-checkdoc c/c++-clang c/c++-cppcheck c/c++-gcc) ) (setq-default flycheck-gcc-language-standard "c++17") (setq-default flycheck-clang-language-standard "c++17") )

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/emacs-lsp/lsp-ui/issues/347?email_source=notifications&email_token=AAHONJGFUVNVUXCO4CMBPFDQ5SXU7A5CNFSM4JVBGZJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIZZIQI#issuecomment-573805633, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHONJC77JZYGFSADT4NP4DQ5SXU7ANCNFSM4JVBGZJA.