bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Dario Gjorgjevski]

How to reproduce:

1. In one shell, fire up GNU Emacs, open a file, and start something
that uses track-changes; e.g., Eglot.  In my case it was a Python file
called test.py with contents foo = 0.  Everything is happy so far.
2. In another shell, fire up a Node.js interpreter and execute const
fs = require("node:fs/promises") followed by await
fs.writeFile("test.py", "foo = 1").  This will overwrite the file.
3. Track-changes (and by extension Eglot) will error out.  Restarting
Eglot makes everything go back to normal.

The *Warnings* buffer now contains:

    Warning (emacs): Missing/incorrect calls to
‘before/after-change-functions’!!
    Details logged to `track-changes--error-log'

And `track-changes--error-log' contains:

    (("test.py" #1=(unexpected-after 1 8 0)
      ((t track-changes--recover-from-error (#1#) nil)
       (t track-changes--after (1 8 0) nil)
       (t revert-buffer-insert-file-contents--default-function
          ("/path/to/test.py"
           nil)
          nil)
       (t revert-buffer--default (ignore-auto dont-ask) nil)
       (t revert-buffer (ignore-auto dont-ask preserve-modes) nil)
       (t auto-revert-handler nil nil)
       (t auto-revert-buffer (#<buffer test.py>) nil)
       (t #<subr auto-revert-buffers> nil nil)
       (t auto-revert-buffers@buffer-list-filter
          (#<subr auto-revert-buffers>) nil)
       (t apply
          (auto-revert-buffers@buffer-list-filter #<subr
                              auto-revert-buffers> nil)
          nil)
       (t auto-revert-buffers nil nil)
       (t apply (auto-revert-buffers nil) nil)
       (t timer-event-handler
          ([nil 26305 53479 773611 5 auto-revert-buffers nil nil 0 nil])
          nil))
      [(nil . minibuffer-complete-and-exit) 5 (nil . move-end-of-line) 127
       (nil . python-indent-dedent-line-backspace) 48
       (nil . self-insert-command) 24 24 19 (nil . save-buffer) 27 91 79
       27 91 73 27 91 79]))

Despite `python-indent-dedent-line-backspace' being present in the
backtrace, the error is not limited to `python-mode'.

My GNU Emacs is built from:

    Repository revision: 40eecd594ac60f38b6729fd9cf3474a8b9d133b9
    Repository branch: master
    System Description: macOS 14.5
    Configured using:
     'configure --without-x --without-ns --with-mailutils
     --with-kerberos5 --with-json --with-tree-sitter
     --with-native-compilation

Best regards,
Dario

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Eli Zaretskii]

> From: Dario Gjorgjevski <dario.gjorgjevski@gmail.com>
> Date: Sun, 18 Aug 2024 12:58:24 +0200
> 
> How to reproduce:
> 
> 1. In one shell, fire up GNU Emacs, open a file, and start something
> that uses track-changes; e.g., Eglot.  In my case it was a Python file
> called test.py with contents foo = 0.  Everything is happy so far.
> 2. In another shell, fire up a Node.js interpreter and execute const
> fs = require("node:fs/promises") followed by await
> fs.writeFile("test.py", "foo = 1").  This will overwrite the file.
> 3. Track-changes (and by extension Eglot) will error out.  Restarting
> Eglot makes everything go back to normal.
> 
> The *Warnings* buffer now contains:
> 
>     Warning (emacs): Missing/incorrect calls to
> ‘before/after-change-functions’!!
>     Details logged to `track-changes--error-log'
> 
> And `track-changes--error-log' contains:
> 
>     (("test.py" #1=(unexpected-after 1 8 0)
>       ((t track-changes--recover-from-error (#1#) nil)
>        (t track-changes--after (1 8 0) nil)
>        (t revert-buffer-insert-file-contents--default-function
>           ("/path/to/test.py"
>            nil)
>           nil)
>        (t revert-buffer--default (ignore-auto dont-ask) nil)
>        (t revert-buffer (ignore-auto dont-ask preserve-modes) nil)
>        (t auto-revert-handler nil nil)
>        (t auto-revert-buffer (#<buffer test.py>) nil)
>        (t #<subr auto-revert-buffers> nil nil)
>        (t auto-revert-buffers@buffer-list-filter
>           (#<subr auto-revert-buffers>) nil)
>        (t apply
>           (auto-revert-buffers@buffer-list-filter #<subr
>                               auto-revert-buffers> nil)
>           nil)
>        (t auto-revert-buffers nil nil)
>        (t apply (auto-revert-buffers nil) nil)
>        (t timer-event-handler
>           ([nil 26305 53479 773611 5 auto-revert-buffers nil nil 0 nil])
>           nil))
>       [(nil . minibuffer-complete-and-exit) 5 (nil . move-end-of-line) 127
>        (nil . python-indent-dedent-line-backspace) 48
>        (nil . self-insert-command) 24 24 19 (nil . save-buffer) 27 91 79
>        27 91 73 27 91 79]))
> 
> Despite `python-indent-dedent-line-backspace' being present in the
> backtrace, the error is not limited to `python-mode'.
> 
> My GNU Emacs is built from:
> 
>     Repository revision: 40eecd594ac60f38b6729fd9cf3474a8b9d133b9
>     Repository branch: master
>     System Description: macOS 14.5
>     Configured using:
>      'configure --without-x --without-ns --with-mailutils
>      --with-kerberos5 --with-json --with-tree-sitter
>      --with-native-compilation

Adding Stefan and João.

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 1. In one shell, fire up GNU Emacs, open a file, and start something
> that uses track-changes; e.g., Eglot.  In my case it was a Python file
> called test.py with contents foo = 0.  Everything is happy so far.
> 2. In another shell, fire up a Node.js interpreter and execute const
> fs = require("node:fs/promises") followed by await
> fs.writeFile("test.py", "foo = 1").  This will overwrite the file.
> 3. Track-changes (and by extension Eglot) will error out.  Restarting
> Eglot makes everything go back to normal.
>
> The *Warnings* buffer now contains:
>
>     Warning (emacs): Missing/incorrect calls to
> ‘before/after-change-functions’!!
>     Details logged to `track-changes--error-log'

The above is only a "warning": it just logged the occurrence of something
undesired, but track-changes supposedly recovered from that.
IOW you describe two problems:

- Somehow/somewhere there is a "Missing/incorrect calls to
  ‘before/after-change-functions".
  [ But these problems are sufficiently common that track-changes has
    code devoted to detecting and handling those cases.  ]
  We want to try and fix the origin of this problem, but it's
  been with us "for ever", so it's not super extra urgent because there
  will still be other cases where it can show up.
  Eglot has had to deal with this problem since its inception, it just
  used to keep silent about it, contrary to track-changes (because
  I want to get rid of those problems).

- Eglot errors out.  This should definitely not happen and seems like
  a regression.  Restarting Eglot should definitely not be needed.

Could you clarify what you mean by Eglot erroring out and why
restarting it is necessary?

> And `track-changes--error-log' contains:
>
>     (("test.py" #1=(unexpected-after 1 8 0)
>       ((t track-changes--recover-from-error (#1#) nil)
>        (t track-changes--after (1 8 0) nil)
>        (t revert-buffer-insert-file-contents--default-function
>           ("/path/to/test.py"
>            nil)
>           nil)
>        (t revert-buffer--default (ignore-auto dont-ask) nil)
>        (t revert-buffer (ignore-auto dont-ask preserve-modes) nil)

Hmm... assuming the file really started with content "foo = 0" (and no
terminating newline), "track-changes--after (1 8 0)" looks OK: the
revert has changed the file size by 0 chars and the new content spans
chars from 1 to 8.  So my guess is that revert-buffer failed to call the
`before-changes-functions` or that it called it with a smaller region
(e.g. only the region that's actually modified (7..8), where "0" is
turned into "1").

Will have to dig deeper.

BTW, your recipe does not mention enabling auto-revert.  Have you
confirmed on your side that the problem shows up also in
a vanilla config?


        Stefan

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Dario Gjorgjevski]

Hi, Stefan,

On Sun, Aug 18, 2024 at 6:10 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> - Eglot errors out.  This should definitely not happen and seems like
>   a regression.  Restarting Eglot should definitely not be needed.
>
> Could you clarify what you mean by Eglot erroring out and why
> restarting it is necessary?

Eglot gets in a weird state and begins reporting erroneous data to the
LSP server.

>
> Hmm... assuming the file really started with content "foo = 0" (and no
> terminating newline), "track-changes--after (1 8 0)" looks OK: the
> revert has changed the file size by 0 chars and the new content spans
> chars from 1 to 8.  So my guess is that revert-buffer failed to call the
> `before-changes-functions` or that it called it with a smaller region
> (e.g. only the region that's actually modified (7..8), where "0" is
> turned into "1").
>
> Will have to dig deeper.

One thing I see is that the buffer becomes empty for a split second,
and is then overwritten with the new contents.  I don't know how
exactly Node.js does the overwriting, but I guess it's worth looking
into.

>
> BTW, your recipe does not mention enabling auto-revert.  Have you
> confirmed on your side that the problem shows up also in
> a vanilla config?

You're right, the problem depends on auto-revert-mode being active.
Sorry about that.

Best regards,
Dario

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> - Eglot errors out.  This should definitely not happen and seems like
>>   a regression.  Restarting Eglot should definitely not be needed.
>> Could you clarify what you mean by Eglot erroring out and why
>> restarting it is necessary?
> Eglot gets in a weird state and begins reporting erroneous data to the
> LSP server.

Can you give any further details?


        Stefan

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Dario Gjorgjevski]

Hi, Stefan,

On Tue, Aug 20, 2024 at 2:15 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> >> - Eglot errors out.  This should definitely not happen and seems like
> >>   a regression.  Restarting Eglot should definitely not be needed.
> >> Could you clarify what you mean by Eglot erroring out and why
> >> restarting it is necessary?
> > Eglot gets in a weird state and begins reporting erroneous data to the
> > LSP server.
>
> Can you give any further details?

Consider a Python file with the following contents (including a
trailing newline):

    from urllib import urlretrieve
    local_filename, headers = urlretrieve('http://python.org/')

Using the Pyright LSP server, Eglot correctly reports that:

    Pyright [reportAttributeAccessIssue]: "urlretrieve" is unknown import symbol

Now, rewrite the file using Node.js as instructed above so that its
contents become (again, including a trailing newline):

    from urllib.request import urlretrieve
    local_filename, headers = urlretrieve('http://python.org/')

Eglot now weirdly reports that:

    Pyright [reportMissingImports]: Import "urllib.request.request"
could not be resolved

Somehow it thinks that the ".request" part appears twice in a row.
Here is the relevant communication between Eglot and Pyright:

    [jsonrpc] e[13:00:21.956] --> textDocument/didOpen
    {
      "jsonrpc": "2.0",
      "method": "textDocument/didOpen",
      "params": {
        "textDocument": {
          "uri": "file:///Volumes/workplace/playground/test.py",
          "version": 0,
          "languageId": "python",
          "text": "from urllib import urlretrieve\nlocal_filename,
headers = urlretrieve('http://python.org/')\n"
        }
      }
    }

    [jsonrpc] e[13:00:22.617] <-- textDocument/publishDiagnostics
    {
      "jsonrpc": "2.0",
      "method": "textDocument/publishDiagnostics",
      "params": {
        "uri": "file:///Volumes/workplace/playground/test.py",
        "version": 0,
        "diagnostics": [
          {
            "range": {
              "start": {
                "line": 0,
                "character": 19
              },
              "end": {
                "line": 0,
                "character": 30
              }
            },
            "message": "\"urlretrieve\" is unknown import symbol",
            "severity": 1,
            "code": "reportAttributeAccessIssue",
            "source": "Pyright",
            "codeDescription": {
              "href":
"https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportAttributeAccessIssue"
            }
          }
        ]
      }
    }

    [jsonrpc] e[13:00:26.362] --> textDocument/didClose
    {
      "jsonrpc": "2.0",
      "method": "textDocument/didClose",
      "params": {
        "textDocument": {
          "uri": "file:///Volumes/workplace/playground/test.py"
        }
      }
    }

    [jsonrpc] e[13:00:26.364] --> textDocument/didOpen
    {
      "jsonrpc": "2.0",
      "method": "textDocument/didOpen",
      "params": {
        "textDocument": {
          "uri": "file:///Volumes/workplace/playground/test.py",
          "version": 0,
          "languageId": "python",
          "text": "from urllib.request import
urlretrieve\nlocal_filename, headers =
urlretrieve('http://python.org/')\n"
        }
      }
    }

    [jsonrpc] e[13:00:26.366] --> textDocument/didChange
    {
      "jsonrpc": "2.0",
      "method": "textDocument/didChange",
      "params": {
        "textDocument": {
          "uri": "file:///Volumes/workplace/playground/test.py",
          "version": 1
        },
        "contentChanges": [
          {
            "range": {
              "start": {
                "line": 0,
                "character": 11
              },
              "end": {
                "line": 0,
                "character": 11
              }
            },
            "rangeLength": 0,
            "text": ".request"
          }
        ]
      }
    }

    [jsonrpc] e[13:00:26.629] <-- textDocument/publishDiagnostics
    {
      "jsonrpc": "2.0",
      "method": "textDocument/publishDiagnostics",
      "params": {
        "uri": "file:///Volumes/workplace/playground/test.py",
        "version": 1,
        "diagnostics": [
          {
            "range": {
              "start": {
                "line": 0,
                "character": 5
              },
              "end": {
                "line": 0,
                "character": 27
              }
            },
            "message": "Import \"urllib.request.request\" could not be
resolved",
            "severity": 1,
            "code": "reportMissingImports",
            "source": "Pyright",
            "codeDescription": {
              "href":
"https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportMissingImports"
            }
          }
        ]
      }
    }

As far as I can see, the testDocument/didChange request should not be
there.  The rewrite seems to have triggered a textDocument/didClose
followed by a textDocument/didOpen, so the erroneous
textDocument/didChange ended up making the LSP server think the
".request" part appears twice in a row.

Best regards,
Dario

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Dario Gjorgjevski]

BTW, I am using Python for illustrative purposes only.  This issue is
particularly annoying when writing JavaScript/TypeScript with a LSP
server and ESLint.  Running eslint --fix for in-place linting makes
ESLint rewrite the files in exactly this way (see
https://github.com/eslint/eslint/blob/5dbdd63dc83428447e25f1fc1d05d8a69e3b006a/lib/eslint/eslint.js#L752),
which then makes the LSP server exhibit weird diagnostics and
necessitates a restart of Eglot.

Best regards,
Dario

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Eli Zaretskii]

> Cc: 72696@debbugs.gnu.org
> From: Dario Gjorgjevski <dario.gjorgjevski@gmail.com>
> Date: Thu, 22 Aug 2024 13:27:06 +0200
> 
> BTW, I am using Python for illustrative purposes only.  This issue is
> particularly annoying when writing JavaScript/TypeScript with a LSP
> server and ESLint.  Running eslint --fix for in-place linting makes
> ESLint rewrite the files in exactly this way (see
> https://github.com/eslint/eslint/blob/5dbdd63dc83428447e25f1fc1d05d8a69e3b006a/lib/eslint/eslint.js#L752),
> which then makes the LSP server exhibit weird diagnostics and
> necessitates a restart of Eglot.

Stefan, any comments?

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

> As far as I can see, the testDocument/didChange request should not be
> there.

Either the `didClose/didOpen` should not be there, or the `didChange` should
not be there.  The problem is that we have both.

Can you try the patch below, which I suspect should fix your problem?
It should get rid of the `didChange` in your recipe.

We could also change Eglot so it doesn't do `didClose/didOpen` upon
`revert-buffer` (in which case it should/will do the `didChange`), but
that should be a separate concern.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eglot.patch --]
[-- Type: text/x-diff, Size: 529 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 82e99a2c920..a2c9f73fc73 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2813,6 +2813,8 @@ eglot--signal-textDocument/didChange
 
 (defun eglot--signal-textDocument/didOpen ()
   "Send textDocument/didOpen to server."
+  ;; Flush any potential pending change.
+  (eglot--track-changes-fetch eglot--track-changes)
   (setq eglot--recent-changes nil
         eglot--versioned-identifier 0
         eglot--TextDocumentIdentifier-cache nil)

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Dario Gjorgjevski]

Hi, Stefan,

On Mon, Sep 9, 2024 at 1:06 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > As far as I can see, the testDocument/didChange request should not be
> > there.
>
> Either the `didClose/didOpen` should not be there, or the `didChange` should
> not be there.  The problem is that we have both.
>
> Can you try the patch below, which I suspect should fix your problem?
> It should get rid of the `didChange` in your recipe.

I gave your patch a try and it indeed fixes the issue.  Thank you very
much for looking into it!

Best regards,
Dario

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Eli Zaretskii]

> Cc: 72696@debbugs.gnu.org
> From: Dario Gjorgjevski <dario.gjorgjevski@gmail.com>
> Date: Thu, 19 Sep 2024 00:02:34 +0200
> 
> Hi, Stefan,
> 
> On Mon, Sep 9, 2024 at 1:06 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >
> > > As far as I can see, the testDocument/didChange request should not be
> > > there.
> >
> > Either the `didClose/didOpen` should not be there, or the `didChange` should
> > not be there.  The problem is that we have both.
> >
> > Can you try the patch below, which I suspect should fix your problem?
> > It should get rid of the `didChange` in your recipe.
> 
> I gave your patch a try and it indeed fixes the issue.  Thank you very
> much for looking into it!

João, is Stefan's patch okay with you?  AFAIU, the same problem exists
on the emacs-30 release branch, so I'd like to install the patch
there, if you agree.

Here's the patch in case you didn't see it (I CC'ed you on the
original report, but subsequent discussion omitted you for some
reason, sigh):

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 82e99a2c920..a2c9f73fc73 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2813,6 +2813,8 @@ eglot--signal-textDocument/didChange
 
 (defun eglot--signal-textDocument/didOpen ()
   "Send textDocument/didOpen to server."
+  ;; Flush any potential pending change.
+  (eglot--track-changes-fetch eglot--track-changes)
   (setq eglot--recent-changes nil
         eglot--versioned-identifier 0
         eglot--TextDocumentIdentifier-cache nil)

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> João, is Stefan's patch okay with you?  AFAIU, the same problem exists
> on the emacs-30 release branch, so I'd like to install the patch
> there, if you agree.

Indeed, it belongs there.
It should have been part of the original commit d7a83e23d47c.


        Stefan

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[João Távora]

On Thu, Sep 19, 2024 at 1:58 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > João, is Stefan's patch okay with you?  AFAIU, the same problem exists
> > on the emacs-30 release branch, so I'd like to install the patch
> > there, if you agree.
>
> Indeed, it belongs there.
> It should have been part of the original commit d7a83e23d47c.

You can go ahead and merge it.  I dont' know what it does
but i trust Stefan.

Although I have to say that on the topic of synchronization failures
between Eglot and servers, I'm not sure that things have improved
much since this new layer of abstraction was added.  By "I'm not
sure" I really mean that.  Maybe pre-existing failures are just more
visible right now (because the new code warns and logs them), but
I have a nagging feeling that there are more of them.  No hard
proof though.

I also half-expected things to get fixed in Emacs proper, using
Eglot as testing bed, but I'm not sure that has happened either.
Anyway this is just a comment.

João

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 19 Sep 2024 22:47:14 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, Dario Gjorgjevski <dario.gjorgjevski@gmail.com>, 72696@debbugs.gnu.org
> 
> On Thu, Sep 19, 2024 at 1:58 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >
> > > João, is Stefan's patch okay with you?  AFAIU, the same problem exists
> > > on the emacs-30 release branch, so I'd like to install the patch
> > > there, if you agree.
> >
> > Indeed, it belongs there.
> > It should have been part of the original commit d7a83e23d47c.
> 
> You can go ahead and merge it.  I dont' know what it does
> but i trust Stefan.

Thanks.  Stefan, please install on the emacs-30 branch.

> Although I have to say that on the topic of synchronization failures
> between Eglot and servers, I'm not sure that things have improved
> much since this new layer of abstraction was added.  By "I'm not
> sure" I really mean that.  Maybe pre-existing failures are just more
> visible right now (because the new code warns and logs them), but
> I have a nagging feeling that there are more of them.  No hard
> proof though.
> 
> I also half-expected things to get fixed in Emacs proper, using
> Eglot as testing bed, but I'm not sure that has happened either.
> Anyway this is just a comment.

I agree with your general feeling, and I think this case might belong
to those pre-existing failures which track-changes uncovers.

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> You can go ahead and merge it.  I dont' know what it does
>> but i trust Stefan.
> Thanks.  Stefan, please install on the emacs-30 branch.

Thanks, done.

> I agree with your general feeling, and I think this case might belong
> to those pre-existing failures which track-changes uncovers.

FWIW, I think this bug was one that I introduced when changing Eglot to
use track-changes (basically, because track-changes does its own
"buffering" in addition to the one Eglot does in
`eglot--recent-changes`, and I forgot to flush that buffer when
`eglot--recent-changes` is flushed).


        Stefan

bug#72696: Track-changes errors out when file is overwritten using Node.js's fs.writeFile (at least on macOS)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> You can go ahead and merge it.  I dont' know what it does
>>> but i trust Stefan.
>> Thanks.  Stefan, please install on the emacs-30 branch.
> Thanks, done.

Closing,


        Stefan