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

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

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

> If the hook function does set a dynamic variable that's hooked, and
> has no terminating condition for the recursion, you'll immediately
> find out when it exceeds max-lisp-eval-depth. So, don't do that. ;-)

That's right.

> So the user can notice, during debugging, if setq is setting the symbol
> in a different environment than the one he intended,

OK, so it's just informative, thanks.

>> Hmm, no symbol-setter-function should be a variable (holding
>> a function), modified via add-function/remove-function.
> I don't see why; the only difference is using add-function with an unquoted
> variable name vs. using advice-add with a quoted function name. It also
> makes the hook run a bit slower.

So far our coding style is to use *-hook, *-functions, and *-function
for places where we expect that it's normal to override/modify existing
behavior, whereas add-advice/defadvice is restricted to "last recourse".

> And it results in the help page for the function exposing the advice
> mechanism's internal representation of the advice, rather than cleanly
> showing e.g. ⌜:before advice: `mywatcher'⌝.

I agree that this is unfortunate, and I hope we can improve the *Help*
for those cases.

In the future we may also use `defgeneric' for such "extension points".

> But anyway I changed it to what you want, IIUC. I hope I misunderstood.

Thanks.

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

> Unless maybe you want to have multiple pieces of advice wrapped around
> each other, all blocking/overriding attempts to set variables?

That's the intention, yes.

> +  /* When masked with SYMBOL_CONSTANT_MASK, non-zero means symbol is
> +     constant, i.e. changing its value should signal an error.
> +     When masked with SYMBOL_HOOKED_MASK, non-zero means setting
> +     symbol will run varhook.  These two fields are combined into one
> +     in order to optimize the fast path of unhooked non-constants by
> +     having only one conditional branch for that case.  */

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

> +DEFUN ("void-p", Fvoid_p, Svoid_p, 1, UNEVALLED, 0,
> +       doc: /* Return t if ARG has no value.
> +If ARG is a non-lexical variable, this is equivalent to
> +(not (boundp (quote ARG))).
> +
> +Unlike `boundp', this function can also test a lexical variable.

I doubt this works on lexical-vars in compiled code.  Why do we need it?

> +This is the only built-in Elisp function that does not return a value.
> +Returning the result of this function enables any other function
> +to avoid returning a value.

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.
Why do we need that?

> +  DEFVAR_LISP ("symbol-setter-function", Vsymbol_setter_function,
[...]
> +If the variable is currently void, OLDVAL will be void.

Ah, right, I see.  No, we don't want to do that.  I see some
alternatives:
- one is we don't provide OLDVAL at all (let the Elisp code get it
  using `symbol-value' or `symbol-default' according to ENV, instead).
- OLDVAL is either a list of one element containing the old value, or
  nil (when that old value is Qunbound).
- Use a special value to represent "unbound", e.g. a special keyword like
  `::value--unbound::'.
Of course, the same holds for NEWVAL except that the code can't get
NEWVAL via `symbol-value', so for NEWVAL we can't choose option 1.

> +  Vsymbol_setter_function =
> +    list4 (Qclosure, list1 (Qt), list4 (Qsym, Qenv, Qoldval, Qnewval), Qnewval);

You can make it Qnil here and set it to a real value in subr.el, so as
to avoid having to hard-code the internal representation of
a particular function.

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.


        Stefan