Don't activate eglot in invisible buffers

[muffinmad]

1. To correctly fontify each diff hunk (e.g. on vc-annotate-show-diff-revision-at-line) diff-syntax-fontify-hunk creates invisible buffers containing specific file revision and enables major-mode of the file.

2. Eglot sends didOpen to server and server sends diagnostics for that specific version of the file.

3. Diagnostic info is displayed in real file buffer.

It leads to wrong flymake error reports.

[joaotavora]

I'm not sure I understand what's going on here. How can an "insivisible" buffer have a buffer-file-name? In other words, is the contents of these buffers ever reflected in an actual file on disk? Then how can the buffer-name start with a space?

Can you provide an Emacs -Q recipe for triggering this?

[muffinmad]

Sure.

Emacs 27.0.50 emacs-mirror/emacs@af04738

Prepare test file. I use mercurial, but it can be git or another VCS supported by Emacs VC:

cd ~/workspace
mkdir eglot-test
cd eglot-test
hg init
echo "1import sys" > test.py
hg add test.py
hg commit -m "commit"
echo "import sys" > test.py

Now in emacs -Q:

1. M-x package-initialize
2. C-x C-f ~/workspace/eglot-test/test.py
3. M-x eglot Eglot is connected and there is 1 flymake warning: pyflakes: 'sys' imported but unused
4. C-x v =
5. q

Now there is 1 flymake error: pyflakes: invalid syntax
But this error is for the first revision of test.py

didOpen events:

client-notification Sat Feb 23 22:32:28 2019:
(:jsonrpc "2.0" :method "textDocument/didOpen" :params
	  (:textDocument
	   (:uri "file:///Users/mad/workspace/eglot-test/test.py" :version 0 :languageId "python" :text "import sys\n")))

client-notification Sat Feb 23 22:33:47 2019:
(:jsonrpc "2.0" :method "textDocument/didOpen" :params
	  (:textDocument
	   (:uri "file:///Users/mad/workspace/eglot-test/test.py" :version 0 :languageId "python" :text "1import sys\n")))

client-notification Sat Feb 23 22:33:47 2019:
(:jsonrpc "2.0" :method "textDocument/didOpen" :params
	  (:textDocument
	   (:uri "file:///Users/mad/workspace/eglot-test/test.py" :version 0 :languageId "python" :text "1import sys\n")))

[nemethf]

@muffinmad, I'm not able to reproduce the bug by following your detailed description. Unfortunately, it's been a long time since you reported the issue. Do you still see the problem? (Hopefully, it solved itself, and doing nothing was a good strategy.)

[muffinmad]

@nemethf Patch is outdated indeed. But I can still reproduce the bug. Are you on Emacs 27?

[muffinmad]

Patch is updated to handle

(add-hook 'python-mode-hook 'eglot-ensure)

[nemethf]

I don't know what I did wrong first time, but I am now able to reproduce the problem. I think the culprit is in diff-syntax-fontify-props, which has the following:

    (let (...
          (buffer-file-name file))

After these lines two buffers have the same value for buffer-file-name. Which is strange, but I guess it's not necessarily a bug in diff-mode.el. There are different ways to modify Eglot to cope with the situation, and it's a personal preference, but I think this PR can be improved to be more clear. Currently, Eglot determines whether a buffer is "normal" by checking buffer-file-name. We know now that this is not enough. So instead, we should introduce a new function eglot--ordinary-buffer-p that additionally checks the space prefix of buffer-name and adds comments to explain the situation. What do you think?

(Additionally, trailing whitespace is not good, but removing it is an independent change, and should not be part of this PR.)

[joaotavora]

eglot--ordinary-buffer-p not a particularly good name, right? Many buffers are ordinary and can't be managed by eglot. So eglot--manageable-buffer-p might be better. Also double check there isn't already something in emacs that does this.

Which is strange, but I guess it's not necessarily a bug in diff-mode.el.

We should double check, but your proposed fix isn't too bad (with some better name, not necessarily the one I suggested).

[muffinmad]

eglot--ordinary-buffer-p not a particularly good name, right? Many buffers are ordinary and can't be managed by eglot. So eglot--manageable-buffer-p might be better. Also double check there isn't already something in emacs that does this.

eglot--eglotable-buffer-p ;)

[muffinmad]

After these lines two buffers have the same value for buffer-file-name.

Two buffers with different content have the same buffer-file-name.

Currently, Eglot determines whether a buffer is "normal" by checking buffer-file-name. We know now that this is not enough. So instead, we should introduce a new function eglot--ordinary-buffer-p that additionally checks the space prefix of buffer-name and adds comments to explain the situation. What do you think?

Sounds great. I'll continue to work on it.

(Additionally, trailing whitespace is not good, but removing it is an independent change, and should not be part of this PR.)

Got it.

[nemethf]

I asked it here: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39190

[nemethf]

Customizing diff-font-lock-syntax to nil is an obvious quick workaround for the bug.

A good long term solution would be fixing #218, because if Eglot starts to manage a buffer only when the buffer is visible, this issue won't happen. By the way, Stefan suggested a similar solution in "emacs.bugs". However, I'm not going to work on #218 in the foreseeable future.

@muffinmad, would you like to work on the eglot--manageable-buffer-p approach as a interim solution? (Or on #218? But that surely requires your copyright paperwork sorted out.)

[muffinmad]

Customizing diff-font-lock-syntax to nil is an obvious quick workaround for the bug.

That feature is too cool to turn it off. I rather use locally patched eglot :)

Meanwhile there are some nice patches in you bug report so maybe it will be fixed in Emacs 27.

A good long term solution would be fixing #218, because if Eglot starts to manage a buffer only when the buffer is visible, this issue won't happen. By the way, Stefan suggested a similar solution in "emacs.bugs". However, I'm not going to work on #218 in the foreseeable future.

Possibility to start eglot in visible buffers only must be optional, isn't it? Like eglot-deferred. In this case eglot--manageable-buffer-p is still needed.

@muffinmad, would you like to work on the eglot--manageable-buffer-p approach as a interim solution? (Or on #218? But that surely requires your copyright paperwork sorted out.)

Sure, I'll try. And copyright paperwork is done already.

[muffinmad]

Can't reproduce this on recent Emacs master and 27 pretest.

Looks like the problem is fixed in Emacs.

Closing this.

Thanks @nemethf !