Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

Michael.Albinus@gmx.de (Michael Albinus) writes:

> diff --git a/lisp/net/tramp-crypt.el b/lisp/net/tramp-crypt.el
> index f8de708..278fb9d 100644
> --- a/lisp/net/tramp-crypt.el
> +++ b/lisp/net/tramp-crypt.el
> @@ -112,6 +112,14 @@ initializing a new crypted remote directory."
>    "Non-nil when encryption support is available.")
>  (setq tramp-crypt-enabled (executable-find tramp-crypt-encfs-program))
>  
> +;; This function takes action since Emacs 28.1, when
> +;; `read-extended-command-predicate' is set to
> +;; `command-completion-default-include-p'.
> +(defun tramp-crypt-enabled-p (_symbol _buffer)
> +  "A predicate for Tramp interactive commands.
> +They are completed by \"M-x TAB\" only when encryption support is enabled."
> +  tramp-crypt-enabled)
[...]
> +;; Starting with Emacs 28.1, this can be replaced by the "(declare ...)" form.
> +;;;###tramp-autoload
> +(function-put
> + #'tramp-crypt-add-directory 'completion-predicate #'tramp-crypt-enabled-p)
[...]
> +;; Starting with Emacs 28.1, this can be replaced by the "(declare ...)" form.
> +(function-put
> + #'tramp-crypt-remove-directory 'completion-predicate #'tramp-crypt-enabled-p)

Here, the completion-predicate property is autoloaded, but
tramp-crypt-enabled-p is not, resulting in the following unfortunate
interaction when only tramp.el is loaded:

0. emacs -Q
1. M-x toggle-debug-on-error RET
2. M-x load-library RET tramp RET
3. M-x customize-set-variable RET read-extended-command-predicate RET
4. e TAB RET
5. M-x TAB

  Debugger entered--Lisp error: (void-function tramp-crypt-enabled-p)
    tramp-crypt-enabled-p(tramp-crypt-add-directory #<buffer *scratch*>)
    command-completion-default-include-p(tramp-crypt-add-directory
                                         #<buffer *scratch*>)

My question is, which combination of the following should happen:

- The completion-predicate properties are not autoloaded.
- The function tramp-crypt-enabled-p is autoloaded.
- The function command-completion-default-include-p checks whether
  completion-predicate is functionp.

Thanks,

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Stefan Monnier]

> My question is, which combination of the following should happen:
> - The completion-predicate properties are not autoloaded.

This means that autoloaded functions will always be listed in `M-x`
(except after loading the corresponding package), which is undesirable.

> - The function tramp-crypt-enabled-p is autoloaded.

Sounds good.

> - The function command-completion-default-include-p checks whether
>   completion-predicate is functionp.

I think this is *also* needed, tho a better option is
`with-demoted-errors` since no matter what error is signal'd we don't
want it to prevent `M-x` from doing its job.


        Stefan

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Michael Albinus]

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

Hi Stefan & Basil,

>> My question is, which combination of the following should happen:
>> - The completion-predicate properties are not autoloaded.
>
> This means that autoloaded functions will always be listed in `M-x`
> (except after loading the corresponding package), which is undesirable.

Calling `tramp-crypt-add-directory' interactively is the only way to
activate package tramp-crypt.el. As long as the package isn't loaded
yet, this command must be visible.

>> - The function tramp-crypt-enabled-p is autoloaded.
>
> Sounds good.

No, because otherwise tramp-crypt.el would be loaded unconditionally due
to the command completion of `tramp-crypt-enabled-p'.

>> - The function command-completion-default-include-p checks whether
>>   completion-predicate is functionp.
>
> I think this is *also* needed, tho a better option is
> `with-demoted-errors` since no matter what error is signal'd we don't
> want it to prevent `M-x` from doing its job.

Yes.

I have adapted tramp-crypt.el such a way, that `completion-predicate'
for `tramp-crypt-add-directory' is set only after loading tramp-crypt.el.

>         Stefan

Best regards, Michael.

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

Michael Albinus <michael.albinus@gmx.de> writes:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> - The function command-completion-default-include-p checks whether
>>>   completion-predicate is functionp.
>> I think this is *also* needed, tho a better option is
>> `with-demoted-errors` since no matter what error is signal'd we don't
>> want it to prevent `M-x` from doing its job.
> Yes.

Like so?  Or do we want the safety net higher up, e.g. in
read-extended-command or its callers?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Demote-completion-predicate-errors.patch --]
[-- Type: text/x-diff, Size: 1271 bytes --]

From 41cbd533d7fb770ef3ce8a8a88180f9e0e1ecc54 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sun, 14 Mar 2021 12:52:31 +0000
Subject: [PATCH] Demote completion-predicate errors

For discussion, see the following thread:
https://lists.gnu.org/r/emacs-devel/2021-03/msg00682.html

* lisp/simple.el (command-completion-default-include-p): Demote
errors when calling completion-predicate so M-x doesn't break.
---
 lisp/simple.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 98fccf4ff2..295c01e21f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2025,7 +2025,12 @@ command-completion-default-include-p
 BUFFER."
   (if (get symbol 'completion-predicate)
       ;; An explicit completion predicate takes precedence.
-      (funcall (get symbol 'completion-predicate) symbol buffer)
+      ;; Demote any errors so M-x continues to work.
+      (condition-case-unless-debug err
+          (funcall (get symbol 'completion-predicate) symbol buffer)
+        (error
+         (message "command-completion-default-include-p: %s: %S" symbol err)
+         nil))
     (or (null (command-modes symbol))
         (command-completion-using-modes-p symbol buffer))))
 
-- 
2.30.1


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


> I have adapted tramp-crypt.el such a way, that `completion-predicate'
> for `tramp-crypt-add-directory' is set only after loading tramp-crypt.el.

Thanks,

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Stefan Monnier]

>>> My question is, which combination of the following should happen:
>>> - The completion-predicate properties are not autoloaded.
>> This means that autoloaded functions will always be listed in `M-x`
>> (except after loading the corresponding package), which is undesirable.
> Calling `tramp-crypt-add-directory' interactively is the only way to
> activate package tramp-crypt.el. As long as the package isn't loaded
> yet, this command must be visible.

Presumably, if there's a `completion-predicate` it's because there are
circumstances where it makes no sense to use `M-x
tramp-crypt-add-directory`.  Is it really the case that such situations
can only happen once `tramp-crypto` is loaded?  If so, I'd tend to think
that they are sufficiently marginal that it's not worth having
a `completion-predicate`.


        Stefan

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Stefan Monnier]

> Like so?  Or do we want the safety net higher up, e.g. in
> read-extended-command or its callers?

LGTM, but I think Lars would know better,


        Stefan

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Michael Albinus]

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

Hi Stefan,

>>>> My question is, which combination of the following should happen:
>>>> - The completion-predicate properties are not autoloaded.
>>> This means that autoloaded functions will always be listed in `M-x`
>>> (except after loading the corresponding package), which is undesirable.
>> Calling `tramp-crypt-add-directory' interactively is the only way to
>> activate package tramp-crypt.el. As long as the package isn't loaded
>> yet, this command must be visible.
>
> Presumably, if there's a `completion-predicate` it's because there are
> circumstances where it makes no sense to use `M-x
> tramp-crypt-add-directory`.  Is it really the case that such situations
> can only happen once `tramp-crypto` is loaded?

Yes. For example, during load of tramp-crypt.el it is checked, whether
the encfs program is available.

> If so, I'd tend to think that they are sufficiently marginal that it's
> not worth having a `completion-predicate`.

Of course, we can live w/o that predicate. We did until now. But OTOH,
it doesn't hurt to check this, that's why this kind of check has been
added to Emacs.

>         Stefan

Best regards, Michael.

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Stefan Monnier]

> Of course, we can live w/o that predicate. We did until now. But OTOH,
> it doesn't hurt to check this, that's why this kind of check has been
> added to Emacs.

What I mean is that this is a predicate which should be most useful to
those who never use `tramp-crypt` (e.g. because `encfs` is not
installed), so only activating it when `tramp-crypt` is loaded makes it
*much* less useful.

This predicate is only useful for those users who don't have
`encfs` installed.  Which proportion of those users do you think has
`tramp-crypt` loaded in their Emacs session?  My guess is that this is
pretty close to 0%.


        Stefan

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Michael Albinus]

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

Hi Stefan,

>> Of course, we can live w/o that predicate. We did until now. But OTOH,
>> it doesn't hurt to check this, that's why this kind of check has been
>> added to Emacs.
>
> What I mean is that this is a predicate which should be most useful to
> those who never use `tramp-crypt` (e.g. because `encfs` is not
> installed), so only activating it when `tramp-crypt` is loaded makes it
> *much* less useful.

I don't want to activate the predicate before tramp-crypt.el is loaded,
because this would counteract the idea to keep tramp-crypt related code
in tramp-crypt.el.

> This predicate is only useful for those users who don't have
> `encfs` installed.  Which proportion of those users do you think has
> `tramp-crypt` loaded in their Emacs session?  My guess is that this is
> pretty close to 0%.

Well, it is also useful for users who have tried
tramp-crypt-add-directory, and see afterwards that it doesn't work for
them. But yes, this is a small number of users.

OTOH, it doesn't hurt. I don't see any price we have to pay for this
predicate, in terms of performance or whatsoever. People who don't use
tramp-crypt.el are not affected in any way.

And I have extended the predicate just now to be more useful for the
other command in tramp-crypt.el, tramp-crypt-remove-directory.

>         Stefan

Best regards, Michael.

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

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

>> Like so?  Or do we want the safety net higher up, e.g. in
>> read-extended-command or its callers?
>
> LGTM, but I think Lars would know better,

Lars, any thoughts on how best to protect M-x from errors in
completion-predicate?  Is the following okay?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Demote-completion-predicate-errors.patch --]
[-- Type: text/x-diff, Size: 1271 bytes --]

From 41cbd533d7fb770ef3ce8a8a88180f9e0e1ecc54 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sun, 14 Mar 2021 12:52:31 +0000
Subject: [PATCH] Demote completion-predicate errors

For discussion, see the following thread:
https://lists.gnu.org/r/emacs-devel/2021-03/msg00682.html

* lisp/simple.el (command-completion-default-include-p): Demote
errors when calling completion-predicate so M-x doesn't break.
---
 lisp/simple.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 98fccf4ff2..295c01e21f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2025,7 +2025,12 @@ command-completion-default-include-p
 BUFFER."
   (if (get symbol 'completion-predicate)
       ;; An explicit completion predicate takes precedence.
-      (funcall (get symbol 'completion-predicate) symbol buffer)
+      ;; Demote any errors so M-x continues to work.
+      (condition-case-unless-debug err
+          (funcall (get symbol 'completion-predicate) symbol buffer)
+        (error
+         (message "command-completion-default-include-p: %s: %S" symbol err)
+         nil))
     (or (null (command-modes symbol))
         (command-completion-using-modes-p symbol buffer))))
 
-- 
2.30.1


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


Thanks,

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Michael Albinus]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

Hi Basil,

> +      ;; Demote any errors so M-x continues to work.
> +      (condition-case-unless-debug err
> +          (funcall (get symbol 'completion-predicate) symbol buffer)
> +        (error
> +         (message "command-completion-default-include-p: %s: %S" symbol err)

I would also report buffer. Otherwise, it LGTM (but I'm not Lars :-)

> Thanks,

Best regards, Michael.

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Lars, any thoughts on how best to protect M-x from errors in
> completion-predicate?  Is the following okay?

[...]

> +      (condition-case-unless-debug err
> +          (funcall (get symbol 'completion-predicate) symbol buffer)
> +        (error
> +         (message "command-completion-default-include-p: %s: %S" symbol err)
> +         nil))

What's the performance impact here?  This is going to be called a whole
bunch of times when the user hits TAB...

I'm also not quite sure why we should be ignoring these errors --
they're code errors, like any others, and shouldn't be expected to
fail, should they?

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

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Stefan Monnier]

> What's the performance impact here?  This is going to be called a whole
> bunch of times when the user hits TAB...

It should be OK to do the condition-case wrapping around the loop rather
than within it, so the performance impact should be negligible.

> I'm also not quite sure why we should be ignoring these errors --
> they're code errors, like any others, and shouldn't be expected to
> fail, should they?

The purpose is to make sure people can still `M-x` even in the presence
of such errors.  They *should* never happen, but an error there shouldn't
render Emacs unusable ;-)


        Stefan

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Lars Ingebrigtsen]

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

>> What's the performance impact here?  This is going to be called a whole
>> bunch of times when the user hits TAB...
>
> It should be OK to do the condition-case wrapping around the loop rather
> than within it, so the performance impact should be negligible.

Yup; if it's hoisted out of the loop, there'd be no performance impact,
but it would make it less useful, though -- as with erroring out, you'd
get no completion.  With it in the loop, you'd just ignore the specific
predicate that fails, I think?

>> I'm also not quite sure why we should be ignoring these errors --
>> they're code errors, like any others, and shouldn't be expected to
>> fail, should they?
>
> The purpose is to make sure people can still `M-x` even in the presence
> of such errors.  They *should* never happen, but an error there shouldn't
> render Emacs unusable ;-)

I'm all for adding resiliency to central parts of Emacs, so it's good in
that sense.  But we don't, in general, ignore coding errors like this, I
think?  For instance, if there's an error when loading an .el file, we
bug out -- we don't catch it and carry on.

So I don't oppose catching errors here, really -- I'm just wondering why
we're designing for allowing broken code in this particular
circumstance.  Do we expect these code snippets to break for some
reason?

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

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Stefan Monnier]

> Yup; if it's hoisted out of the loop, there'd be no performance impact,
> but it would make it less useful, though -- as with erroring out, you'd
> get no completion.

It'd make more sense to just keep the current (not-filtered) completion
list, so you still get completion to work.

> So I don't oppose catching errors here, really -- I'm just wondering why
> we're designing for allowing broken code in this particular
> circumstance.  Do we expect these code snippets to break for some
> reason?

I think it's because such errors have already occurred, and not being
able to `M-x` just because of a silly mistake in a "secondary" feature
is rather frustrating.
Also, this feature runs code from "all" packages, so it is inherently
exposed to a lot of potential messups.

IOW, I agree that there's no hard-and-fast reason to do that, but
I think it can make a substantial difference to the end users because
it protects them from very minor but potentially common bugs.


        Stefan

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Michael Albinus]

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

>> So I don't oppose catching errors here, really -- I'm just wondering why
>> we're designing for allowing broken code in this particular
>> circumstance.  Do we expect these code snippets to break for some
>> reason?
>
> I think it's because such errors have already occurred, and not being
> able to `M-x` just because of a silly mistake in a "secondary" feature
> is rather frustrating.
> Also, this feature runs code from "all" packages, so it is inherently
> exposed to a lot of potential messups.
>
> IOW, I agree that there's no hard-and-fast reason to do that, but
> I think it can make a substantial difference to the end users because
> it protects them from very minor but potentially common bugs.

IIUC, such errors can happen only, if command completion happens while
`read-extended-command-predicate' is a non-nil predicate. So we could give
the user an error message, which recommends

- to set `read-extended-command-predicate' to nil, and
- to write a bug report.

>         Stefan

Best regards, Michael.

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

Michael Albinus <michael.albinus@gmx.de> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> +      ;; Demote any errors so M-x continues to work.
>> +      (condition-case-unless-debug err
>> +          (funcall (get symbol 'completion-predicate) symbol buffer)
>> +        (error
>> +         (message "command-completion-default-include-p: %s: %S" symbol err)
>
> I would also report buffer.

That's what I had initially, but I found it a bit noisy since the buffer
is usually the current one (at least in the read-extended-command case
for sure).

> Otherwise, it LGTM (but I'm not Lars :-)

Thanks,

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Michael Albinus]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>> I would also report buffer.
>
> That's what I had initially, but I found it a bit noisy since the buffer
> is usually the current one (at least in the read-extended-command case
> for sure).

Good point.

> Thanks,

Best regards, Michael.

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> Lars, any thoughts on how best to protect M-x from errors in
>> completion-predicate?  Is the following okay?
>
> [...]
>
>> +      (condition-case-unless-debug err
>> +          (funcall (get symbol 'completion-predicate) symbol buffer)
>> +        (error
>> +         (message "command-completion-default-include-p: %s: %S" symbol err)
>> +         nil))
>
> What's the performance impact here?  This is going to be called a whole
> bunch of times when the user hits TAB...

Compared to everything else going on around this, negligible.

Running the attached benchmark with

  emacs -Q -batch -f batch-byte-compile bench.el
  emacs -Q -l bench.elc

gives the following excerpts of timings:

  TAB RET
  bench-pred-expensive 1.804303 36 0.564483
  bench-pred-expensive 1.790578 35 0.551621
  bench-pred-expensive 1.777639 35 0.549471
  bench-pred-none      0.982336 35 0.550554
  bench-pred-none      0.980999 35 0.549281
  bench-pred-none      0.982774 35 0.550178
  bench-pred-old       0.834787 29 0.452026
  bench-pred-old       0.831663 29 0.450019
  bench-pred-old       0.830596 29 0.450292
  bench-pred-new       0.831019 29 0.450161
  bench-pred-new       0.831628 29 0.451437
  bench-pred-new       0.835570 29 0.452512

  e w w - TAB C-a C-k RET
  bench-pred-expensive 1.080096 30 0.413564
  bench-pred-expensive 1.078444 30 0.413685
  bench-pred-expensive 1.077938 30 0.414732
  bench-pred-none      1.003878 30 0.416143
  bench-pred-none      0.987872 30 0.413814
  bench-pred-none      0.991742 30 0.416257
  bench-pred-old       0.889184 25 0.343545
  bench-pred-old       0.889369 25 0.344027
  bench-pred-old       0.885496 25 0.343227
  bench-pred-new       0.901361 25 0.345625
  bench-pred-new       0.887233 25 0.344885
  bench-pred-new       0.889201 25 0.344515

Where bench-pred-new extends bench-pred-old with the suggested
condition-case-unless-debug in the "tight" loop.

> I'm also not quite sure why we should be ignoring these errors --
> they're code errors, like any others, and shouldn't be expected to
> fail, should they?

No, but code rarely listens to expectations ;).

Of course this use case isn't as critical/irreversible as
post-command-hook or process filters/sentinels, but the general notion
is similar: it would be nice if code errors didn't bork M-x completion.

-- 
Basil


[-- Attachment #2: bench.el --]
[-- Type: application/emacs-lisp, Size: 2186 bytes --]

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

>> What's the performance impact here?  This is going to be called a whole
>> bunch of times when the user hits TAB...
>
> It should be OK to do the condition-case wrapping around the loop rather
> than within it, so the performance impact should be negligible.

"Around the loop" means "around completing-read", and I'm not sure we
want to catch all its errors.  It makes sense to me to protect
read-extended-command-predicate (or at least its specific instance of
command-completion-default-include-p), as that can be polluted by user
and external library code.  But other errors would be a bug in M-x, no?

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

>> Yup; if it's hoisted out of the loop, there'd be no performance impact,
>> but it would make it less useful, though -- as with erroring out, you'd
>> get no completion.
>
> It'd make more sense to just keep the current (not-filtered) completion
> list, so you still get completion to work.

If we stick with the original patch, then that would mean returning
non-nil rather than nil if completion-predicate fails, right?

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> What's the performance impact here?  This is going to be called a whole
>>> bunch of times when the user hits TAB...
>>
>> It should be OK to do the condition-case wrapping around the loop rather
>> than within it, so the performance impact should be negligible.
>
> Yup; if it's hoisted out of the loop, there'd be no performance impact,

I see no discernible performance impact even if the
condition-case-unless-debug is added into the completing-read predicate,
to catch errors from user-provided read-extended-command-predicate
values.

Running the new attached benchmark with:

  emacs -Q -batch -f batch-byte-compile bench2.el
  emacs -Q -l bench2.elc

gives the following timing excerpts:

  TAB RET

  read-extended-command
    bench--expensive                     1.801461 36 0.568592
    nil                                  0.985272 35 0.552688
    command-completion-default-include-p 0.837255 29 0.453390
    bench--safe                          0.834056 29 0.452686
  bench-read-extended-command
    bench--expensive                     1.803410 36 0.566920
    nil                                  0.977062 35 0.549054
    command-completion-default-include-p 0.831130 29 0.452036
    bench--safe                          0.831380 29 0.451495

  e w w - TAB C-a C-k RET

  read-extended-command
    bench--expensive                     1.082884 30 0.415545
    nil                                  0.992358 30 0.417027
    command-completion-default-include-p 0.895481 25 0.343474
    bench--safe                          0.886560 25 0.344851
  bench-read-extended-command
    bench--expensive                     1.081067 30 0.414472
    nil                                  0.990662 30 0.415502
    command-completion-default-include-p 0.886688 25 0.343094
    bench--safe                          0.898391 25 0.344109

So back to the original question: do we want this safety net in
command-completion-default-include-p, around
read-extended-command-predicate (which is user-customisable), or not at
all?

And if we do add it, should it include a helpful message as per
Michael's suggestion?

I'm happy either way.

-- 
Basil


[-- Attachment #2: bench2.el --]
[-- Type: application/emacs-lisp, Size: 4919 bytes --]

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Stefan Monnier]

>>> What's the performance impact here?  This is going to be called a whole
>>> bunch of times when the user hits TAB...
>> It should be OK to do the condition-case wrapping around the loop rather
>> than within it, so the performance impact should be negligible.
> "Around the loop" means "around completing-read",

Not necessarily, e.g.


        Stefan


diff --git a/lisp/simple.el b/lisp/simple.el
index 959bd83117..91069aa02a 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1982,7 +1982,8 @@ read-extended-command
        (concat (cond
 	        ((eq current-prefix-arg '-) "- ")
 	        ((and (consp current-prefix-arg)
-		      (eq (car current-prefix-arg) 4)) "C-u ")
+		      (eq (car current-prefix-arg) 4))
+		 "C-u ")
 	        ((and (consp current-prefix-arg)
 		      (integerp (car current-prefix-arg)))
 	         (format "%d " (car current-prefix-arg)))
@@ -1998,11 +1999,12 @@ read-extended-command
 	           "M-X "
 	         "M-x "))
        (lambda (string pred action)
-         (if (and suggest-key-bindings (eq action 'metadata))
-	     '(metadata
-	       (affixation-function . read-extended-command--affixation)
-	       (category . command))
-           (complete-with-action action obarray string pred)))
+         (with-demoted-errors "Completion error: %S"
+           (if (and suggest-key-bindings (eq action 'metadata))
+	       '(metadata
+	         (affixation-function . read-extended-command--affixation)
+	         (category . command))
+             (complete-with-action action obarray string pred))))
        (lambda (sym)
          (and (commandp sym)
               (or (null read-extended-command-predicate)

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Stefan Monnier]

>>> Yup; if it's hoisted out of the loop, there'd be no performance impact,
>>> but it would make it less useful, though -- as with erroring out, you'd
>>> get no completion.
>>
>> It'd make more sense to just keep the current (not-filtered) completion
>> list, so you still get completion to work.
>
> If we stick with the original patch, then that would mean returning
> non-nil rather than nil if completion-predicate fails, right?

I don't think whether we include the command for which the predicate
burped is important: either way is fine.  OTOH, it would be preferable
if a burping predicate wouldn't prevent the inclusion of other commands
in the completion list.  It's probably tolerable to just prevent
completion altogether in case of an error, but not to prevent
`M-x` altogether.


        Stefan

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

>>>> What's the performance impact here?  This is going to be called a whole
>>>> bunch of times when the user hits TAB...
>>> It should be OK to do the condition-case wrapping around the loop rather
>>> than within it, so the performance impact should be negligible.
>> "Around the loop" means "around completing-read",
>
> Not necessarily, e.g.

> @@ -1998,11 +1999,12 @@ read-extended-command
>  	           "M-X "
>  	         "M-x "))
>         (lambda (string pred action)
> -         (if (and suggest-key-bindings (eq action 'metadata))
> -	     '(metadata
> -	       (affixation-function . read-extended-command--affixation)
> -	       (category . command))
> -           (complete-with-action action obarray string pred)))
> +         (with-demoted-errors "Completion error: %S"
> +           (if (and suggest-key-bindings (eq action 'metadata))
> +	       '(metadata
> +	         (affixation-function . read-extended-command--affixation)
> +	         (category . command))
> +             (complete-with-action action obarray string pred))))

Oops, indeed.

But as we've agreed elsewhere, leaving the completion table empty is not
ideal.

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

>>>> Yup; if it's hoisted out of the loop, there'd be no performance impact,
>>>> but it would make it less useful, though -- as with erroring out, you'd
>>>> get no completion.
>>>
>>> It'd make more sense to just keep the current (not-filtered) completion
>>> list, so you still get completion to work.
>>
>> If we stick with the original patch, then that would mean returning
>> non-nil rather than nil if completion-predicate fails, right?
>
> I don't think whether we include the command for which the predicate
> burped is important: either way is fine.

I think including it is slightly better: say 5x5 has a
completion-predicate that burps.  If this excludes it from completions,
then typing M-x 5x5 RET will actually invoke 5x5-crack, which I think is
undesirable.

> OTOH, it would be preferable if a burping predicate wouldn't prevent
> the inclusion of other commands in the completion list.  It's probably
> tolerable to just prevent completion altogether in case of an error,
> but not to prevent `M-x` altogether.

I therefore propose we put the safety net around the call to
read-extended-command-predicate, with burps having the same effect as if
read-extended-command-predicate were nil.

This allows both M-x and M-x completion to continue to work, has no
discernible performance cost AFAICT, and does not invoke unintended
commands.

The only downside I can see is that the error message becomes visible in
the echo area only after the user has entered some text which matches
the burping command (otherwise the error message just gets printed to
*Messages*).  But that's fine AFAIC.

WDYT?

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

Michael Albinus <michael.albinus@gmx.de> writes:

> IIUC, such errors can happen only, if command completion happens while
> `read-extended-command-predicate' is a non-nil predicate. So we could give
> the user an error message, which recommends
>
> - to set `read-extended-command-predicate' to nil, and
> - to write a bug report.

If this is deemed too verbose for 'message', we could perhaps use
'lwarn' instead.

Thanks,

-- 
Basil

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>   e w w - TAB C-a C-k RET
>   bench-pred-expensive 1.080096 30 0.413564
>   bench-pred-expensive 1.078444 30 0.413685
>   bench-pred-expensive 1.077938 30 0.414732
>   bench-pred-none      1.003878 30 0.416143
>   bench-pred-none      0.987872 30 0.413814
>   bench-pred-none      0.991742 30 0.416257
>   bench-pred-old       0.889184 25 0.343545
>   bench-pred-old       0.889369 25 0.344027
>   bench-pred-old       0.885496 25 0.343227
>   bench-pred-new       0.901361 25 0.345625
>   bench-pred-new       0.887233 25 0.344885
>   bench-pred-new       0.889201 25 0.344515
>
> Where bench-pred-new extends bench-pred-old with the suggested
> condition-case-unless-debug in the "tight" loop.

Hm...  I'm not quite sure I understand these benchmarks.  There are no
completion predicates for any eww-* symbols, I think?  So there
shouldn't be any differences here at all, if I read the patch correctly.

What I was wondering was -- if (at some point in the future) we have,
say, 2K symbols with a completion predicate -- would putting that
condition-case inside the loop make things slower or not?  My gut
feeling says that it's probably not an issue (so we should just go ahead
and do it), but it'd be nice to know...

I just did a simple comparison of

(defun foo1 ()
  (+ 1 2))

(defun foo2 ()
  (condition-case ()
      (+ 1 2)
    (error nil)))

and the latter is 25% slower...  But:

(benchmark-run 10000000 (foo2))
=> (0.42124799 0 0.0)

So ten million condition-cases doesn't take a lot of time, so it's fine
to have it in the completion loop here.

> Of course this use case isn't as critical/irreversible as
> post-command-hook or process filters/sentinels, but the general notion
> is similar: it would be nice if code errors didn't bork M-x completion.

Yup; true.

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

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Lars Ingebrigtsen]

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

> Also, this feature runs code from "all" packages, so it is inherently
> exposed to a lot of potential messups.

Yes, that's a good point.

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

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  I'm not quite sure I understand these benchmarks.  There are no
> completion predicates for any eww-* symbols, I think?  So there
> shouldn't be any differences here at all, if I read the patch correctly.

Oops, sorry, I conflated completion-predicate with command-modes.

The eww completion is only one half of the benchmark; the rest operates
over all commands in obarray (but less than 1% of those have a
completion-predicate, hence the (failed) attempt to prefilter obarray).

> What I was wondering was -- if (at some point in the future) we have,
> say, 2K symbols with a completion predicate -- would putting that
> condition-case inside the loop make things slower or not?  My gut
> feeling says that it's probably not an issue (so we should just go ahead
> and do it), but it'd be nice to know...

Here's the attached benchmark on 16K commands (warning: takes a while):

  == TAB RET ==

  bench-vanilla-no-pred    3.533359 100 2.058253
  bench-vanilla-no-pred    3.531937 100 2.054076
  bench-vanilla-no-pred    3.529376 100 2.052615
  bench-vanilla-with-pred  3.627937 105 2.127567
  bench-vanilla-with-pred  3.616596 105 2.117001
  bench-vanilla-with-pred  3.612720 105 2.114463
  bench-vanilla-safe-pred  3.639222 105 2.132278
  bench-vanilla-safe-pred  3.647615 105 2.134284
  bench-vanilla-safe-pred  3.636989 105 2.127953
  bench-safe-no-pred       3.573388 100 2.087055
  bench-safe-no-pred       3.555347 100 2.069622
  bench-safe-no-pred       3.545747 100 2.063041
  bench-safe-with-pred     3.653066 105 2.145138
  bench-safe-with-pred     3.643867 105 2.140400
  bench-safe-with-pred     3.659689 105 2.150498

  == b e n c h - TAB C-a C-k RET ==

  bench-vanilla-no-pred    0.649064  0 0.000000
  bench-vanilla-no-pred    0.652256  0 0.000000
  bench-vanilla-no-pred    0.677194  0 0.000000
  bench-vanilla-with-pred  1.652515 40 0.750009
  bench-vanilla-with-pred  1.653861 40 0.753575
  bench-vanilla-with-pred  1.672873 40 0.747151
  bench-vanilla-safe-pred  1.669653 40 0.742116
  bench-vanilla-safe-pred  1.673809 40 0.743255
  bench-vanilla-safe-pred  1.672750 40 0.741095
  bench-safe-no-pred       0.646114  0 0.000000
  bench-safe-no-pred       0.644843  0 0.000000
  bench-safe-no-pred       0.646764  0 0.000000
  bench-safe-with-pred     1.672326 40 0.743054
  bench-safe-with-pred     1.677331 40 0.742163
  bench-safe-with-pred     1.711009 40 0.759025

So condition-case still looks negligible relative to the rest of the
work, and my vote's for putting it around the call to
read-extended-command-predicate.

-- 
Basil


[-- Attachment #2: bench3.el --]
[-- Type: application/emacs-lisp, Size: 5730 bytes --]

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Here's the attached benchmark on 16K commands (warning: takes a while):

[...]

>   bench-vanilla-with-pred  1.672873 40 0.747151
>   bench-vanilla-safe-pred  1.669653 40 0.742116

Thanks.

> So condition-case still looks negligible relative to the rest of the
> work, and my vote's for putting it around the call to
> read-extended-command-predicate.

Sure; sounds good to me.

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

Re: master 695f679: Remove ; ; ; ###tramp-autoload cookie from Tramp defcustoms (Bug#47063)

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> So condition-case still looks negligible relative to the rest of the
>> work, and my vote's for putting it around the call to
>> read-extended-command-predicate.
>
> Sure; sounds good to me.

Thanks, done.

Demote read-extended-command-predicate errors
8bf186b229 2021-03-27 20:05:10 +0000
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=8bf186b2297dc0aeba76e8e743dec7efa6a568e4

-- 
Basil