Doing c-fill-paragraph in comments causes eglot to parse comments as code and produce errors

[finalpatch]

Steps to reproduce:

1. Open a well formatted C++ file, where eglot shows no errors and warnings.
2. Add 2 lines of comments, for example

   // comment1
   // comment2
   class MyClass {
   ...
   more code
   ...
   

3. Put cursor in the comments, and press M-q (c-fill-paragraph)
4. Observe flymake now shows some errors even though there is no change in actual code
5. Keep pressing M-q and we can observe the error lines move down

It feels as if eglot didn't realize the input command happens in comment and doesn't affect code at all. Instead, eglot seems to send comments as code for incremental parsing and generated errors.

The errors can be fixed by reverting the buffer.

[joaotavora]

Hmmm. I'd venture to say this is a bug in C mode. You see, the way eglot "catches" changed text is by relying on before- and after- modification hooks, and cc-mode sometime has some special behaviour.

Can you try a similar thing in js-mode? (I mean, I know you don't want js-mode, but you could make Eglot run the same C langserver in js-mode and try out the same paragraph-filling thing). If the bug happens, it's probably Eglot's bug, if it doesn't, it's probably cc-mode's fault

Anyway, I'll try to have a look, but it might take a while.

[finalpatch]

I tried to get eglot to run ccls in js-mode but it didn't work. I'm happy to try again if you could tell me the steps to follow.

I tested c-fill-paragraph in comments with lsp-mode and it doesn't show this bug. I don't know whether it has some workaround for this special case or it just does things differently.

[finalpatch]

Also, the funny thing is, c-fill-paragraph doesn't have to actually change the text to trigger the bug. Pressing M-q on an already filled paragraph results in no text change at all but will still cause the errors to appear.

[joaotavora]

I tried to get eglot to run ccls in js-mode but it didn't work. I'm happy to try again if you could tell me the steps to follow.

It should be as simple as C-u M-x eglot RET ccls RET in a .cpp that you happened to put in js-mode before by typing M-x js-mode.

But never mind... I just saw js-fill-paragraph uses c-fill-paragraph, too. So this experiment would tell us nothing.

[joaotavora]

You know what I'd do? Just set fill-paragraph-function to nil in c++-mode-hook or something. It stills fills basic paragraphs (like the one you showed me), though it'll probably fail on fancy \* comments.

But it that works with ccls at least we can know that the problem in is c-fill-paragraph, or rather that it is closer to c-fill-paragraph.

[finalpatch]

Hi @joaotavora , I just tested setting fill-paragraph-function to nil in a c++ buffer and I'm still seeing this bug

[joaotavora]

That's good! Probably means the bug is in Eglot, or Emacs. I'll have a look later on.

…

On Tue, Dec 10, 2019 at 10:59 AM finalpatch ***@***.***> wrote: Hi @joaotavora , I just tested setting fill-paragraph-function to nil in a c++ buffer and I'm still seeing this bug — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

-- João Távora

[casouri]

I want to add that this bug also occurs occasionally when third party functions modifies the buffer, but I cannot reliably reproduce it.

[casouri]

Any news?

[joaotavora]

Any news?

No sorry, nothing. You can help by coming up with a reliable reproduction recipe. Try to spot when it happens, and then isolate the case.

[casouri]

I thought we already have a reliable way to reproduce (fill-paragraph)?

[joaotavora]

I thought we already have a reliable way to reproduce (fill-paragraph)?

That is correct, Indeed I misread this thread and focused on your own comment. Then if you can reliably reproduce "your bug", it would be of help.

[casouri]

I had a look at c-fiil-paragraph and found that it messes with after-change-function. To be exact, it calls c-mask-paragraph which uses c-save-buffer-state which uses with-silent-modifications. Then I went to yasnippet and found inhibit-modification-hooks setting to nil deep in a function called by yas--move-to-field.

I'm not an expert but I think that's why eglot breaks when I fill paragraph and use signature expansion for functions in C.

[joaotavora]

I had a look at c-fiil-paragraph and found that it messes with after-change-function. To be exact, it calls c-mask-paragraph which uses c-save-buffer-state which uses with-silent-modifications. Then I went to yasnippet and found inhibit-modification-hooks setting to nil deep in a function called by yas--move-to-field.

Thanks for this analysis, it is very promising, but I don't understand some of your sentences, you need to write a bit more.

What is the relation with Yasnippet here? Are you saying yasnippet already has a fix for this similar problem? If so, that would be great and wouldn't suprise me, as I (but mostly @npostavs) dealed with a lot of c-mode problems in Yasnippet.

[casouri]

I have the problem when 1) I use c-fill-paragraph and 2) I use function signature expansion (if I'm not mistaken it is handled by yasnippet). After I found c-fill-paragraph using with-silent-modifications I thought I'd see if yanippet does similar stuff. And lo and behold, it does. That more or less backs my theory that messing with after-change-functions causes the break.

[joaotavora]

Ahh, so you see a similar problem in two distinct situations? And both of them point to a use of with-silent-modifications. Can you confirm this?

[casouri]

Let me have a try :-)

[Miciah]

I see a similar problem with go-mode. The following steps can be used to reproduce the problem:

1. Create a file with the name /tmp/go-mode/main.go and the following content:

package main

import (
	"fmt"
)

// main prints,
// "Hello, world!"
func main() {
	fmt.Println("Hello, world!")
}

2. Start Emacs with the go-mode and eglot packages loaded and the /tmp/go-test/main.go file open: emacs -Q -l ~/.cache/emacs/elpa/go-mode-20200822.1936/go-mode.el -l ~/.cache/emacs/elpa/eglot-20200821.2227/eglot.el /tmp/go-test/main.go
3. Start eglot: M-x eglot RET.
4. Move to the comment and press M-q.

Eglot marks the line after the comment (func main() {) with two red exclamation marks in the fringe and a red squiggle running under the line. Flymake reports an error: syntax: expected declaration, found "Hello, world!".

The value of fill-paragraph-function is go--fill-paragraph, which is defined in the go-mode package as follows:

(defun go--fill-paragraph (&rest args)
  "Wrap fill-paragraph to set custom fill-prefix."
  (let ((fill-prefix (go--fill-prefix))
        (fill-paragraph-function nil))
    (apply 'fill-paragraph args)))

[stephe-ada-guru]

I had the same problem with the ada-mode parser; the problem is that an intermediate step in fill-paragraph leaves the comment lines with no comment-start syntax; then the parser gets triggered by change hooks and reports that error. This is actually a bug in fill-comment; it might be better to fix it. I worked around this in ada-mode by writing my own ada-fill-comment-paragraph, and inhibiting parsing until it was done; there is a flag wisi-inhibit-parse for this purpose (also used in a couple other places, notably inserting skeletons). Does eglot have a similar inhibit?

…

-- -- Stephe

[joaotavora]

Thanks Miciah and Stephen. Stephen, your insight might be the clue to fix this. I'll have a better read later on.

…

On Tue, Aug 25, 2020, 18:13 Stephen Leake ***@***.***> wrote: I had the same problem with the ada-mode parser; the problem is that an intermediate step in fill-paragraph leaves the comment lines with no comment-start syntax; then the parser gets triggered by change hooks and reports that error. This is actually a bug in fill-comment; it might be better to fix it. I worked around this in ada-mode by writing my own ada-fill-comment-paragraph, and inhibiting parsing until it was done; there is a flag wisi-inhibit-parse for this purpose (also used in a couple other places, notably inserting skeletons). Does eglot have a similar inhibit? -- -- Stephe — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#367 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC6PQ56R6YFVLV7ONHVXSLSCPWKPANCNFSM4JXLRFVQ> .

[bcmills]

the problem is that an intermediate step in fill-paragraph leaves the comment lines with no comment-start syntax; then the parser gets triggered by change hooks and reports that error.

For me these errors are sticky. Shouldn't the change hook fire again after fill-comment completes if the contents have changed relative to the last invocation? (That is, shouldn't any errors from the intermediate state be ephemeral and very short-lived?)

[bcmills]

fill-paragraph does not appear to use with-silent-modifications, but it does use set-buffer-modified-p to set the buffer back to unmodified after making the edit:
https://github.com/emacs-mirror/emacs/blob/d0e2a341dd9a9a365fd311748df024ecb25b70ec/lisp/textmodes/fill.el#L896-L901

[muffinmad]

Offtopic I have a dream: all LSP servers stops publishing diagnostic on every document change.

Sorry for the noise :)

[andyleejordan]

I can confirm that the OP's repro steps work, but also with many different Python files instead of a C++ file. python-mode sets fill-paragraph-function (used by fill-paragraph) to python-fill-paragraph.

Emacs "27.1.50"

;; Version: 1.6
;; Package-Version: 20201103.1026
;; Package-Commit: 21726416e6e580b20dfa90833c6dab2a8a15ea48

I'll post here the next time I see this with the exact file and cursor position. I've seen this a lot, it just doesn't repro 100%.

[andyleejordan]

Ok, I have a repro of something related to this, but worse. I want to note that I have also seen the effect of a ghost flymake error being introduced on the use of fill-paragraph in Python mode. But this next thing is bonkers:

Start with buffer contents, and cursor at that first docstring, which is too long and needs to be filled, say before the 'T' in 'The':

class Foo:
    """The `SSH` platform simply connects to existing targets. The `SSH` platform simply connects to existing targets. The `SSH` platform simply connects to existing targets.

    It does not deploy nor delete the target. The default ``host`` is
    ``localhost`` so this can be used for testing against the user's
    system (if SSH is enabled).

    """

    def foo(self) -> str:
        return "foo"

Run eglot-format-buffer just and assert that nothing changes.

Run fill-paragraph (or python-fill-paragraph, which calls the former), and either do or do nor save the buffer (it repros both ways, I know because I found this since due adding eglot-format-buffer to before-save-hook, and then after-save-hook, and then took saving out of the equation). Get:

class Foo:
    """The `SSH` platform simply connects to existing targets. The `SSH`
platform simply connects to existing targets. The `SSH` platform
simply connects to existing targets.

    It does not deploy nor delete the target. The default ``host`` is
    ``localhost`` so this can be used for testing against the user's
    system (if SSH is enabled).

    """

    def foo(self) -> str:
        return "foo"

(Cool, it's filled, it's just wrong for a Python docstring since it should be indented.) Now run eglot-format-buffer:

class Foo:
    """The `SSH` platform simply connects to existing targets. The `SSH`
    platform simply connects to existing targets. The `SSH` platform
    simply connects to existing targets.

        It does not deploy nor delete the target. The default ``host`` is
        ``localhost`` so this can be used for testing against the user's
        system (if SSH is enabled).

    """

    def foo(self) -> str:
        return "foo"

It applied the indents to the paragraph below too! And this isn't the thing I've seen. I was first trying to repo the fill-paragraph which causes the ghost flymake error, which I have seen, then I was trying to repro where the last N lines get duplicated (I saw it as I was making this repro, and then undid and didn't see it again, WTF, wasn't able to repro again):

    def foo(self) -> str:
        return "foo"
        return "foo"

I futzed around with a different example, and got that to repro, but then as I undid and tried to copy the pieces out, it stopped repro'ing again.

So something is up with eglot-format-buffer and fill-paragraph, and something else is up with fill-paragraph alone.

[notmgsk]

This is hella annoying. The most frustrating thing about it is that the "error" persists, and, at least in go-mode, it means that the rest of the buffer is no longer type-checked, etc. Essentially it renders eglot/LSP useless until I issue eglot-reconnect. Is there a work-around? don't use fill-paragraph?

[joaotavora]

@notmgsk Understood, I guess I have to put some hours into this. I think we have sufficient repros here. I'll just quickly try out hailing @monnier in case he has some world-shattering insight into fill-paragraph's purported mischiefs.

[joaotavora]

Don't forget to include the simplest instructions you can on how to download your python ls.

[andyleejordan]

Wah, trying to get an emacs -Q to load eglot is...not going well...getting (void-function project-root) even though I put (require 'project) into the simple init.el file.

[joaotavora]

Look in the Makefile. You need packages if you're not using the latest nightly or something equivalent.

[andyleejordan]

Oh shoot, yeah that makes sense. Thanks! Forgot that just how much gets wiped by -Q 😅

[joaotavora]

Though you should have those packages. Then again, if you're completely sandboxing this init, you need to reinstall them.

-Q doesn't wipe packages, as far as I know. But you have to package-initialize or something like that.

[andyleejordan]

Hope this helps: https://github.com/andschwa/eglot-repro

[andyleejordan]

I can repro all the things from #367 (comment) reliably in this, it's just that the actual sequence of edits which confuses eglot/lsp/pyls are strange. Usually consists of fill-paragraph and undo and eglot-format-buffer.

[joaotavora]

Gotta say a whole repo for a reproduction recipe is a first! But I'll take it! :-D

[joaotavora]

@andschwa I tried your recipe and idneed I reproduced, something. However, I don't think I reproduced this particular bug. There doesn't seem to be anything inconsistent when you M-x fill-paragraph, rather the problem seems to come from M-x eglot-format-buffer. It might or might not be a server bug. I probably have to turn on super-extra debugging in the pyls server so I can see its version of the file.

Anyway, that's for another issue. Closing this one.

[andyleejordan]

Something's wrong is about where I'm at too, when I sat down trying to repro the OP's issue (which I have also seen while working in Python projects). Thanks @joaotavora!

[andyleejordan]

For what it's worth, while working today, I found that the OP repro does repro for me in Python. Not when filling docstrings, but when filling comments, and it doesn't seem fixed with that commit 😢

[joaotavora]

I've reproduced the problem in terms of a simple repro

emacs -Q -f package-initialize -L . -l eglot ~/tmp/coiso.py -f eglot

Where ~/tmp/coiso.py is a simple Python file:

# indented
# four spaces
def foo():
    x = 1
    return x

and Emacs version is 26.3 and all the packages are up to date.

M-q on the first comment and watch it break. @andschwa you can delete that repo.

[joaotavora]

And this is what Emacs sends pyls for filling in that paragraph. IT's exactly what it sends for the Go server, as explained here #367 (comment), but the Go server can deal with it correctly, and the Python pyls server can't. It seems to think we've reported a deletion of the second line and its entire contents.

[client-notification] Tue Dec 15 08:35:30 2020:
(:jsonrpc "2.0" :method "textDocument/didChange" :params
          (:textDocument
           (:uri "file:///home/capitaomorte/tmp/coiso.py" :version 2)
           :contentChanges
           [(:range
             (:start
              (:line 1 :character 0)
              :end
              (:line 1 :character 2))
             :rangeLength 2 :text "")
            (:range
             (:start
              (:line 0 :character 10)
              :end
              (:line 1 :character 11))
             :rangeLength 1 :text " ")]))
[server-notification] Tue Dec 15 08:35:31 2020:
(:jsonrpc "2.0" :method "textDocument/publishDiagnostics" :params
          (:uri "file:///home/capitaomorte/tmp/coiso.py" :diagnostics
                [(:source "pycodestyle" :range
                          (:start
                           (:line 0 :character 10)
                           :end
                           (:line 0 :character 12))
                          :message "W291 trailing whitespace" :code "W291" :severity 2)]))

[joaotavora]

Alright, I pushed a new, more ambitious fix, which I've tested manually in Python and Go modes. Please everybody try that out.

[notmgsk]

Cannot reproduce in either gopls or python-language-server. 👍

[andyleejordan]

This seems to have done it! I haven't run into a single issue today. Thank you SO MUCH @joaotavora. 🤩

[joaotavora]

Thanks. You're very welcome. I'll soon be doing some go myself, and it seems to have an excellent LS, so likely I'll devote some more time to Eglot in the near future.