bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Ole Jørgen Brønner]

I'm using aspell as the "ispell provider".

    M-x ispell-change-dictionary

helpfully gives a list of dictionaries to choose from. The list however contains
dictionaries I don't have installed on my system. Only after spellchecking is
attempted I get an error in *Messages*.

Diving into the code I find quite an effort to build a list of
dictionaries for the three different ispell providers.

There's a dedicated function, `ispell-find-aspell-dictionaries`, to find valid
aspell dictionaries. It sanely ask bin/aspell for the list and then does some
post-processing to find the rest of the spec.

But after the *correct* list has been found this code[1] is run:

    ;; 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)))))

Adding invalid entries to the list. There might be a rationale behind that, but
to me is seems like `ispell-dictionary-base-alist` mostly exist due to lack of
*ispell* (old aspell?) introspection? (When using *ispell* as the
provider, invalid entries
 are filtered out in `ispell-valid-dictionary-list`)

Removing that code isn't enough though:

In `ispell-set-spellchecker-params` the invalid entries are added again:
(where `found-dicts-alist` is the result of `ispell-find-aspell-dictionaries`
in when aspell is used)

    (run-hooks 'ispell-initialize-spellchecker-hook)

    ;; Add dicts to ``ispell-dictionary-alist'' unless already present.
    (dolist (dict (append found-dicts-alist
                          ispell-base-dicts-override-alist
                          ispell-dictionary-base-alist))
      (unless (assoc (car dict) all-dicts-alist)
        (add-to-list 'all-dicts-alist dict)))
    (setq ispell-dictionary-alist all-dicts-alist))

This code seems to have two purposes:
- When *ispell* is the provider, `found-dicts-alist` is nil at this
point, so the
  default entries must be added
- Apply dictionary overrides (possibly set by the hook above)

I would suggest simply removing [1] and not add the *base-alist above when using
aspell/hunspell, but the following comment makes it seem like the "invalid"
entries are added intentionally:

    ;; Substitute ispell-dictionary-alist with the list of
    ;; dictionaries corresponding to the given spellchecker.
    ;; If a recent aspell or hunspell, use the list of really
    ;; installed dictionaries and add to it elements of the original
    ;; list that are not present there*. Allow distro info.


In GNU Emacs 24.5.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.20.6)
 of 2016-06-04 on juergen
Windowing system distributor `The X.Org Foundation', version 11.0.11803000
Configured using:
 `configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe
 -fstack-protector-strong' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro'

Important settings:
  value of $LC_COLLATE: en_US.UTF-8
  value of $LC_CTYPE: en_US.UTF-8
  value of $LC_MESSAGES: en_US.UTF-8
  value of $LC_MONETARY: en_US.UTF-8
  value of $LC_NUMERIC: en_US.UTF-8
  value of $LC_TIME: nb_NO.utf8
  value of $LANG: en_US.utf8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
Making completion list... [2 times]
Local Ispell dictionary set to german
Entering debugger...
Back to top level.
<C-tab> is undefined
Making completion list... [2 times]
Starting new Ispell process /usr/bin/aspell with german dictionary...
ispell-init-process: Error: The file "/usr/lib/aspell-0.60/german" can
not be opened for reading.
ESC M-x is undefined [3 times]
Making completion list... [2 times]

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev
gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util
help-fns mail-prsvr mail-utils debug ispell help-mode easymenu time-date
tooltip electric uniquify ediff-hook vc-hooks lisp-float-type mwheel
x-win x-dnd tool-bar dnd fontset image regexp-opt fringe tabulated-list
newcomment lisp-mode prog-mode register page menu-bar rfn-eshadow timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
frame cham georgian utf-8-lang misc-lang vietnamese tibetan thai
tai-viet lao korean japanese hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese case-table epa-hook jka-cmpr-hook help
simple abbrev minibuffer nadvice loaddefs button faces cus-face macroexp
files text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote make-network-process
dbusbind gfilenotify dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty emacs)

Memory information:
((conses 16 76572 7275)
 (symbols 48 18057 0)
 (miscs 40 46 163)
 (strings 32 10715 4971)
 (string-bytes 1 296127)
 (vectors 16 9261)
 (vector-slots 8 387535 14421)
 (floats 8 65 422)
 (intervals 56 220 26)
 (buffers 960 13)
 (heap 1024 26108 1053))

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Lars Ingebrigtsen]

Ole Jørgen Brønner <olejorgenb@gmail.com> writes:

> There's a dedicated function, `ispell-find-aspell-dictionaries`, to find valid
> aspell dictionaries. It sanely ask bin/aspell for the list and then does some
> post-processing to find the rest of the spec.
>
> But after the *correct* list has been found this code[1] is run:
>
>     ;; 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)))))
>
> Adding invalid entries to the list. There might be a rationale behind that, but
> to me is seems like `ispell-dictionary-base-alist` mostly exist due to lack of
> *ispell* (old aspell?) introspection? (When using *ispell* as the
> provider, invalid entries
>  are filtered out in `ispell-valid-dictionary-list`)
>
> Removing that code isn't enough though:
>
> In `ispell-set-spellchecker-params` the invalid entries are added again:
> (where `found-dicts-alist` is the result of `ispell-find-aspell-dictionaries`
> in when aspell is used)
>
>     (run-hooks 'ispell-initialize-spellchecker-hook)
>
>     ;; Add dicts to ``ispell-dictionary-alist'' unless already present.
>     (dolist (dict (append found-dicts-alist
>                           ispell-base-dicts-override-alist
>                           ispell-dictionary-base-alist))
>       (unless (assoc (car dict) all-dicts-alist)
>         (add-to-list 'all-dicts-alist dict)))
>     (setq ispell-dictionary-alist all-dicts-alist))
>
> This code seems to have two purposes:
> - When *ispell* is the provider, `found-dicts-alist` is nil at this
> point, so the
>   default entries must be added
> - Apply dictionary overrides (possibly set by the hook above)

(I'm going through old Emacs bug reports that haven't received any
response.)

Thanks for the analysis; altering the code along these lines does fix
the problem, and `M-x ispell-change-dictionary' now seems a lot more
useful now that it just lists the dictionaries I actually have
installed.

> I would suggest simply removing [1] and not add the *base-alist above when using
> aspell/hunspell, but the following comment makes it seem like the "invalid"
> entries are added intentionally:
>
>     ;; Substitute ispell-dictionary-alist with the list of
>     ;; dictionaries corresponding to the given spellchecker.
>     ;; If a recent aspell or hunspell, use the list of really
>     ;; installed dictionaries and add to it elements of the original
>     ;; list that are not present there*. Allow distro info.

It may be intentional, but the other comments seem to indicate that it's
not...  and I don't see the utility of listing dictionaries we don't
have, so I think it's a good change anyway.

So I've made it on the Emacs trunk now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 28 Jul 2019 12:25:16 +0200
> Cc: 24050@debbugs.gnu.org
> 
> Thanks for the analysis; altering the code along these lines does fix
> the problem, and `M-x ispell-change-dictionary' now seems a lot more
> useful now that it just lists the dictionaries I actually have
> installed.
> 
> > I would suggest simply removing [1] and not add the *base-alist above when using
> > aspell/hunspell, but the following comment makes it seem like the "invalid"
> > entries are added intentionally:
> >
> >     ;; Substitute ispell-dictionary-alist with the list of
> >     ;; dictionaries corresponding to the given spellchecker.
> >     ;; If a recent aspell or hunspell, use the list of really
> >     ;; installed dictionaries and add to it elements of the original
> >     ;; list that are not present there*. Allow distro info.
> 
> It may be intentional, but the other comments seem to indicate that it's
> not...  and I don't see the utility of listing dictionaries we don't
> have, so I think it's a good change anyway.
> 
> So I've made it on the Emacs trunk now.

I think this is a mistake, and should be reverted.  It prevents users
from telling ispell.el to switch to a dictionary that aspell/hunspell
didn't return in the list.  That's a regression.

The OP's suggestion would make sense if the list created by that
function was only used for completion.  However, in fact we also force
the user to specify a dictionary from the list, i.e. the selection
must match one of the members of the list.  So this change is for the
worse.

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think this is a mistake, and should be reverted.  It prevents users
> from telling ispell.el to switch to a dictionary that aspell/hunspell
> didn't return in the list.  That's a regression.
>
> The OP's suggestion would make sense if the list created by that
> function was only used for completion.  However, in fact we also force
> the user to specify a dictionary from the list, i.e. the selection
> must match one of the members of the list.  So this change is for the
> worse.

OK; I can revert, but I don't quite understand your objection.  If you
choose a dictionary that's not in the list aspell/hunspell returned, the
next time you hit `M-$' you'll just get an error message.  I don't
understand the utility of having ispell let you make make a choice
that'll lead to an inevitable error.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: olejorgenb@gmail.com,  24050@debbugs.gnu.org
> Date: Sun, 28 Jul 2019 16:25:39 +0200
> 
> > The OP's suggestion would make sense if the list created by that
> > function was only used for completion.  However, in fact we also force
> > the user to specify a dictionary from the list, i.e. the selection
> > must match one of the members of the list.  So this change is for the
> > worse.
> 
> OK; I can revert, but I don't quite understand your objection.  If you
> choose a dictionary that's not in the list aspell/hunspell returned, the
> next time you hit `M-$' you'll just get an error message.

You assume that a dictionary not in the list doesn't exist; but that
isn't a given.  I've heard enough reports from users who for some
reason or other bumped into situations where ispell.el couldn't figure
out correctly what dictionaries are installed.  The standard list
(which is also customizable) leaves a "fire escape" for those cases.

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> You assume that a dictionary not in the list doesn't exist; but that
> isn't a given.  I've heard enough reports from users who for some
> reason or other bumped into situations where ispell.el couldn't figure
> out correctly what dictionaries are installed.  The standard list
> (which is also customizable) leaves a "fire escape" for those cases.

OK, so aspell/hunspell won't output the real list of dictionaries it has
access to, but find more later when you give them a dictionary name?

ispell.el itself doesn't look around for dictionaries -- it just calls
aspell/hunspell and it outputs the dictionaries (see below).

If the backend is something other than aspell/hunspell, ispell will
still output the entire list of built-in dictionary names.

$ aspell dicts
en
en-variant_0
en-variant_1
en-variant_2
en-w_accents
en-wo_accents
en_AU
en_AU-variant_0
en_AU-variant_1
en_AU-w_accents
en_AU-wo_accents
en_CA
[...]

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: olejorgenb@gmail.com,  24050@debbugs.gnu.org
> Date: Sun, 28 Jul 2019 17:08:26 +0200
> 
> OK, so aspell/hunspell won't output the real list of dictionaries it has
> access to, but find more later when you give them a dictionary name?

If aspell/hunspell is misconfigured, or has some bug, it might not
output the correct list of installed dictionaries, or not at all.

> ispell.el itself doesn't look around for dictionaries -- it just calls
> aspell/hunspell and it outputs the dictionaries (see below).

Yes.

> If the backend is something other than aspell/hunspell, ispell will
> still output the entire list of built-in dictionary names.

Right.

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If aspell/hunspell is misconfigured, or has some bug, it might not
> output the correct list of installed dictionaries, or not at all.

But if it's misconfigured, then it'll bug out when you ask it to do
other things as well, as hitting `M-$'.  So I don't understand why we
can't rely on aspell/hunspell to tell us what it knows about.

With the patch, ispell-change-directory (on this system) seems to list
all the dictionaries it can use.  This is quite useful, because now that
command allows me to actually choose the dictionary variant I most want
to use, which was impossible before when they were hidden among all the
dictionaries Emacs was claiming I could use (but can't).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Agustin Martin]

On Sun, Jul 28, 2019 at 05:51:34PM +0300, Eli Zaretskii wrote:
> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Cc: olejorgenb@gmail.com,  24050@debbugs.gnu.org
> > Date: Sun, 28 Jul 2019 16:25:39 +0200
> > 
> > > The OP's suggestion would make sense if the list created by that
> > > function was only used for completion.  However, in fact we also force
> > > the user to specify a dictionary from the list, i.e. the selection
> > > must match one of the members of the list.  So this change is for the
> > > worse.
> > 
> > OK; I can revert, but I don't quite understand your objection.  If you
> > choose a dictionary that's not in the list aspell/hunspell returned, the
> > next time you hit `M-$' you'll just get an error message.
> 
> You assume that a dictionary not in the list doesn't exist; but that
> isn't a given.  I've heard enough reports from users who for some
> reason or other bumped into situations where ispell.el couldn't figure
> out correctly what dictionaries are installed.  The standard list
> (which is also customizable) leaves a "fire escape" for those cases.

IIRC the original discussion was in the thread starting with

https://lists.gnu.org/archive/html/emacs-devel/2005-10/msg00566.html

The problem was with aspell dicts not having .dat file, and the workaround
was to preserve default values. Not sure if code has been changed
afterwards.

-- 
Agustin

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: olejorgenb@gmail.com,  24050@debbugs.gnu.org
> Date: Mon, 29 Jul 2019 13:12:15 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If aspell/hunspell is misconfigured, or has some bug, it might not
> > output the correct list of installed dictionaries, or not at all.
> 
> But if it's misconfigured, then it'll bug out when you ask it to do
> other things as well, as hitting `M-$'.

Not necessarily, because the dictionary can be specified as a full
absolute file name.  Also, ispell-dictionary-alist provides for
specifying command-line options for the speller, and some of those
could instruct the speller to look for the dictionary in a
non-standard directory, where the speller doesn't look when we query
it about installed dictionaries.

> With the patch, ispell-change-directory (on this system) seems to list
> all the dictionaries it can use.  This is quite useful, because now that
> command allows me to actually choose the dictionary variant I most want
> to use, which was impossible before when they were hidden among all the
> dictionaries Emacs was claiming I could use (but can't).

I see your point and agree with the convenience part, but
unfortunately things are not as simple as they seem.

First, if this kind of filtering of the potentially available
dictionaries should happen, its place is in
ispell-valid-dictionary-list, not where the proposed change was made.

Second, ispell-find-aspell-dictionaries was (before the change you
made) modifying in-place the entries for the dictionaries it found
(and now it doesn't -- this could be a bug in itself in some other use
case), so after ispell-find-aspell-dictionaries returned,
ispell-dictionary-base-alist had some of its entries modified to adapt
them to the underlying installation.  Thus, you cannot disregard
ispell-dictionary-base-alist, because it includes other valuable
information, and at least part of it is not theoretical, it reflects
what the underlying system has installed.

Third, by ignoring ispell-dictionary-base-alist, you make it
impossible to install a dictionary while Emacs is running with
ispell.el loaded.

So all of this is ... complicated.  And since the original complaint
is about the completion candidates shown by ispell-change-directory, I
think we should solve that problem by tweaking only the completion,
not the data structures it uses.  If you agree, maybe you or someone
else could come up with an alternative patch which only modified how
the collection of completion candidates is calculated.  (And btw, I
think it is a mistake to call completing-read with MUST-MATCH argument
non-nil here, because it prevents users from typing a full absolute
file name of the dictionary, an entirely valid and useful response.)

OK?

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Not necessarily, because the dictionary can be specified as a full
> absolute file name.  Also, ispell-dictionary-alist provides for
> specifying command-line options for the speller, and some of those
> could instruct the speller to look for the dictionary in a
> non-standard directory, where the speller doesn't look when we query
> it about installed dictionaries.

OK; I see now that this is more complicated than I assumed.  Thanks for
explaining.  I've now reverted the patch and reopened the bug.

>> With the patch, ispell-change-directory (on this system) seems to list
>> all the dictionaries it can use.  This is quite useful, because now that
>> command allows me to actually choose the dictionary variant I most want
>> to use, which was impossible before when they were hidden among all the
>> dictionaries Emacs was claiming I could use (but can't).
>
> I see your point and agree with the convenience part, but
> unfortunately things are not as simple as they seem.
>
> First, if this kind of filtering of the potentially available
> dictionaries should happen, its place is in
> ispell-valid-dictionary-list, not where the proposed change was made.

Right.

> So all of this is ... complicated.  And since the original complaint
> is about the completion candidates shown by ispell-change-directory, I
> think we should solve that problem by tweaking only the completion,
> not the data structures it uses.  If you agree, maybe you or someone
> else could come up with an alternative patch which only modified how
> the collection of completion candidates is calculated.

A different option might be to use the current way of doing the
completion candidates, but mark (for instance in bold) the ones that
aspell/hunspell has said are available on the system.

> (And btw, I think it is a mistake to call completing-read with
> MUST-MATCH argument non-nil here, because it prevents users from
> typing a full absolute file name of the dictionary, an entirely valid
> and useful response.)

Yup.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: olejorgenb@gmail.com,  24050@debbugs.gnu.org
> Date: Tue, 30 Jul 2019 13:22:18 +0200
> 
> > So all of this is ... complicated.  And since the original complaint
> > is about the completion candidates shown by ispell-change-directory, I
> > think we should solve that problem by tweaking only the completion,
> > not the data structures it uses.  If you agree, maybe you or someone
> > else could come up with an alternative patch which only modified how
> > the collection of completion candidates is calculated.
> 
> A different option might be to use the current way of doing the
> completion candidates, but mark (for instance in bold) the ones that
> aspell/hunspell has said are available on the system.

Yes.  Although that would be different from the way we display
completion candidates in other cases, and might require inline
explanation.

bug#24050: 24.5; ispell-change-dictionary suggests non-existent dicts (aspell)

[Lars Ingebrigtsen]

Ole Jørgen Brønner <olejorgenb@gmail.com> writes:

> I'm using aspell as the "ispell provider".
>
>     M-x ispell-change-dictionary
>
> helpfully gives a list of dictionaries to choose from. The list however contains
> dictionaries I don't have installed on my system. Only after spellchecking is
> attempted I get an error in *Messages*.

I've now fixed this in Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no