bug#44318: 28.0.50; Problem with ispell/flyspell and ""enchant"" backend

[Reuben Thomas via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

On Mon, 2 Nov 2020 at 16:10, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Reuben Thomas <rrt@sc3d.org>
> > Date: Mon, 2 Nov 2020 15:49:19 +0000
> > Cc: dinkonin <dinkonin@gmail.com>, 44318@debbugs.gnu.org
> >
> > So I, as upstream maintainer, am telling you that indeed, it is not
> correct that Emacs should take
> > enchant-lsmod's stderr as part of its output! (As I said above, it would
> be possible to capture stderr
> > separately and display it to the user, though I don't recommend that
> currently, because the warnings are not
> > written for users. Other programs that use Enchant do not show these
> warnings, they simply omit providers
> > that do not load from the list of languages/spelling checkers that are
> available.)
>
> OK, I agree to this change, under protest.
>

Thanks Eli, I have installed it; of course, contact me if it causes any
problems. After installing that patch, I discovered that in
ispell--call-enchant-lsmod, it did need to use with-current-buffer, so I
have installed a patch that restores the use of with-current-buffer;
apologies for getting that wrong first time.

In fact, this change is not sufficient to fix the original problem, because
the enchant program outputs the same warning when a provider module cannot
be loaded. In this case, stderr is added to the list of suggested words.
Again, I believe Enchant is correct to output a warning, and again, I
believe Emacs is wrong to amalgamate stderr and stdout.

I attach three patches that do the following:

1. Simplify ispell-call-process{,-region} by factoring out the macro
ispell-with-safe-default-directory.

2. I also found that ispell-check-version can be simplified slightly:
aspell has accepted -vv since 2004, so use it always.
3. When spell-checking, collect only standard output. This leaves some
spell-checker-specific calls to ispell-call-process to collect stderr as
well, which as far as I can tell is needed in only one case,
ispell-find-hunspell-dictionaries; but it doesn't hurt to leave the rest
unchanged. I have tested this patch with all supported spellcheckers
(ispell, aspell, hunspell, enchant).

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

[-- Attachment #2: 0002-Factor-out-some-common-code-in-ispell.el.patch --]
[-- Type: text/x-patch, Size: 1982 bytes --]

From 1aa6c9f5e7ce89f9440a2a5b4c591e65c8b4e5a0 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Mon, 2 Nov 2020 21:45:40 +0000
Subject: [PATCH 2/3] Factor out some common code in ispell.el

* lisp/textmodes/ispell.el (ispell-with-safe-default-directory): Add
  macro.
  (ispell-call-process, ispell-call-process-region): Use it.
---
 lisp/textmodes/ispell.el | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index 9e56923d60..49da3d1ed4 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -769,18 +769,23 @@ ispell-check-version
 	    (setq ispell-really-hunspell nil))))))
     result))
 
+(defmacro ispell-with-safe-default-directory (&rest body)
+  "Execute the forms in BODY with a reasonable
+`default-directory'."
+  (declare (indent 0) (debug t))
+  `(let ((default-directory default-directory))
+     (unless (file-accessible-directory-p default-directory)
+       (setq default-directory (expand-file-name "~/")))
+     ,@body))
+
 (defun ispell-call-process (&rest args)
-  "Like `call-process' but defend against bad `default-directory'."
-  (let ((default-directory default-directory))
-    (unless (file-accessible-directory-p default-directory)
-      (setq default-directory (expand-file-name "~/")))
+  "Like `call-process', but defend against bad `default-directory'."
+  (ispell-with-safe-default-directory
     (apply 'call-process args)))
 
 (defun ispell-call-process-region (&rest args)
-  "Like `call-process-region' but defend against bad `default-directory'."
-  (let ((default-directory default-directory))
-    (unless (file-accessible-directory-p default-directory)
-      (setq default-directory (expand-file-name "~/")))
+  "Like `call-process-region', but defend against bad `default-directory'."
+  (ispell-with-safe-default-directory
     (apply 'call-process-region args)))
 
 (defvar ispell-debug-buffer)
-- 
2.25.1


[-- Attachment #3: 0001-Simplify-ispell-check-version-s-use-of-vv-flag.patch --]
[-- Type: text/x-patch, Size: 1333 bytes --]

From c83ffcbe1ba950b9270bcde56d117c2c50e35112 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Mon, 2 Nov 2020 21:41:05 +0000
Subject: [PATCH 1/3] =?UTF-8?q?Simplify=20ispell-check-version=E2=80=99s?=
 =?UTF-8?q?=20use=20of=20-vv=20flag?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/textmodes/ispell.el (ispell-check-version): All supported spell
checker programs accept -vv, including aspell for many years, so use
it.
---
 lisp/textmodes/ispell.el | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index da3c3f4d6f..9e56923d60 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -684,13 +684,11 @@ ispell-check-version
     (with-temp-buffer
       (setq status (ispell-call-process
 		    ispell-program-name nil t nil
-		    ;; aspell doesn't accept the -vv switch.
 		    (let ((case-fold-search
 			   (memq system-type '(ms-dos windows-nt)))
 			  (speller
 			   (file-name-nondirectory ispell-program-name)))
-		      ;; Assume anything that isn't `aspell' is Ispell.
-		      (if (string-match "\\`aspell" speller) "-v" "-vv"))))
+		      "-vv")))
       (goto-char (point-min))
       (if interactivep
 	  ;; Report version information of ispell
-- 
2.25.1


[-- Attachment #4: 0003-Prevent-ispell-being-confused-by-spellchecker-output.patch --]
[-- Type: text/x-patch, Size: 1471 bytes --]

From 5d772177536dcfed607159802eeadcd22eab1969 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Mon, 2 Nov 2020 21:45:52 +0000
Subject: [PATCH 3/3] Prevent `ispell' being confused by spellchecker output on
 stderr

* lisp/textmodes/ispell.el (ispell-send-string, ispell-lookup-words):
Capture only stdout. This avoids warning messages, e.g. from Enchant,
from being interpreted as output from the spell-checker process.
---
 lisp/textmodes/ispell.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index 49da3d1ed4..8d90cab653 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -1861,7 +1861,7 @@ ispell-send-string
 		    (apply 'ispell-call-process-region
 			   (point-min) (point-max)
 			   ispell-program-name nil
-			   output-buf nil
+			   '(output-buf nil) nil
 			   "-a"
 			   ;; hunspell -m option means something different
 			   (if ispell-really-hunspell "" "-m")
@@ -2566,7 +2566,7 @@ ispell-lookup-words
         (while (search-backward "*" nil t) (insert "."))
         (setq word (buffer-string))
         (erase-buffer))
-      (setq status (apply 'ispell-call-process prog nil t nil
+      (setq status (apply 'ispell-call-process prog nil '(t nil) nil
                           (nconc (if (and args (> (length args) 0))
                                      (list args)
                                    (if look-p nil
-- 
2.25.1