bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers

bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers

[Daniel Pettersson]

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

Tags: patch


As I understand the manual there is nothing that states that one should
assume that you cant move the point in buffer with timers.  This is of
course impossible to guarantee as any code can be calling
accept-process-output.  But it seams like an good idea to have code that
is triggered by post-command-hook to not impose such conditions. 

This might seam like an mole whacking activity, and it most definitely
is.

In GNU Emacs 30.0.50 (build 1, aarch64-apple-darwin23.1.0, NS
 appkit-2487.20 Version 14.1.1 (Build 23B81)) of 2023-12-20 built on
 Daniels-Air
Repository revision: 281be72422f42fcc84d43f50723a3e91b7d03cbc
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2487
System Description:  macOS 14.1.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Flyspell-flyspell-word-do-not-force-save-excursion-o.patch --]
[-- Type: text/patch, Size: 1330 bytes --]

From 56bdf4df8048b4053719e33d08753a2eb088dc57 Mon Sep 17 00:00:00 2001
From: Daniel Pettersson <daniel@dpettersson.net>
Date: Mon, 11 Mar 2024 15:01:32 +0100
Subject: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on
 timers

* lisp/textmode/flyspell.el (flyspell-word): Do not force
save-excursion on timers.  This especially needed here as
flyspell-word is fired from an post command hook.
---
 lisp/textmodes/flyspell.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index de59294e9f0..5dfe0f17aef 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -1150,7 +1150,9 @@ flyspell-word
 		  (set-process-query-on-exit-flag ispell-process nil)
                   ;; Wait until ispell has processed word.
                   (while (progn
-                           (accept-process-output ispell-process)
+                           ;; don't force save-excursion to timers.
+                           ;; only accept output from ispell-process.
+                           (accept-process-output ispell-process nil nil t)
                            (not (string= "" (car ispell-filter)))))
                   ;; (ispell-send-string "!\n")
                   ;; back to terse mode.
-- 
2.39.3 (Apple Git-145)

bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers

[Eli Zaretskii]

> From: Daniel Pettersson <daniel@dpettersson.net>
> Date: Mon, 11 Mar 2024 15:37:55 +0100
> 
> As I understand the manual there is nothing that states that one should
> assume that you cant move the point in buffer with timers.  This is of
> course impossible to guarantee as any code can be calling
> accept-process-output.  But it seams like an good idea to have code that
> is triggered by post-command-hook to not impose such conditions. 

Thanks, but I don't think I understand the relation between what you
say above (and in the comments in your patch below) and the code
change you propose:

>                    (while (progn
> -                           (accept-process-output ispell-process)
> +                           ;; don't force save-excursion to timers.
> +                           ;; only accept output from ispell-process.
> +                           (accept-process-output ispell-process nil nil t)

Where's save-excursion and point moving to which you allude here?

More importantly, why is it a good idea to stop running timers during
this accept-process-output call and ignore output from other
subprocesses?

bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers

[Daniel Pettersson]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but I don't think I understand the relation between what you
> say above (and in the comments in your patch below) and the code
> change you propose:

flyspell-word wraps it's body inside an `save-excursion'.

(defun flyspell-word (&optional following known-misspelling)
...
  (save-excursion
...
>>                    (while (progn
>> -                           (accept-process-output ispell-process)
>> +                           ;; don't force save-excursion to timers.
>> +                           ;; only accept output from ispell-process.
>> +                           (accept-process-output ispell-process nil nil t)

> Where's save-excursion and point moving to which you allude here?

See above and for point moving I think that the stack trace at the
bottom of the email highlights my point.

> More importantly, why is it a good idea to stop running timers during
> this accept-process-output call and ignore output from other
> subprocesses?

I don't think that is the optimal solution.  But this does not ignore
output right, it just delays timers and filter functions.  If flyspell
is active in an comit buffer it might also screw up the point if process
outputs stuff during `accept-process-output'.

The best solution would be to run `accept-process-output' outside of the
body of save-excursion, but that would require a bit more mangling,
which might be worth the effort.

Let's create an simple example to illustrate the issue with current
implementation.  This is a highly construed example but there is always
the chance that flyspell has unexpected interactions with all buffers
that filter functions and timer functions modify the point of.

(defun timer-goto-point-min ()
  (goto-char (point-min)))

(defun some-command ()
  (interactive)
  ;; flyspell needs point to have been and at word.
  (goto-char (point-min))
  (re-search-forward "word.")
  ;; Goto top of buffer with timer
  (run-with-timer 0 0 'timer-goto-point-min))

(trace-function #'timer-goto-point-min)
(trace-function 'flyspell-word)
(trace-function 'accept-process-output)

;; Prepare buffer
(with-current-buffer (get-buffer-create "buffer")
  (save-excursion
    (insert "Some sentence with words."))
  (flyspell-mode))

;; Open buffer M-x some-command expects point to be at the start of
;; buffer 

;; 1 -> (flyspell-word)                    -- start of save-excursion
;; | 2 -> (accept-process-output #<process ispell>)
;; | | 3 -> (timer-goto-point-min)         -- moves point
;; | | 3 <- timer-goto-point-min: 1
;; | 2 <- accept-process-output: t
;; 1 <- flyspell-word: nil                 -- end of save-excursion
;;                                            resets the point


/Daniel

bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers

[Eli Zaretskii]

> From: Daniel Pettersson <daniel@dpettersson.net>
> Cc: 69733@debbugs.gnu.org
> Date: Tue, 12 Mar 2024 15:28:22 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but I don't think I understand the relation between what you
> > say above (and in the comments in your patch below) and the code
> > change you propose:
> 
> flyspell-word wraps it's body inside an `save-excursion'.
> 
> (defun flyspell-word (&optional following known-misspelling)
> ...
>   (save-excursion
> ...
> >>                    (while (progn
> >> -                           (accept-process-output ispell-process)
> >> +                           ;; don't force save-excursion to timers.
> >> +                           ;; only accept output from ispell-process.
> >> +                           (accept-process-output ispell-process nil nil t)
> 
> > Where's save-excursion and point moving to which you allude here?
> 
> See above and for point moving I think that the stack trace at the
> bottom of the email highlights my point.
> 
> > More importantly, why is it a good idea to stop running timers during
> > this accept-process-output call and ignore output from other
> > subprocesses?
> 
> I don't think that is the optimal solution.  But this does not ignore
> output right, it just delays timers and filter functions.  If flyspell
> is active in an comit buffer it might also screw up the point if process
> outputs stuff during `accept-process-output'.
> 
> The best solution would be to run `accept-process-output' outside of the
> body of save-excursion, but that would require a bit more mangling,
> which might be worth the effort.
> 
> Let's create an simple example to illustrate the issue with current
> implementation.  This is a highly construed example but there is always
> the chance that flyspell has unexpected interactions with all buffers
> that filter functions and timer functions modify the point of.
> 
> (defun timer-goto-point-min ()
>   (goto-char (point-min)))
> 
> (defun some-command ()
>   (interactive)
>   ;; flyspell needs point to have been and at word.
>   (goto-char (point-min))
>   (re-search-forward "word.")
>   ;; Goto top of buffer with timer
>   (run-with-timer 0 0 'timer-goto-point-min))
> 
> (trace-function #'timer-goto-point-min)
> (trace-function 'flyspell-word)
> (trace-function 'accept-process-output)
> 
> ;; Prepare buffer
> (with-current-buffer (get-buffer-create "buffer")
>   (save-excursion
>     (insert "Some sentence with words."))
>   (flyspell-mode))
> 
> ;; Open buffer M-x some-command expects point to be at the start of
> ;; buffer 
> 
> ;; 1 -> (flyspell-word)                    -- start of save-excursion
> ;; | 2 -> (accept-process-output #<process ispell>)
> ;; | | 3 -> (timer-goto-point-min)         -- moves point
> ;; | | 3 <- timer-goto-point-min: 1
> ;; | 2 <- accept-process-output: t
> ;; 1 <- flyspell-word: nil                 -- end of save-excursion
> ;;                                            resets the point

Thanks, but I'm still confused regarding what you are trying to fix
and why you are trying to fix it with the patch you proposed.

First, AFAIU, save-excursion is there because flyspell-get-word might
move point.  So this is justified.

Next, you seem to be saying that it is for some reason bad to run
timers under save-excursion, but you haven't explained why.  IMO, if a
timer that runs during post-command-hook moves point, that can have
negative effect on UX.  Also, code that runs from a timer cannot
possibly rely on Emacs leaving point where the timer moves it.

Are there any examples of timers that run like this which _must_ be
able to move point and make sure point stays where the timer moved it?

bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers

[Daniel Pettersson]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but I'm still confused regarding what you are trying to fix
> and why you are trying to fix it with the patch you proposed.

Lets just preface with that for accept-process-output non idle timers
and process filters are the same thing, right?  So any filter or timer
might run inside of accept-process-output if JUST-THIS-ONE is nil.

The issue was noticed with the elpa package dape, with
flyspell-prog-mode.  But I was able to reproduce it in gdb-mi.el
(gud-next) as well.  The common denominator here is moving the point
from filter/timer functions, in both cases source buffers. 

It's definitely a bit more rare for this to happen for gdb-mi.el as it
moves it's point to the beginning of the line where it's highly unlikely
for there to be a word which flyspell-prog-mode sends of to
ispell-process to spell (and waits with accept-process-output),
additionally it depends on the speed of gdb and the ispell program. If
ispell stdouts before gdb, flyspell-word finishes before gud-filter is
evaled. 

> First, AFAIU, save-excursion is there because flyspell-get-word might
> move point.  So this is justified.

No doubt that the save-excursion is justified but it surly does not need
to wrap everything, one could be a bit more exact (wrap those parts)
that actually move the point.

> Next, you seem to be saying that it is for some reason bad to run
> timers under save-excursion, but you haven't explained why.

This has the chance to break both gdb-mi.el, dape and all other packages
that moves a point from filter and timer functions. I say chance here
because it all depends the timing of the process output and If the
buffer that the point is moved in is checked by flyspell think I gave my
explanation to what could break above. 

> Also, code that runs from a timer cannot
> possibly rely on Emacs leaving point where the timer moves it.

I have a feeling that there are a lot of other code, except dape and gud
that do.  As this is not only timers but any code that is called from an
process filter.

> Are there any examples of timers that run like this which _must_ be
> able to move point and make sure point stays where the timer moved it?

See above.

After some thinking I it might be impossible to impose anything on the
caller of accept-process-output.  And the bug is in dape and gdb-mi.el
that gud should call gud-display-line inside of an idle timer to ensure
that the point is moved, if I understand how idle timers are called
which might be false.

Maybe it would be a good idea if "(elisp) Timers" would mention these
things.  Would that be an good idea?  I could be up for writing something
up,  even if don't consider myself to be that good at writing
documentation.

/Daniel

bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers

[Eli Zaretskii]

> From: Daniel Pettersson <daniel@dpettersson.net>
> Cc: 69733@debbugs.gnu.org
> Date: Fri, 15 Mar 2024 12:29:46 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but I'm still confused regarding what you are trying to fix
> > and why you are trying to fix it with the patch you proposed.
> 
> Lets just preface with that for accept-process-output non idle timers
> and process filters are the same thing, right?  So any filter or timer
> might run inside of accept-process-output if JUST-THIS-ONE is nil.
> 
> The issue was noticed with the elpa package dape, with
> flyspell-prog-mode.  But I was able to reproduce it in gdb-mi.el
> (gud-next) as well.  The common denominator here is moving the point
> from filter/timer functions, in both cases source buffers. 

I think (and you agree below) that these are bugs in these packages.
Non-idle timer cannot safely move point, and neither can a process
filter.  It basically means we move point while the user might be
typing something, which would be a terrible misfeature!

> > First, AFAIU, save-excursion is there because flyspell-get-word might
> > move point.  So this is justified.
> 
> No doubt that the save-excursion is justified but it surly does not need
> to wrap everything, one could be a bit more exact (wrap those parts)
> that actually move the point.

That's correct, so if you want to move save-excursion closer to
flyspell-get-word, it would be fine.

> After some thinking I it might be impossible to impose anything on the
> caller of accept-process-output.  And the bug is in dape and gdb-mi.el
> that gud should call gud-display-line inside of an idle timer to ensure
> that the point is moved, if I understand how idle timers are called
> which might be false.

Exactly.  Idle timers can move point, provided that they wait a
reasonable amount of idleness time, to avoid interpreting a 0.01 sec
gap in user typing as "idle".

> Maybe it would be a good idea if "(elisp) Timers" would mention these
> things.  Would that be an good idea?

Yes, I think so.  We already advise not to do certain things in a
timer function, so this could be an addition to those parts.

> I could be up for writing something up, even if don't consider
> myself to be that good at writing documentation.

Thanks, please do.

bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers

[Dmitry Gutov]

On 11/03/2024 16:37, Daniel Pettersson wrote:
> As I understand the manual there is nothing that states that one should
> assume that you cant move the point in buffer with timers.  This is of
> course impossible to guarantee as any code can be calling
> accept-process-output.  But it seams like an good idea to have code that
> is triggered by post-command-hook to not impose such conditions.
> 
> This might seam like an mole whacking activity, and it most definitely
> is.

Speaking of mole whacking, a long-time potential FIXME here has been to 
rewrite flyspell in terms of timers, rather than sit-for.

Dropping unnecessary save-excusion-s would either happen automatically, 
or would be in the same general area anyway.