minad / cape

🦸cape.el - Completion At Point Extensions
GNU General Public License v3.0
584 stars 20 forks source link

try-completion and cape-dabbrev: obarray interns every fragment! #38

Closed jdtsmith closed 2 years ago

jdtsmith commented 2 years ago

I use a super-capf that incorporates cape-dabbrev, which is quite nice while editing not-yet-eval'd code. A distilled version:

(cl-nsubst (cape-super-capf #'elisp-completion-at-point #'cape-dabbrev)
           'elisp-completion-at-point
           completion-at-point-functions)

Unfortunately this defeats corfu-preselect-first, because the final test-completion with the table this CAPF returns incorrectly returns true for any match. I think this is because the dabbrev list in the table includes the very symbol at point in its list. It doesn't even respect cape-dabbrev-min-length.

UPDATE: This is not correct... the final test-completion match against obarray is the culprit...

Looking into this, all the normal completion actions with the cached dabbrev backend work fine. cape--dabbrev-list runs just once, and returns just a few strings (even when the probe is "", cape wisely ignores that).

Then the final test-completion in corfu--recompute-candidates compares the candidate against obarray, and... always returns t.

This seems to be far beyond cape or corfu, but here's a simple recipe to reproduce for emacs -Q:

(require 'seq)
(intern-soft "eve") ; nil
(test-completion "eve" obarray nil) ; nil
(length obarray) ; 15121
(eve[Tab here to complete])
(length obarray) ; 15121, same
(intern-soft "eve") ; eve
(test-completion "eve" obarray nil) ; t!  Sigh

It seems each and every partial fragment of text introduced to the completion system gets put into obarray? This entirely defeats test-completion in that context. I can only conclude this is an Emacs bug of some sort.

minad commented 2 years ago

Oh wow, that's an interesting bug! I can confirm this in emacs -Q on Emacs 28 (and also Emacs 27). Instead of TAB I have to press M-TAB in emacs -Q. The minimal recipe is this:

(intern-soft "ohno") <C-M-x> -> nil
(ohno <M-TAB>                -> No match
(intern-soft "ohno") <C-M-x> -> ohno :(

I tried to investigate this a bit but I have no idea where the interning happens. Can you please report this upstream via report-emacs-bug?

jdtsmith commented 2 years ago

Done. I can't figure out where the fragment is being interned either. I've traced intern during the completion attempt and it's not the obvious route. It must be hand-inserted into obarray somehow.

In the meantime, for super-capfs involving obarray, test-completion is clearly unreliable. I noticed in corfu that the table function actually changes between the all-completions and test-completion calls; not sure why.

minad commented 2 years ago

On 5/12/22 16:14, JD Smith wrote:

Done. I can't figure out where the fragment is being interned either. I've traced intern during the completion attempt and it's not the obvious route. It must be hand-inserted into obarray somehow.

It seems that it would have been better to report this as a bug via report-emacs-bug, not only via the mailing list.

jdtsmith commented 2 years ago

For me that goes to the maintainer of the emacs-mac fork, which isn't the right place. If you are able to forward/re-post to the gnu emacs bug list, totally fine with me.

jdtsmith commented 2 years ago

I posted a report and heard back. The response led me to discover that the elisp capf specifies predicates like elisp--shorthand-aware-fboundp via :predicate, which cape--super-capf's table isn't always respecting. See #40 for a fix.

minad commented 2 years ago

Can you please link to the upstream bug report?

minad commented 2 years ago

Found it https://debbugs.gnu.org/cgi/bugreport.cgi?bug=55491

minad commented 2 years ago

Hmm, I consider the obarray pollution questionable, maybe even as a bug since this leads to a small memory leak. Resource leakage is generally bad style. I also consider it a bug since I don't see a reason why this has to happen. Does it simplify anything inside the completion function which justifies that it leaves a mess? Is this accidental or intentional?

jdtsmith commented 2 years ago

Found it

I had linked it from the PR too.

Does it simplify anything inside the completion function which justifies that it leaves a mess? Is this accidental or intentional?

These are good questions. Please do jump in there with these. I assume it's something deep in the completion logic that uses symbols in the obarray for speed or convenience. But no idea if it's intentional.

The one useful thing it did is to allow us to discover this small bug in cape!

jdtsmith commented 2 years ago

BTW, do you have any insight as to why a CAPF can return a :predicate prop, and a predicate argument can be passed to its collection table function, and predicate can be passed to try-completion and friends? I have the feeling that some of these predicate slots are designed to consume the output of others. I guess a CAPF prop says "this is my own predicate, please respect it", then perhaps a completion style can add to that. Are there examples in the wild of these various predicates at work?

The issue for cape-super-capf of course is that there is no simple one-size-fits-all super-predicate from its combined tables.

minad commented 2 years ago

On 5/18/22 20:21, JD Smith wrote:

BTW, do you have any insight as to why a CAPF can return a :predicate prop, and a predicate argument can be passed to its collection table function, and predicate can be passed to try-completion and friends? You can pass a predicate argument to completing-read and also to completion-in-region. The :predicate returned by the Capf is then handed over to completion-in-region to filter the table.

Overall the predicate is then passed all the way down in the completion pipeline:

completion-in-region/completing-read -> completion-all-completions/completion-try-completion -> the completion style implementation -> try-completion/all-completions called on the table

If the table is a function, the table itself is called and receives the predicate as argument.

the table -> complete-with-action -> try-completion/all-completions

On the top level, these predicates are a misfeature since they are nothing else than a filter applied to the table. A better design would have been to use plain function composition to create a new table.

(completing-read "Prompt: " table) ;; without predicate (completing-read "Prompt: " (filtered table)) ;; with predicate

Incidentally such a transformation function exists with the name completion-table-with-predicate (it must be applied partially). For Capfs we have cape-capf-predicate and cape-wrap-predicate.

One should note that the predicate argument of completion tables is necessary. The filtering by predicate is much more efficient since the completion candidate list is only allocated for candidates which satisfy the predicate. On the level of the tables, the predicate argument is therefore justified and not a misfeature. Only on the top level completing-read/completion-in-region, the argument is unnecessary.

jdtsmith commented 2 years ago

I see, thanks. So oddly a CAPF both specifies and later receives the same predicate function.

minad commented 2 years ago

I see, thanks. So oddly a CAPF both specifies and later receives the same predicate function.

Yes, that's the case. But the completion styles could in principle compose the outer predicate with some custom predicate to achieve its filtering. Therefore it is not guaranteed that we will end up with exactly the same predicate.

Anyway, the :predicate field of Capfs is a misfeature as I argued. The Capf should itself return a prefiltered table directly. In a potential redesign of the completing-read/capf API the outer predicate argument and the :predicate field should be removed.

jdtsmith commented 2 years ago

One thing the current design allows I guess that a composed table would not is a style which completely ignores the CAPF’s predicate. I’m wondering if our #40 fix defeats any styles by hand-combining the table + style predicates in the table function call. Orderless does not use the predicate I think.