Re: [PATCH] (Updated) Run hook when variable is set

[Kelly Dean <kelly@prtime.org>]

Stefan Monnier wrote:
>> The docstring for symbol-setter-function says lexical binding is required
>> for the hook function,
>
> I don't think that's true, BTW.  I agree it's recommended (I recommend
> using lexical-binding for all new code), but other than that it should
> work just fine with dynamically scoped code.

It was to enable the standard function to always just return newval, even if it's Qunbound. Works for lexical binding (the interpreter doesn't barf), but not for dynamic. But that doesn't matter anymore, since you said I need to use a different way of handling Qunbound.

>> The docstring already says to use :before if you just need to watch
>> variables,
>
> It shouldn't either.  This issue of which kind of advice to use is not
> specific to this variable, so there's no need to put it there.

It's an unusual function. The effect of overriding the return value, and interaction with «let», and how to deal with the case of makunbound, all need to be documented somewhere, and for completeness, it makes sense for the documentation to also cover the case of not overriding the return value. If not in the docstring, then maybe in the manual?

> Actually I don't see these as two independent elements combined
> into one.  I see it as a single value with 3 levels:
> - any write you want
> - write vetted by Elisp code
> - no write at all

If there's just :before advice (e.g. a profiler), then the write is not vetted.

> We could even drop the last setting and implement it in Elisp via
> symbol-setter-function, except that it's currently used for variables
> where we'd rather not give Elisp the opportunity to allow writes.

Yeah, changing the value of t or nil would be quite fun. ;-)

>> +DEFUN ("void-p", Fvoid_p, Svoid_p, 1, UNEVALLED, 0,
[snip]
> I doubt this works on lexical-vars in compiled code.  Why do we need it?

It was to enable checking for Qunbound for lexical newval and oldval in symbol-setter-function.

I've removed it.

> So far we've been extra careful to try and make sure Qunbound doesn't
> escape into Elisp world.  I don't think we want to reconsider this
> design choice, since adding this Qunbound as a "normal" value in Elisp
> will just create the need for another special "really unbound" value.

Qunbound already escapes: you just have to use makunbound instead of «set» to set it, and boundp instead of symbol-value to read it. Lexical variables are second-class because they can't be set to all the values that dynamic variables can be; the purpose of my «void» function was to fix that.

Of course, you'll say makunbound doesn't set a value; it unbinds the variable. But that's not true! For example, consider this:
(defvar foo 0)
(defvar bar 1)
(let ((foo 2)) bar) → 1
(let ((foo 2)) (makunbound 'foo) foo) → Lisp error: (void-variable foo)
foo → 0

In the dynamic environment created by the first «let», foo is bound, but bar is not bound. Therefore an attempt to read the not-bound bar within that environment will look in outer environments until it finds bar bound in one of them. An error would occur if bar weren't bound in any of those outer environments, not even the global one.

In the environment created by the second «let», foo is first bound to 2. If it were unbound, then an attempt to read it would return 0, for the same reason that an attempt to read bar in the first environment returns 1. So what does makunbound do? If it really unbound foo, then the subsequent attempt to read foo would return 0, because foo would be not bound in the dynamic environment. But in fact the attempt to read foo returns Qunbound, which was the value set by makunbound, and the interpreter barfs because you didn't use the special function boundp to read that special value. IOW, Qunbound escapes into the Lisp world.

Then when the «let» exits, the dynamic environment is destroyed, which actually _does_ unbind foo (from Qunbound). So now, the attempt to read foo returns 0, because global foo is no longer shadowed by the dynamic binding of foo to the value Qunbound.

I agree that setting lexicals to Qunbound is absurd. «void» was just to enables lexicals and dynamics to be set to the same values. My point is that letting Qunbound escape into the Lisp world in the first place is also absurd. (CL does it too.) Makunbound should actually make a variable be unbound, not bind it to some special value that triggers an error (and lies about the cause) when you try to read it. Because it makes no sense to actually unbind a dynamic or lexical variable except by destruction of the environment in which the variable occurs, this means makunbound should apply only to global variables.

Because global variables don't shadow anything, binding them to Qunbound is the same as unbinding them so far as Lisp code is concerned, so this implementation detail doesn't escape. And Lisp doesn't let you bind lexicals to Qunbound, so they don't have a problem either. But it _does_ let you bind dynamic variables to Qunbound (and Elisp lets you bind buffer-locals to Qunbound too), which is a problem because they shadow variables in outer environments. I know it's officially been a feature for a long time. But a bug by any other name is still a bug.

I've removed «void». I recommend making makunbound stop working on non-globals too.

> I see some alternatives:
[snip]
> - OLDVAL is either a list of one element containing the old value, or
>   nil (when that old value is Qunbound).

Then run_varhook must cons. That'll generate a lot of garbage if you use it for profiling, or for debugging in a way where you don't just pause to inspect every hooked variable. Is that ok?

> - Use a special value to represent "unbound", e.g. a special keyword like
>   `::value--unbound::'.

Then varhook would be defective. If the hook function returns a special keyword to represent Qunbound (to avoid blocking makunbound), then just hooking unavoidably changes the behavior of Emacs (because it's impossible to set a hooked variable to that keyword), which is the wrong thing to do.

Of course, telling the hook function that a variable is bound to Qunbound without having to cons is easy (could just pass an additional argument, t or nil). The problem is needing to _return_ Qunbound. If run_varhook will cons, then the hook function can just return nil to represent Qunbound, or do (setcar newval ...) and then return newval to set the variable to another value without needing yet another cons.

Or to avoid needing to cons in run_varhook, it could specbind something like Qsymbol_newval_void (to t if the new value is Qunbound), so the hook function could setq symbol-newval-void to t or nil to indicate whether to set the hooked variable to Qunbound. This way, run_varhook could pass the newval argument directly (as it currently does), without wrapping it, except pass nil as a dummy value if the new value is Qunbound. This is what I prefer, but if you don't want me to do it this way, then I'll wrap newval.

> Oh, and I think we can keep the name `constant' rather than use the
> verbose `constant_or_hooked'.  It's not perfect, but at least it reduces
> the patch's size and the number of lines that are too long.

Would ⌜const_hooked⌝ be ok? It's only 4 extra characters. Using ⌜constant⌝ for something that doesn't just mean «constant» is as gross as ⌜set-default⌝ for «set-global». As noted above, hooked doesn't imply vetted, so hooked doesn't mean sort-of-constant. Even if it did, ⌜constant⌝ is a misleading term for something that can be sort-of-constant; ⌜constantness⌝ would be better, since the term allows for the possibility of a range of values. If it's too long, ⌜constness⌝ is only 1 extra character.

The patch is big, but it'll just be a one-time change to the source code. After that, nobody reading the code will care what the patch was, and everybody will be stuck with the misleading name if it isn't changed.