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

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

[Matthew Malcomson]

[-- Attachment #1: Type: text/plain, Size: 3040 bytes --]

Hello,

I’m inlining some elisp which has behaviour I find unintuitive.
To view the bug I would run each top-level form with C-x C-e in turn in an elisp buffer.
This behaviour may be expected — the manual mentions something related — but I believe this is an unintended edge-case.
N.b. the use of `auto-fill-function’ is just for a variable which turns buffer-local when set, not as anything related to this particular symbol.
FWIW I believe this behaviour to be the root cause of https://github.com/joaotavora/yasnippet/issues/919 (which was closed due to not being able to reproduce it).

—————
;; Ensure that `auto-fill-function' has a buffer-local version and a global
;; version.
(setq auto-fill-function 'local-symbol)
(describe-variable 'auto-fill-function)
;; `auto-fill-function' is let-bound in the buffer scope
(let ((auto-fill-function 'temp-symbol))
  ;; Now there is no buffer-local variable for `auto-fill-function', but the
  ;; `let' unwrapping info is still there.
  (kill-local-variable 'auto-fill-function)
  ;; Since the check in the emacs source is
  ;; a) Is there a buffer-local variable.
  ;; b) Is there a let-binding shadowing the current variable.
  ;; Then this `setq' sets the *global* variable.
  (setq auto-fill-function 'other-symbol))
;; Exiting the `let' has special handling to avoid resetting a local variable
;; when the local variable was `let' bound, which means that overall the `setq'
;; set the global variable and the `let' has been lost.
(describe-variable 'auto-fill-function)
—————


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.

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

I’m attaching the source code hack that I’ve started using to work around this.
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.
(N.b. for this hack I guessed the meanings of the `let` enum values).


[-- Attachment #2: emacs-patch.diff --]
[-- Type: application/octet-stream, Size: 1872 bytes --]

diff --git a/src/data.c b/src/data.c
index 57205d8..af427ea 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1607,7 +1607,7 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
 	    if (idx > 0 && bindflag == SET_INTERNAL_SET
 	        && !PER_BUFFER_VALUE_P (buf, idx))
 	      {
-		if (let_shadows_buffer_binding_p (sym))
+		if (let_shadows_buffer_binding_p_and_is_global (sym))
 		  set_default_internal (symbol, newval, bindflag);
 		else
 		  SET_PER_BUFFER_VALUE_P (buf, idx, 1);
diff --git a/src/eval.c b/src/eval.c
index d002e81..9200d71 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -3491,6 +3491,25 @@ let_shadows_buffer_binding_p (struct Lisp_Symbol *symbol)
   return 0;
 }
 
+bool
+let_shadows_buffer_binding_p_and_is_global (struct Lisp_Symbol *symbol)
+{
+  union specbinding *p;
+  Lisp_Object buf = Fcurrent_buffer ();
+
+  for (p = specpdl_ptr; p > specpdl; )
+    if ((--p)->kind == SPECPDL_LET_DEFAULT)
+      {
+	struct Lisp_Symbol *let_bound_symbol = XSYMBOL (specpdl_symbol (p));
+	eassert (let_bound_symbol->u.s.redirect != SYMBOL_VARALIAS);
+	if (symbol == let_bound_symbol
+	    && EQ (specpdl_where (p), buf))
+	  return 1;
+      }
+
+  return 0;
+}
+
 static void
 do_specbind (struct Lisp_Symbol *sym, union specbinding *bind,
              Lisp_Object value, enum Set_Internal_Bind bindflag)
diff --git a/src/lisp.h b/src/lisp.h
index 0cad97c..0a296cd 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4221,6 +4221,7 @@ extern void mark_specpdl (union specbinding *first, union specbinding *ptr);
 extern void get_backtrace (Lisp_Object array);
 Lisp_Object backtrace_top_function (void);
 extern bool let_shadows_buffer_binding_p (struct Lisp_Symbol *symbol);
+extern bool let_shadows_buffer_binding_p_and_is_global (struct Lisp_Symbol *symbol);
 
 /* Defined in unexmacosx.c.  */
 #if defined DARWIN_OS && defined HAVE_UNEXEC

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

[Eli Zaretskii]

> From: Matthew Malcomson <hardenedapple@gmail.com>
> Date: Fri, 24 Mar 2023 13:37:57 +0000
> 
> I’m inlining some elisp which has behaviour I find unintuitive.
> To view the bug I would run each top-level form with C-x C-e in turn in an elisp buffer.
> This behaviour may be expected — the manual mentions something related — but I believe this is an unintended edge-case.
> N.b. the use of `auto-fill-function’ is just for a variable which turns buffer-local when set, not as anything related to this particular symbol.
> FWIW I believe this behaviour to be the root cause of https://github.com/joaotavora/yasnippet/issues/919 (which was closed due to not being able to reproduce it).
> 
> —————
> ;; Ensure that `auto-fill-function' has a buffer-local version and a global
> ;; version.
> (setq auto-fill-function 'local-symbol)
> (describe-variable 'auto-fill-function)
> ;; `auto-fill-function' is let-bound in the buffer scope
> (let ((auto-fill-function 'temp-symbol))
>   ;; Now there is no buffer-local variable for `auto-fill-function', but the
>   ;; `let' unwrapping info is still there.
>   (kill-local-variable 'auto-fill-function)
>   ;; Since the check in the emacs source is
>   ;; a) Is there a buffer-local variable.
>   ;; b) Is there a let-binding shadowing the current variable.
>   ;; Then this `setq' sets the *global* variable.
>   (setq auto-fill-function 'other-symbol))
> ;; Exiting the `let' has special handling to avoid resetting a local variable
> ;; when the local variable was `let' bound, which means that overall the `setq'
> ;; set the global variable and the `let' has been lost.
> (describe-variable 'auto-fill-function)
> —————
> 
> 
> 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?

> 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)?

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

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?

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> (setq auto-fill-function 'local-symbol)
> (describe-variable 'auto-fill-function)
> ;; `auto-fill-function' is let-bound in the buffer scope
> (let ((auto-fill-function 'temp-symbol))
>   ;; Now there is no buffer-local variable for `auto-fill-function', but the
>   ;; `let' unwrapping info is still there.
>   (kill-local-variable 'auto-fill-function)
>   ;; Since the check in the emacs source is
>   ;; a) Is there a buffer-local variable.
>   ;; b) Is there a let-binding shadowing the current variable.
>   ;; Then this `setq' sets the *global* variable.
>   (setq auto-fill-function 'other-symbol))
> ;; Exiting the `let' has special handling to avoid resetting a local variable
> ;; when the local variable was `let' bound, which means that overall the `setq'
> ;; set the global variable and the `let' has been lost.

AFAIK the behavior is "as intended": the `let` only affects *one*
binding, either the global one or the buffer-local one.

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

The "`newline` let-binds `auto-fill-mode`" seems like the source of the
problem :-(

As for yasnippet.el, I suspect that I patch like the one below would
avoid the problem.


        Stefan


diff --git a/yasnippet.el b/yasnippet.el
index 78ef38ac39..98124c7a61 100644
--- a/yasnippet.el
+++ b/yasnippet.el
@@ -860,10 +828,9 @@ which decides on the snippet to expand.")
   "Hook run when `yas-minor-mode' is turned on.")
 
 (defun yas--auto-fill-wrapper ()
-  (when (and auto-fill-function
-             (not (eq auto-fill-function #'yas--auto-fill)))
-    (setq yas--original-auto-fill-function auto-fill-function)
-    (setq auto-fill-function #'yas--auto-fill)))
+  (when auto-fill-function ;Turning the mode ON.
+    (cl-assert (local-variable-p 'auto-fill-function))
+    (add-function :around (local 'auto-fill-function) #'yas--auto-fill)))
 
 ;;;###autoload
 (define-minor-mode yas-minor-mode
@@ -906,8 +873,8 @@ Key bindings:
          ;; auto-fill handler.
          (remove-hook 'post-command-hook #'yas--post-command-handler t)
          (remove-hook 'auto-fill-mode-hook #'yas--auto-fill-wrapper)
-         (when (local-variable-p 'yas--original-auto-fill-function)
-           (setq auto-fill-function yas--original-auto-fill-function))
+         (when (local-variable-p 'auto-fill-function)
+           (remove-function (local 'auto-fill-function) #'yas--auto-fill))
          (setq emulation-mode-map-alists
                (remove 'yas--direct-keymaps emulation-mode-map-alists)))))
 
@@ -3880,7 +3847,7 @@ field start.  This hook does nothing if an undo is in progress."
                    snippet (yas--snippet-field-mirrors snippet)))
       (setq yas--todo-snippet-indent nil))))
 
-(defun yas--auto-fill ()
+(defun yas--auto-fill (orig-fun &rest args)
   ;; Preserve snippet markers during auto-fill.
   (let* ((orig-point (point))
          (end (progn (forward-paragraph) (point)))
@@ -3897,44 +3864,7 @@ field start.  This hook does nothing if an undo is in progress."
             reoverlays))
     (goto-char orig-point)
     (let ((yas--inhibit-overlay-hooks t))
-      (if yas--original-auto-fill-function
-          (funcall yas--original-auto-fill-function)
-        ;; Shouldn't happen, gather more info about it (see #873/919).
-        (let ((yas--fill-fun-values `((t ,(default-value 'yas--original-auto-fill-function))))
-              (fill-fun-values `((t ,(default-value 'auto-fill-function))))
-              ;; Listing 2 buffers with the same value is enough
-              (print-length 3))
-          (save-current-buffer
-            (dolist (buf (let ((bufs (buffer-list)))
-                           ;; List the current buffer first.
-                           (setq bufs (cons (current-buffer)
-                                            (remq (current-buffer) bufs)))))
-              (set-buffer buf)
-              (let* ((yf-cell (assq yas--original-auto-fill-function
-                                    yas--fill-fun-values))
-                     (af-cell (assq auto-fill-function fill-fun-values)))
-                (when (local-variable-p 'yas--original-auto-fill-function)
-                  (if yf-cell (setcdr yf-cell (cons buf (cdr yf-cell)))
-                    (push (list yas--original-auto-fill-function buf) yas--fill-fun-values)))
-                (when (local-variable-p 'auto-fill-function)
-                  (if af-cell (setcdr af-cell (cons buf (cdr af-cell)))
-                    (push (list auto-fill-function buf) fill-fun-values))))))
-          (lwarn '(yasnippet auto-fill bug) :error
-                 "`yas--original-auto-fill-function' unexpectedly nil in %S!  Disabling auto-fill.
-  %S
-  `auto-fill-function': %S\n%s"
-                 (current-buffer) yas--fill-fun-values fill-fun-values
-                 (if (fboundp 'backtrace--print-frame)
-                     (with-output-to-string
-                       (mapc (lambda (frame)
-                               (apply #'backtrace--print-frame frame))
-                             yas--watch-auto-fill-backtrace))
-                   ""))
-          ;; Try to avoid repeated triggering of this bug.
-          (auto-fill-mode -1)
-          ;; Don't pop up more than once in a session (still log though).
-          (defvar warning-suppress-types) ; `warnings' is autoloaded by `lwarn'.
-          (add-to-list 'warning-suppress-types '(yasnippet auto-fill bug)))))
+      (apply orig-fun args))
     (save-excursion
       (setq end (progn (forward-paragraph) (point)))
       (setq beg (progn (backward-paragraph) (point))))

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> The "`newline` let-binds `auto-fill-mode`" seems like the source of the
> problem :-(

We could fix it with a patch like the one below (intended for `master`).


        Stefan


diff --git a/lisp/simple.el b/lisp/simple.el
index 3e50e888dad..6f0215bfb1d 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -659,7 +659,7 @@ newline
          (beforepos (point))
          (last-command-event ?\n)
          ;; Don't auto-fill if we have a prefix argument.
-         (auto-fill-function (if arg nil auto-fill-function))
+         (inhibit-auto-fill (or inhibit-auto-fill arg))
          (arg (prefix-numeric-value arg))
          (procsym (make-symbol "newline-postproc")) ;(bug#46326)
          (postproc
@@ -9269,11 +9269,15 @@ default-indent-new-line
        ;; If we're not inside a comment, just try to indent.
        (t (indent-according-to-mode))))))
 
+(defvar inhibit-auto-fill nil
+  "Non-nil means to do as if `auto-fill-mode' was disabled.")
+
 (defun internal-auto-fill ()
   "The function called by `self-insert-command' to perform auto-filling."
-  (when (or (not comment-start)
-            (not comment-auto-fill-only-comments)
-            (nth 4 (syntax-ppss)))
+  (when (and (not inhibit-auto-fill)
+             (or (not comment-start)
+                 (not comment-auto-fill-only-comments)
+                 (nth 4 (syntax-ppss))))
     (funcall auto-fill-function)))
 
 (defvar normal-auto-fill-function #'do-auto-fill

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

[Matthew Malcomson]

> On 26 Mar 2023, at 15:01, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> (setq auto-fill-function 'local-symbol)
>> (describe-variable 'auto-fill-function)
>> ;; `auto-fill-function' is let-bound in the buffer scope
>> (let ((auto-fill-function 'temp-symbol))
>>  ;; Now there is no buffer-local variable for `auto-fill-function', but the
>>  ;; `let' unwrapping info is still there.
>>  (kill-local-variable 'auto-fill-function)
>>  ;; Since the check in the emacs source is
>>  ;; a) Is there a buffer-local variable.
>>  ;; b) Is there a let-binding shadowing the current variable.
>>  ;; Then this `setq' sets the *global* variable.
>>  (setq auto-fill-function 'other-symbol))
>> ;; Exiting the `let' has special handling to avoid resetting a local variable
>> ;; when the local variable was `let' bound, which means that overall the `setq'
>> ;; set the global variable and the `let' has been lost.
> 
> AFAIK the behavior is "as intended": the `let` only affects *one*
> binding, either the global one or the buffer-local one.
> 

Not going to push much on this since your suggested change to `newline` would fix everything to me.
But the part I think is strange is `setq` not creating a buffer-local binding in this environment.
(i.e. not to do with what `let` is affecting).

The “special behaviour” that a `setq` may act on a global binding of an automatic buffer-local variable when inside a let binding seems to me like the intention is to avoid “bypassing” a let on a global binding.

I.e. currently the behaviour of `setq` on automatic	buffer-local variables is:
	- Outside `let`, always affect buffer-local (creating if necessary)
	- In `let` of global binding, affect global binding.
	- In `let` of buffer-local binding, affect buffer-local
	- In `let` of buffer-local binding but where buffer-local value has been killed, affect global value.

I believe that last condition is strange and the behaviour of `setq` would be more understandable without it.
Especially since when viewed from the top-level (i.e. evaluate some lisp which contains a `setq` of an automatic buffer-local variable and may or may not have a `let`) the options are:
	- If `setq` is outside any `let`, it will affect a buffer-local binding.
	- If `setq` is inside a `let` then it won’t be visible once exited the `let` (i.e. it affects the binding that the `let` acted on)
	- *Unless* the `let` was of a buffer-local binding and that buffer-local binding was killed, in which case the `setq` will affect the global binding.

Either way, thanks for looking into this!
Matthew

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> (setq auto-fill-function 'local-symbol)
>>> (describe-variable 'auto-fill-function)
>>> ;; `auto-fill-function' is let-bound in the buffer scope
>>> (let ((auto-fill-function 'temp-symbol))
>>>  ;; Now there is no buffer-local variable for `auto-fill-function', but the
>>>  ;; `let' unwrapping info is still there.
>>>  (kill-local-variable 'auto-fill-function)
>>>  ;; Since the check in the emacs source is
>>>  ;; a) Is there a buffer-local variable.
>>>  ;; b) Is there a let-binding shadowing the current variable.
>>>  ;; Then this `setq' sets the *global* variable.
>>>  (setq auto-fill-function 'other-symbol))
>>> ;; Exiting the `let' has special handling to avoid resetting a local variable
>>> ;; when the local variable was `let' bound, which means that overall the `setq'
>>> ;; set the global variable and the `let' has been lost.
>> 
>> AFAIK the behavior is "as intended": the `let` only affects *one*
>> binding, either the global one or the buffer-local one.
>> 
>
> Not going to push much on this since your suggested change to
> `newline` would fix everything to me.  But the part I think is strange
> is `setq` not creating a buffer-local binding in this environment.

Hmm... maybe you're right that the (setq auto-fill-function 'other-symbol)
shouldn't set the global variable but the local one.
It might be a bug in how we check whether there's a let-binding that
should make us refrain from obeying the "automatically set buffer-locally".

Good point.  I'll have to take a closer look.

> I.e. currently the behaviour of `setq` on automatic	buffer-local variables is:
> 	- Outside `let`, always affect buffer-local (creating if necessary)
> 	- In `let` of global binding, affect global binding.
> 	- In `let` of buffer-local binding, affect buffer-local
> 	- In `let` of buffer-local binding but where buffer-local value has
> been killed, affect global value.
>
> I believe that last condition is strange and the behaviour of `setq` would
> be more understandable without it.

Agreed.


        Stefan

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I.e. currently the behaviour of `setq` on automatic	buffer-local variables is:
> 	- Outside `let`, always affect buffer-local (creating if necessary)
> 	- In `let` of global binding, affect global binding.
> 	- In `let` of buffer-local binding, affect buffer-local
> 	- In `let` of buffer-local binding but where buffer-local value has
> been killed, affect global value.
>
> I believe that last condition is strange and the behaviour of `setq` would
> be more understandable without it.

I think the patch below would fix this corner case.


        Stefan


diff --git a/src/eval.c b/src/eval.c
index ef7ca33f834..82ada40b309 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -3364,7 +3364,7 @@ DEFUN ("fetch-bytecode", Ffetch_bytecode, Sfetch_bytecode,
   return object;
 }
 \f
-/* Return true if SYMBOL currently has a let-binding
+/* Return true if SYMBOL's default currently has a let-binding
    which was made in the buffer that is now current.  */
 
 bool
@@ -3379,6 +3379,7 @@ let_shadows_buffer_binding_p (struct Lisp_Symbol *symbol)
 	struct Lisp_Symbol *let_bound_symbol = XSYMBOL (specpdl_symbol (p));
 	eassert (let_bound_symbol->u.s.redirect != SYMBOL_VARALIAS);
 	if (symbol == let_bound_symbol
+	    && !p->kind == SPECPDL_LET_LOCAL /* bug#62419 */
 	    && EQ (specpdl_where (p), buf))
 	  return 1;
       }

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

[Matthew Malcomson]

Thanks!

Just for the record I’ve been running using essentially this patch (but with !=) since you suggested it and have not had any problems.

FWIW I also think the change to avoid `let` binding `auto-fill-function` in `normal-mode` is good.

Regards,
Matthew

> On 26 Mar 2023, at 16:30, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> I.e. currently the behaviour of `setq` on automatic	buffer-local variables is:
>> 	- Outside `let`, always affect buffer-local (creating if necessary)
>> 	- In `let` of global binding, affect global binding.
>> 	- In `let` of buffer-local binding, affect buffer-local
>> 	- In `let` of buffer-local binding but where buffer-local value has
>> been killed, affect global value.
>> 
>> I believe that last condition is strange and the behaviour of `setq` would
>> be more understandable without it.
> 
> I think the patch below would fix this corner case.
> 
> 
>        Stefan
> 
> 
> diff --git a/src/eval.c b/src/eval.c
> index ef7ca33f834..82ada40b309 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -3364,7 +3364,7 @@ DEFUN ("fetch-bytecode", Ffetch_bytecode, Sfetch_bytecode,
>   return object;
> }
> 
> -/* Return true if SYMBOL currently has a let-binding
> +/* Return true if SYMBOL's default currently has a let-binding
>    which was made in the buffer that is now current.  */
> 
> bool
> @@ -3379,6 +3379,7 @@ let_shadows_buffer_binding_p (struct Lisp_Symbol *symbol)
> 	struct Lisp_Symbol *let_bound_symbol = XSYMBOL (specpdl_symbol (p));
> 	eassert (let_bound_symbol->u.s.redirect != SYMBOL_VARALIAS);
> 	if (symbol == let_bound_symbol
> +	    && !p->kind == SPECPDL_LET_LOCAL /* bug#62419 */
> 	    && EQ (specpdl_where (p), buf))
> 	  return 1;
>       }
> 

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Just for the record I’ve been running using essentially this patch (but
> with !=) since you suggested it and have not had any problems.

Thanks for confirming it fixes the problem for you.
I pushed it to `master` (doesn't seem "obviously safe" enough for `emacs-29`).

> FWIW I also think the change to avoid `let` binding
> `auto-fill-function` in `normal-mode` is good.

I also pushed it to `master`.


        Stefan

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

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Just for the record I’ve been running using essentially this patch (but
>> with !=) since you suggested it and have not had any problems.
>
> Thanks for confirming it fixes the problem for you.
> I pushed it to `master` (doesn't seem "obviously safe" enough for `emacs-29`).
>
>> FWIW I also think the change to avoid `let` binding
>> `auto-fill-function` in `normal-mode` is good.
>
> I also pushed it to `master`.

It seems like this issue was fixed, but it was left open in the bug
tracker.  I'm therefore closing it now.

Please remember to close bug reports when they are fixed.