bug#72787: 31.0.50; Invalid describe-function completion candidates

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

Hi,

I've stumbled upon an issue with C-h f completions, both on the release
branch and on the master branch:

1. emacs -Q
2. C-h f string-edit- TAB

In Emacs 29, this pops up the *Completions* buffer, with 3 completion
candidates, string-edit-{abort,done,mode}.  That's the expected
behavior, because "string-edit-" is not itself a valid candidate.

However, in the release and master branches I get a minibuffer message
saying "Complete, but not unique".  This is incorrect, because the input
is not complete.  Another TAB pops up the *Completions* buffer, which is
now showing 4 candidates: the expected 3 plus "string-edit-" itself.
Typing RET exits the minibuffer without asking for confirmation, and
yields an error: "Symbol’s function definition is void: string-edit-".

So it seems like "string-edit-" is being considered as a valid
completion candidate, while it shouldn't be.


Thanks,

Eshel

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

"Eshel Yaron via \"Bug reports for GNU Emacs, the Swiss army knife of text editors\"" <bug-gnu-emacs@gnu.org> writes:

> I've stumbled upon an issue with C-h f completions, both on the release
> branch and on the master branch:
>
> 1. emacs -Q
> 2. C-h f string-edit- TAB
>
> In Emacs 29, this pops up the *Completions* buffer, with 3 completion
> candidates, string-edit-{abort,done,mode}.  That's the expected
> behavior, because "string-edit-" is not itself a valid candidate.
>
> However, in the release and master branches I get a minibuffer message
> saying "Complete, but not unique".  This is incorrect, because the input
> is not complete.  Another TAB pops up the *Completions* buffer, which is
> now showing 4 candidates: the expected 3 plus "string-edit-" itself.
> Typing RET exits the minibuffer without asking for confirmation, and
> yields an error: "Symbol’s function definition is void: string-edit-".
>
> So it seems like "string-edit-" is being considered as a valid
> completion candidate, while it shouldn't be.

This appears to be caused by commit
45ae4de0e7ce99c88c62f940f605bca693b8e33f:

* lisp/help-fns.el (help-definition-prefixes): Don't delete the hashtable

Pip

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

Hello,

Stefan's commit indeed seems related.  Stefan, could you maybe
have a look please?


Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs@gnu.org> writes:

> > 1. emacs -Q
> > 2. C-h f string-edit- TAB
> >
> > In Emacs 29, this pops up the *Completions* buffer, with 3 completion
> > candidates, string-edit-{abort,done,mode}.  That's the expected
> > behavior, because "string-edit-" is not itself a valid candidate.
> >
> > However, in the release and master branches I get a minibuffer message
> > saying "Complete, but not unique".  This is incorrect, because the input
> > is not complete.  Another TAB pops up the *Completions* buffer, which is
> > now showing 4 candidates: the expected 3 plus "string-edit-" itself.
> > Typing RET exits the minibuffer without asking for confirmation, and
> > yields an error: "Symbol’s function definition is void: string-edit-".
> >
> > So it seems like "string-edit-" is being considered as a valid
> > completion candidate, while it shouldn't be.
>
> This appears to be caused by commit
> 45ae4de0e7ce99c88c62f940f605bca693b8e33f:
>
> * lisp/help-fns.el (help-definition-prefixes): Don't delete the hashtable


TIA,

Michael.

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

>> This appears to be caused by commit
>> 45ae4de0e7ce99c88c62f940f605bca693b8e33f:
>>
>> * lisp/help-fns.el (help-definition-prefixes): Don't delete the hashtable
> Stefan's commit indeed seems related.  Stefan, could you maybe
> have a look please?

I think this report makes it clear that
45ae4de0e7ce99c88c62f940f605bca693b8e33f should not have gone to
`emacs-30` but to `master`.  AFAIK it did not fix a regression or even
a user-visible bug.


        Stefan

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> > Stefan's commit indeed seems related.  Stefan, could you maybe
> > have a look please?
>
> I think this report makes it clear that
> 45ae4de0e7ce99c88c62f940f605bca693b8e33f should not have gone to
> `emacs-30` but to `master`.  AFAIK it did not fix a regression or even
> a user-visible bug.

So you will revert it on the release branch?

Michael.

bug#72787: 31.0.50; Invalid describe-function completion candidates

[Eli Zaretskii]

> Cc: 72787@debbugs.gnu.org, me@eshelyaron.com, Pip Cet <pipcet@protonmail.com>
> Date: Sun, 25 Aug 2024 22:37:01 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> >> This appears to be caused by commit
> >> 45ae4de0e7ce99c88c62f940f605bca693b8e33f:
> >>
> >> * lisp/help-fns.el (help-definition-prefixes): Don't delete the hashtable
> > Stefan's commit indeed seems related.  Stefan, could you maybe
> > have a look please?
> 
> I think this report makes it clear that
> 45ae4de0e7ce99c88c62f940f605bca693b8e33f should not have gone to
> `emacs-30` but to `master`.  AFAIK it did not fix a regression or even
> a user-visible bug.

Thanks, I've therefore reverted the above commit from the emacs-30
release branch, and I'm closing this bug.

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 72787@debbugs.gnu.org, me@eshelyaron.com, Pip Cet <pipcet@protonmail.com>
>> Date: Sun, 25 Aug 2024 22:37:01 -0400
>> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> >> This appears to be caused by commit
>> >> 45ae4de0e7ce99c88c62f940f605bca693b8e33f:
>> >>
>> >> * lisp/help-fns.el (help-definition-prefixes): Don't delete the hashtable
>> > Stefan's commit indeed seems related.  Stefan, could you maybe
>> > have a look please?
>> 
>> I think this report makes it clear that
>> 45ae4de0e7ce99c88c62f940f605bca693b8e33f should not have gone to
>> `emacs-30` but to `master`.  AFAIK it did not fix a regression or even
>> a user-visible bug.
>
> Thanks, I've therefore reverted the above commit from the emacs-30
> release branch, and I'm closing this bug.

Thanks, but the issue remains, so shouldn't the bug remain open as well?


Best,

Eshel

bug#72787: 31.0.50; Invalid describe-function completion candidates

[Eli Zaretskii]

reopen 72787
thanks

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  michael_heerdegen@web.de,
>   72787-done@debbugs.gnu.org,  pipcet@protonmail.com
> Date: Sun, 01 Sep 2024 19:41:47 +0200
> 
> Thanks, but the issue remains, so shouldn't the bug remain open as well?

I reopened it.

bug#72787: 31.0.50; Invalid describe-function completion candidates

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  michael_heerdegen@web.de,
>>   72787-done@debbugs.gnu.org,  pipcet@protonmail.com
>> Date: Sun, 01 Sep 2024 19:41:47 +0200
>> 
>> Thanks, but the issue remains, so shouldn't the bug remain open as well?
>
> I reopened it.

Is it possible to revert this change on master as well until a new
working patch is available?  IIUC, Stefan noted upthread:

  I think this report makes it clear that
  45ae4de0e7ce99c88c62f940f605bca693b8e33f should not have gone to
  `emacs-30` but to `master`.  AFAIK it did not fix a regression or even
  a user-visible bug.

The current behavior is somewhat annoying.

Best, Arash

bug#72787: 31.0.50; Invalid describe-function completion candidates

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: Eshel Yaron <me@eshelyaron.com>,  michael_heerdegen@web.de,
>   pipcet@protonmail.com,  72787@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Thu, 10 Oct 2024 06:56:58 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Eshel Yaron <me@eshelyaron.com>
> >> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  michael_heerdegen@web.de,
> >>   72787-done@debbugs.gnu.org,  pipcet@protonmail.com
> >> Date: Sun, 01 Sep 2024 19:41:47 +0200
> >> 
> >> Thanks, but the issue remains, so shouldn't the bug remain open as well?
> >
> > I reopened it.
> 
> Is it possible to revert this change on master as well until a new
> working patch is available?  IIUC, Stefan noted upthread:
> 
>   I think this report makes it clear that
>   45ae4de0e7ce99c88c62f940f605bca693b8e33f should not have gone to
>   `emacs-30` but to `master`.  AFAIK it did not fix a regression or even
>   a user-visible bug.
> 
> The current behavior is somewhat annoying.

AFAIU, Stefan later explained that this is a feature, but maybe I
misunderstood him.  See

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73473#16

Stefan?

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Arash Esbati <arash@gnu.org>
>> Cc: Eshel Yaron <me@eshelyaron.com>,  michael_heerdegen@web.de,
>>   pipcet@protonmail.com,  72787@debbugs.gnu.org,  monnier@iro.umontreal.ca
>> Date: Thu, 10 Oct 2024 06:56:58 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Eshel Yaron <me@eshelyaron.com>
>> >> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  michael_heerdegen@web.de,
>> >>   72787-done@debbugs.gnu.org,  pipcet@protonmail.com
>> >> Date: Sun, 01 Sep 2024 19:41:47 +0200
>> >> 
>> >> Thanks, but the issue remains, so shouldn't the bug remain open as well?
>> >
>> > I reopened it.
>> 
>> Is it possible to revert this change on master as well until a new
>> working patch is available?  IIUC, Stefan noted upthread:
>> 
>>   I think this report makes it clear that
>>   45ae4de0e7ce99c88c62f940f605bca693b8e33f should not have gone to
>>   `emacs-30` but to `master`.  AFAIK it did not fix a regression or even
>>   a user-visible bug.
>> 
>> The current behavior is somewhat annoying.
>
> AFAIU, Stefan later explained that this is a feature, but maybe I
> misunderstood him.  See
>
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73473#16

AFAIU, the feature here would be loading library foo just in time for
you to get help about symbol foo-bar.  But the way this feature is
currently implemented is by including incorrect completion candidates.
That's not a feature, that's an implementation artifact, which
unfortunately happens to create an unpleasant UX :/

So I suggest doing one of the following:

- implement the feature differently, without this side-effect; or
- make it opt-in; or at least
- allow users to opt-out.


Best,

Eshel

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

Eshel Yaron <me@eshelyaron.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Arash Esbati <arash@gnu.org>
>>> 
>>> Is it possible to revert this change on master as well until a new
>>> working patch is available?  IIUC, Stefan noted upthread:
>>> 
>>>   I think this report makes it clear that
>>>   45ae4de0e7ce99c88c62f940f605bca693b8e33f should not have gone to
>>>   `emacs-30` but to `master`.  AFAIK it did not fix a regression or even
>>>   a user-visible bug.
>>> 
>>> The current behavior is somewhat annoying.
>>
>> AFAIU, Stefan later explained that this is a feature, but maybe I
>> misunderstood him.  See
>>
>>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73473#16
>
> AFAIU, the feature here would be loading library foo just in time for
> you to get help about symbol foo-bar.  But the way this feature is
> currently implemented is by including incorrect completion candidates.
> That's not a feature, that's an implementation artifact, which
> unfortunately happens to create an unpleasant UX :/
>
> So I suggest doing one of the following:
>
> - implement the feature differently, without this side-effect; or
> - make it opt-in; or at least
> - allow users to opt-out.

Correction: I forgot that there's already a way to opt-out, by setting
help-enable-completion-autoload to nil.


Cheers,

Eshel

bug#72787: 31.0.50; Invalid describe-function completion candidates

[Arash Esbati]

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

Eshel Yaron <me@eshelyaron.com> writes:

>> AFAIU, the feature here would be loading library foo just in time for
>> you to get help about symbol foo-bar.  But the way this feature is
>> currently implemented is by including incorrect completion candidates.
>> That's not a feature, that's an implementation artifact, which
>> unfortunately happens to create an unpleasant UX :/

Thanks for your response.  For me, this is what I see with

  • emacs Q
  • (setq completions-format 'vertical)
  • C-h v TAB

[-- Attachment #2: Emacs-Q.png --]
[-- Type: image/png, Size: 45191 bytes --]

[-- Attachment #3: Type: text/plain, Size: 94 bytes --]


This is what I see when I do `package-initialize'; I have dash.el
installed as a dependency:

[-- Attachment #4: Emacs-Q-package.png --]
[-- Type: image/png, Size: 35813 bytes --]

[-- Attachment #5: Type: text/plain, Size: 389 bytes --]


>> So I suggest doing one of the following:
>>
>> - implement the feature differently, without this side-effect; or

From the second image above, I'd say that the feature isn't working
correctly.

> Correction: I forgot that there's already a way to opt-out, by setting
> help-enable-completion-autoload to nil.

This is what I see after setting `help-enable-completion-autoload' to
nil:

[-- Attachment #6: Emacs-Q-package-h-e-c-a-nil.png --]
[-- Type: image/png, Size: 47482 bytes --]

[-- Attachment #7: Type: text/plain, Size: 76 bytes --]


Thanks for the pointer, I've set the variable to nil for now.

Best, Arash

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

>>> AFAIU, the feature here would be loading library foo just in time for
>>> you to get help about symbol foo-bar.

Actually, it's not so much for the case where you know you want to see
`foo-bar` (which should be handled by `help-enable-symbol-autoload`
already) but also to let you discover that there might be a `foo-bar`
because there's a `foo-`.

>>> So I suggest doing one of the following:
>>> - implement the feature differently, without this side-effect; or

I'm all for it but I don't know what that would look like.

> From the second image above, I'd say that the feature isn't working
> correctly.

Could you clarify which part of the picture make you think so?
[ Also, does (featurep 'dash) return nil or t before you perform that
  completion?  ]
[ Also did you get this with an Emacs which includes:

    commit 8683d64cc571500347a16e7cb7d144d723250489
    Author: Stefan Monnier <monnier@iro.umontreal.ca>
    Date:   Thu Oct 3 10:25:13 2024 -0400
    
        (help--symbol-completion-table): Try and fix bug#73473
        
        * lisp/help-fns.el (help--symbol-completion-table): Be more
        careful with `help-enable-completion-autoload` so we don't load
        a package in cases where we already know it won't impact the result.

]

> This is what I see after setting `help-enable-completion-autoload' to nil:
[...]
> Thanks for the pointer, I've set the variable to nil for now.

Sorry for not mentioning it earlier, I stupidly assumed you were aware
of that variable.


        Stefan

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

> I've stumbled upon an issue with C-h f completions, both on the release
> branch and on the master branch:
>
> 1. emacs -Q
> 2. C-h f string-edit- TAB
>
> In Emacs 29, this pops up the *Completions* buffer, with 3 completion
> candidates, string-edit-{abort,done,mode}.  That's the expected
> behavior, because "string-edit-" is not itself a valid candidate.

I just pushed a fix to `master`.  Now you do get a *Completions* buffer
again (with more entries, tho, because
`help-enable-completions-autoload` loaded the `string-edit` package).


        Stefan

bug#72787: 31.0.50; Invalid describe-function completion candidates

[Arash Esbati]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Could you clarify which part of the picture make you think so?
> [ Also, does (featurep 'dash) return nil or t before you perform that
>   completion?  ]

I do:

• emacs -Q
• Eval:
    (progn
      (setq completions-format 'vertical)
      (package-initialize)
      (featurep 'dash))
  which returns nil
• 'C-h v TAB' and see this:

[-- Attachment #2: Emacs-Q-package.png --]
[-- Type: image/png, Size: 35813 bytes --]

[-- Attachment #3: Type: text/plain, Size: 792 bytes --]


Take for example "-keep".  I do 'C-h v -keep RET' and get:

  -keep is void as a variable.

  Not documented as a variable.

Now I eval (require 'dash), do 'C-h v -keep RET' again and get:

  -keep is void as a variable.

  Not documented as a variable.

whereas 'C-h f -keep' returns:

  -keep is a native-comp-function in ‘dash.el’.

  (-keep FN LIST)

  Return a new list of the non-nil results of applying FN to each item in LIST.
  Like ‘-filter’, but returns the non-nil results of FN instead of
  the corresponding elements of LIST.

  Its anaphoric counterpart is ‘--keep’.

In summary, I see a void variable in the list of completions where the
function is defined.  Also, when I do 'emacs -Q' followed directly with
'C-h f TAB', I see this:

[-- Attachment #4: Emacs-Q-funcs.png --]
[-- Type: image/png, Size: 40015 bytes --]

[-- Attachment #5: Type: text/plain, Size: 750 bytes --]


The (setf ...) part doesn't look right to me.

> [ Also did you get this with an Emacs which includes:
>
>     commit 8683d64cc571500347a16e7cb7d144d723250489
>     Author: Stefan Monnier <monnier@iro.umontreal.ca>
>     Date:   Thu Oct 3 10:25:13 2024 -0400
> ]

$ git show 8683d64cc571500347a16e7cb7d144d723250489
commit 8683d64cc571500347a16e7cb7d144d723250489
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Thu Oct 3 10:25:13 2024 -0400

    (help--symbol-completion-table): Try and fix bug#73473

I'm runnig Emacs from master (ff4de9ef) on macOS.

> Sorry for not mentioning it earlier, I stupidly assumed you were aware
> of that variable.

I'm sorry I didn't investigate enough to find out about that variable
myself.

Best, Arash

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

> In summary, I see a void variable in the list of completions where the
> function is defined.

Right, indeed this mechanism doesn't distinguish between variables and
functions.  It is intended for real namespace prefixes and presumes
these will be populated by both vars and funs, and that fails with
packages that don't obey the usual namespace prefixing rules.

We should make this mechanism more strict in this respect.
I see in `dash-autoloads.el` we end up using:

    (register-definition-prefixes "dash" '("!cdr" "!cons" "--" "->" "-a" "-butlast" "-c" "-d" "-e" "-f" "-gr" "-i" "-juxt" "-keep" "-l" "-m" "-no" "-o" "-p" "-r" "-s" "-t" "-u" "-value-to-list" "-when-let" "-zip" "dash-"))

It would be better to bail if our guessed set of prefixes is too large,
since it's usually a sign that our heuristic just failed, as here.

> Also, when I do 'emacs -Q' followed directly with
> 'C-h f TAB', I see this:
>
> The (setf ...) part doesn't look right to me.

These look correct to me, they're setter functions.


        Stefan

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

Hi Stefan,

Stefan Monnier writes:

>>>> AFAIU, the feature here would be loading library foo just in time for
>>>> you to get help about symbol foo-bar.
>
> Actually, it's not so much for the case where you know you want to see
> `foo-bar` (which should be handled by `help-enable-symbol-autoload`
> already) but also to let you discover that there might be a `foo-bar`
> because there's a `foo-`.

I see, so this is meant as a hint that should aid with discoverability.
Perhaps annotating these prefix candidates could make that more obvious:
I'm not sure that seeing "foo-" in *Completions* immediately suggests
"try to complete this prefix to see more candidates".

>>>> So I suggest doing one of the following:
>>>> - implement the feature differently, without this side-effect; or
>
> I'm all for it but I don't know what that would look like.

Personally I don't see this use case as important enough to break the
invariant that completion candidates are valid inputs, so I'd go a
different route.  The ability to load possibly-relevant libraries from
within the minibuffer is great, but I think that providing a command
that does that on demand would provide the same benefits.  Basically,
you would press a key when you want to check if some unloaded maybe
library defines something relevant.  Such a command could take into
account the current minibuffer input, so it can be as efficient as the
current facility. I can share a prototype if that sounds intriguing.


Best,

Eshel

bug#72787: 31.0.50; Invalid describe-function completion candidates

[Arash Esbati]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Right, indeed this mechanism doesn't distinguish between variables and
> functions.

Thanks, this was a source of confusion, at least for me.

> It would be better to bail if our guessed set of prefixes is too
> large, since it's usually a sign that our heuristic just failed, as
> here.

That sounds like a good approach.

> These look correct to me, they're setter functions.

Ok, but I confess that I'd expect a symbol only here:

,----[ C-h f (setf seq-elt) RET ]
| \(setf\ seq-elt\) is a byte-code-function in ‘seq.el’.
| 
| (\(setf\ seq-elt\) ARG0 ARG &rest ARGS)
| 
| Not documented.
| 
| 
| This is a generic function.
| 
| Implementations:
| 
| (\(setf\ seq-elt\) STORE (SEQUENCE cons) N) in ‘seq.el’.
| 
| Undocumented
| 
| (\(setf\ seq-elt\) STORE (SEQUENCE array) N) in ‘seq.el’.
| 
| Undocumented
| 
`----

Best, Arash

bug#72787: 31.0.50; Invalid describe-function completion candidates

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

>> These look correct to me, they're setter functions.
>
> Ok, but I confess that I'd expect a symbol only here:

They are symbols:

> ,----[ C-h f (setf seq-elt) RET ]
> | \(setf\ seq-elt\) is a byte-code-function in ‘seq.el’.

See those backslashes?
It is the symbol returned by (intern "(setf seq-elt)").

In Common Lisp, you can define those functions for example with

    (defun (setf my-foo) (...) ...)

and ELisp supports that to some extent as well, tho currently
not for `defun`.


        Stefan