bug#49245: Enchant dictionaries list not being correctly set, and other minor fixes

bug#49245: Enchant dictionaries list not being correctly set, and other minor fixes

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

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

I found a bug recently where, when using Enchant as the back-end for
ispell.el, spellchecking was locking up for some languages.

This turned out to be the combination of two problems.

First, ispell-find-enchant-dictionaries was incorrectly merging
ispell-dictionary-base-alist into its result. This caused the
ispell-set-spellchecker-params to fail to add the correct "-d LANG" flag
arguments to the list of dictionaries that it used to start Enchant, which
in turn meant that the process was started as e.g.

enchant-2 -d francais # rather than -d fr_FR

and failed to start properly. I have fixed this by simply removing the
incorrect code (patch 0003 attached).

Secondly, ispell.el failed to notice that it had not actually started an
Enchant process, and hung while trying to read from it in
ispell-accept-output. I fixed this by testing that the process is live
before trying to read from or write to it (patch 0002 attached).

Finally, while reading the source code I found an ancient comment that is
more of a commit message in spirit (it explains how the current code came
to be that way, rather than explaining something about how it works), so I
removed it (patch 0001 attached).

As usual with my infrequent patches, I would appreciate other eyes on them
before I install them, if possible. I remain an active user of ispell with
Enchant, though, so I will give them plenty of manual testing. Thanks in
advance!

-- 
https://rrt.sc3d.org

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

[-- Attachment #2: 0002-lisp-textmodes-ispell.el-Check-process-is-live-befor.patch --]
[-- Type: text/x-patch, Size: 1931 bytes --]

From e08065da5db9dc1c30b1b83adcc88d38b1ed671f Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Sun, 27 Jun 2021 22:07:06 +0100
Subject: [PATCH 2/3] * lisp/textmodes/ispell.el: Check process is live before
 interacting.

Check that `ispell-process' is live before trying to read from or
write to it. This avoids a hang if the process has died.
---
 lisp/textmodes/ispell.el | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index e3c1e61772..8b799b08c0 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -1765,10 +1765,12 @@ ispell-accept-output
 If asynchronous subprocesses are not supported, call function `ispell-filter'
 and pass it the output of the last Ispell invocation."
   (if ispell-async-processp
-      (let ((timeout (if timeout-msecs
-			 (+ (or timeout-secs 0) (/ timeout-msecs 1000.0))
-		       timeout-secs)))
-	(accept-process-output ispell-process timeout))
+      (if (process-live-p ispell-process)
+       (let ((timeout (if timeout-msecs
+			  (+ (or timeout-secs 0) (/ timeout-msecs 1000.0))
+		        timeout-secs)))
+	 (accept-process-output ispell-process timeout))
+       (error "No Ispell process to read output from!"))
     (if (null ispell-process)
 	(error "No Ispell process to read output from!")
       (let ((buf ispell-output-buffer)
@@ -1793,7 +1795,8 @@ ispell-send-replacement
 (defun ispell-send-string (string)
   "Send the string STRING to the Ispell process."
   (if ispell-async-processp
-      (process-send-string ispell-process string)
+      (if (process-live-p ispell-process)
+       (process-send-string ispell-process string))
     ;; Asynchronous subprocesses aren't supported on this losing system.
     ;; We keep all the directives passed to Ispell during the entire
     ;; session in a buffer, and pass them anew each time we invoke
-- 
2.25.1


[-- Attachment #3: 0003-lisp-textmodes-ispell.el-Fix-finding-dictionaries-fo.patch --]
[-- Type: text/x-patch, Size: 1928 bytes --]

From e3cd5361bea36ecd9c026bca93eb3e3809721e10 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Sun, 27 Jun 2021 22:08:40 +0100
Subject: [PATCH 3/3] * lisp/textmodes/ispell.el: Fix finding dictionaries for
 Enchant.

(ispell-find-enchant-dictionaries):

I originally copied this code from the equivalent code for
Aspell. Unfortunately it was wrong for the case of Enchant: it should
find only dictionaries that Enchant knows about, and not merge in
`ispell-dictionary-base-alist' or add a default element, as these
are dealt with in `ispell-set-spellchecker-params'.

This caused a bug where the correct `-d' argument would not be added
to the invocation of enchant, leading to the process not being
correctly started.
---
 lisp/textmodes/ispell.el | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index 8b799b08c0..5570800776 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -1211,18 +1211,7 @@ ispell-find-enchant-dictionaries
                     `(,lang "[[:alpha:]]" "[^[:alpha:]]"
                             ,(ispell--get-extra-word-characters lang) t nil nil utf-8))
                   dictionaries)))
-    ;; Merge into FOUND any elements from the standard ispell-dictionary-base-alist
-    ;; which have no element in FOUND at all.
-    (dolist (dict ispell-dictionary-base-alist)
-      (unless (assoc (car dict) found)
-	(setq found (nconc found (list dict)))))
-    (setq ispell-enchant-dictionary-alist found)
-    ;; Add a default entry
-    (let ((default-dict
-            `(nil "[[:alpha:]]" "[^[:alpha:]]"
-                  ,(ispell--get-extra-word-characters)
-                  t nil nil utf-8)))
-      (push default-dict ispell-enchant-dictionary-alist))))
+    (setq ispell-enchant-dictionary-alist found)))
 
 ;; Set params according to the selected spellchecker
 
-- 
2.25.1


[-- Attachment #4: 0001-lisp-textmodes-ispell.el-ispell-word-Remove-a-redund.patch --]
[-- Type: text/x-patch, Size: 918 bytes --]

From e12c8a51ccee5cd2cc89ae503d2ce5a085b05566 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Sun, 27 Jun 2021 22:04:17 +0100
Subject: [PATCH 1/3] * lisp/textmodes/ispell.el (ispell-word): Remove a
 redundant comment

Remove a comment of purely historical interest. (It reads like a
commit message, not a comment.)
---
 lisp/textmodes/ispell.el | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index 4dbc7640bc..e3c1e61772 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -1911,8 +1911,6 @@ ispell-word
 	    end (car (cdr (cdr word)))
 	    word (car word))
 
-      ;; At this point it used to ignore 2-letter words.
-      ;; But that is silly; if the user asks for it, we should do it. - rms.
       (or quietly
 	  (message "Checking spelling of %s..."
 		   (funcall ispell-format-word-function word)))
-- 
2.25.1

bug#49245: Enchant dictionaries list not being correctly set, and other minor fixes

[Eli Zaretskii]

> Date: Sun, 27 Jun 2021 22:20:10 +0100
> From:  Reuben Thomas via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I found a bug recently where, when using Enchant as the back-end for ispell.el, spellchecking was locking up
> for some languages.
> 
> This turned out to be the combination of two problems.
> 
> First, ispell-find-enchant-dictionaries was incorrectly merging ispell-dictionary-base-alist into its result. This
> caused the ispell-set-spellchecker-params to fail to add the correct "-d LANG" flag arguments to the list of
> dictionaries that it used to start Enchant, which in turn meant that the process was started as e.g.
> 
> enchant-2 -d francais # rather than -d fr_FR
> 
> and failed to start properly. I have fixed this by simply removing the incorrect code (patch 0003 attached).
> 
> Secondly, ispell.el failed to notice that it had not actually started an Enchant process, and hung while trying to
> read from it in ispell-accept-output. I fixed this by testing that the process is live before trying to read from or
> write to it (patch 0002 attached).
> 
> Finally, while reading the source code I found an ancient comment that is more of a commit message in
> spirit (it explains how the current code came to be that way, rather than explaining something about how it
> works), so I removed it (patch 0001 attached).
> 
> As usual with my infrequent patches, I would appreciate other eyes on them before I install them, if possible.

OK for the first 2 patches, but please don't remove that comment, it
explains something important, and there's no reason to remove it (or
many other similar comments we have throughout our code).

Thanks.

bug#49245: Enchant dictionaries list not being correctly set, and other minor fixes

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

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

On Mon, 28 Jun 2021 at 13:13, Eli Zaretskii <eliz@gnu.org> wrote:

> OK for the first 2 patches,


Thanks for the review.


> but please don't remove that comment, it
> explains something important, and there's no reason to remove it (or
> many other similar comments we have throughout our code).
>

In that case, please can you explain it, and I can rewrite it so that its
significance is more evident. As I said, it explains why something was
changed in the past (which is useful information in a commit message)
rather than how or why the current code does something that may not be
obvious just from reading the code (which would be suitable for a comment).
Commit f0a1f8bdb5, which introduces it, has the message "Do not ignore
short words". The current code does not have to *do* anything to check
short words; that commit simply removed a check. I do not see anything in
the current code that raises any questions that need answering by a
comment. On the contrary, the comment raises a question: "is there some
setting for minimum word length that I need to be aware of?". So I feel
I've missed something here that a rewording of the comment could fix.

-- 
https://rrt.sc3d.org

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

bug#49245: Enchant dictionaries list not being correctly set, and other minor fixes

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Mon, 28 Jun 2021 13:31:01 +0100
> Cc: 49245@debbugs.gnu.org
> 
>  but please don't remove that comment, it
>  explains something important, and there's no reason to remove it (or
>  many other similar comments we have throughout our code).
> 
> In that case, please can you explain it, and I can rewrite it so that its significance is more evident. As I said, it
> explains why something was changed in the past (which is useful information in a commit message) rather
> than how or why the current code does something that may not be obvious just from reading the code
> (which would be suitable for a comment). Commit f0a1f8bdb5, which introduces it, has the message "Do
> not ignore short words". The current code does not have to *do* anything to check short words; that commit
> simply removed a check. I do not see anything in the current code that raises any questions that need
> answering by a comment. On the contrary, the comment raises a question: "is there some setting for
> minimum word length that I need to be aware of?". So I feel I've missed something here that a rewording of
> the comment could fix.

It is customary to leave a comment when we delete some code, but are
not 100% sure that code was a clear mistake.  Since deleting code
leaves nothing behind (unlike if you add or change code), the comment
serves as a kind of "trace" for what once was there, but is no more.

If you want a practical case where this could be useful, imagine a bug
report regarding special treatment of words shorter than 3 letters
(which are normally ignored).

bug#49245: Enchant dictionaries list not being correctly set, and other minor fixes

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

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

On Mon, 28 Jun 2021 at 15:37, Eli Zaretskii <eliz@gnu.org> wrote:

> It is customary to leave a comment when we delete some code, but are
> not 100% sure that code was a clear mistake.  Since deleting code
> leaves nothing behind (unlike if you add or change code), the comment
> serves as a kind of "trace" for what once was there, but is no more.
>

Thanks for the explanation, sounds like a culture of commenting previously
unfamiliar to me.

I will close this bug now.

-- 
https://rrt.sc3d.org

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

bug#49245: Closing

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

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

Fixed by 7c93009d11 and 881e75873d.

-- 
https://rrt.sc3d.org

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