Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

[Stefan Monnier]

>     This completion style is meant to be used with a "programmable
>     completion" table that interfaces with an external providing

An external .... ?

> +;; overriden.  This can be seen as a drawback, but, on the other hand,
> +;; the regular the full data set to be available in Emacs' addressing
> +;; space, which is often not feasible.

There something wrong around "the regular the full data".

> +;; The programmable completion table amounts to a function taking
> +;; (PATTERN PRED ACTION) as arguments respond to at least three values
> +;; for ACTION:
> +;;
> +;; * The symbol `metadata', where the table should reply with a list
> +;;   that looks like:
> +;;
> +;;      (metadata (category . backend-completion) MORE...)
> +;;
> +;;    where MORE... can be other "metadata" items like
> +;;    `cycle-sort-function'.
> +;;
> +;;    Other categories can be used in place of `backend-completion',
> +;;    as long as the `styles' property of such categories contains the
> +;;    sole element `backend-completion-backend-style'.

Hmmm... I don't think we should recommend `backend-completion` as
a category.  It is fundamentally wrong.  So I think `backend-completion`
should be the name of the completion style, and the category name should
reflect the user-facing category of elements being completed rather than
the internal details of how the completion is performed.

Maybe a good way to do that is to provide a (backend-completion-table
FUNC CATEGORY &optional METADATA) where FUN is a function which handles
the tryc and allc cases (maybe the `tryc` handling could even be
optional, depending on how the `tryc` case is usually handled (do
backends often provide something usable for `tryc`?)).  That function
would then return an appropriate completion-table (function), and would
make sure along the way that CATEGORY is mapped to `completion-backend`
in the `completion-category-defaults`.

That `backend-completion-table` would also have the benefit that it
could make sure the completion-table it returns also behaves sanely
with other styles (i.e. also handle the "normal" completion table
requests), whereas otherwise the temptation would be very high for users
of this package to use "completion table functions" which only handle
the `backend-completion-tryc/allc` requests and fail pathetically when
another style is used.  Similarly `backend-completion-table` could also
make sure PRED is obeyed.

>  (add-to-list 'completion-category-overrides
> -             '(eglot-indirection-joy (styles . (eglot--lsp-backend-style))))
> +             '(eglot-indirection-joy (styles . (backend-completion-backend-style))))

Any reason not to call that completion category just `eglot`?


        Stefan

Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

[João Távora]

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

>>     This completion style is meant to be used with a "programmable
>>     completion" table that interfaces with an external providing
>
> An external .... ?

tool

>> +;; overriden.  This can be seen as a drawback, but, on the other hand,
>> +;; the regular the full data set to be available in Emacs' addressing
>> +;; space, which is often not feasible.
>
> There something wrong around "the regular the full data".

"the regular styles require the full data set", obviously.  Geez, can't
you read moronese?

>> +;; The programmable completion table amounts to a function taking
>> +;; (PATTERN PRED ACTION) as arguments respond to at least three values
>> +;; for ACTION:
>> +;;
>> +;; * The symbol `metadata', where the table should reply with a list
>> +;;   that looks like:
>> +;;
>> +;;      (metadata (category . backend-completion) MORE...)
>> +;;
>> +;;    where MORE... can be other "metadata" items like
>> +;;    `cycle-sort-function'.
>> +;;
>> +;;    Other categories can be used in place of `backend-completion',
>> +;;    as long as the `styles' property of such categories contains the
>> +;;    sole element `backend-completion-backend-style'.
>
> Hmmm... I don't think we should recommend `backend-completion` as
> a category.  It is fundamentally wrong.  So I think `backend-completion`
> should be the name of the completion style, and the category name should
> reflect the user-facing category of elements being completed rather than
> the internal details of how the completion is performed.
>
> Maybe a good way to do that is to provide a (backend-completion-table
> FUNC CATEGORY &optional METADATA) where FUN is a function which handles
> the tryc and allc cases (maybe the `tryc` handling could even be
> optional, depending on how the `tryc` case is usually handled (do
> backends often provide something usable for `tryc`?)).  That function
> would then return an appropriate completion-table (function), and would
> make sure along the way that CATEGORY is mapped to `completion-backend`
> in the `completion-category-defaults`.

I get the idea, I think.  I thought about this, but I'm not sure this
can be made generic enough to service the three examples I know of:
Eglot, SLY and the eel.el thing I've been using lately.

> That `backend-completion-table` would also have the benefit that it
> could make sure the completion-table it returns also behaves sanely
> with other styles (i.e. also handle the "normal" completion table
> requests), whereas otherwise the temptation would be very high for users
> of this package to use "completion table functions" which only handle
> the `backend-completion-tryc/allc` requests and fail pathetically when
> another style is used.  Similarly `backend-completion-table` could also
> make sure PRED is obeyed.

Alright.  But please, follow up with some code.  You're probably right,
but this whole thing is too complicated for me to understand.

If it helps, use the current Eglot.el code as an example client of that
new thing.

Or you can assume I also have another function:

   (defun eel (pattern) ...)

That returns a list of completions from an HTTP server (blockingly,
sometimes slowly) or may returns the symbols 'user-input', 'timeout'
depending on whether whether these two events happened before the
completions became available.  'eel' knows how the HTTP server matches
STRINGS and propertizes the strings it returns.

How would I plug this 'eel' function into the backend-completion-table
function you're envisioning?  And would it return a working table with
the backend completion style that I can put wherever (most likely
completing-read)?

>>  (add-to-list 'completion-category-overrides
>> -             '(eglot-indirection-joy (styles . (eglot--lsp-backend-style))))
>> +             '(eglot-indirection-joy (styles . (backend-completion-backend-style))))
>
> Any reason not to call that completion category just `eglot`?

Yes, 'eglot' is already a different completion category, for
completion-at-point, with a different structure.
`eglot-indirection-joy' is just a product of my usual confusion with
this whole jungle of concepts: I just vaguely understand it's an
indirection so I called it accordingly for mental reference.  But feel
free to suggest a better name.

João

Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

[Stefan Monnier]

>> Maybe a good way to do that is to provide a (backend-completion-table
>> FUNC CATEGORY &optional METADATA) where FUN is a function which handles
>> the tryc and allc cases (maybe the `tryc` handling could even be
>> optional, depending on how the `tryc` case is usually handled (do
>> backends often provide something usable for `tryc`?)).  That function
>> would then return an appropriate completion-table (function), and would
>> make sure along the way that CATEGORY is mapped to `completion-backend`
>> in the `completion-category-defaults`.
>
> I get the idea, I think.  I thought about this, but I'm not sure this
> can be made generic enough to service the three examples I know of:
> Eglot, SLY and the eel.el thing I've been using lately.

I can't see why it wouldn't be just as general as what we have now.

    (defun backend-completion-table ( tryc-function allc-function
                                      category &optional metadata)
      (unless (assq category completion-category-defaults)
        (push `(,category (styles completion-backend))
              `completion-category-defaults))
      (lambda (string pred action)
        (pcase action
          (`metadata
           `(metadata (category . ,category) . ,metadata))
          (`(backend-completion-tryc . ,point)
           ;; FIXME: Obey `pred`?  Pass it to `tryc`?
           `(backend-completion-tryc . ,(funcall tryc string point)))
          (`(backend-completion-allc . ,point)
           (let ((all (funcall allc string point)))
             `(backend-completion-allc . ,(seq-filter pred all))))
          (`(boundaries . ,_) nil)
          (t
           (let ((all (funcall allc string (length string))))
             (complete-with-action action all string pred))))))

>> That `backend-completion-table` would also have the benefit that it
>> could make sure the completion-table it returns also behaves sanely
>> with other styles (i.e. also handle the "normal" completion table
>> requests), whereas otherwise the temptation would be very high for users
>> of this package to use "completion table functions" which only handle
>> the `backend-completion-tryc/allc` requests and fail pathetically when
>> another style is used.  Similarly `backend-completion-table` could also
>> make sure PRED is obeyed.
>
> Alright.  But please, follow up with some code.  You're probably right,
> but this whole thing is too complicated for me to understand.

See above.

>>>  (add-to-list 'completion-category-overrides
>>> -             '(eglot-indirection-joy (styles . (eglot--lsp-backend-style))))
>>> +             '(eglot-indirection-joy (styles . (backend-completion-backend-style))))

Wow!  I didn't pay attention at first, but I now see you're modifying
`completion-category-overrides` which is the user-facing variable.
You should modify `completion-category-defaults` instead!

>> Any reason not to call that completion category just `eglot`?
>
> Yes, 'eglot' is already a different completion category, for
> completion-at-point, with a different structure.
> `eglot-indirection-joy' is just a product of my usual confusion with
> this whole jungle of concepts: I just vaguely understand it's an
> indirection so I called it accordingly for mental reference.  But feel
> free to suggest a better name.

I'm not very familiar with Eglot's code but based on a cursory check, it
seems this is used specifically when completing an identifier for things
like `xref`, so maybe `eglot-identifier` (and maybe `eglot` should be
renamed to `eglot-code` since it's used to complete the actual code in
the buffer)?


        Stefan

Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

[João Távora]

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

>>> Maybe a good way to do that is to provide a (backend-completion-table
>>> FUNC CATEGORY &optional METADATA) where FUN is a function which handles
>>> the tryc and allc cases (maybe the `tryc` handling could even be
>>> optional, depending on how the `tryc` case is usually handled (do
>>> backends often provide something usable for `tryc`?)).  That function
>>> would then return an appropriate completion-table (function), and would
>>> make sure along the way that CATEGORY is mapped to `completion-backend`
>>> in the `completion-category-defaults`.
>>
>> I get the idea, I think.  I thought about this, but I'm not sure this
>> can be made generic enough to service the three examples I know of:
>> Eglot, SLY and the eel.el thing I've been using lately.
>
> I can't see why it wouldn't be just as general as what we have now.
>
>     (defun backend-completion-table ( tryc-function allc-function
>                                       category &optional metadata)
>       (unless (assq category completion-category-defaults)
>         (push `(,category (styles completion-backend))
>               `completion-category-defaults))
>       (lambda (string pred action)
>         (pcase action
>           (`metadata
>            `(metadata (category . ,category) . ,metadata))
>           (`(backend-completion-tryc . ,point)
>            ;; FIXME: Obey `pred`?  Pass it to `tryc`?
>            `(backend-completion-tryc . ,(funcall tryc string point)))
>           (`(backend-completion-allc . ,point)
>            (let ((all (funcall allc string point)))
>              `(backend-completion-allc . ,(seq-filter pred all))))
>           (`(boundaries . ,_) nil)
>           (t
>            (let ((all (funcall allc string (length string))))
>              (complete-with-action action all string pred))))))


OK great.  But where do you plug the current eglot functions into it.

>
>>> That `backend-completion-table` would also have the benefit that it
>>> could make sure the completion-table it returns also behaves sanely
>>> with other styles (i.e. also handle the "normal" completion table
>>> requests), whereas otherwise the temptation would be very high for users
>>> of this package to use "completion table functions" which only handle
>>> the `backend-completion-tryc/allc` requests and fail pathetically when
>>> another style is used.  Similarly `backend-completion-table` could also
>>> make sure PRED is obeyed.
>>
>> Alright.  But please, follow up with some code.  You're probably right,
>> but this whole thing is too complicated for me to understand.
>
> See above.
>
>>>>  (add-to-list 'completion-category-overrides
>>>> -             '(eglot-indirection-joy (styles . (eglot--lsp-backend-style))))
>>>> +             '(eglot-indirection-joy (styles . (backend-completion-backend-style))))
>
> Wow!  I didn't pay attention at first, but I now see you're modifying
> `completion-category-overrides` which is the user-facing variable.
> You should modify `completion-category-defaults` instead!

Yep.  I actually meant to correct that in the patch.  The reason I was
using overrides is because fido-mode nullifies
completion-category-defaults.  It shouldn't, and I added another patch
correcting that.  So yes, you're right.

>>> Any reason not to call that completion category just `eglot`?
>>
>> Yes, 'eglot' is already a different completion category, for
>> completion-at-point, with a different structure.
>> `eglot-indirection-joy' is just a product of my usual confusion with
>> this whole jungle of concepts: I just vaguely understand it's an
>> indirection so I called it accordingly for mental reference.  But feel
>> free to suggest a better name.
>
> I'm not very familiar with Eglot's code but based on a cursory check, it
> seems this is used specifically when completing an identifier for things
> like `xref`, so maybe `eglot-identifier` (and maybe `eglot` should be
> renamed to `eglot-code` since it's used to complete the actual code in
> the buffer)?

Maybe.  As I've often told you, all this completion code is hard to
grasp.  I do "get" it somewhat, and I've been working with it intensely
for a long time.  But also, I don't really get it :-) For example, I
have no solid concept of what a category or a style is, so everything
just reads like "an indirection from this thing to that other thing".
So I named this thing 'eglot-indirection' . 'eglot-code' may be logical
for someone who knows what a category means, now but sounds something
that will make absolutely no sense to me 2 weeks from now.

João

Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

[Stefan Monnier]

> OK great.  But where do you plug the current eglot functions into it.

Something like the patch below?

> Maybe.  As I've often told you, all this completion code is hard to
> grasp.  I do "get" it somewhat, and I've been working with it intensely
> for a long time.  But also, I don't really get it :-) For example, I
> have no solid concept of what a category or a style is, so everything
> just reads like "an indirection from this thing to that other thing".

Some sets of identifiers are designed to play well with prefix
completion, while others are a very poor fit.

The "category" is a loose way to describe to users what kind of "sets of
identifiers" this is completing so users can customize the behavior for
specific cases.

> So I named this thing 'eglot-indirection' . 'eglot-code' may be logical
> for someone who knows what a category means, now but sounds something
> that will make absolutely no sense to me 2 weeks from now.

The `category` should be a name that describe the "kind of thing this
inserts/selects by completion".


        Stefan


diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index e1041666754..d88c6b78dc7 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2571,7 +2571,7 @@ eglot--workspace-symbols
                   (let ((probe (gethash pat cache :missing)))
                     (if (eq probe :missing) (puthash pat (refresh pat) cache)
                       probe)))
-                (lookup (pat)
+                (lookup (pat _point)
                   (let ((res (lookup-1 pat))
                         (def (and (string= pat "") (gethash :default cache))))
                     (append def res nil)))
@@ -2579,16 +2579,13 @@ eglot--workspace-symbols
                   (cl-getf (get-text-property
                             0 'eglot--lsp-workspaceSymbol c)
                            :score 0)))
-      (lambda (string _pred action)
-        (pcase action
-          (`metadata `(metadata
-                       (cycle-sort-function
+      (backend-completion-table
+       (lambda (string point) `(,string . ,point))
+       #'lookup
+       'eglot-indirection-joy
+       `((cycle-sort-function
                         . ,(lambda (completions)
-                             (cl-sort completions #'> :key #'score)))
-                       (category . eglot-indirection-joy)))
-          (`(eglot--lsp-tryc . ,point) `(eglot--lsp-tryc . (,string . ,point)))
-          (`(eglot--lsp-allc . ,_point) `(eglot--lsp-allc . ,(lookup string)))
-          (_ nil))))))
+                             (cl-sort completions #'> :key #'score))))))))
 
 (defun eglot--recover-workspace-symbol-meta (string)
   "Search `eglot--workspace-symbols-cache' for rich entry of STRING."
@@ -2600,7 +2597,7 @@ eglot--recover-workspace-symbol-meta
                  (setq v (cdr v))))
              eglot--workspace-symbols-cache)))
 
-(add-to-list 'completion-category-overrides
+(add-to-list 'completion-category-defaults
              '(eglot-indirection-joy (styles . (eglot--lsp-backend-style))))
 
 (cl-defmethod xref-backend-identifier-at-point ((_backend (eql eglot)))

Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

[João Távora]

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

Hey Stefan,

I just pushed to scratch/backend-completion.

I integrated most of your suggestions and added docstrings.  I'm sure
you'll want to comment and tweak some stuff (some of the
documentation is purposely cheeky and there is also a FIXME
or two.).

I quite like this and it's working well for both Eglot and that other
library.  I haven't tried it with SLY but I'm sure it will do the job
nicely.

One of the most important things though is the naming.
"Backend completion" is not a good name, there are already a ton
of "backend" semantics that are entirely Emacs only, and this new
style is really exclusive for tools _outside_ of Emacs's address
 space.

Let me know what you think, João

On Tue, Nov 29, 2022 at 11:44 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > OK great.  But where do you plug the current eglot functions into it.
>
> Something like the patch below?
>
> > Maybe.  As I've often told you, all this completion code is hard to
> > grasp.  I do "get" it somewhat, and I've been working with it intensely
> > for a long time.  But also, I don't really get it :-) For example, I
> > have no solid concept of what a category or a style is, so everything
> > just reads like "an indirection from this thing to that other thing".
>
> Some sets of identifiers are designed to play well with prefix
> completion, while others are a very poor fit.
>
> The "category" is a loose way to describe to users what kind of "sets of
> identifiers" this is completing so users can customize the behavior for
> specific cases.
>
> > So I named this thing 'eglot-indirection' . 'eglot-code' may be logical
> > for someone who knows what a category means, now but sounds something
> > that will make absolutely no sense to me 2 weeks from now.
>
> The `category` should be a name that describe the "kind of thing this
> inserts/selects by completion".
>
>
>         Stefan
>
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index e1041666754..d88c6b78dc7 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -2571,7 +2571,7 @@ eglot--workspace-symbols
>                    (let ((probe (gethash pat cache :missing)))
>                      (if (eq probe :missing) (puthash pat (refresh pat)
> cache)
>                        probe)))
> -                (lookup (pat)
> +                (lookup (pat _point)
>                    (let ((res (lookup-1 pat))
>                          (def (and (string= pat "") (gethash :default
> cache))))
>                      (append def res nil)))
> @@ -2579,16 +2579,13 @@ eglot--workspace-symbols
>                    (cl-getf (get-text-property
>                              0 'eglot--lsp-workspaceSymbol c)
>                             :score 0)))
> -      (lambda (string _pred action)
> -        (pcase action
> -          (`metadata `(metadata
> -                       (cycle-sort-function
> +      (backend-completion-table
> +       (lambda (string point) `(,string . ,point))
> +       #'lookup
> +       'eglot-indirection-joy
> +       `((cycle-sort-function
>                          . ,(lambda (completions)
> -                             (cl-sort completions #'> :key #'score)))
> -                       (category . eglot-indirection-joy)))
> -          (`(eglot--lsp-tryc . ,point) `(eglot--lsp-tryc . (,string .
> ,point)))
> -          (`(eglot--lsp-allc . ,_point) `(eglot--lsp-allc . ,(lookup
> string)))
> -          (_ nil))))))
> +                             (cl-sort completions #'> :key #'score))))))))
>
>  (defun eglot--recover-workspace-symbol-meta (string)
>    "Search `eglot--workspace-symbols-cache' for rich entry of STRING."
> @@ -2600,7 +2597,7 @@ eglot--recover-workspace-symbol-meta
>                   (setq v (cdr v))))
>               eglot--workspace-symbols-cache)))
>
> -(add-to-list 'completion-category-overrides
> +(add-to-list 'completion-category-defaults
>               '(eglot-indirection-joy (styles .
> (eglot--lsp-backend-style))))
>
>  (cl-defmethod xref-backend-identifier-at-point ((_backend (eql eglot)))
>
>

-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 5402 bytes --]

Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

[Stefan Monnier]

> I quite like this and it's working well for both Eglot and that other
> library.  I haven't tried it with SLY but I'm sure it will do the job
> nicely.

One thing I was wondering is how useful is the `tryc` method.  I suspect
that SLY can provide a real implementation for it but in most other
cases I'd expect that the only option is to use a degenerate `tryc`
method.  So I was thinking that maybe the `tryc-function` arg should
be optional.

> One of the most important things though is the naming.
> "Backend completion" is not a good name, there are already a ton
> of "backend" semantics that are entirely Emacs only, and this new
> style is really exclusive for tools _outside_ of Emacs's address
>  space.

I agree that "backend" is not great since it's largely a matter of
perspective.  I can't remember which other names have been considered
but how 'bout `external`?


        Stefan

Re: scratch/backend-completion c807121fbd 1/2: Add lisp/backend-completion.el

[João Távora]

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

On Sat, Dec 3, 2022 at 2:04 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> method.  So I was thinking that maybe the `tryc-function` arg should
> be optional.

Been there done that.

> I agree that "backend" is not great since it's largely a matter of
> perspective.  I can't remember which other names have been considered
> but how 'bout `external`?

Ditto!

[-- Attachment #2: Type: text/html, Size: 527 bytes --]