bug#62419: 28.2; Elisp let-bound buffer-local variable and kill-local-variable

[Matthew Malcomson]

> On 25 Mar 2023, at 11:49, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Matthew Malcomson <hardenedapple@gmail.com>
>> Date: Fri, 24 Mar 2023 13:37:57 +0000
>> 
>> 
>> 
>> I think the final state after having done the above should be:
>> 1) Global value has not changed.
>> 2) Local value has not changed.
>> 
>> While the observed state after the above is:
>> 1) Global value has been set to `other-symbol'.
>> 2) Local value has been removed.
>> 
>> - The `setq` inside the `let` sets the *global* value rather than creating a 
>> buffer-local variable.
>> - The `let` on the buffer-local version of the variable is lost.
> 
> What is the purpose of doing this? what are you trying to accomplish,
> and why?
> 

I came across this when looking into that yasnippet bug I linked in my previous 
email.
This sequence is not performed on purpose.
The let binding and kill-local-variable happen outside that plugins control, 
and are unexpected.

The specific context is:
When yasnippet minor mode is turned on it records the “original” 
`auto-fill-function` and replaces it with its own wrapper using `setq`.
It uses the “original” to `funcall` from the wrapper and to resetting when 
yasnippet minor mode turned off.
Yasnippet sets `auto-fill-function` to its wrapper using ’setq’ to ensure the 
value is buffer-local.

AIUI the minor mode is usually turned on outside of any let binding, but the 
described situation happens due to an edge-case:
- `newline` let-binds `auto-fill-mode`
- Visited file has changed outside of emacs, so user is queried whether to 
`revert-buffer` while in `newline`
- `revert-buffer` calls `normal-mode`, which runs `kill-all-local-variables`
- Later, `yasnippet-mode` is turned on and the `setq` is evaluated.

N.b. The change I’m suggesting only means that whatever was outside the `let` 
binding is kept.
That implies that there would still likely be some problem edge-case in the 
yasnippet code.
I’m just raising the bug on the premise that the existing behaviour seems 
likely to cause harder to debug problems that my suggestion.
I.e. once at the top-level, seeing a `setq` of a buffer-local variable has 
changed the top-level global value is very surprising to me.
But not seeing the effect of a `setq` because it was performed inside some 
unexpected `let` binding is less surprising.
So I believe that debugging unexpected behaviour due to the second would be 
easier than behaviour due to the first.


>> The manual for `make-variable-buffer-local` — in `(elisp) Creating 
>> Buffer-Local` — does mention that if a variable is in a `let` binding then a 
>> `setq` does not create a buffer-local version.
>> That said, I’m guessing the intention of this behaviour is so a `let` 
>> binding on a global variable is modified rather than bypassed by a `setq`.
>> I’d suggest that is not relevant to the above code since the use of 
>> `kill-local-variable` means the `let` is effectively lost already (e.g. it 
>> does not get “reset” on an unwind).
> 
> Did you also read about default-toplevel-value and
> set-default-toplevel-value (in the "Default Value" node of the ELisp
> manual)?

Honestly I haven’t looked hard into how to fix the bug I was hitting in elisp.
A solution seems like it would have to answer the more general question of what 
yasnippet should do if it’s minor-mode is turned on while in a `let` binding of 
`auto-fill-mode`, and I’m not a yasnippet developer.
I'm reporting this on the premise that the elisp behaviour seemed strange 
enough to me to ask whether it should be different.

> 
>> I’m not proposing this as a change, just including it for extra context 
>> about the cause.
>> I *believe* that the form of the C code around here looks like the special 
>> case of a forwarded variable not having a buffer-local value but having a 
>> buffer-local `let` binding could easily have been an oversight rather than 
>> intention.
> 
> Stefan, any comments?