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

[Kelly Dean]

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.