bug#50370: Fix bug in ispell-init-process error handling

bug#50370: Fix bug in ispell-init-process error handling

[Ian W]

[-- Attachment #1.1: Type: text/plain, Size: 2019 bytes --]

This fixes the error handling in ispell-init-process. I encountered this bug, and also saw it mentioned here: https://mail.gnu.org/archive/html/auctex/2021-08/msg00007.html

The previous behavior involved the ispell process terminating
prematurely (correct behavior, invalid input) and then calling
ispell-accept-output. Because ispell-process had terminated,
ispell-accept-output threw its own error, which prevented the underlying
error from being displayed.

The bug resulted in the following interaction:
ispell-word would print out:
"Starting new Ispell process aspell with default dictionary...done"
"ispell-accept-output: No Ispell process to read output from!"

The correct behavior is:
"Starting new Ispell process aspell with default dictionary...done"
"cond: Error: ~/.emacs.d/.local/etc/ispell/.pws: The language "nil" is not known. This is probably because: the file "/usr/local/Cellar/aspell/0.60.8/lib/aspell-0.60/nil.dat" can not be opened for reading.""


In GNU Emacs 28.0.50 (build 1, x86_64-apple-darwin20.5.0, NS appkit-2022.50 Version 11.4 (Build 20F71))
 of 2021-08-15 built on Ians-MacBook-Pro-2.local
Windowing system distributor 'Apple', version 10.3.2022
System Description: macOS 11.5.2

Configured using:
 'configure --disable-dependency-tracking --disable-silent-rules
 --enable-locallisppath=/usr/local/share/emacs/site-lisp
 --infodir=/usr/local/Cellar/emacs-plus@28/28.0.50/share/info/emacs
 --prefix=/usr/local/Cellar/emacs-plus@28/28.0.50 --with-xml2
 --with-gnutls --with-native-compilation --without-dbus
 --with-imagemagick --with-modules --with-rsvg --with-xwidgets --with-ns
 --disable-ns-self-contained 'CFLAGS=-I/usr/local/opt/gcc/include
 -I/usr/local/opt/libgccjit/include -I/usr/local/opt/gmp/include
 -I/usr/local/opt/jpeg/include' 'LDFLAGS=-L/usr/local/lib/gcc/11
 -I/usr/local/opt/gcc/include -I/usr/local/opt/libgccjit/include
 -I/usr/local/opt/gmp/include -I/usr/local/opt/jpeg/include''

The patch is attached.

Ian

[-- Attachment #1.2: Type: text/html, Size: 2633 bytes --]

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 977 bytes --]

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index c2f6b35df8..ce0653c434 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -2923,7 +2923,13 @@ Keeps argument list for future Ispell invocations for no async support."
 	     ;; But first wait to see if some more output is going to arrive.
 	     ;; Otherwise we get cool errors like "Can't open ".
 	     (sleep-for 1)
-	     (ispell-accept-output 3)
+             ;; To avoid `ispell-accept-output' throwing it's own
+             ;; error, we check that it will receive from a valid
+             ;; process.
+	     (when (if ispell-async-processp
+                         (process-live-p ispell-process)
+                       ispell-process)
+               (ispell-accept-output 3))
 	     (error "%s" (mapconcat #'identity ispell-filter "\n"))))
       (setq ispell-filter nil)		; Discard version ID line
       (let ((extended-char-mode (ispell-get-extended-character-mode)))

bug#50370: Fix bug in ispell-init-process error handling

[Eli Zaretskii]

> Date: Fri, 3 Sep 2021 20:53:24 -0700
> From: Ian W <ian@wahbe.com>
> 
> This fixes the error handling in ispell-init-process. I encountered this bug, and also saw it mentioned here:
> https://mail.gnu.org/archive/html/auctex/2021-08/msg00007.html
> 
> The previous behavior involved the ispell process terminating
> prematurely (correct behavior, invalid input) and then calling
> ispell-accept-output. Because ispell-process had terminated,
> ispell-accept-output threw its own error, which prevented the underlying
> error from being displayed.
> 
> The bug resulted in the following interaction:
> ispell-word would print out:
> "Starting new Ispell process aspell with default dictionary...done"
> "ispell-accept-output: No Ispell process to read output from!"
> 
> The correct behavior is:
> "Starting new Ispell process aspell with default dictionary...done"
> "cond: Error: ~/.emacs.d/.local/etc/ispell/.pws: The language "nil" is not known. This is probably because: the
> file "/usr/local/Cellar/aspell/0.60.8/lib/aspell-0.60/nil.dat" can not be opened for reading.""

I'm not sure I agree that the "correct behavior" better than the
"incorrect".  The error message is not more self-explanatory, at
least.

Anyway, do you have a recipe for reproducing this problem?  The URL of
the AUCTeX discussion seems to say it happens every time?  I don't
think I understand what's going wrong to trigger the problem.

Thanks.

bug#50370: Fix bug in ispell-init-process error handling

[Eli Zaretskii]

[Please use Reply All to keep the bug address on the CC list.]

> Date: Sat, 4 Sep 2021 11:33:42 -0700
> From: Ian W <ian@wahbe.com>
> 
> I think it’s a better error message, but that is just my opinion. It tells me that ispell is trying to read an invalid
> dictionary file. In my case, I deleted that file and everything started working. I also think it’s the error message
> intended to be displayed (by the ispell program and ispell package), as it is the error message designed to
> help the user solve their problem.
> 
> (t
>  ;; Otherwise, it must be an error message. Show the user.
>  ;; But first wait to see if some more output is going to arrive.
>  ;; Otherwise we get cool errors like "Can't open ".
>  (sleep-for 1)
>  (ispell-accept-output 3) ;; <<< This is the line that errors >>>
>  (error "%s" (mapconcat #'identity ispell-filter "\n"))))
> 
> So the intended error message is never displayed. 
> 
> I’m sorry for not including a repo. I got it consistently because I had a bad file cached by aspell (or maybe
> doom). You also get the same message when you didn’t install your dictionaries correctly. This is a problem
> in how ispell displays errors, so it’s consistent but only if you already had a real problem with ispell.
> 
> To repo:
> (setq ispell-local-dictionary-overridden t
>           ispell-local-dictionary "not-a-dict")
> 
> This misconfigures your local dictionary, and ensures that the ispell process will error on start. Then run
> ispell-word on any word. Thanks for the prompt response. 

With the above recipe, I get:

  ispell-phaf: No matching entry for not-a-dict in ‘ispell-hunspell-dict-paths-alist’.

(My speller is Hunspell.)

So does this mean this problem doesn't happen with Hunspell?  Ir is
the recipe incomplete or something?

bug#50370: Fix bug in ispell-init-process error handling

[Ian W]

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

I think that hunspell is different. It validates that the dictionary really exists in the beginning of ispell-start-process:

(defun ispell-start-process ()
 "Start the Ispell process, with support for no asynchronous processes.
Keeps argument list for future Ispell invocations for no async support."
 ;; `ispell-current-dictionary' and `ispell-current-personal-dictionary'
 ;; are properly set in `ispell-internal-change-dictionary'.

 ;; Parse hunspell affix file if using hunspell and entry is uninitialized.
 (if ispell-really-hunspell
 (or (cadr (assoc ispell-current-dictionary ispell-dictionary-alist))
 (ispell-hunspell-fill-dictionary-entry ispell-current-dictionary)))
…)

Having seen this, I think that this will only be a problem for ispell and aspell. I just tested the recipe and it works on “emacs -Q”  (I have ispell installed correctly, and emacs defaults to that).
On Sep 4, 2021, 11:40 AM -0700, Eli Zaretskii <eliz@gnu.org>, wrote:
> [Please use Reply All to keep the bug address on the CC list.]
>
> > Date: Sat, 4 Sep 2021 11:33:42 -0700
> > From: Ian W <ian@wahbe.com>
> >
> > I think it’s a better error message, but that is just my opinion. It tells me that ispell is trying to read an invalid
> > dictionary file. In my case, I deleted that file and everything started working. I also think it’s the error message
> > intended to be displayed (by the ispell program and ispell package), as it is the error message designed to
> > help the user solve their problem.
> >
> > (t
> > ;; Otherwise, it must be an error message. Show the user.
> > ;; But first wait to see if some more output is going to arrive.
> > ;; Otherwise we get cool errors like "Can't open ".
> > (sleep-for 1)
> > (ispell-accept-output 3) ;; <<< This is the line that errors >>>
> > (error "%s" (mapconcat #'identity ispell-filter "\n"))))
> >
> > So the intended error message is never displayed.
> >
> > I’m sorry for not including a repo. I got it consistently because I had a bad file cached by aspell (or maybe
> > doom). You also get the same message when you didn’t install your dictionaries correctly. This is a problem
> > in how ispell displays errors, so it’s consistent but only if you already had a real problem with ispell.
> >
> > To repo:
> > (setq ispell-local-dictionary-overridden t
> > ispell-local-dictionary "not-a-dict")
> >
> > This misconfigures your local dictionary, and ensures that the ispell process will error on start. Then run
> > ispell-word on any word. Thanks for the prompt response.
>
> With the above recipe, I get:
>
> ispell-phaf: No matching entry for not-a-dict in ‘ispell-hunspell-dict-paths-alist’.
>
> (My speller is Hunspell.)
>
> So does this mean this problem doesn't happen with Hunspell? Ir is
> the recipe incomplete or something?

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

bug#50370: Fix bug in ispell-init-process error handling

[Eli Zaretskii]

> Date: Sat, 4 Sep 2021 12:38:40 -0700
> From: Ian W <ian@wahbe.com>
> Cc: 50370@debbugs.gnu.org
> 
> I think that hunspell is different. It validates that the dictionary really exists in the beginning of
> ispell-start-process:
> 
> (defun ispell-start-process ()
>  "Start the Ispell process, with support for no asynchronous processes.
> Keeps argument list for future Ispell invocations for no async support."
>  ;; `ispell-current-dictionary' and `ispell-current-personal-dictionary'
>  ;; are properly set in `ispell-internal-change-dictionary'.
> 
>  ;; Parse hunspell affix file if using hunspell and entry is uninitialized.
>  (if ispell-really-hunspell
>  (or (cadr (assoc ispell-current-dictionary ispell-dictionary-alist))
>  (ispell-hunspell-fill-dictionary-entry ispell-current-dictionary)))
> …)
> 
> Having seen this, I think that this will only be a problem for ispell and aspell. I just tested the recipe and it
> works on “emacs -Q”  (I have ispell installed correctly, and emacs defaults to that). 

Thanks, I installed the change, and I'm therefore closing this bug.