bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Richard Copley]

(Moved here from https://github.com/joaotavora/eglot/discussions/1389)
On master.

The warning:

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

... is raised, while editing in an Eglot buffer, with an input method.

* Ensure clangd is on the path
* Start editing a scratch file like this in C++ mode:

int main() {
  const char * s = "";
}

* Start Eglot (M-x eglot RET)
* Enable TeX input method (C-u C-\ TeX RET)
* Start typing the superscript alphabet into the string (^ a ^ b ^ c ^ d ...)

How far you get before the warning pops up seems to depend on timing.
In a run of this experiment from emacs -Q on master just now, I got as
far as k.

track-changes--error-log value:

((#3="z.cpp"
     ((t track-changes--recover-from-error nil nil)
      (t track-changes-fetch
(#1=
 #s(track-changes--tracker :signal
   eglot--track-changes-signal :state
   #s(track-changes--state :beg 50
   :end 1
   :before
   nil :next
   nil)
   :nobefore nil :immediate nil)
 #f(compiled-function (beg end before) #<bytecode
      0xae090d111a>))
nil)
      (t eglot--track-changes-fetch (#1#) nil)
      (t eglot--signal-textDocument/didChange nil nil)
      (t run-hooks (eglot--document-changed-hook) nil)
      (t #2=#f(compiled-function () #<bytecode 0x1bb90600ea83b761>)
nil nil)
      (t apply (#2# nil) nil)
      (t timer-event-handler ([t 0 0 500000 nil #2# nil idle 0 nil])
nil))
     [101 #6=(nil . self-insert-command) 94 102 #7=
 (nil . self-insert-command) 94 103 #8=
 (nil . self-insert-command) 94 104 #9=
 (nil . self-insert-command) 94 105 #10=
 (nil . self-insert-command) 94 106 #11=
 (nil . self-insert-command) 94 107
 (nil . self-insert-command)])
 (#3#
  ((t track-changes--recover-from-error nil nil)
   (t track-changes-fetch
      (#1#
       #f(compiled-function (beg end before) #<bytecode 0xae090d111a>))
      nil)
   (t eglot--track-changes-fetch (#1#) nil)
   (t eglot--signal-textDocument/didChange nil nil)
   (t run-hooks (eglot--document-changed-hook) nil)
   (t #5=#f(compiled-function () #<bytecode 0x1bb90600ea83b761>) nil
      nil)
   (t apply (#5# nil) nil)
   (t timer-event-handler ([t 0 0 500000 nil #5# nil idle 0 nil]) nil)
   (t read-key-sequence (nil nil nil t) nil)
   (t quail-start-translation (94) nil)
   (t quail-input-method (94) nil))
  [(nil . self-insert-command) 94 101 #6# 94 102 #7# 94 103 #8# 94 104
   #9# 94 105 #10# 94 106 #11# 94]))

Lossage:

 M-x     ;; execute-extended-command
 e     ;; self-insert-command
 g     ;; self-insert-command
 l     ;; self-insert-command
 o     ;; self-insert-command
 t     ;; self-insert-command
 <return>     ;; minibuffer-complete-and-exit
 C-u     ;; universal-argument
 C-\     ;; toggle-input-method
 T     ;; self-insert-command
 e     ;; self-insert-command
 X     ;; self-insert-command
 <return>     ;; minibuffer-complete-and-exit
 C-s     ;; isearch-forward
 "     ;; isearch-printing-char
 <with-input-method> ;; isearch-with-input-method
 <return>     ;; isearch-exit
 ^ a     ;; self-insert-command
 ^ b     ;; self-insert-command
 ^ c     ;; self-insert-command
 ^ d     ;; self-insert-command
 ^ e     ;; self-insert-command
 ^ f     ;; self-insert-command
 ^ g     ;; self-insert-command
 ^ h     ;; self-insert-command
 ^ i     ;; self-insert-command
 ^ j     ;; self-insert-command
 ^ k     ;; self-insert-command
 C-h l     ;; view-lossage

(Stefan notes:

If you want to silence these messages until the problem is fixed, you
can `(setq track-changes-record-errors nil)`.)

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

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

> * Ensure clangd is on the path
> * Start editing a scratch file like this in C++ mode:
>
> int main() {
>   const char * s = "";
> }
>
> * Start Eglot (M-x eglot RET)
> * Enable TeX input method (C-u C-\ TeX RET)
> * Start typing the superscript alphabet into the string (^ a ^ b ^ c ^ d ...)
>
> How far you get before the warning pops up seems to depend on timing.

To make it reliable, here's what you need to do after enabling the TeX
input method:

    type "quickly"; foo^
    wait a couple seconds

and you should get the 

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

The problem is that Quail inserts an underlined `^` in the buffer to
show you the part of the input that's already been typed and it does so
stealthily (i.e. within `with-silent-modifications`), which implies that
the `before/after-change-functions` have not been called, and as
a consequence the size of the buffer is not the one that track-changes
expects (and the content of the buffer doesn't corresponds to what Eglot
will send to the LSP server based on the changes it has witnessed,
which can cause errors since Eglot has to send buffer positions and
those may not mean the same any more for the LSP server).

I suggest the patch below (the second hunk is unrelated, I just bumped
into it while tracking this bug).


        Stefan

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

diff --git a/lisp/international/quail.el b/lisp/international/quail.el
index 48d2ccb8828..cb7aa89b252 100644
--- a/lisp/international/quail.el
+++ b/lisp/international/quail.el
@@ -1334,9 +1334,13 @@ quail-input-method
     (quail-setup-overlays (quail-conversion-keymap))
     (with-silent-modifications
       (unwind-protect
-	  (let ((input-string (if (quail-conversion-keymap)
+	  (let* (;; `with-silent-modifications' inhibits the modification
+                 ;; hooks, but that's a part of `with-silent-modifications'
+                 ;; we don't actually want here (bug#70541).
+		 (inhibit-modification-hooks nil)
+		 (input-string (if (quail-conversion-keymap)
 				  (quail-start-conversion key)
-				(quail-start-translation key))))
+				  (quail-start-translation key))))
 	    (setq quail-guidance-str "")
 	    (when (and (stringp input-string)
 		       (> (length input-string) 0))
@@ -1871,10 +1875,9 @@ quail-delete-last-char
 
 (defsubst quail-point-in-conversion-region ()
   "Return non-nil value if the point is in conversion region of Quail mode."
-  (let (start pos)
-    (and (setq start (overlay-start quail-conv-overlay))
-	 (>= (setq pos (point)) start)
-	 (<= pos (overlay-end quail-conv-overlay)))))
+  (let ((start (overlay-start quail-conv-overlay)))
+    (and start
+	 (<= start (point) (overlay-end quail-conv-overlay)))))
 
 (defun quail-conversion-backward-char ()
   (interactive)

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Eli Zaretskii]

> Cc: 70541@debbugs.gnu.org,
>  João Távora <joaotavora@gmail.com>
> Date: Tue, 23 Apr 2024 23:14:16 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> The problem is that Quail inserts an underlined `^` in the buffer to
> show you the part of the input that's already been typed and it does so
> stealthily (i.e. within `with-silent-modifications`), which implies that
> the `before/after-change-functions` have not been called, and as
> a consequence the size of the buffer is not the one that track-changes
> expects (and the content of the buffer doesn't corresponds to what Eglot
> will send to the LSP server based on the changes it has witnessed,
> which can cause errors since Eglot has to send buffer positions and
> those may not mean the same any more for the LSP server).
> 
> I suggest the patch below (the second hunk is unrelated, I just bumped
> into it while tracking this bug).

Are you sure we want buffer-change hooks to be called here?  Quail
intentionally hides some of the modifications it does, because it many
times replaces the inserted text with something else, and from the
Emacs Lisp program's POV only the final input is the actual
"insertion" Emacs should know about.

So I'm not sure we should install this, as it could break something
elsewhere.  Aren't there any alternatives to this?  More generally,
why should Eglot care about these low-level details of Quail?

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> Are you sure we want buffer-change hooks to be called here?

Adding/removing chars from the buffer without running those hooks breaks
all kinds of assumptions.  It can be acceptable to do it in a very
temporary way, but Quail doesn't do it temporarily: the whole
redisplay+timers+filters gets run in the middle of the input of any
multi-key entry like `^a`, so the "temporary" state is very
much exposed.

> Quail intentionally hides some of the modifications it does, because
> it many times replaces the inserted text with something else, and from
> the Emacs Lisp program's POV only the final input is the actual
> "insertion" Emacs should know about.

I'm not sure why it's important to hide the intermediate steps,
especially since they're not very well hidden (as evidenced by this bug
report, and by the fact that font-lock is also triggered every time the
transient display is modified).

Note that when users use Abbrev instead to turn \lambda into λ, the
intermediate steps are not hidden, and that's not been a problem.

> So I'm not sure we should install this, as it could break something
> elsewhere.  Aren't there any alternatives to this?

We could change Quail so it refrains from temporarily modifying the
buffer, using an `after/before-string` on an overlay instead.
It'd be a fairly significant change in `quail.el` and would probably
come with its own share of problems.

> More generally,
> why should Eglot care about these low-level details of Quail?

Because Eglot syncs up with the LSP server via a timer, so it sees the
buffer with an extra "^" char in it.


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  70541@debbugs.gnu.org,  joaotavora@gmail.com
> Date: Wed, 24 Apr 2024 10:26:40 -0400
> 
> > Are you sure we want buffer-change hooks to be called here?
> 
> Adding/removing chars from the buffer without running those hooks breaks
> all kinds of assumptions.  It can be acceptable to do it in a very
> temporary way, but Quail doesn't do it temporarily: the whole
> redisplay+timers+filters gets run in the middle of the input of any
> multi-key entry like `^a`, so the "temporary" state is very
> much exposed.

It's exposed to some parts of Emacs, but not to others.

> > Quail intentionally hides some of the modifications it does, because
> > it many times replaces the inserted text with something else, and from
> > the Emacs Lisp program's POV only the final input is the actual
> > "insertion" Emacs should know about.
> 
> I'm not sure why it's important to hide the intermediate steps,

Because Quail emulates keyboard input.  Conceptually, everything that
the user types into a buffer that Quail processes and replaces "did
not happen", only the final insertion of the produced character(s) are
real.

> especially since they're not very well hidden (as evidenced by this bug
> report, and by the fact that font-lock is also triggered every time the
> transient display is modified).

Actually, it's hidden quite well, it's just that Eglot gets confused
because it looks at stuff it isn't supposed to see ;-)

> Note that when users use Abbrev instead to turn \lambda into λ, the
> intermediate steps are not hidden, and that's not been a problem.

Abbrev cannot guide the user regarding the next steps when an initial
string typed by user has several possible candidates for further
input.  So Abbrev basically solves a simpler problem.

> > So I'm not sure we should install this, as it could break something
> > elsewhere.  Aren't there any alternatives to this?
> 
> We could change Quail so it refrains from temporarily modifying the
> buffer, using an `after/before-string` on an overlay instead.
> It'd be a fairly significant change in `quail.el` and would probably
> come with its own share of problems.

I'm not at all sure the solution should be in Quail.

> > More generally,
> > why should Eglot care about these low-level details of Quail?
> 
> Because Eglot syncs up with the LSP server via a timer, so it sees the
> buffer with an extra "^" char in it.

So maybe Eglot should learn that when it sees this and a Quail input
is in progress, it should pretend it didn't see anything?  IOW, why
not solve this in Eglot instead?  It's Eglot that makes incorrect
assumptions about what happens, so let's teach Eglot how to do better.

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> Actually, it's hidden quite well, it's just that Eglot gets confused
> because it looks at stuff it isn't supposed to see ;-)

Who's supposed to see it and who isn't?
Tree-sitter is told about it, font-lock is told about it, why not others
via the usual mechanism?  What's different about Eglot?

>> Note that when users use Abbrev instead to turn \lambda into λ, the
>> intermediate steps are not hidden, and that's not been a problem.
> Abbrev cannot guide the user regarding the next steps when an initial
> string typed by user has several possible candidates for further
> input.  So Abbrev basically solves a simpler problem.

When combined with appropriate completion tables and UIs, abbrev also
guide the user, so it's not that different.
Also, I fail to see how that's relevant.

The buffer state is modified by Quail.  It's somewhat temporary but
there's still a lot that can happen during that temporary state.

> So maybe Eglot should learn that when it sees this and a Quail input
> is in progress, it should pretend it didn't see anything?

That seems very yucky.  Suddenly packages like Eglot, lsp-mode, crdt,
TeXpresso, CriticalMarkup, ... need to learn about that special
interaction with Quail.  And how are they going to deal with it?

The only sane way I can see to deal with it would be for Quail to
provide a way to temporarily return to the "real" state (e.g. deleting
the `^`) and then to get back to the temporary state (i.e. re-insert the
`^`).

This is pretty ugly in my book, sounds like workarounds on top
of workarounds.  Can we try the patch I suggested first?
It makes more code normal instead of adding more special code to deal
with special code, so if that works it's a much nicer option.

> IOW, why not solve this in Eglot instead?  It's Eglot that makes
> incorrect assumptions about what happens, so let's teach Eglot how to
> do better.

I don't think these are incorrect assumptions because there's not much
else it could do.  If packages like Eglot can't do their work correctly
without knowing about Quail, I think it means we have a poor API.


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  70541@debbugs.gnu.org,  joaotavora@gmail.com
> Date: Wed, 24 Apr 2024 15:02:47 -0400
> 
> > Actually, it's hidden quite well, it's just that Eglot gets confused
> > because it looks at stuff it isn't supposed to see ;-)
> 
> Who's supposed to see it and who isn't?

Those who need to know are supposed to see, the rest aren't ;-)

Seriously, though:

> The buffer state is modified by Quail.  It's somewhat temporary but
> there's still a lot that can happen during that temporary state.

It isn't just temporary: it's a change that "isn't supposed to exist".
It's just a side effect of how Quail is implemented.

> > So maybe Eglot should learn that when it sees this and a Quail input
> > is in progress, it should pretend it didn't see anything?
> 
> That seems very yucky.  Suddenly packages like Eglot, lsp-mode, crdt,
> TeXpresso, CriticalMarkup, ... need to learn about that special
> interaction with Quail.

It isn't suddenly, it's because you switched Eglot to the new
track-changes method, right?  It worked fine before that, with the
same Quail, right?  Or am I missing something?

And why "yucky"?  This is the same Quail in all those cases, and the
same track-changes machinery.  So if Quail gets in the way of
track-changes, how about if Quail sets some flag which tells the
application level that changes in progress are to be ignored?  If we
can handle that as part of track-changes, fine; otherwise, Eglot and
the rest that use track-changes will have to ignore that on the
application level.  Doesn't sound yucky to me.

> And how are they going to deal with it?

By ignoring the changes performed while that flag is set.

> The only sane way I can see to deal with it would be for Quail to
> provide a way to temporarily return to the "real" state (e.g. deleting
> the `^`) and then to get back to the temporary state (i.e. re-insert the
> `^`).

Why is it not enough to ignore the changes?

> This is pretty ugly in my book, sounds like workarounds on top
> of workarounds.  Can we try the patch I suggested first?

We could try, but how many times do we need to make changes like that
in Quail that bite us elsewhere before we learn the simple truth that
we shouldn't try that anymore?

> It makes more code normal instead of adding more special code to deal
> with special code, so if that works it's a much nicer option.

Once again: Quail uses with-silent-modifications for a reason.  You
are basically suggesting to ignore that reason, and hwy? because it
makes the solution much easier?  A simpler solution is only preferable
when we know for sure it is correct, and here we are just guessing,
since we don't really have a clear idea what will that cause in other
cases.

> > IOW, why not solve this in Eglot instead?  It's Eglot that makes
> > incorrect assumptions about what happens, so let's teach Eglot how to
> > do better.
> 
> I don't think these are incorrect assumptions because there's not much
> else it could do.  If packages like Eglot can't do their work correctly
> without knowing about Quail, I think it means we have a poor API.

I'm suggesting a way for Quail to tell those applications that it is
in the process of making changes that should be ignored.  If such a
mechanism is possible, I think those applications could DTRT without
much effort.  Or am I missing something?

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[João Távora]

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

On Wed, Apr 24, 2024, 20:24 Eli Zaretskii <eliz@gnu.org> wrote:

> .
>
> It isn't suddenly, it's because you switched Eglot to the new
> track-changes method, right?  It worked fine before that, with the
> same Quail, right?  Or am I missing something?
>

I haven't much to add to this discussion where I'm being Cc'ed except that
I generally agree with Stefan's stance and fix and that I am fairly sure
this has always a problem before Stefan's track-changes.el framework, only
that we did not know about it because the raw usage eglot.el made of b-c-f
and a-c-f didn't sanity-check anything. You just got subtly wrong server
behaviour because it was being misinformed, and ended up reconnecting to
fix these things.

>
>

[-- Attachment #2: Type: text/html, Size: 1249 bytes --]

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

>> The buffer state is modified by Quail.  It's somewhat temporary but
>> there's still a lot that can happen during that temporary state.
> It isn't just temporary: it's a change that "isn't supposed to exist".
> It's just a side effect of how Quail is implemented.

But what does it mean concretely?  In which sense is it supposed not
to exist?
And more to the point, what makes it important to hide those changes?

>> > So maybe Eglot should learn that when it sees this and a Quail input
>> > is in progress, it should pretend it didn't see anything?
>> 
>> That seems very yucky.  Suddenly packages like Eglot, lsp-mode, crdt,
>> TeXpresso, CriticalMarkup, ... need to learn about that special
>> interaction with Quail.
>
> It isn't suddenly, it's because you switched Eglot to the new
> track-changes method, right?

No, the problem was there before just as well.  The difference is that
`track-changes.el` is more careful both to detect and to report
such problems.

> It worked fine before that, with the same Quail, right?

Yes and no: in some cases the old code failed to detect the problem and
that could result in broken behavior.  When the old and new code detect
the problem, they both "work fine" in the sense that the behavior is
correct but at an extra cost because after detecting the inconsistency
Eglot does a full resync with the server.

> Or am I missing something?

It also works correctly with the new code.  The difference is that we
report it (notice the `Subject:` says "warning").
[ Note also that `track-changes.el` does not warn about it when running
  in a released version of Emacs (see `track-changes-record-errors`),
  because I assume it's less useful.  ]

>> And how are they going to deal with it?
> By ignoring the changes performed while that flag is set.

Define "ignore".
The change are there.  `point`, `point-max`, `current-column`,
etc... are affected.

>> This is pretty ugly in my book, sounds like workarounds on top
>> of workarounds.  Can we try the patch I suggested first?
> We could try, but how many times do we need to make changes like that
> in Quail that bite us elsewhere before we learn the simple truth that
> we shouldn't try that anymore?

Which other times are you referring to?


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  70541@debbugs.gnu.org,  joaotavora@gmail.com
> Date: Sun, 28 Apr 2024 14:21:19 -0400
> 
> >> The buffer state is modified by Quail.  It's somewhat temporary but
> >> there's still a lot that can happen during that temporary state.
> > It isn't just temporary: it's a change that "isn't supposed to exist".
> > It's just a side effect of how Quail is implemented.
> 
> But what does it mean concretely?  In which sense is it supposed not
> to exist?
> And more to the point, what makes it important to hide those changes?

The actual change is the character(s) inserted when the Quail input
function completes its job.  Everything else is ephemeral, and Quail
deletes it as part of its job.  So those insertions followed by
deletions are not meant to change the buffer, and should therefore be
ignored.

> >> > So maybe Eglot should learn that when it sees this and a Quail input
> >> > is in progress, it should pretend it didn't see anything?
> >> 
> >> That seems very yucky.  Suddenly packages like Eglot, lsp-mode, crdt,
> >> TeXpresso, CriticalMarkup, ... need to learn about that special
> >> interaction with Quail.
> >
> > It isn't suddenly, it's because you switched Eglot to the new
> > track-changes method, right?
> 
> No, the problem was there before just as well.  The difference is that
> `track-changes.el` is more careful both to detect and to report
> such problems.
> 
> > It worked fine before that, with the same Quail, right?
> 
> Yes and no: in some cases the old code failed to detect the problem and
> that could result in broken behavior.  When the old and new code detect
> the problem, they both "work fine" in the sense that the behavior is
> correct but at an extra cost because after detecting the inconsistency
> Eglot does a full resync with the server.
> 
> > Or am I missing something?
> 
> It also works correctly with the new code.  The difference is that we
> report it (notice the `Subject:` says "warning").
> [ Note also that `track-changes.el` does not warn about it when running
>   in a released version of Emacs (see `track-changes-record-errors`),
>   because I assume it's less useful.  ]

So this is actually a non-issue?

> >> And how are they going to deal with it?
> > By ignoring the changes performed while that flag is set.
> 
> Define "ignore".

Do nothing and wait for the flag to become reset again.

> The change are there.  `point`, `point-max`, `current-column`,
> etc... are affected.

Why does Eglot need to react to those changes immediately when they
happen?

> >> This is pretty ugly in my book, sounds like workarounds on top
> >> of workarounds.  Can we try the patch I suggested first?
> > We could try, but how many times do we need to make changes like that
> > in Quail that bite us elsewhere before we learn the simple truth that
> > we shouldn't try that anymore?
> 
> Which other times are you referring to?

For example, we made several changes in Quail and elsewhere to fix the
recording of characters and the related keyboard-macro issues.  It
took quite some effort to get that right and fix all the regressions
the initial too-naïve attempts caused elsewhere.

So I'd very much prefer that Quail signaled to applications that it's
in the middle of handling some complex input, and that applications
which track changes ignored the changes made during this period.

We might already have variables in Quail which could be used as such
flags: quail-translating, quail-converting, quail-current-str, and
quail-guidance-str, to name a few candidates.  Could any of them be
used for this purpose?

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[João Távora]

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

On Mon, Apr 29, 2024 at 7:09 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> initial too-naïve attempts caused elsewhere.
>
> So I'd very much prefer that Quail signaled to applications that it's
> in the middle of handling some complex input, and that applications
> which track changes ignored the changes made during this period.

This idea is good, if I understand it correctly, though I would
prefer if Eglot interfaced only with track-changes.el, and
it would tell it that it should momentarily halt reports
of changes to the server.

Can someone clarify in a simple example exactly what Eglot tells
the LSP server as someone is inputting something with the Quail.
I need to understand exactly where the "lie" is happening.

João

[-- Attachment #2: Type: text/html, Size: 905 bytes --]

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Ihor Radchenko]

João Távora <joaotavora@gmail.com> writes:

> On Mon, Apr 29, 2024 at 7:09 AM Eli Zaretskii <eliz@gnu.org> wrote:
>>
>> initial too-naïve attempts caused elsewhere.
>>
>> So I'd very much prefer that Quail signaled to applications that it's
>> in the middle of handling some complex input, and that applications
>> which track changes ignored the changes made during this period.

Would adding `combine-change-calls' to Quail be an option?
AFAIU, calling the change hooks in the middle of quail input is the main
culprit of the problems we are discussing here.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, rcopley@gmail.com, 70541@debbugs.gnu.org,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 29 Apr 2024 08:36:37 +0000
> 
> João Távora <joaotavora@gmail.com> writes:
> 
> > On Mon, Apr 29, 2024 at 7:09 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >>
> >> initial too-naïve attempts caused elsewhere.
> >>
> >> So I'd very much prefer that Quail signaled to applications that it's
> >> in the middle of handling some complex input, and that applications
> >> which track changes ignored the changes made during this period.
> 
> Would adding `combine-change-calls' to Quail be an option?

I don't think so, since more than a single function is involved
(AFAIU).

> AFAIU, calling the change hooks in the middle of quail input is the main
> culprit of the problems we are discussing here.

The original Quail code inhibits the modification hooks, so this
cannot be the culprit, can it?

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[João Távora]

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

On Mon, Apr 29, 2024 at 7:09 AM Eli Zaretskii <eliz@gnu.org> wrote:

>
> > It also works correctly with the new code.  The difference is that we
> > report it (notice the `Subject:` says "warning").
> > [ Note also that `track-changes.el` does not warn about it when running
> >   in a released version of Emacs (see `track-changes-record-errors`),
> >   because I assume it's less useful.  ]
>
> So this is actually a non-issue?
>

FTR I just managed to crash clangd after following the recipe. Eglot
detects the crash and reconnects automatically.  In my book, if Eglot's
misreporting causes a server to crash, it is an issue in Eglot (even
if it's also a server-side issue that a client's misreporting causes a
crash).

João

[-- Attachment #2: Type: text/html, Size: 1447 bytes --]

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> AFAIU, calling the change hooks in the middle of quail input is the main
>> culprit of the problems we are discussing here.
>
> The original Quail code inhibits the modification hooks, so this
> cannot be the culprit, can it?

You are right.
The problem here is more like
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51766

Then, I tend to agree with Stefan that the current behavior is not right
- a lot of things are happening during the intermediate Quail input,
including, fontification, for example (and that runs custom Elisp!). I
see not how the transient input
to-be-replaced-by-quail-in-the-near-future can be ignored in such
scenario.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> This idea is good, if I understand it correctly, though I would
> prefer if Eglot interfaced only with track-changes.el, and
> it would tell it that it should momentarily halt reports
> of changes to the server.

I guess we could add a function `track-change-inconsistent-state-p`
which Eglot could consult before calling `track-changes-fetch` and if
that returns non-nil, Eglot could reschedule the update to "later".

But that would just be a kludge usable by some packages like Eglot
because they have the luxury to reschedule.  For example, if some code
is run (e.g. via font-lock) which uses `syntax-ppss` while Quail's extra
chars are inserted, then `syntax-ppss` can't reschedule, and it may
record the resulting state in its cache after which
`inhibit-modification-hooks` may in turn prevent the cache from being
properly flushed when Quail removes its extra chars.

Removing the `inhibit-modification-hooks` would be a much
cleaner solution.  I asked Kenichi why he added that binding (back in
2005), but he hasn't replied yet.

> Can someone clarify in a simple example exactly what Eglot tells
> the LSP server as someone is inputting something with the Quail.
> I need to understand exactly where the "lie" is happening.

Here's the scenario: the user wants to insert `xᵃ` using the TeX input
method (the same problem can occur with other methods, of course), so
Emacs will receive the following inputs:

    x ^ a

but let's assume the user pauses between `^` and `a`.
At this point, the `before/after-change-functions` have been called to
announce the insertion of `x` and nothing more, but the buffer also has
an (unannounced) `^` inserted at point.

So the buffer's content as seen by `before/after-change-functions` (and
hence by the LSP server) is out of sync because of this additional `^`.

Since Quail does not call the `before/after-change-functions`, Eglot
doesn't specifically send Quail's input to the LSP server: in the above
scenario, `eglot--signal-textDocument/didChange` is called because the
`before/after-change-functions` was called in response to the previous
`x`.


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

>> >> The buffer state is modified by Quail.  It's somewhat temporary but
>> >> there's still a lot that can happen during that temporary state.
>> > It isn't just temporary: it's a change that "isn't supposed to exist".
>> > It's just a side effect of how Quail is implemented.
>> 
>> But what does it mean concretely?  In which sense is it supposed not
>> to exist?
>> And more to the point, what makes it important to hide those changes?
>
> The actual change is the character(s) inserted when the Quail input
> function completes its job.  Everything else is ephemeral, and Quail
> deletes it as part of its job.

While we can think of it as ephemeral, the fact is that it lasts through
redisplay, running timers, etc...

So it's not at all obvious why it should be treated differently from the
case where the users type something and then hit undo because they
realize they made a mistake, or when they type a short string followed
by `M-x expand-abbrev`.

There are many other forms of "ephemeral" changes, where we don't
inhibit modification hooks.  E.g. in `replace-match` we sometimes insert
the replacement and then capitalize it.  The intermediate
non-capitalized string is very much ephemeral: much more ephemeral than
Quail's (there's *very* little code run between the two operations).
Yet we call `before/after-change-functions` separately for both changes,
thereby exposing the intermediate state.

> So those insertions followed by deletions are not meant to change the
> buffer,

Yet they do.

> and should therefore be ignored.

I don't understand the "therefore".

Also, ignored by what?
Clearly they should not be ignored by redisplay (since the whole
purpose is to give guidance to the users).

Also, currently they're not ignored by `font-lock`.  Is that a bug?

>> It also works correctly with the new code.  The difference is that we
>> report it (notice the `Subject:` says "warning").
>> [ Note also that `track-changes.el` does not warn about it when running
>>   in a released version of Emacs (see `track-changes-record-errors`),
>>   because I assume it's less useful.  ]
> So this is actually a non-issue?

It's a performance bug.

>> The change are there.  `point`, `point-max`, `current-column`,
>> etc... are affected.
> Why does Eglot need to react to those changes immediately when they
> happen?

It doesn't: it reacts to the changes in an idle timer.

But the idle timer can see Quail's intermediate state, exactly because
it is not nearly as ephemeral as would be needed for
`with-silent-modifications` to be acceptable.

> So I'd very much prefer that Quail signaled to applications that it's
> in the middle of handling some complex input, and that applications
> which track changes ignored the changes made during this period.

Eglot could pause its work during Quail input.  But note that with some
input methods, being in the middle of a Quail input can be very
common, so it's not a great solution because it could unduly delay the
work of Eglot.

And of course, other packages may not have that luxury.

> We might already have variables in Quail which could be used as such
> flags: quail-translating, quail-converting, quail-current-str, and
> quail-guidance-str, to name a few candidates.  Could any of them be
> used for this purpose?

I definitely would not want Track-Changes to lookup a Quail variable.
We should instead use/introduce some package-neutral var (which other
packages could use).  But I'd first like to know why it is that Quail
needs `inhibit-modification-hooks` to make sure we really need such
a workaround.
[ BTW, let-binding some dynbound variable would not do since
  track-changes would not see it if called from a separate thread.
  It would have to be a buffer-local var.  ]


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> I guess we could add a function `track-change-inconsistent-state-p`
> which Eglot could consult before calling `track-changes-fetch` and if
> that returns non-nil, Eglot could reschedule the update to "later".

I just pushed a patch to `master` which does that.
Richard, can you confirm it fixes the problem on your end?

The problem of Quail binding
`inhibit-modification-hooks` remains, of course.  We should fix it
because it can affect many more things: all the code run in the middle
of a Quail input sequence (e.g. timers and process filters) are
effected, not just because they may see an "inconsistent" buffer state
but because the changes they make to buffers will themselves be
affected by the `inhibit-modification-hooks` binding.


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> I just pushed a patch to `master` which does that.
> Richard, can you confirm it fixes the problem on your end?

Hmm... apparently my machine failed to read my mind and dutifully used
the Emacs I told it to use rather than the one I was working on, so
no wonder the tests worked fine.

I have pushed further fixes to my typo-laden code.
It seems to work OK for me now, but I'd like to hear other people's
experience with it.


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Richard Copley]

On Fri, 3 May 2024 at 18:27, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> I just pushed a patch to `master` which does that.
> Richard, can you confirm it fixes the problem on your end?

On Fri, 3 May 2024 at 21:56, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> It seems to work OK for me now, but I'd like to hear other people's
> experience with it.

Thanks Stefan.

One can trigger the track-changes signal in such a way as to defeat
the new check, as follows.

* Ensure clangd is on the path.
* Visit a C++ file with these contents:

void f () {
  int x = 0;
  int a = b;
}

* Start Eglot. (The "b" is highlighted to indicate an error.)
* Enable the TeX input method.
* Put point after the zero.
* (Enter the bad state.) Quickly, type [<backspace> ^]. Pause.
* Type [<backspace> y]. (No error highlight on "y".)
* Move point to before the "x". (No "variable x" echo from Eldoc.)
* Move point back to after the "y".
* Type [<backspace> z] again. (Still no error highlight on "z".)
* Type [SPC]. (Highlight on "b" is now rendered incorrectly.)
* (Exit the bad state.) Type [C-n SPC <backspace>].

Expected behaviour: After typing "y", Eglot sends "textDocument/didChange",
clangd sends "textDocument/publishDiagnostics" and Flymake highlights the
"y" to indicate an error.

Actual behaviour: After typing "y", no "textDocument/didChange" is sent
and the "y" is not highlighted. Further edits at the same buffer position
(even after moving point away and back again) do not recover the expected
highlighting, and point motion doesn't trigger LSP hover messages, until
one returns to a normal state by making changes elsewhere in the buffer.

If this happens when an edit was made with the intention of getting
feedback from the language server, it will be inconvenient.

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> * Start Eglot. (The "b" is highlighted to indicate an error.)
> * Enable the TeX input method.
> * Put point after the zero.
> * (Enter the bad state.) Quickly, type [<backspace> ^]. Pause.
> * Type [<backspace> y]. (No error highlight on "y".)
> * Move point to before the "x". (No "variable x" echo from Eldoc.)
> * Move point back to after the "y".
> * Type [<backspace> z] again. (Still no error highlight on "z".)
> * Type [SPC]. (Highlight on "b" is now rendered incorrectly.)
> * (Exit the bad state.) Type [C-n SPC <backspace>].

So you confirm that you don't see the "Missing/incorrect calls
..." warnings.  Thanks.  The fact that Eglot doesn't update until you
modify some "disjoint" part of the buffer was a known limitation of the
fix I installed.  I'll push a more complete fix RealSoonNow.


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> I'll push a more complete fix RealSoonNow.

Done.


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Richard Copley]

On Sat, 4 May 2024 at 22:16, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > I'll push a more complete fix RealSoonNow.
>
> Done.

Seems good, thanks.

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> Seems good, thanks.

Great, closing,


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[João Távora]

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

On Sun, May 5, 2024 at 2:40 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > Seems good, thanks.
>
> Great, closing,
>

Is this really the final word on this whole topic?   The current solution
involves a one
shot addition to `post-command-hook` in Eglot and is really ugly.  Can you
really
 really no better solution so that Eglot is only compelled to send changes
to the
server once track-changes.el announces it safe to do so (and gives the
change
to send while it's at it?

Also, tangentially , can we get rid of the fboundp's and make the next GNU
ELPA version run the same code as Emacs master's by depending on
track-changes.el GNU core?

Also also, can you fix indentation in the function that you recently
touched
in Eglot? (same goes for Philip, but I'll contact him separately).

João

[-- Attachment #2: Type: text/html, Size: 1317 bytes --]

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> Is this really the final word on this whole topic?   The current
> solution involves a one shot addition to `post-command-hook` in Eglot
> and is really ugly.  Can you really really no better solution so that
> Eglot is only compelled to send changes to the server once
> track-changes.el announces it safe to do so (and gives the change to
> send while it's at it?

Until we've gotten rid of all the code chunks that incorrectly use
`inhibit-modification-hooks` (which will take a lot of time since not
only we have to find them but we have to figure out why they used
`inhibit-modification-hooks` and then argue hard to get the change to be
accepted), I don't see a significantly simpler solution, no. 🙁

I mean, a simpler solution is to live with the performance bug, of course.

> Also, tangentially , can we get rid of the fboundp's and make the next GNU
> ELPA version run the same code as Emacs master's by depending on
> track-changes.el GNU core?

Yes, coming right up.

> Also also, can you fix indentation in the function that you recently
> touched in Eglot? (same goes for Philip, but I'll contact him
> separately).

Hmm... hadn't notice anything wrong.  Will take a closer look, thanks
for the heads up.


        Stefan

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[João Távora]

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

On Sun, May 5, 2024 at 3:58 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > Is this really the final word on this whole topic?   The current
> > solution involves a one shot addition to `post-command-hook` in Eglot
> > and is really ugly.  Can you really really no better solution so that
> > Eglot is only compelled to send changes to the server once
> > track-changes.el announces it safe to do so (and gives the change to
> > send while it's at it?
>
> Until we've gotten rid of all the code chunks that incorrectly use
> `inhibit-modification-hooks` (which will take a lot of time since not
> only we have to find them but we have to figure out why they used
> `inhibit-modification-hooks` and then argue hard to get the change to be
> accepted), I don't see a significantly simpler solution, no. 🙁
>
> I mean, a simpler solution is to live with the performance bug, of course.
>

I don't know that this is a "performance bug".  I mean, misleading the
server,
crashing it is only such a thing if one squints very hard.

So while may be "significantly simpler" solution isn't in the cards,
I still think some simplification can happen (I don't understand
why post-command-hook needs to be used for example).

Anyway,  I would need to  see the quil failure scenario encoded as a
test in eglot-tests.el so I can have a clear shot at it.  Should be
 possible, no?


>
> > Also, tangentially , can we get rid of the fboundp's and make the next
> GNU
> > ELPA version run the same code as Emacs master's by depending on
> > track-changes.el GNU core?
>
> Yes, coming right up.
>

Thanks. That's great. It would also help to document your new
eglot--track-changes-fetch.  I'm afraid I've lost track of how the
code is supposed to work after track-changes.el

* why is :emacs-messup still a thing?  By the way did :emacs-messup
solve this very problem too via a full resync?
* what exactly does the local eglot--track-changes do and why would
it be re-registered in the same buffer (why isn't the typical add-hook
enough).
* There seems to be a track-changes.el signalling function and the
longstanding LSP signalling function that informs the server of
things.  Why can't it be the same function, i.e. why can't Eglot
tell track-changes.el to call Eglot's function when track-changes.el
knows it's a safe time to call such a function?  The flow of
information via the eglot--recent-changes variable is complicated
and would seem not worth bringing in track-changes.el at all.
Is it only to accommodate Eglot versions when track-changes.el
isn't available or is there some deeper reason?


> > Also also, can you fix indentation in the function that you recently
> > touched in Eglot? (same goes for Philip, but I'll contact him
> > separately).
>
> Hmm... hadn't notice anything wrong.  Will take a closer look, thanks
> for the heads up.
>

Not serious, it's just I was misled by the misindentation when reading
one of the new if's, thinking it only had a then branch.

João

[-- Attachment #2: Type: text/html, Size: 4204 bytes --]

bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

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

> I don't know that this is a "performance bug".  I mean, misleading the
> server, crashing it is only such a thing if one squints very hard.

🙂

> So while may be "significantly simpler" solution isn't in the cards,
> I still think some simplification can happen (I don't understand
> why post-command-hook needs to be used for example).

Here's the reason I used `post-command-hook`:

As you know, the affected code is run from an idle timer, and the change
I introduced is to try and detect when the timer is run at an
"inconvenient time".  So my code delays the execution to "later".
In general calling `run-with-idle-timer` from an idle timer is a bit
tricky: if the wait time is smaller (or equal) than the current timer,
then the new timer will fire immediately.  Furthermore, in order for the
buffer state to change, we generally want to wait for the end of the
current idle time, so we want to fire a new timer that will run at the
*next* idle time.  I used `post-command-hook` to wait for the end of the
current idle time.

Arguably, we could/should move this trick to `timer.el` to provide
a `run-with-idle-timer-but-not-now` kind of function.
But AFAICT even using "insider info" we'd still have to use
a contraption like `post-command-hook`, or otherwise we'd have to make
more pervasive changes to the way timers are implemented.  🙁

> Anyway,  I would need to  see the quil failure scenario encoded as a
> test in eglot-tests.el so I can have a clear shot at it.  Should be
> possible, no?

Possible yes, but probably not very easy.  You can probably start with

    (insert "a")
    (with-silent-modifications
      (insert "^")
      (sit-for (* 2 eglot-send-changes-idle-time))
      (delete-region (1- (point)) (point)))

The first `insert` is there to make sure Eglot is told that there's been
some change, so it arms the idle-timer, and then the insert+wait+delete mimics
the Quail behavior.

> Thanks. That's great. It would also help to document your new
> eglot--track-changes-fetch.  I'm afraid I've lost track of how the
> code is supposed to work after track-changes.el
>
> * why is :emacs-messup still a thing?

Because `track-changes` can't hide the errors.  The only thing it can do
is do the detection for you and send you a "clean" error indication.  🙁

> By the way did :emacs-messup solve this very problem too via
> a full resync?

Yes/no.  With the old code, I think in the Quail situation your code
would *not* have detected the inconsistency (so no `:emacs-messup`) and
it would have occasionally sent inconsistent info the LSP server.

`track-changes` works a tiny bit harder to detect inconsistencies, so in
this Quail case it does detect it, which means it sends `:error` which
Eglot turns into `:emacs-messup` which means the LSP server receives
consistent info *but* it's costly and that info is equivalent to if the
user had manually typed an "^" followed by deleting that "^" and
replacing it with a `ᵃ` (for example), so the LSP server will sometimes
see the buffer with a `^` char which the user may never have meant to
include in the buffer.

And with the new code, Eglot postpones sending the state of the buffer
until we're not in the middle of a Quail insertion (so track-changes
never sends a `:error`, we don't use any `:emacs-messup`, and the LSP
server never sees that `^` either).
[ "never" here applies only to the Quail case, there can still be cases
  where we get `:error`, sadly.  ]

> * what exactly does the local eglot--track-changes do and why would
> it be re-registered in the same buffer (why isn't the typical add-hook
> enough).

Every client of `track-changes` can consume the changes at its own
rhythm, so track-changes needs to keep track of which changes each
client has already consumed.

> * There seems to be a track-changes.el signalling function and the
> longstanding LSP signalling function that informs the server of
> things.  Why can't it be the same function, i.e. why can't Eglot
> tell track-changes.el to call Eglot's function when track-changes.el
> knows it's a safe time to call such a function?  The flow of
> information via the eglot--recent-changes variable is complicated
> and would seem not worth bringing in track-changes.el at all.
> Is it only to accommodate Eglot versions when track-changes.el
> isn't available or is there some deeper reason?

We could turn `eglot--recent-changes` into a boolean, or probably even
eliminate it altogether, yes.  The reason I kept it is that if we don't
have it, then all the changes since the last
`eglot--signal-textDocument/didChange` get combined into a single
change, which can become quite large if the changes are spread out.


        Stefan