bug#41423: 27.0.91; eshell file completion in tramp dir is slow (3 minutes) [regression on pretest]

[Tim Vaughan]

Just a heads up: I believe I'm experiencing this bug under 27.1. (Before finding this bug report I also did a bisection and found the same culprit commit that Andres did.). Thus I expect it somehow manage to escape pretest.

Additionally, I've noticed that although the effect is severe only in tramp directories, there is a noticeable (~0.5s) pause when executing commands in local directories, provided autocompletion has been used to create the command string. (The exact same command does not seem to cause the delay when typed in without invoking autocompletion.)

I'm not at all familiar with the eshell code, but I spent some time trying to identify the cause. Weirdly the behaviour does not seem to occur when I instrument `eshell-send-input', making it a bit of a heisenbug.

Tim

[Gregory Heytings]

The root of this bug is that `eshell-complete-commands-list' loops through
all executables *on the remote server* with (while comps-in-path ...).
That means typically 1000-2000 commands to check, one by one, hence the 3
minutes delay.

The easy fix is to hit C-g, which stops this loop.

[Gregory Heytings]

Another note: in fact this bug exists because
`eshell-complete-commands-list' is, in this context, called in Emacs 27,
but not in Emacs 26 and earlier.

The backtrace is:

* eshell-complete-commands-list()
#f(compiled-function () #<bytecode 0x1e0009b1b5c5>)()
pcomplete--here(#f(compiled-function () #<bytecode 0x1e0009b1b5c5>) nil nil nil)
#f(compiled-function () #<bytecode 0x1fff46e348cd94>)()
pcomplete-completions()
pcomplete-completions-at-point()
#f(compiled-function () #<bytecode 0xcbc6f2b2c706bdb>)()
completion-in-region--postch()

In Emacs 26 `pcomplete--here' is called only once, with
`pcomplete-entries' and `file-directory-p' in its `form' argument, and
`pcomplete-completions' returns.

In Emacs 27 `pcomplete--here' is called twice with these arguments, and a
third time with `eshell-complete-commands-list' as its `form' argument.
It is this third call which takes about three minutes to complete.

[Gregory Heytings]

A last note: this bug exists because in Emacs 27 eshell uses
`pcomplete-completions-at-point': TAB is bound to `completion-at-point'
and `completion-at-point-functions' is `(pcomplete-completions-at-point
t)'.

In Emacs 26 eshell used the (now obsolete) `pcomplete' function: TAB was
bound to `eshell-pcomplete', which was defined as follows:

(defun eshell-pcomplete (&optional interactively)
"Eshell wrapper for `pcomplete'."
(interactive "p")
(setq this-command 'pcomplete)
(condition-case nil
(if interactively
(call-interactively 'pcomplete)
(pcomplete))
(text-read-only (completion-at-point))))

IOW, pcomplete-completions-at-point was called only if `pcomplete' failed,
and is now called by default. (`completion-at-point-functions' was set to
`(pcomplete-completions-at-point t)', as in Emacs 27.)

A simple fix is to eval

(setq completion-at-point-functions '(pcomplete t))

after starting eshell (or to put this in one of the eshell hooks), which
will restore the previous default behavior.

I have no idea how `pcomplete-completions-at-point' should be adapted to
avoid this bug.

[Gregory Heytings]

Apparently my previous last note was not the last one ;-)

I still don't know how this bug should be fixed (except by using (setq
completion-at-point-functions '(pcomplete t))), but here is a more
detailed explanation of what is happening, at least how I understand it:

1. TAB calls completion-at-point
2. completion-at-point calls pcomplete-completions-at-point, which calls pcomplete/cd; this completes the directory name
3. completion-at-point let-binds completion-in-region-mode-predicate to a lambda, which contains pcomplete-completion-at-point
4. completion-at-point calls completion-in-region, which adds completion-in-region--postch to post-command-hook
5. post-command-hook calls completion-in-region--postch
6. completion-in-region--postch funcalls completion-in-region-mode-predicate
7. this calls pcomplete-completions-at-point a second time, which again calls pcomplete/cd (and adds a '/' after the directory name (?))
8. RET is pressed
9. post-command-hook still contains completion-in-region--postch: it is called again
10. completion-in-region--postch funcalls completion-in-region-mode-predicate again
11. this calls pcomplete-completions-at-point a third (!) time
12. at this point pcomplete-completions-at-point considers (for some reason, possibly because the last character of the input is '/' (?)) that it is a command that it must now complete
13. therefore instead of calling pcomplete/cd a third time, pcomplete-completions-at-point now calls eshell-complete-commands-list
14. this loops through all possible command names (on the local or remote host)
15. if all command names had had a common prefix, that prefix would have been inserted now, but this is not the case, so the effect of this loop (apart from a waste of time) is nil

The fact that default-directory is remote is not important here, the exact
same steps take place when it is local, except that step 15 is executed
much faster.

At step 15 it is possible to interrupt the loop with C-g. At step 8 it is
possible to remove completion-in-region--postch from post-command-hook for
example by switching buffers with C-x C-b.

Why such a overly complicated mechanism is used, or what should be done to
avoid this behavior, is beyond my understanding.

[Michael Albinus]

Gregory Heytings <g...@sdf.org> writes:

Hi,

I don't know the completion machinery, so I'm adding Stefan who might
know better.

However, the remote case could be improved. Tramp uses caches. They
expire after a while (10 seconds per default), but this might be
improved. The appended patch disables Tramp cache expiry while being in
eshell-complete-commands-list, so completion might be faster once the
cache has been filled. Could you pls check?

Best regards, Michael.

[Gregory Heytings]

Hi Michael,

>
> However, the remote case could be improved. Tramp uses caches. They
> expire after a while (10 seconds per default), but this might be
> improved. The appended patch disables Tramp cache expiry while being in
> eshell-complete-commands-list, so completion might be faster once the
> cache has been filled. Could you pls check?
>

I just checked, and see no visible improvement.

That being said, I don't think this bug should be fixed on the eshell
level. It's a bug in the completion mechanisms, and
`eshell-complete-commands-list' should simply not be called here.

Gregory

[Stefan Monnier]

>> 1. TAB calls completion-at-point
>> 2. completion-at-point calls pcomplete-completions-at-point, which
>> calls pcomplete/cd; this completes the directory name

Sounds like a bug: the `completion-at-point-functions` should not
perform the completion but should only return a description of the part
of the text that is the subject of completion, along with a description
(generally in the form of a function) of the set of elements from which
the possible completions can be taken.


Stefan

[Gregory Heytings]

Please don't take my words too literally. "This completes the directory
name" does not mean that `pcomplete/cd' performs the completion itself.
Indeed it returns something with which the completion is performed.

The bug is clearly not there (at step 2), but later (in the fact that
`completion-at-point' calls `pcomplete-completions-at-point' three times).

[Michael Albinus]

Gregory Heytings <g...@sdf.org> writes:

> Hi Michael,

Hi Gregory,

>> However, the remote case could be improved. Tramp uses caches. They
>> expire after a while (10 seconds per default), but this might be
>> improved. The appended patch disables Tramp cache expiry while being
>> in eshell-complete-commands-list, so completion might be faster once
>> the cache has been filled. Could you pls check?
>>
>
> I just checked, and see no visible improvement.

The first time you try completion, there's no difference. The cache must
be filled. But all next times you complete, it shall be faster for
remote directoriues.

> That being said, I don't think this bug should be fixed on the eshell
> level. It's a bug in the completion mechanisms, and
> `eshell-complete-commands-list' should simply not be called here.

I do not claim my patch is the solution. But it shall be useful, if
eshell-complete-commands-list is called somewhere.

> Gregory

Best regards, Michael.

[Gregory Heytings]

>
>> I just checked, and see no visible improvement.
>
> The first time you try completion, there's no difference. The cache must
> be filled. But all next times you complete, it shall be faster for
> remote directoriues.
>

Yes, this is what I did. The first time there was no difference. The
second time I did not see any difference either, alas. I just tried it
again.

>
> I do not claim my patch is the solution. But it shall be useful, if
> eshell-complete-commands-list is called somewhere.
>

A much better patch at this point is to just restore the previous default
behavior (in Emacs 26 and earlier) by setting
completion-at-point-functions to '(pcomplete t) instead of
'(pcomplete-completions-at-point t), for example with:

(add-hook 'eshell-mode-hook (function (lambda () (setq completion-at-point-functions '(pcomplete t)))))

pcomplete is obsolete, but it is still there, and it works.

[Michael Albinus]

Gregory Heytings <g...@sdf.org> writes:

> A much better patch at this point is to just restore the previous
> default behavior (in Emacs 26 and earlier) by setting
> completion-at-point-functions to '(pcomplete t) instead of
> '(pcomplete-completions-at-point t), for example with:
>
> (add-hook 'eshell-mode-hook (function (lambda () (setq
> completion-at-point-functions '(pcomplete t)))))
>
> pcomplete is obsolete, but it is still there, and it works.

I guess Stefan had a reason when he applied
047c1b19353ff58d8cd45935c7b44c911b70e312 last year. Let's see his
analysis.

Best regards, Michael.

[Stefan Monnier]

> Please don't take my words too literally. "This completes the directory
> name" does not mean that `pcomplete/cd' performs the completion
> itself. Indeed it returns something with which the completion is performed.

Hmm... any hope you could refine your description accordingly?

> The bug is clearly not there (at step 2), but later (in the fact that
> `completion-at-point' calls `pcomplete-completions-at-point' three times).

Calling `pcomplete-completions-at-point` several times is not
necessarily a problem. E.g. it's normal to call it a second time after
completion to check whether we're still in the same completion area (in
order to detect when completion is "finished").


Stefan

[Gregory Heytings]

>
> Hmm... any hope you could refine your description accordingly?
>

I do not understand why I should explain to you how the code you wrote
works. Anyway, here we go (and I fear you will now tell me that my
description is now too refined):

1. start emacs -Q
2. in an eshell buffer, type "<command> <first letters of a directory name> TAB" (command can be "cd", "ls", "rm", ...)
3. TAB calls completion-at-point
4. completion-at-point looks at completion-at-point-functions, whose value is (pcomplete-completions-at-point t), and calls pcomplete-completions-at-point
5. pcomplete-completions-at-point calls pcomplete-completions, which calls pcomplete/cd, which calls pcomplete--here, ...
6. pcomplete-completions returns a collection of completion candidates, and pcomplete-completions-at-point returns that collection together with a function pointer (to pcomplete-completions-at-point itself), a start position, an end position, and a property list
7. we are now back in completion-at-point, and enter the second case in its pcase
8. completion-at-point let-binds completion-in-region-mode-predicate to a lambda, which calls pcomplete-completion-at-point
9. completion-at-point then calls completion-in-region, which calls completion--in-region
10. completion--in-region enters completion-in-region-mode, which adds completion-in-region--postch to post-command-hook
11. completion--in-region calls completion--in-region-1, which calls completion--do-completion, which finally does the actual completion based on the collection of completion candidates returned at step 6
12. at this point the eshell buffer contains the completed directory name, with a trailing slash
13. completion-at-point is now finished, and post-command-hook is executed
14. post-command-hook calls completion-in-region--postch
15. completion-in-region--postch calls completion-in-region-mode-predicate (in fact, completion-in-region-mode--predicate which has been set to completion-in-region-mode-predicate when entering completion-in-region-mode at step 10)
16. this calls pcomplete-completions-at-point a second time, which calls pcomplete-completions, which calls pcomplete/cd, which calls pcomplete--here, ...
17. pcomplete/cd and pcomplete-completions return the exact same values as in step 6
18. pcomplete-completions-at-point returns almost the same value as in step 6 (the only difference is the value of the end position)
19. given that the value of the start position did not change, the lambda let-bound at step 8 returns t, and therefore completion-in-region--postch exits completion-in-region-mode entered at step 10
20. completion-in-region--postch is now finished, it did not change anything in the eshell buffer
21. RET is pressed
22. post-command-hook is executed, and still contains completion-in-region--postch, so it is called again
23. completion-in-region--postch calls completion-in-region-mode-predicate
24. this calls pcomplete-completions-at-point a third time, which calls pcomplete-completions
25. for some reason, pcomplete-completions considers that it must now complete a command name and not a directory name
26. therefore pcomplete-completions does not call pcomplete/cd but eshell-complete-commands-list
27. eshell-complete-commands-list loops through all possible command names
28. if these command names had a common prefix, it would have been inserted in the eshell buffer (?), but this is not the case, so the effect of this loop (apart from a waste of time) is nil
29. when eshell-complete-commands-list has finished its job, eshell prints its next prompt

At step 27 it is possible to interrupt the loop with C-g. At step 21 it
is possible to remove completion-in-region--postch from post-command-hook,

for example by switching buffers with C-x C-b.

>

> Calling `pcomplete-completions-at-point` several times is not
> necessarily a problem. E.g. it's normal to call it a second time after
> completion to check whether we're still in the same completion area (in
> order to detect when completion is "finished").
>

For some general case , I don't know (I admit I can't think of a case in
which this would be useful, but I know my experience is limited). To
complete a directory name in a shell, I don't see why this should be the
case. The (now obsolete) mechanism calls pcomplete (which also calls
pcomplete/cd) a single time, and it worked. I don't see what steps 13-29
could possibly do that would be useful, at least in the context of a
shell.

[Gregory Heytings]

Two corrections:

Step 19 should be:

19. given that the value of the start position did not change, the lambda let-bound at step 8 returns t, and therefore completion-in-region--postch does not exit completion-in-region-mode

And the last steps should be:

29. when eshell-complete-commands-list has finished its job, pcomplete-completions-at-point returns a value in which the start position has changed
30. therefore the lambda let-bound at step 8 returns nil, and therefore completion-in-region--postch exits completion-in-region-mode entered at step 10
31. this removes completion-in-region--postch from post-command-hook
32. eshell finally prints its next prompt

[Stefan Monnier]

> I do not understand why I should explain to you how the code you wrote
> works.

[ Because I remember ow it's supposed to work, but I don't know how it
actually behaves in this specific case. ]

> 19. given that the value of the start position did not change, the lambda
> let-bound at step 8 returns t, and therefore completion-in-region--postch
> does not exit completion-in-region-mode

> 20. completion-in-region--postch is now finished, it did not change
> anything in the eshell buffer
> 21. RET is pressed
> 22. post-command-hook is executed, and still contains
> completion-in-region--postch, so it is called again
> 23. completion-in-region--postch calls completion-in-region-mode-predicate
> 24. this calls pcomplete-completions-at-point a third time, which calls
> pcomplete-completions

Looks OK so far.

> 25. for some reason, pcomplete-completions considers that it must now
> complete a command name and not a directory name

I guess this is because after RET we're now at BOL so it looks like
a brand new command is starting.

> 26. therefore pcomplete-completions does not call pcomplete/cd but
> eshell-complete-commands-list

I guess this is the culprit and this is where the time is spent.

`eshell-complete-commands-list` eagerly builds the list of possible
candidates and it takes a while whereas we should here return something
quickly and cheaply, e.g. by returning a function which will build and
return this list more lazily when completion is actually performed.

Of course, the slowdown will presumably still be seen when you do
actually want to complete a command name, so we should probably try and
figure out more precisely where the slowdown comes from and how to
avoid/reduce it.

I guess part of the slowdown comes from the fact that we don't just use
`file-name-all-completions` in each directory in PATH but we
additionally call `file-executable-p` (or `file-readable-p`) on every
command found, which I expect will take quite a while when it goes
through Tramp.

Still, I'm not completely sure where the time is spent because I'm not
sure which files/dirs will go through tramp. AFAICT, `eshell-get-path`
will return the "local" $PATH rather than that of the remote host ... oh
wait, no I see that `tramp-eshell-directory-change` will set that to the
remote host's $PATH, so it should indeed work correctly (but slowly).

Maybe `tramp-eshell-directory-change` should tell
`eshell-complete-commands-list` to cache the list and also not to bother
checking `file-executable-p`?

> And the last steps should be:
>
> 29. when eshell-complete-commands-list has finished its job,
> pcomplete-completions-at-point returns a value in which the start position
> has changed
> 30. therefore the lambda let-bound at step 8 returns nil, and therefore
> completion-in-region--postch exits completion-in-region-mode entered at step
> 10
> 31. this removes completion-in-region--postch from post-command-hook
> 32. eshell finally prints its next prompt

Yes, these steps look fine.


Stefan

[Gregory Heytings]

>
>> 26. therefore pcomplete-completions does not call pcomplete/cd but
>> eshell-complete-commands-list
>
> I guess this is the culprit
>

No, see below.

>
> and this is where the time is spent.
>

Yes, this is what I said.

>
> `eshell-complete-commands-list` eagerly builds the list of possible
> candidates and it takes a while whereas we should here return something
> quickly and cheaply, e.g. by returning a function which will build and
> return this list more lazily when completion is actually performed.
>

No, the bug is in the completion mechanism, not in eshell. I don't know
exactly (because the mechanism is so complicated) where the completion
functions should be fixed, but it is clear that there is no reason to call
pcomplete-completions-at-point *three* times.

There is no reason to call pcomplete-completions-at-point when RET is
pressed.

Typing "cd foo/ RET" does not call pcomplete-completions-at-point.
Typing "cd foo TAB RET" should only call pcomplete-completions-at-point to
add the trailing slash, and should not build a list of all possible
commands.

>
> Of course, the slowdown will presumably still be seen when you do
> actually want to complete a command name, so we should probably try and
> figure out more precisely where the slowdown comes from and how to
> avoid/reduce it.
>

That's another, separate issue, and it is not relevant here. BTW, when
you want to complete a command name, at least you have some characters in
the prefix (or if you don't, you expect that listing all candidates will
be slow).

Typing "cd TAB" works almost instantaneously (and prints "Sole
completion"), even on a remote host. Typing "ls TAB" works almost
instantaneously, too, even on a remote host. It prints "Complete, but not
unique", and pressing TAB again lists the alternate completion candidates.
Typing "l TAB" takes (on a remote host) five to ten seconds before
printing all completion candidates, but again this is what a user expects.

Nobody expects that typing RET, when the completion has already been done,
would take three minutes.

>
> Maybe `tramp-eshell-directory-change` should tell
> `eshell-complete-commands-list` to cache the list and also not to bother
> checking `file-executable-p`?
>

That's yet another, separate issue, that is not relevant here. Indeed
there are possible improvements in eshell and tramp, but this bug is in
the completion mechanism, not in eshell or in tramp.

[Eli Zaretskii]

On September 1, 2020 11:31:14 AM GMT+03:00, "Gregory Heytings via Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gn...@gnu.org> wrote:
>
>>
>>> 26. therefore pcomplete-completions does not call pcomplete/cd but
>>> eshell-complete-commands-list
>>
>> I guess this is the culprit
>>
>
>No, see below.
>
>>
>> and this is where the time is spent.
>>
>
>Yes, this is what I said.
>
>>
>> `eshell-complete-commands-list` eagerly builds the list of possible
>> candidates and it takes a while whereas we should here return
>something
>> quickly and cheaply, e.g. by returning a function which will build
>and
>> return this list more lazily when completion is actually performed.
>>
>
>No, the bug is in the completion mechanism, not in eshell. I don't
>know
>exactly (because the mechanism is so complicated) where the completion
>functions should be fixed, but it is clear that there is no reason to
>call
>pcomplete-completions-at-point *three* times.

Would it help to profile the completion process in this use case using the built-in Lisp profiler?

[Eli Zaretskii]

[Gregory Heytings]

>
> Would it help to profile the completion process in this use case using
> the built-in Lisp profiler?
>

No, because the bug is not about cpu or mem usage, but about doing
something useless that just takes time (without eating much resources).

A much better way to profile this is to use:

(defvar pcomplete-completions-at-point-time 0)
(defun around-pcomplete-completions-at-point (fun)
(message "calling pcomplete-completions-at-point")
(setq pcomplete-completions-at-point-time (float-time))
(let ((ret (funcall fun)))
(message "returning from pcomplete-completions-at-point, call took %.2f seconds" (- (float-time) pcomplete-completions-at-point-time))
ret))
(advice-add 'pcomplete-completions-at-point :around #'around-pcomplete-completions-at-point)

(let ((default-directory "/ssh:user@host:~/")) (eshell))

This will print:

calling pcomplete-completions-at-point
returning from pcomplete-completions-at-point, call took 0.00 seconds
calling pcomplete-completions-at-point
returning from pcomplete-completions-at-point, call took 0.00 seconds
calling pcomplete-completions-at-point
returning from pcomplete-completions-at-point, call took N seconds

The value of N depends on the speed of your connection. On a fast
connection it will be something around 50, on a slower one something
around 100. With a local directory (that is, without let-binding
default-directory before entering eshell) it depends on your machine. On
a fast one it will be something around 0.10, on a slower one something
around 0.50.

Note again that this third call to pcomplete-completions-at-point does
nothing useful. It just build a list of all possible commands, and throws
it away.

[Gregory Heytings]

[Stefan Monnier]

>> `eshell-complete-commands-list` eagerly builds the list of possible
>> candidates and it takes a while whereas we should here return something
>> quickly and cheaply, e.g. by returning a function which will build and
>> return this list more lazily when completion is actually performed.
> No, the bug is in the completion mechanism, not in eshell. I don't know
> exactly (because the mechanism is so complicated) where the completion
> functions should be fixed, but it is clear that there is no reason to call
> pcomplete-completions-at-point *three* times.

The reason why it does so, is that it wants to know when a "completion
session" terminates, e.g. to hide the *Completions* buffer or to run the
exit-function.

So it calls it:

- once to do the actual completion.
- once per command in post-command-hook to see if we're done.

Since in your example you have 2 commands (TAB and RET), that gives you
a total of 3. This design relies on the fact that completion tables can
be lazy, so it should always be possible to make the
completion-at-point-function very cheap and harmless, so it's OK to call
it repeatedly (or even needlessly).

> There is no reason to call pcomplete-completions-at-point when RET
> is pressed.

If running that function is costly, it's a bug.

That's how completion-at-point-functions was designed.

If you want to change that design, be my guest, but it likely implies
changes to a fair bit of code, including outside Emacs.

>> Of course, the slowdown will presumably still be seen when you do actually
>> want to complete a command name, so we should probably try and figure out
>> more precisely where the slowdown comes from and how to avoid/reduce it.
> That's another, separate issue, and it is not relevant here.

Agreed.


Stefan

[Stefan Monnier]

> This will print:
>
> calling pcomplete-completions-at-point
> returning from pcomplete-completions-at-point, call took 0.00 seconds
> calling pcomplete-completions-at-point
> returning from pcomplete-completions-at-point, call took 0.00 seconds
> calling pcomplete-completions-at-point
> returning from pcomplete-completions-at-point, call took N seconds

[...]

> Note again that this third call to pcomplete-completions-at-point does
> nothing useful. It just build a list of all possible commands, and throws
> it away.

Exactly.

And the fix is to make this third call return in 0.00 seconds like the
others by making it return not the list of commands but a mere function
(which will return that list of commands only when called, but in the
present case it won't be called).


Stefan

[Stefan Monnier]

> And the fix is to make this third call return in 0.00 seconds like the
> others by making it return not the list of commands but a mere function
> (which will return that list of commands only when called, but in the
> present case it won't be called).

See for example patch below (which shouldn't be applied as-is since the
body of the function ends up misindented).


Stefan


diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index 48c99acac3..e41afea9ef 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -399,11 +399,15 @@

(defun eshell-complete-commands-list ()
"Generate list of applicable, visible commands."
- (let ((filename (pcomplete-arg)) glob-name)
+ ;; Building the commands list can take quite a while over Tramp
+ ;; (bug#41423), so do it lazily.
+ (completion-table-dynamic
+ (lambda (filename)
(if (file-name-directory filename)
(if eshell-force-execution
(pcomplete-dirs-or-entries nil #'file-readable-p)
(pcomplete-executables))
+ (let (glob-name)
(if (and (> (length filename) 0)
(eq (aref filename 0) eshell-explicit-command-char))
(setq filename (substring filename 1)
@@ -455,7 +459,7 @@
(and eshell-show-lisp-alternatives
(null completions)))
(all-completions filename obarray #'functionp))
- completions)))))))
+ completions)))))))))

(define-obsolete-function-alias 'eshell-pcomplete #'completion-at-point "27.1")

[Gregory Heytings]

>
>> No, the bug is in the completion mechanism, not in eshell. I don't
>> know exactly (because the mechanism is so complicated) where the
>> completion functions should be fixed, but it is clear that there is no
>> reason to call pcomplete-completions-at-point *three* times.
>
> The reason why it does so, is that it wants to know when a "completion
> session" terminates, e.g. to hide the *Completions* buffer or to run the
> exit-function.
>
> So it calls it:
>
> - once to do the actual completion.
> - once per command in post-command-hook to see if we're done.
>
> Since in your example you have 2 commands (TAB and RET), that gives you
> a total of 3.
>

Hmmm... It seems to me that in this case, we're done after the first call
to pcomplete-completions-at-point, so the second call to
pcomplete-completions-at-point in post-command-hook should see this (or at
least could see this), and remove completion-in-region--postch from
post-command-hook. RET would then not call pcomplete-completions-at-point
anymore.

>
> This design relies on the fact that completion tables can be lazy, so it
> should always be possible to make the completion-at-point-function very
> cheap and harmless, so it's OK to call it repeatedly (or even
> needlessly).
>

This is not at all documented AFAICS. Given that it's a crucial aspect
for your design to work, it should be.

>
>> There is no reason to call pcomplete-completions-at-point when RET is
>> pressed.
>
> If running that function is costly, it's a bug.
>

It was not before you declared `pcomplete' obsolete and removed
`eshell-pcomplete'.

>
> That's how completion-at-point-functions was designed.
>
> If you want to change that design, be my guest, but it likely implies
> changes to a fair bit of code, including outside Emacs.
>

I don't want to change that design, but I ask myself why you documented
`pcomplete-completions-at-point' as follows:

"Provide standard completion using pcomplete's completion tables. Same as
`pcomplete' but using the standard completion UI."

It's NOT the same as `pcomplete', it relies on different conditions. All
completion functions called by `pcomplete-completions-at-point' should be
checked and possibly changed with this new design.

Given this, why did you declared `pcomplete' obsolete (it would make sense
to have both a simple mechanism for simple cases and a more complex one
for more complex cases), and why did you remove `eshell-pcomplete'?

[Gregory Heytings]

>
> See for example patch below (which shouldn't be applied as-is since the
> body of the function ends up misindented).
>

Indeed this patch works.

But now my question is: whould it not be possible to do this (namely,
returning a lazy completion table) in one of the pcomplete-* functions (in
`pcomplete-completions-at-point' itself, or in `pcomplete-completions',
or...), instead of doing this in the individual functions ultimately
called by `pcomplete-completions-at-point'?

[Stefan Monnier]

>> Since in your example you have 2 commands (TAB and RET), that gives you
>> a total of 3.
> Hmmm... It seems to me that in this case, we're done after the first call
> to pcomplete-completions-at-point,

Usually it depends on whether the completion inserts a separator
(e.g. a space in the case of Eshell/Pcomplete). Otherwise, point is
still placed "within" (tho probably right at the END position) the
completion area and hence completion is not considered to be finished.
You can refine/tune the completion tables so that completion will
considered as finished in more cases, but that wouldn't solve the
problem in the general case anyway.

>> This design relies on the fact that completion tables can be lazy, so it
>> should always be possible to make the completion-at-point-function very
>> cheap and harmless, so it's OK to call it repeatedly (or even needlessly).
> This is not at all documented AFAICS. Given that it's a crucial aspect for
> your design to work, it should be.

The lispref says the following:

The functions on this hook should generally return quickly, since they
may be called very often (e.g., from @code{post-command-hook}).
Supplying a function for @var{collection} is strongly recommended if
generating the list of completions is an expensive operation. Emacs
may internally call functions in @code{completion-at-point-functions}
many times, but care about the value of @var{collection} for only some
of these calls. By supplying a function for @var{collection}, Emacs
can defer generating completions until necessary. You can use
@code{completion-table-dynamic} to create a wrapper function:

>>> There is no reason to call pcomplete-completions-at-point when RET
>>> is pressed.
>> If running that function is costly, it's a bug.
> It was not before you declared `pcomplete' obsolete and removed
> `eshell-pcomplete'.

Indeed, making the Pcomplete infrastructure compatible with the standard
completion UI introduced some incompatibilities.

This was true before `pcomplete` was declared obsolete, tho: it has been
true since the introduction of `pcomplete-completions-at-point` (some
time around Emacs-24, IIRC).

> "Provide standard completion using pcomplete's completion tables. Same as
> `pcomplete' but using the standard completion UI."
> It's NOT the same as `pcomplete', it relies on different conditions.
> All completion functions called by `pcomplete-completions-at-point' should
> be checked and possibly changed with this new design.

In practice it works fairly well for most cases, but yes some functions
needed to be changed, and others remain, obviously.

> Given this, why did you declared `pcomplete' obsolete (it would make sense
> to have both a simple mechanism for simple cases and a more complex one for
> more complex cases), and why did you remove `eshell-pcomplete'?

There are several questions in there. I deprecated `pcomplete` and
`eshell-pcomplete` because AFAIK they didn't have noticeably fewer bugs
than `completion-at-point` any more and neither did they offer any
feature available from `completion-at-point`.

W.r.t simple mechanism for simple cases, I'm not sure what that would
concretely look like in this particular case.

Some motivations for `pcomplete-completions-at-point`:
- make it possible to remove duplicate code that deals with the UI
aspect of completion (i.e. the `pcomplete` command) rather than the
core purpose of `pcomplete.el` which is to provide a way to specify
which completion table applies where on a command line.
- let the `pcomplete` machinery work with the standard UI, which means
it can also (mostly) obey `completion-styles`.
- let the `pcomplete` machinery work with other UIs such as
`company-mode`. I believe this last point is more important now.

> Indeed this patch works.

Thanks.

> But now my question is: whould it not be possible to do this (namely,
> returning a lazy completion table) in one of the pcomplete-* functions (in
> `pcomplete-completions-at-point' itself, or in `pcomplete-completions',
> or...), instead of doing this in the individual functions ultimately called
> by `pcomplete-completions-at-point'?

IIRC it wasn't really easy/possible, no. E.g. in the patch I sent
I think there's a bug in that a leading * should change the START..END
returned by `pcomplete-completion-at-point-function` so the `glob-name`
computation should be done outside of the `completion-table-dynamic`.

It could have been made a bit simpler and cleaner I guess if I had
decided to "start over from scratch" and force a rewrite of every
pcomplete/<foo> function (basically every such function would have to
return the same kind of info as returned by
`completion-at-point-functions`).

It would not have had any code in common with `pcomplete` any more,
other than the underlying design (which I find very clever&elegant).


Stefan

[Gregory Heytings]

Hi Stefan,

Thank you very much for your detailed answer!

>
>>> This design relies on the fact that completion tables can be lazy, so
>>> it should always be possible to make the completion-at-point-function
>>> very cheap and harmless, so it's OK to call it repeatedly (or even
>>> needlessly).
>>
>> This is not at all documented AFAICS. Given that it's a crucial aspect
>> for your design to work, it should be.
>
> The lispref says the following:
>
> The functions on this hook should generally return quickly, since
> they may be called very often (e.g., from @code{post-command-hook}).
> Supplying a function for @var{collection} is strongly recommended if
> generating the list of completions is an expensive operation. Emacs
> may internally call functions in @code{completion-at-point-functions}
> many times, but care about the value of @var{collection} for only
> some of these calls. By supplying a function for @var{collection},
> Emacs can defer generating completions until necessary. You can use
> @code{completion-table-dynamic} to create a wrapper function:
>

I see, thanks for the pointer. I did not find this because I was
searching for `pcomplete-completions-at-point' and
`pcomplete-completions'. It would make sense to put this pointer in
pcomplete.el.

>
> W.r.t simple mechanism for simple cases, I'm not sure what that would
> concretely look like in this particular case.
>

In this particular case, in Emacs 24-26, `eshell-pcomplete' called
`pcomplete', which did the completion without triggering the machinery of
`pcomplete-completion-at-point', that is, without entering a transient
`completion-in-region-mode', without modifying `post-command-hook', and so
forth. In particular, `pcomplete/cd' was called a single time. It seems
to me (even now that I understand the design of
`pcomplete-completion-at-point', and that I understand how the more
complex mechanism can be made as efficient as the simple one) that this
simple mechanism is often sufficient, that it is easier to understand, and
that it should remain available. OTOH I also understand the point that
you want to avoid duplicating code.

>
> Some motivations for `pcomplete-completions-at-point`:
> - make it possible to remove duplicate code that deals with the UI aspect of completion (i.e. the `pcomplete` command) rather than the core purpose of `pcomplete.el` which is to provide a way to specify which completion table applies where on a command line.
> - let the `pcomplete` machinery work with the standard UI, which means it can also (mostly) obey `completion-styles`.
> - let the `pcomplete` machinery work with other UIs such as `company-mode`. I believe this last point is more important now.
>

I see. This makes perfect sense, thanks for the clarification.

What is still missing IMO is a general description/documentation of the
various parts of the completion mechanisms (completion-at-point,
completion-in-region, pcomplete, pcomplete-completion-at-point,
comint-completion-at-point, icomplete, ...) in Emacs behave and interact.
I was completely lost when I started working on this bug, things are a bit
clearer now, but still not very clear.

>
>> Indeed this patch works.
>
> Thanks.
>

[...]
>
> In the patch I sent I think there's a bug in that a leading * should

> change the START..END returned by
> `pcomplete-completion-at-point-function` so the `glob-name` computation
> should be done outside of the `completion-table-dynamic`.
>

*sigh* So this bug can still not be considered fixed?

[Michael Albinus]

Gregory Heytings <g...@sdf.org> writes:

> Hi Stefan,

Hi,

> What is still missing IMO is a general description/documentation of
> the various parts of the completion mechanisms (completion-at-point,
> completion-in-region, pcomplete, pcomplete-completion-at-point,
> comint-completion-at-point, icomplete, ...) in Emacs behave and
> interact. I was completely lost when I started working on this bug,
> things are a bit clearer now, but still not very clear.

+1

Exactly this is the reason I've tried and failed to work on completion
problems. Several times.

Best regards, Michael.

[Drew Adams]

>> What is still missing IMO is a general description/documentation of
>> the various parts of the completion mechanisms (completion-at-point,
>> completion-in-region, pcomplete, pcomplete-completion-at-point,
>> comint-completion-at-point, icomplete, ...) in Emacs behave and
>> interact. I was completely lost when I started working on this bug,
>> things are a bit clearer now, but still not very clear.
>
> +1
>
> Exactly this is the reason I've tried and failed to work on completion
> problems. Several times.

+1

And it's not just a lack of doc, I think.
The completion mechanism (e.g. minibuffer.el) is a labyrinth.

[Stefan Monnier]

>> The lispref says the following:
>>
>> The functions on this hook should generally return quickly, since
>> they may be called very often (e.g., from @code{post-command-hook}).
>> Supplying a function for @var{collection} is strongly recommended if
>> generating the list of completions is an expensive operation. Emacs
>> may internally call functions in @code{completion-at-point-functions}
>> many times, but care about the value of @var{collection} for only
>> some of these calls. By supplying a function for @var{collection},
>> Emacs can defer generating completions until necessary. You can use
>> @code{completion-table-dynamic} to create a wrapper function:
>>
>
> I see, thanks for the pointer. I did not find this because I was searching
> for `pcomplete-completions-at-point' and `pcomplete-completions'. It would
> make sense to put this pointer in pcomplete.el.

Indeed, it might make sense to add a reminder that `pcomplete-here` is
the function where we "choose which completion table to use" and that
this choice should always be quick (so if the set of candidates can take
a while to compute, make sure the completion table computes it lazily).

> It seems to me (even now that I understand the design of
> `pcomplete-completion-at-point', and that I understand how the more
> complex mechanism can be made as efficient as the simple one) that
> this simple mechanism is often sufficient, that it is easier to
> understand, and that it should remain available.

You can definitely write such a "simple completion UI" on top of
`completion-at-point-functions`, and make sure it calls things like
`pcomplete-completion-at-point` only once.

Basically, take `completion-at-point`, throw out the "minor mode"
part and you're done.

> What is still missing IMO is a general description/documentation of the
> various parts of the completion mechanisms (completion-at-point,
> completion-in-region, pcomplete, pcomplete-completion-at-point,
> comint-completion-at-point, icomplete, ...) in Emacs behave and
> interact. I was completely lost when I started working on this bug, things
> are a bit clearer now, but still not very clear.

I find it hard to write such things because I'm too familiar with it to
know what's non-obvious. Maybe you could try writing something that
you'd have found useful, and then we can collaboratively improve it?

>> In the patch I sent I think there's a bug in that a leading * should
>> change the START..END returned by `pcomplete-completion-at-point-function`
>> so the `glob-name` computation should be done outside of the
>> `completion-table-dynamic`.
> *sigh* So this bug can still not be considered fixed?

Yup :-(
I'll try to come up with something better if noone else beats me to it.


Stefan

[Gregory Heytings]

>
>> What is still missing IMO is a general description/documentation of the
>> various parts of the completion mechanisms (completion-at-point,
>> completion-in-region, pcomplete, pcomplete-completion-at-point,
>> comint-completion-at-point, icomplete, ...) in Emacs behave and
>> interact. I was completely lost when I started working on this bug,
>> things are a bit clearer now, but still not very clear.
>
> I find it hard to write such things because I'm too familiar with it to
> know what's non-obvious. Maybe you could try writing something that
> you'd have found useful, and then we can collaboratively improve it?
>

Based on the time I already spent on this bug, I guess writing something
like this will require a substantial amount of work, but if you are
willing to help me, I'm willing to do that.

[Stefan Monnier]

That'd be great. There's a chance others will chime in as well.


Stefan