bug#47992: [External] : bug#47992: 27; 28; Phase out use of `equal` in `add-hook`, `remove-hook`

[Drew Adams <drew.adams@oracle.com>]

> (Follow-up to bug#46326 as suggested by Stefan Monnier)
> 
> The functions `add/remove-hook` make use of `equal` to test equality of
> hooks. Using `equal` can lead to excessive memory allocations
> (bug#46326) or hangups (see comment in `set-transient-map`), when large
> closures or cyclic closures are used as hooks.
> 
> Right now there are at least three places which have to work around the
> use of `equal` in `add/remove-hook` using a symbol indirection:
> 
> * `set-transient-map`
> * `minibuffer-with-setup-hook`
> * `eval-after-load`
> 
> It would be good to change `add/remove-hook` such that it only relies
> on `eq` to test hook equality. Then the symbol indirection workarounds
> can be avoided.
> 
> However making such a change directly can lead to subtle breakage.
> Perhaps one could introduce some deprecation behavior first, before
> making the final change to `eq`.  If a hook is added/removed and the
> added/removed object is not found via `eq` but found via `equal`, show
> a deprecation warning?

So instead of just advising users not to use lambda forms
(which makes sense), you'd make it no longer work at all
for interpreted lambda forms (except rare cases where
they might actually be `eq' - e.g., same list structure)?

Perhaps `equal' can be fixed to do something better with
closures?  E.g., if the `eq' test in `equal' fails for a
closure arg then return nil?  (I'm not proposing that.)

Either closure equality needs an `equal' comparison or it
doesn't, no?  It sounds like the effect of what you're
suggesting would be for `eq' to be the closure equality
test - but only for `add|remove-hook' (?).

What's so wrong with the cases you mention using a symbol?
["work around the use of `equal` in `add/remove-hook`
using a symbol indirection"]

Isn't that, in effect, what all uses of `add|remove-hook'
would have to do after your proposal - either that or
make the use amenable to `eq' in some other way?  (Does
byte-compilation of two structurally equivalent lambda
forms generally produce `eq' results?)

I'm no doubt missing something in the motivation for
this change.  It sounds like sacrificing programmatic
flexibility for some performance optimization.  Can you
elaborate on why this is needed (worth it)?

`set-transient-map' not being able to use `letrec',
because of the `add-hook' equality test, doesn't sound
like a good reason to change `add-hook'.

The point of `equal' is to test with `eq' first, then
if nil go beyond that to test for equal structure.  Of
course, real functions don't have structure, and real
function equality is altogether problematic.  But this
is Lisp, and some Lisp representations of "functions",
at least when interpreted, do have structure (list,
string, vector).

When I look at bug #46326, I see this wrt the problem
(motivation):

"The issue can be mitigated by using a modified version
of minibuffer-with-setup-hook, where I am creating a
symbol and fsetting  instead of adding a lambda directly
via add-hook."

and

"It is the add-hook implementation or more precisely
the minibuffer-with-setup-hook implementation which
is responsible for the excessive allocations."

So it sounds like it's not really about `add-hook';
it's about `minibuffer-with-setup-hook'.

And it looks like your `m-w-s-h' replacement does
just what you'd require everything that uses
`add|remove-hook' to do: replace a lambda form by a
symbol (or equivalent workaround to get `eq'-ness).

You also say this in bug #46326, as possible
alternative remedies:

"1. Replace minibuffer-with-setup-hook with my version
    if you think my version is better and an acceptable fix.
 2. Investigate the reasons why add-hook with priorities
    somehow copies large closures during sorting. This
    is unacceptably costly."

And Stefan says, there:

"IOW I think the better fix is to change
`minibuffer-with-setup-hook` to use an indirection
via a symbol."

And that fix was already pushed.  Why instead now
propose changing `add|remove-hook' to use only `eq'?