bug#47711: [PATCH VERSION 2] Add new `completion-filter-completions` API and deferred highlighting

[Daniel Mendler <mail@daniel-mendler.de>]

On 8/13/21 2:37 PM, João Távora wrote:
> On Fri, Aug 13, 2021 at 1:22 PM Daniel Mendler <mail@daniel-mendler.de> wrote:
> 
>> It is important to keep this property since this will preclude many bugs
>> due to string mutation.
> 
> I am aware of this, of course.  Can you give examples of these "many bugs"?
> Perhaps other than the one I already described and addressed?

No, João, this is not how it goes.  I don't have to prove to you that
your idea introduces bugs.  You have to show that mutation of the
completion table strings (which are not supposed to be mutated) will not
lead to bugs, which are hard to find.

In contrast with the new API `completion-filter-completions` this entire
class of bugs is avoided by construction of the API.  Furthermore the
`completion-filter-completions` API is easy to use in comparison to your
idea, where "compliant" backends have to apply string manipulations to
apply the highlighting and revert the strings back to their old pristine
state.  The only thing the API user has to do is to call the `highlight`
function returned in the alist by `completion-filter-completions`.

>> By separating the filtering and mutation
>> (highlighting, scoring) my patch addresses the problem at hand in the
>> proper way.
>> [ ... ]
>> Mutation would be a reasonable choice here if the problem could not be
>> solved in a proper way.  But in fact it can be solved in a proper way
>> without mutating the strings at all as my patch shows.
> 
> "proper" is just an reasonably empty adjective.  There are different ways to
> go about this, of course.  What's "proper" and isn't is hard to debate
> objectively.

You are contradicting yourself here. You agree that string mutation is
better be avoid. If we define "proper" as avoids string mutation if this
is easily possible, then my patch implements a proper solution to the
problem.

>>> The advantage that I see is that those adaptations apart, it is a small
>>> localized and effective change.
>>
>> Note that your idea also does not address the other issues which are
>> addressed by my patch.
> 
> That's for sure.  My patch idea addresses only that single problem.
> I think this is a good property of patches: to solve one thing, not many.

No, this is not necessarily true.  This is only good if the problem is
solved in a way which is future proof.  The idea of mutating the strings
is a hack and not a solution. In contrast, I am presenting a
future-proof new API as solution which addresses multiple problems.  If
you look at the patch, only 196 new lines are added to minibuffer.el.
Furthermore the patch adds 213 lines of new tests.

> Look, one needs to evaluate things quantitively. Your patch is not
> to Vertico, it's to Emacs. I'm concerned with changes to Emacs and their
> effect on all completion frontends.  So trying Vertico isn't very useful.
> 
> If you're solving a performance problem (and it seems that you are, among
> other things) we really need benchmarks, a description of an experiment whose
> results can be reproduced independently. It's the normal scientific method.

João, you don't have to lecture me on these things.  Of course I can
provide such numbers.  You cannot reasonably make the claim that
`copy-sequence` is the problem and at the same time claim that my patch
does not solve the performance issues, when in fact my patch avoids this
exact string copying.

>>>> The second problem addressed by the new API
>>>> `completion-filter-completions` is that `completion-all-completions` is
>>>> limited in what it can return.  For example it cannot return the end
>>>> position of the completion.
>>>
>>> And why is this a problem? Can you post an example of something you'd
>>> like to do, but can't?  Regardless, it does seem indeed like a "second" problem
>>> (as you state) so perhaps something that can be addressed separately.
>>
>> Please look at the FIXMEs in minibuffer.el which address this.
>> Currently only the beginning position of the completion boundary is
>> returned, which is only half of the information.
> 
> OK. It does seem like a separate problem, so maybe open a new bug for it?

There is already a FIXME in minibuffer.el, so I assume Stefan Monnier is
well aware of these issues.  It is an additional win of the new API that
such open problems can be fixed too.  As I see it, a new API is the way
to go here.

Daniel