Stefan Monnier wrote:
> We don't want to install this in emacs-24, so only the trunk (aka
> "master") code is important in this respect.

Understood. Last time I included the 24.4 patch just to ensure you could reproduce my benchmark results. (Due to some logistical issues, I can't run trunk on a system that has stable performance, so I can't reliably benchmark it.)

>> +typedef enum
>> +  {
>> +    Dyn_Unbind = -1,
>> +    Dyn_Current = 0,
>> +    Dyn_Bind = 1,
>> +    Dyn_Skip = 2,
>> +    Dyn_Global = 3
>> +  } Dyn_Bind_Direction;
>
> In which sense is this a "direction"?

Originally I had just the first three values, and it was a direction in the sense of movement up or down the dynamic-binding stack. Later I discovered that I needed the last two values too. I've changed it to a more appropriate name.

> That's a good idea, to circumvent the question of how to not-trigger the
> hooked-p check recursively when the hook function calls the setter (tho
> the question partly remains, in case the hook function *accidentally*
> sets one of the hooked variables).

The docstring for symbol-setter-function says lexical binding is required for the hook function, which means its local variables won't trigger varhook. 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. ;-)

> It does mean that the hooks can't redirect the assignment elsewhere, but
> maybe it's a good thing anyway.

They still can. Just temporarily unhook the destination variable before setting it. But yes, it would be easy to introduce bugs by doing that, so I guess the documentation should discourage it.

>> +	if (shadowed) env = Qdyn_local;
>> +	else if (buf_local) env = Qbuf_local;
>> +	else env = Qglobal;
>
> Why does the hook need to know about those different cases?

So the user can notice, during debugging, if setq is setting the symbol in a different environment than the one he intended, e.g. due to an unexpected buffer-local variable or due to a missing buffer-local variable, or due to an unexpected dynamic binding of a global variable in code that calls code that uses setq with the assumption that the global (not a dynamic local) variable will be set. Also, this enables detailed profiling of globals vs. buffer-locals vs. dynamic bindings. And it lets your hook function filter out the cases you want to ignore, e.g. if you only want to watch global settings, not buffer-local or let-local.

>> +DEFUN ("symbol-setter-function", Fsymbol_setter_function, Ssymbol_setter_function, 4, 4, 0,
>
> 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. 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'⌝. That seems like a bad idea.

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

> Also the docstring should not recommend :override (which should be
> a rather rare case, the more useful cases are probably :before
> and :around)

The docstring already says to use :before if you just need to watch variables, but not block or override attempts to set them. And :around is the same as :override in this case, since all the standard function does is return newval, which is overridden by either :around or :override; the standard function doesn't do any processing. Unless maybe you want to have multiple pieces of advice wrapped around each other, all blocking/overriding attempts to set variables?

Anyway I changed the docstring to recommend :around instead of :override.

I also made the other changes you wanted. And I cleaned up the patch a bit, consolidating set_internal and set_internal_1, so the extra name isn't needed anymore. (This is just a cosmetic change; it doesn't affect the compiled code, since one was just a wrapper for the other and both were inlined.) And I fixed a bug: it was reporting the wrong environment for setq-default if you do:
(setq-local foo 'bar)
(let ((foo 'baz)) (setq-default foo 'biz))

Updated patch attached.