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

[João Távora]

didnOn Fri, Aug 13, 2021 at 1:56 PM Daniel Mendler
<mail@daniel-mendler.de> wrote:
>
> 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.

So you just say it and I have to believe it?  Then I could say the same to
you, right?  I won't of course, that would be silly.

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.

I didn't say it's better avoided, though of course I will avoid _any_ change if
I can. I said I have identified one drawback with doing it.  Then I
have addressed
that drawback. So that's what I said.

I am unaware of _other_ drawbacks.  They might exist, but I am unaware of
them.  Perhaps you are, and indeed you state they exist, but you refuse to
let me know about them.  Or perhaps others know of them and will let me know.
In my long-running discussion with Dmitry they were not presented (again,
except for the one I identified).

> > 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.

OK, but what thing of the future, real or academic, do you envision that
would bring back the problem, or create other problems?

> The idea of mutating the strings is a hack and not a solution.

Without facts to back it up, I have to take this as gratuitous disparagement.
Nicht so gut.

> In contrast, I am presenting a
> future-proof new API as solution which addresses multiple problems.

That's the issue.  The completion system is very complex and there are many
good ideas, different, floated by many people. But if you make a patch to
address "multiple" fuzzily-described problems, it's hard to judge how good
your ideas even are! Maybe they are indeed very good,  I never said
they weren't.  No need to get worked up about it!

Again, my proposal is to first focus on the performance problems caused by
string allocation.  _That_ problem is well understood, at least by me (but it
would help to settle on convenient benchmarks understood by others, too).
Then we can go from there.

> you look at the patch, only 196 new lines are added to minibuffer.el.
> Furthermore the patch adds 213 lines of new tests.

It's a large patch, over 1000 lines.  One does not review a patch
merely by looking at
lines added, when one needs to read much more, to understand implications, etc.
It needs documentation, for one, much more than just docstrings, on
how to use the
new API.

> João, you don't have to lecture me on these things.  Of course I can
> provide such numbers.

Then please do! Not meaning to lecture you, just that your suggestion that
I try Vertico UI as a substitution for these numbers seemed completely
misguided.  So if you have them (or "can provide them") let's see them.
All I'm asking,  preferably from Emacs -Q recipe.

> 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.

I didn't say it didn't solve them! Now, where did I say that? I would
like to see a
benchmark so that I can witness it _and_ study alternative solutions. With
that, there's a better chance that I will be persuaded there are none
as elegant,
clean, proper, pure, etc as yours!

Maybe others review patches on other aspects that's fine.   Maybe
others will. Eli reviewed on minor formatting and documentation aspects.
I review them on substance, using numbers and conducting my own
experiments and tests. This takes time and help from the scientist on the
other end.

Simple and in summary, let's hope your next reply has some benchmarks
so we can make progress.

Thanks,
João