"Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

"Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Phil Sainty]

Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Robert Pluim <rpluim@gmail.com>
> > Debugger entered--Lisp error: (error "Can't find Hunspell dictionary with a
> > .aff affix f...")
> >   signal(error ("Can't find Hunspell dictionary with a .aff affix f..."))
> >   error("Can't find Hunspell dictionary with a .aff affix f...")
> >   ispell-find-hunspell-dictionaries()
> >   ispell-set-spellchecker-params()
> >   ispell-valid-dictionary-list()
> >
> > This is because I have no hunspell dictionaries at all
> 
> So you want to fix this?  I don't mind, but since it's (AFAIU)
> something specific to your installation, is it important to fix on
> the release branch?

While unexpected, it doesn't seem *invalid* to have no dictionaries
installed, so I think it's weird that `ispell-valid-dictionary-list'
triggers (indirectly) an error in this scenario to essentially say
"there were no valid dictionaries", when that's also what a nil return
value would have meant.

`ispell-valid-dictionary-list' or `ispell-set-spellchecker-params'
should perhaps use `with-demoted-errors' to avoid this?

Failing that, I think guarding against this in spelling-tests.el with
`ignore-errors' makes sense.  It's an :unstable test, but it shouldn't
be triggering actual errors.


-Phil

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Robert Pluim]

>>>>> On Thu, 13 Jan 2022 20:51:50 +1300, Phil Sainty <psainty@orcon.net.nz> said:

    Phil> Eli Zaretskii <eliz@gnu.org> wrote:
    >> > From: Robert Pluim <rpluim@gmail.com>
    >> > Debugger entered--Lisp error: (error "Can't find Hunspell dictionary with a
    >> > .aff affix f...")
    >> >   signal(error ("Can't find Hunspell dictionary with a .aff affix f..."))
    >> >   error("Can't find Hunspell dictionary with a .aff affix f...")
    >> >   ispell-find-hunspell-dictionaries()
    >> >   ispell-set-spellchecker-params()
    >> >   ispell-valid-dictionary-list()
    >> >
    >> > This is because I have no hunspell dictionaries at all
    >> So you want to fix this?  I don't mind, but since it's (AFAIU)
    >> something specific to your installation, is it important to fix on
    >> the release branch?

    Phil> While unexpected, it doesn't seem *invalid* to have no dictionaries
    Phil> installed, so I think it's weird that `ispell-valid-dictionary-list'
    Phil> triggers (indirectly) an error in this scenario to essentially say
    Phil> "there were no valid dictionaries", when that's also what a nil return
    Phil> value would have meant.

    Phil> `ispell-valid-dictionary-list' or `ispell-set-spellchecker-params'
    Phil> should perhaps use `with-demoted-errors' to avoid this?

Perhaps

    Phil> Failing that, I think guarding against this in spelling-tests.el with
    Phil> `ignore-errors' makes sense.  It's an :unstable test, but it shouldn't
    Phil> be triggering actual errors.

Itʼs not the test thatʼs triggering errors, itʼs the check to see if
the test should be defined at all. Iʼll take a look at fixing that and
ispell-valid-dictionary-list on master.

Robert
-- 

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Robert Pluim]

>>>>> On Thu, 13 Jan 2022 09:46:31 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Thu, 13 Jan 2022 20:51:50 +1300, Phil Sainty <psainty@orcon.net.nz> said:
    Phil> Eli Zaretskii <eliz@gnu.org> wrote:
    >>> > From: Robert Pluim <rpluim@gmail.com>
    >>> > Debugger entered--Lisp error: (error "Can't find Hunspell dictionary with a
    >>> > .aff affix f...")
    >>> >   signal(error ("Can't find Hunspell dictionary with a .aff affix f..."))
    >>> >   error("Can't find Hunspell dictionary with a .aff affix f...")
    >>> >   ispell-find-hunspell-dictionaries()
    >>> >   ispell-set-spellchecker-params()
    >>> >   ispell-valid-dictionary-list()
    >>> >
    >>> > This is because I have no hunspell dictionaries at all
    >>> So you want to fix this?  I don't mind, but since it's (AFAIU)
    >>> something specific to your installation, is it important to fix on
    >>> the release branch?

    Phil> While unexpected, it doesn't seem *invalid* to have no dictionaries
    Phil> installed, so I think it's weird that `ispell-valid-dictionary-list'
    Phil> triggers (indirectly) an error in this scenario to essentially say
    Phil> "there were no valid dictionaries", when that's also what a nil return
    Phil> value would have meant.

    Phil> `ispell-valid-dictionary-list' or `ispell-set-spellchecker-params'
    Phil> should perhaps use `with-demoted-errors' to avoid this?

    Robert> Perhaps

    Phil> Failing that, I think guarding against this in spelling-tests.el with
    Phil> `ignore-errors' makes sense.  It's an :unstable test, but it shouldn't
    Phil> be triggering actual errors.

Itʼs an unstable test, and when you actually run it, it fails 100% of
the time, whether your ispell/aspell/hunspell installation is correct
or not. Fixed on master.

    Robert> Itʼs not the test thatʼs triggering errors, itʼs the check to see if
    Robert> the test should be defined at all. Iʼll take a look at fixing that and
    Robert> ispell-valid-dictionary-list on master.

Wrapping the test in `ignore-errors' is easy. Of the three dictionary
listing functions, the one for hunspell is the only one that signals
an error, so maybe it should just log a message instead?

Robert
-- 

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Stefan Kangas <stefan@marxist.se>,
>   emacs-devel@gnu.org
> Date: Thu, 13 Jan 2022 11:20:05 +0100
> 
> Of the three dictionary listing functions, the one for hunspell is
> the only one that signals an error, so maybe it should just log a
> message instead?

There's a comment in ispell-find-hunspell-dictionaries that explains
why we signal an error there.  If we replace this with logging a
message, the problem the error attempts to solve will be back.

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Phil Sainty]

On 2022-01-13 23:39, Eli Zaretskii wrote:
> There's a comment in ispell-find-hunspell-dictionaries that explains
> why we signal an error there.  If we replace this with logging a
> message, the problem the error attempts to solve will be back.

Using `with-demoted-errors' would achieve both goals, I think?

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Eli Zaretskii]

> Date: Fri, 14 Jan 2022 00:28:58 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: Robert Pluim <rpluim@gmail.com>, stefan@marxist.se, emacs-devel@gnu.org
> 
> On 2022-01-13 23:39, Eli Zaretskii wrote:
> > There's a comment in ispell-find-hunspell-dictionaries that explains
> > why we signal an error there.  If we replace this with logging a
> > message, the problem the error attempts to solve will be back.
> 
> Using `with-demoted-errors' would achieve both goals, I think?

Using that where? in the test suite?

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Robert Pluim]

>>>>> On Thu, 13 Jan 2022 14:39:23 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> Date: Fri, 14 Jan 2022 00:28:58 +1300
    >> From: Phil Sainty <psainty@orcon.net.nz>
    >> Cc: Robert Pluim <rpluim@gmail.com>, stefan@marxist.se, emacs-devel@gnu.org
    >> 
    >> On 2022-01-13 23:39, Eli Zaretskii wrote:
    >> > There's a comment in ispell-find-hunspell-dictionaries that explains
    >> > why we signal an error there.  If we replace this with logging a
    >> > message, the problem the error attempts to solve will be back.
    >> 
    >> Using `with-demoted-errors' would achieve both goals, I think?

    Eli> Using that where? in the test suite?

Yes, for this one specific test. Although Iʼd prefer `ignore-errors',
since that could later be wrapped in `skip-unless', unlike
`with-demoted-errors'.

Robert
-- 

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Phil Sainty <psainty@orcon.net.nz>,  stefan@marxist.se,
>   emacs-devel@gnu.org
> Date: Thu, 13 Jan 2022 15:05:02 +0100
> 
> >>>>> On Thu, 13 Jan 2022 14:39:23 +0200, Eli Zaretskii <eliz@gnu.org> said:
> 
>     >> Date: Fri, 14 Jan 2022 00:28:58 +1300
>     >> From: Phil Sainty <psainty@orcon.net.nz>
>     >> Cc: Robert Pluim <rpluim@gmail.com>, stefan@marxist.se, emacs-devel@gnu.org
>     >> 
>     >> On 2022-01-13 23:39, Eli Zaretskii wrote:
>     >> > There's a comment in ispell-find-hunspell-dictionaries that explains
>     >> > why we signal an error there.  If we replace this with logging a
>     >> > message, the problem the error attempts to solve will be back.
>     >> 
>     >> Using `with-demoted-errors' would achieve both goals, I think?
> 
>     Eli> Using that where? in the test suite?
> 
> Yes, for this one specific test. Although Iʼd prefer `ignore-errors',
> since that could later be wrapped in `skip-unless', unlike
> `with-demoted-errors'.

Fine with me.

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Phil Sainty]

On 2022-01-14 03:42, Eli Zaretskii wrote:
>>     >> Using `with-demoted-errors' would achieve both goals, I think?
>> 
>>     Eli> Using that where? in the test suite?
>> 
>> Yes, for this one specific test. Although Iʼd prefer `ignore-errors',
> 
> Fine with me.

I agree with `ignore-errors' for the test suite.

My `with-⁠demoted-⁠errors' suggestion was actually regarding
`ispell-⁠valid-⁠dictionary-⁠list' or `ispell-⁠set-⁠spellchecker-⁠params'
as I wouldn't expect an absence of dictionaries to cause those
functions (or at least the former) to fail.

I haven't looked at them in detail, but my gut feeling was that
`ispell-⁠valid-⁠dictionary-⁠list' should be returning an empty list
in that scenario.  I thought that if a warning message was logged
along the way to point out that no dictionaries were installed,
that would seem fine (and preferable to an error).

Maybe not a change for 28.1, but potentially for master?


-Phil

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Eli Zaretskii]

> Date: Fri, 14 Jan 2022 14:05:56 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: Robert Pluim <rpluim@gmail.com>, stefan@marxist.se, emacs-devel@gnu.org
> 
> On 2022-01-14 03:42, Eli Zaretskii wrote:
> >>     >> Using `with-demoted-errors' would achieve both goals, I think?
> >> 
> >>     Eli> Using that where? in the test suite?
> >> 
> >> Yes, for this one specific test. Although Iʼd prefer `ignore-errors',
> > 
> > Fine with me.
> 
> I agree with `ignore-errors' for the test suite.
> 
> My `with-⁠demoted-⁠errors' suggestion was actually regarding
> `ispell-⁠valid-⁠dictionary-⁠list' or `ispell-⁠set-⁠spellchecker-⁠params'
> as I wouldn't expect an absence of dictionaries to cause those
> functions (or at least the former) to fail.
> 
> I haven't looked at them in detail, but my gut feeling was that
> `ispell-⁠valid-⁠dictionary-⁠list' should be returning an empty list
> in that scenario.  I thought that if a warning message was logged
> along the way to point out that no dictionaries were installed,
> that would seem fine (and preferable to an error).
> 
> Maybe not a change for 28.1, but potentially for master?

It is not clear to me what change you have in mind, or what change
could at all be possible.

We signal an error there because without it we will signal an error
later, where the context is lost and we cannot provide a meaningful
error message.

So with the current code, signaling an error there was deemed to be
the best solution for a situation where a spell-checker cannot do
anything useful -- without dictionaries, the speller is useless.  How
would you propose to continue using a speller without dictionaries,
and how would it help to return an empty list of dictionaries to the
caller? what would the caller do with such an empty list?

IOW, when you say "I wouldn't expect an absence of dictionaries to
cause those functions to fail", what do you expect from spell-checking
commands to do instead, when there are no dictionaries available to
them, except fail?

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Phil Sainty]

On 2022-01-14 20:12, Eli Zaretskii wrote:
> IOW, when you say "I wouldn't expect an absence of dictionaries to
> cause those functions to fail", what do you expect from spell-checking
> commands to do instead, when there are no dictionaries available to
> them, except fail?

`ispell-⁠valid-⁠dictionary-⁠list' isn't a spell-checking function per 
se,
though -- it's a helper function for establishing which dictionaries
are available for use *by* a spell-checking command.

If a command to actually check some spelling was invoked and there
were no dictionaries, then it would be fair to signal an error, but
this error is happening when merely asking for a list of dictionaries,
and an empty list is a valid list.

If there are no valid dictionaries, `ispell-⁠valid-⁠dictionary-⁠list'
should simply return an empty list.

IIUC this is only happening if hunspell is the spell checker, so
I think this should be considered a bug in the code for handling
hunspell.

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Eli Zaretskii]

> Date: Fri, 14 Jan 2022 22:06:47 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: rpluim@gmail.com, stefan@marxist.se, emacs-devel@gnu.org
> 
> On 2022-01-14 20:12, Eli Zaretskii wrote:
> > IOW, when you say "I wouldn't expect an absence of dictionaries to
> > cause those functions to fail", what do you expect from spell-checking
> > commands to do instead, when there are no dictionaries available to
> > them, except fail?
> 
> `ispell-⁠valid-⁠dictionary-⁠list' isn't a spell-checking function per 
> se,
> though -- it's a helper function for establishing which dictionaries
> are available for use *by* a spell-checking command.

If you just want to prevent an error in ispell-⁠valid-⁠dictionary-⁠list,
I'm okay with that, although it does seem pointless: you will not be
able to do any real spell-checking anyway.  So I question the wisdom
of letting the user think things are okay when they really aren't.

The error is signaled from ispell-find-hunspell-dictionaries, which is
called from other places as well, where having no dictionaries means
we cannot continue.  That error has to stay there, for this reason.

> If a command to actually check some spelling was invoked and there
> were no dictionaries, then it would be fair to signal an error, but
> this error is happening when merely asking for a list of dictionaries,
> and an empty list is a valid list.

Once again: what would be a point of showing an empty list? which use
cases it solves that signaling an error doesn't?  Is this just
aesthetics, or are there some real-life use cases where this gets in
the way?

> IIUC this is only happening if hunspell is the spell checker, so
> I think this should be considered a bug in the code for handling
> hunspell.

It isn't a bug.  Hunspell cannot do anything useful when we don't know
which dictionaries are available.  That it happens only with hunspell
is because the method of finding the dictionaries is different, and
with Hunspell the failure generally means the speller is misconfigured
in some way we cannot possibly know.

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Phil Sainty]

On 2022-01-13 23:20, Robert Pluim wrote:
> Itʼs an unstable test, and when you actually run it, it fails 100%
> of the time

Commit 385741fae276c7cfe2975e8a64cf084e0f63cf3d might need auditing
in case there was any similar breakage elsewhere?


> Wrapping the test in `ignore-errors' is easy. Of the three dictionary
> listing functions, the one for hunspell is the only one that signals
> an error, so maybe it should just log a message instead?

That was also the intent behind my `with-demoted-errors' suggestion
(but I've not tested that).


-Phil

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Robert Pluim]

>>>>> On Fri, 14 Jan 2022 00:26:24 +1300, Phil Sainty <psainty@orcon.net.nz> said:

    Phil> On 2022-01-13 23:20, Robert Pluim wrote:
    >> Itʼs an unstable test, and when you actually run it, it fails 100%
    >> of the time

    Phil> Commit 385741fae276c7cfe2975e8a64cf084e0f63cf3d might need auditing
    Phil> in case there was any similar breakage elsewhere?

files-tests-executable-find looks suspicious to me, but not for
this kind of error. I didnʼt spot any other issues

Robert
-- 

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Robert Pluim]

>>>>> On Thu, 13 Jan 2022 15:23:51 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Fri, 14 Jan 2022 00:26:24 +1300, Phil Sainty <psainty@orcon.net.nz> said:
    Phil> On 2022-01-13 23:20, Robert Pluim wrote:
    >>> Itʼs an unstable test, and when you actually run it, it fails 100%
    >>> of the time

    Phil> Commit 385741fae276c7cfe2975e8a64cf084e0f63cf3d might need auditing
    Phil> in case there was any similar breakage elsewhere?

    Robert> files-tests-executable-find looks suspicious to me, but not for
    Robert> this kind of error. I didnʼt spot any other issues

And Stefan K already fixed this one with aca235cb94b

Robert
-- 

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Phil Sainty]

On 2022-01-13 23:20, Robert Pluim wrote:
> Fixed on master.

For consistency with the rest of the commit which introduced
this `ert-with-temp-file' call, and what the code was before
that, I think this ought to now be:

     (ert-with-temp-directory tmpdir
       :prefix "so-long."
       :suffix ".ispell"

To match the original:

     (let* ((tmpdir (make-temp-file "so-long." :dir ".ispell"))

Re: "Can't find Hunspell dictionary" error (was Re: Emacs pretest 28.0.91 is out)

[Robert Pluim]

>>>>> On Fri, 14 Jan 2022 00:57:05 +1300, Phil Sainty <psainty@orcon.net.nz> said:

    Phil> On 2022-01-13 23:20, Robert Pluim wrote:
    >> Fixed on master.

    Phil> For consistency with the rest of the commit which introduced
    Phil> this `ert-with-temp-file' call, and what the code was before
    Phil> that, I think this ought to now be:

    Phil>     (ert-with-temp-directory tmpdir
    Phil>       :prefix "so-long."
    Phil>       :suffix ".ispell"

    Phil> To match the original:

    Phil>     (let* ((tmpdir (make-temp-file "so-long." :dir ".ispell"))

Yes. Iʼll queue that up.

(there are quite a few instances of "create temp dir and set VAR
(usually HOME) to it in the test suite. Do we need another macro?) [1]

Robert

Footnotes:
[1]  I am a joke

--