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?