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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> It's basically this: we cannot mutate what we don't own. Across all of
> completion functions out there, there will be such that return
> "shared" strings (meaning, not copied or newly allocated) from their
> completion tables. And modifying them is bad, with consequences which
> can present themselves in unexpected, often subtle ways.

I agree with this premise.  But would you call putting a uniquely named
text property in them a modification or mutation of said strings?  I
don't.

> Since up until now completion-pcm--hilit-commonality copied all
> strings before modifying, completion tables such as described (with
> "shared" strings) have all been "legal".

Again, if I take one of this shared strings, in whichever environment
running row now, and I secretly attach a privatte "joaot/blargh"
property to it, there is very very low likelyhood that that will hurt
anybody.

You seem to be worrying about re-setting the 'face' property (a public
property by excellence) and that's the very same bug I experienced and
have described early.  It's not even a hard bug to see.  Just remove the
copy-sequence in `completion-pcm--hilit-commonality' and see strange
stuff happening.

But if you set some other property, _that_ bug _doesn't_ occur.  Do some
other bugs occur?  I don't know, I don't think we'll ever know, for
_any_ change.

Furthermore, there are other ways to forego the copying in
`completion-pcm--hilit-commonality and not even touch _ANY_ string
property.

> Suddenly deciding to stop supporting them would be a major API
> breakage with consequences that are hard to predict. And while I
> perhaps agree that it's an inconvenience, I don't think it's a choice
> we can simply make as this stage in c-a-p-f's development.
>
> To give an example of a subtle consequence:
>
>   1. (setq s (symbol-name 'car))
>
>   2. (put-text-property 1 3 'face 'error s)
>
>   3. Switch to a buffer in fundamental mode
>
>   4. (insert (symbol-name 'car)) --> see the error face in the buffer

It's not even subtle :-) Yes this is why have seen from the beginning in
bug#48841.  I think it was even I who reported it to you.

The principle to follow can be summarized as this: "Don't touch values
of properties you don't own in objects you don't own."

So just don't touch the 'face' property in things you don't own!  But
feel free to touch the "dmitry/blargh" property even in objects you
don't own.

So 'c-p--h-l' doesn't "own" face.  So it must either create an object
that it owns or set something that it does own.  'completion-score' is
"owned" by 'c-p--h-l'.  Only it can write it (though others can read
it).

> Now imagine that some completion table collects symbol names by
> passing obarray through #'symbol-name rather than #'all-completions,
> and voila, if the completion machinery adds properties (any
> properties, not just face) to those strings, you have just modified a
> bunch of global values. That's not good.

Why?  Maybe I'm missing something.  Why is adding properties -- that
no-one but the completion machinery knows about -- to those shared
strings "not good"?  What bad thing can happen if I do?

> And in the example above, the values are those that the
> lispref/objects.texi says we should not change (though it gives
> (symbol-name 'cons) as example). "Not mutable", in its parlance. IIRC
> the related discussions mentioned that modifying such values could
> lead to a segfault in some previous Emacs versions. Maybe not anymore,
> but it's still not a good idea.

You're extrapolating "change" to also include attaching properties to
symbols.  I think that document just means that you can't do stuff like

    (aset "cons" 0 ?z)

or

    (aset (symbol-name 'cons) 0 ?z)

I don't think it means you can't

    (put-text-property 0 1 'joaot/muahahah 42 (symbol-name 'cons))

But maybe Eli or someone else more knowledgeable in the deep internals
of Emacs can correct me.

If indeed I'm wrong, there are other ways to forego the copying in
`c-p---hilit-commonality` and still don't incurr in any such "mutation".
We must keep our eyes on the prize: copying -- not property-attaching --
is the real bummer here.

scratch/icomplete-lazy-highlight-attempt-2, although still incomplete,
is one such approach, though it still sets `completion-score` on the
"shared" string, used later for sorting.  But also that could be
prevented (again, only if it turns out to be actually problematic IMO).

João

PS: Maybe I've not stated it clearly enough: I *don't* object to -- or
endorse -- Daniel's patch.  My point was solely that it mixes too many
things for me to be intellectually able to review its functional merits,
and that those things should be separated into multiple problems and
patches to make this evaluation easier.  Maybe someone with superior
intellecutal capacity can review -- on substance -- as it stands.

See my other reply containing benchmarks.  Daniel's patch doesn't
perform well there, but for all I know, it can co-exist with my
non-copying approach, and we can all have our cake.