bug#64351: probabe bug associated with “completion-regexp-list”

bug#64351: probabe bug associated with “completion-regexp-list”

[Shynur Xie]

Evaluate this:

    (setq completion-regexp-list '("^[^-]"))

Then type “C-h v -l TAB”, an error will be thrown.

--
shynur

bug#64351: probabe bug associated with “completion-regexp-list”

[Eli Zaretskii]

> From: Shynur Xie <one.last.kiss@outlook.com>
> Date: Thu, 29 Jun 2023 18:53:28 +0000

(Please always state at least in which version of Emacs on what
platform did the problem occurred.  It is best to use the dedicated
command report-emacs-bug, which will collect the important details for
you.  This avoids the need to guess whether the issue is relevant to
Emacs 29 and trying several different versions to see if they
reproduce the problem.)

> Evaluate this:
> 
>     (setq completion-regexp-list '("^[^-]"))
> 
> Then type “C-h v -l TAB”, an error will be thrown.

Stefan, is the below the right fix?  If it is, do you think it is safe
enough for the release branch?

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 4aa1ab3..3e30b68 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4027,7 +4027,8 @@ completion-pcm--merge-completions
               (setq ccs (nreverse ccs))
               (let* ((prefix (try-completion fixed comps))
                      (unique (or (and (eq prefix t) (setq prefix fixed))
-                                 (eq t (try-completion prefix comps)))))
+                                 (and (stringp prefix)
+                                      (eq t (try-completion prefix comps))))))
                 (unless (or (eq elem 'prefix)
                             (equal prefix ""))
                   (push prefix res))

bug#64351: probabe bug associated with “completion-regexp-list”

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

> Stefan, is the below the right fix?  If it is, do you think it is safe
> enough for the release branch?

I haven't had time to investigate if it's really The Right Fix, but it's
definitely a safe fix, yes.


        Stefan

bug#64351: probabe bug associated with “completion-regexp-list”

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Shynur Xie <one.last.kiss@outlook.com>,  64351@debbugs.gnu.org
> Date: Fri, 30 Jun 2023 08:51:18 -0400
> 
> > Stefan, is the below the right fix?  If it is, do you think it is safe
> > enough for the release branch?
> 
> I haven't had time to investigate if it's really The Right Fix, but it's
> definitely a safe fix, yes.

Thanks.

The alternative would be to treat the nil return value from
try-completion here:

              (let* ((prefix (try-completion fixed comps)) <<<<<<<<<<<<<<<
                     (unique (or (and (eq prefix t) (setq prefix fixed))
                                 (and (stringp prefix)
                                      (eq t (try-completion prefix comps))))))
                (unless (or (eq elem 'prefix)
                            (equal prefix ""))
                  (push prefix res))

as if it were "".  Would that be better?

With the change I proposed, "C-h v -l TAB" deletes "-l" that the user
typed -- is that the expected result when there are no completions?

bug#64351: probabe bug associated with “completion-regexp-list”

[Shynur Xie]

Hi Eli, there’s another issue about “completion-regexp-list”.

I launched Emacs with “-Q” and evaluate:

    (advice-add 'describe-variable :around ;“C-h v”
                (lambda (advice-added-function &rest arguments)
                  (let ((completion-regexp-list '("^\\([^-]*$\\|\\([^-]+\\(-[^-]+\\)-?\\)\\)$")))
                    (apply advice-added-function arguments))))

to filter out symbols “*--*”.

Then type “C-h v advice TAB”, I can still see some variables whose names are
“*--*” (e.g., “advice--bytecodes”).  Though “string-match-p” says that
name doesn’t match:

    ELISP> (string-match-p "^\\([^-]*$\\|\\([^-]+\\(-[^-]+\\)-?\\)\\)$" "advice--bytecodes")
    nil

____________________

Emacs 28.2 on MS-Windows

bug#64351: probabe bug associated with “completion-regexp-list”

[Andreas Schwab]

On Jun 30 2023, Shynur Xie wrote:

> I launched Emacs with “-Q” and evaluate:
>
>     (advice-add 'describe-variable :around ;“C-h v”
>                 (lambda (advice-added-function &rest arguments)
>                   (let ((completion-regexp-list '("^\\([^-]*$\\|\\([^-]+\\(-[^-]+\\)-?\\)\\)$")))
>                     (apply advice-added-function arguments))))
>
> to filter out symbols “*--*”.
>
> Then type “C-h v advice TAB”,

Interactive completion runs before the function is called, inside
call-interactively.  You need to advice the interactive spec.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#64351: probabe bug associated with “completion-regexp-list”

[Shynur Xie]

> Andreas> To: Shynur
> Andreas> Interactive completion runs before the function is called,
> Andreas> inside call-interactively.  You need to advice the
> Andreas> interactive spec.

Thanks!

This is a tricky issue to me, because I only want to filter when
calling “describe-variable”.  I guess I have to give up this idea...

No further question now.

bug#64351: probabe bug associated with “completion-regexp-list”

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

>> Evaluate this:
>>     (setq completion-regexp-list '("^[^-]"))

Bad idea.
Admittedly, `completion-regexp-list` is documented poorly enough that
it's not clear what usage is valid and what isn't, but a lot of
completion code will be broken if you set it globally to anything else
than nil.

>> Then type “C-h v -l TAB”, an error will be thrown.
> Stefan, is the below the right fix?  If it is, do you think it is safe
> enough for the release branch?
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index 4aa1ab3..3e30b68 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -4027,7 +4027,8 @@ completion-pcm--merge-completions
>                (setq ccs (nreverse ccs))
>                (let* ((prefix (try-completion fixed comps))
>                       (unique (or (and (eq prefix t) (setq prefix fixed))
> -                                 (eq t (try-completion prefix comps)))))
> +                                 (and (stringp prefix)
> +                                      (eq t (try-completion prefix comps))))))
>                  (unless (or (eq elem 'prefix)
>                              (equal prefix ""))
>                    (push prefix res))

I'd argue that the better fix is to let-bind `completion-regexp-list` to
nil around the `minibuffer.el` code which uses
`try/all/test-completion` to protect against those kinds of misuses.
Or alternatively to say "if it hurts, don't do that".


        Stefan

bug#64351: probabe bug associated with “completion-regexp-list”

[Shynur Xie]

> Shynur> Evaluate this:
> Shynur>     (setq completion-regexp-list '("^[^-]"))
> Stefan> Bad idea.

I have to say that it is not a bad idea to write this _meaningless_
and _short_ expression.  I just want to find a short enough expression
to show the issue.  If I post a _long_ code snippet and say “Hey,
there’s a bug”, nobody wants to see it. :-)

bug#64351: probabe bug associated with “completion-regexp-list”

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

>> Shynur> Evaluate this:
>> Shynur>     (setq completion-regexp-list '("^[^-]"))
>> Stefan> Bad idea.
>
> I have to say that it is not a bad idea to write this _meaningless_
> and _short_ expression.  I just want to find a short enough expression
> to show the issue.

FWIW, the main problem is setting this var while minibuffer.el functions
are called (so setting globally is definitely a problem).
This var is usually expected to be let-bound next to your own calls to
`try/all/test-completion`.

But as I say, this is actually not documented, it's just what I have
found to be general assumptions is various packages that implement
completion UIs (including the one I wrote :-)

> If I post a _long_ code snippet and say “Hey,
> there’s a bug”, nobody wants to see it. :-)

I know what you men.  I'd be interested to hear about your
use-case, tho.  Maybe I can provide a good alternative.


        Stefan

bug#64351: probabe bug associated with “completion-regexp-list”

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Shynur Xie <one.last.kiss@outlook.com>,  64351@debbugs.gnu.org
> Date: Fri, 30 Jun 2023 14:33:49 -0400
> 
> >> Evaluate this:
> >>     (setq completion-regexp-list '("^[^-]"))
> 
> Bad idea.
> Admittedly, `completion-regexp-list` is documented poorly enough that
> it's not clear what usage is valid and what isn't, but a lot of
> completion code will be broken if you set it globally to anything else
> than nil.

Would you mind telling more about valid uses of this?  I'd like to
improve the documentation.

bug#64351: probabe bug associated with “completion-regexp-list”

[Shynur Xie]

> Stefan> I'd be interested to hear about your use-case, tho.

I tried to filter out internal variables “*--*” when typing “C-h v”
several days ago, so I wrote this:

(advice-add 'describe-variable :around ;“C-h v”
            (lambda (advice-added-function &rest arguments)
              (let ((completion-regexp-list
                     '("^\\([^-]*$\\|\\([^-]+\\(-[^-]+\\)-?\\)\\)$")))
                (apply advice-added-function arguments))))

It didn’t work.  I wanted to find out why, so to save time, I set its
value globally:

(setq completion-regexp-list
                      '("^\\([^-]*$\\|\\([^-]+\\(-[^-]+\\)-?\\)\\)$"))

Somehow, Emacs threw an error.  I guess this is a bug, so I tried to
narrow it down and then reported it.

> Stefan> Maybe I can provide a good alternative.

I think it would be nice if Emacs can provides an option, say,
“completions-filter-function” (or “completions-help-filter-function”
only for functions “describe-*”), which receives 2 arguments -- TYPE
and COMPLETION.  If I evaluate:

(setq completions-filter-function
        (lambda (type completion)
          (pcase type
            ('variable
             (not (string-match-p "--" (symbol-name completion)))))))

I can filter out “*--*” variables.  It’s also easier to add other
rules than the original “completion-regexp-list”.  Using regexp to
filter is not very convenient, say if I want to filter out
“*-internal”, I have to write this regexp:

"\\([^l]\\|[^a]l\\|[^n]al\\|[^r]nal\\|[^e]rnal\\|[^t]ernal\\|[^n]ternal\\|[^i]nternal\\|[^-]internal\\)$"

I don’t know whether “completions-filter-function” is a good idea, I’m
just making a suggestion.  Thanks.

bug#64351: probabe bug associated with “completion-regexp-list”

[Shynur Xie]

> Shynur> To: Stefan
> Shynur> (setq completions-filter-function
> Shynur>        (lambda (type completion)
> Shynur>          (pcase type
> Shynur>            ('variable
> Shynur>             (not (string-match-p "--" (symbol-name completion)))))))

Should be

(pcase type
  (...)
  (_ t))

Sorry for this typo.

bug#64351: probabe bug associated with “completion-regexp-list”

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

> (setq completions-filter-function
>         (lambda (type completion)
>           (pcase type
>             ('variable
>              (not (string-match-p "--" (symbol-name completion)))))))

Are you looking for `minibuffer-completion-predicate`, maybe?


        Stefan

bug#64351: probabe bug associated with “completion-regexp-list”

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Shynur Xie <one.last.kiss@outlook.com>,  64351@debbugs.gnu.org
> Date: Fri, 30 Jun 2023 14:33:49 -0400
> 
> > Stefan, is the below the right fix?  If it is, do you think it is safe
> > enough for the release branch?
> >
> > diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> > index 4aa1ab3..3e30b68 100644
> > --- a/lisp/minibuffer.el
> > +++ b/lisp/minibuffer.el
> > @@ -4027,7 +4027,8 @@ completion-pcm--merge-completions
> >                (setq ccs (nreverse ccs))
> >                (let* ((prefix (try-completion fixed comps))
> >                       (unique (or (and (eq prefix t) (setq prefix fixed))
> > -                                 (eq t (try-completion prefix comps)))))
> > +                                 (and (stringp prefix)
> > +                                      (eq t (try-completion prefix comps))))))
> >                  (unless (or (eq elem 'prefix)
> >                              (equal prefix ""))
> >                    (push prefix res))
> 
> I'd argue that the better fix is to let-bind `completion-regexp-list` to
> nil around the `minibuffer.el` code which uses
> `try/all/test-completion` to protect against those kinds of misuses.

You are saying that you don't like my proposed patch for the emacs-29
branch, and would even prefer to leave it unfixed?

The binding you propose might be appropriate for master, but not for
the release branch.

> Or alternatively to say "if it hurts, don't do that".

That could be okay, if we also say how to use this variable so it
doesn't hurt.

bug#64351: probabe bug associated with “completion-regexp-list”

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

>> Bad idea.
>> Admittedly, `completion-regexp-list` is documented poorly enough that
>> it's not clear what usage is valid and what isn't, but a lot of
>> completion code will be broken if you set it globally to anything else
>> than nil.
>
> Would you mind telling more about valid uses of this?
> I'd like to improve the documentation.

Here's what I know: `completion-regexp-list` used to be virtually unused
in Emacs's code base, so I don't know what was its intended use.

But when completion UIs started to offer other styles than just
prefix-based completion, they also started to use
`try/all/test-completion` more "internally" (i.e. for intermediate
computations rather than to compute the final list of completions
displayed to the user) and they did it without paying attention to
`completion-regexp-list`, i.e. in ways which tended to just misbehave
when `completion-regexp-list` wasn't nil.

So, in practice nowadays, the only reliable way to use
`completion-regexp-list` is to think of it as an optional argument to
`try/all/test-completion`, but one that has to be provided via a nearby
let-binding rather than by listing it explicitly at the call site.

> You are saying that you don't like my proposed patch for the emacs-29
> branch, and would even prefer to leave it unfixed?

Your patch is harmless, IMO, so it's OK to install it.

> The binding you propose might be appropriate for master, but not for
> the release branch.

Agreed.

> > Or alternatively to say "if it hurts, don't do that".
> That could be okay, if we also say how to use this variable so it
> doesn't hurt.

My opinion is that we should document that variable as something that's
not meant to be used/set by users, but only one that should be set by
the code that calls `try/all/test-completion`.


        Stefan

bug#64351: probabe bug associated with “completion-regexp-list”

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: one.last.kiss@outlook.com,  64351@debbugs.gnu.org
> Date: Sat, 01 Jul 2023 10:19:50 -0400
> 
> > Would you mind telling more about valid uses of this?
> > I'd like to improve the documentation.
> 
> Here's what I know: `completion-regexp-list` used to be virtually unused
> in Emacs's code base, so I don't know what was its intended use.
> 
> But when completion UIs started to offer other styles than just
> prefix-based completion, they also started to use
> `try/all/test-completion` more "internally" (i.e. for intermediate
> computations rather than to compute the final list of completions
> displayed to the user) and they did it without paying attention to
> `completion-regexp-list`, i.e. in ways which tended to just misbehave
> when `completion-regexp-list` wasn't nil.
> 
> So, in practice nowadays, the only reliable way to use
> `completion-regexp-list` is to think of it as an optional argument to
> `try/all/test-completion`, but one that has to be provided via a nearby
> let-binding rather than by listing it explicitly at the call site.

Thanks, I added something along these lines to the documentation.

> > You are saying that you don't like my proposed patch for the emacs-29
> > branch, and would even prefer to leave it unfixed?
> 
> Your patch is harmless, IMO, so it's OK to install it.

Installed on the emacs-29 branch, and closing the bug.