Subject: bug#44733: Nested let bindings for non-local DEFVAR_PER_BUFFER variables unwind wrong

From: Spencer Baugh <[email-masked]>
Date: Wed, 18 Nov 2020 22:11:00 -0500

The problem is with variables defined in C by DEFVAR_PER_BUFFER.  These
are Lisp variables with special Lisp_Object slots in struct buffer.

These variables can be let-bound.  When these variable are let-bound
when the variable is not buffer-local in the current buffer, the default
value for the variable is changed (which affects all buffers which don't
have a buffer-local value for the variable). In the C code, this is a
SPECPDL_LET_DEFAULT binding.

If a DEFVAR_PER_BUFFER variable is set with setq inside such a
SPECPDL_LET_DEFAULT binding, the resulting situation is somewhat
unusual: The variable is set to the specified value only for the current
buffer, and other buffers keep their old values for the variable, but
the variable does not become buffer-local - e.g. local-variable-p
returns nil. This situation is unusual and undocumented, but not
necessarily buggy. This is somewhat normal.

However, more buggy is what happens when these let bindings are nested.
If we do first SPECPDL_LET_DEFAULT, then setq, then a second nested
SPECPDL_LET_DEFAULT, when the second nested let binding is unwound, the
default value for variable is set to the pseudo-buffer-local value that
was active in (current-buffer) when the nested let was entered.

See the below code example (left-margin is chosen as an arbitrary
DEFVAR_PER_BUFFER variable):

(let ((left-margin 1))
  ;; Set this variable "pseudo-locally", inside a SPECPDL_LET_DEFAULT binding.
  (setq left-margin 123)
  (assert (eq left-margin 123))
  ;; Note, it's not a local variable.
  (assert (not (local-variable-p 'left-margin)))
  ;; The default value doesn't change.
  (assert (eq (default-value 'left-margin) 1))
  (with-temp-buffer (assert (eq left-margin 1)))
  ;; Perform a seemingly unrelated do-nothing let-binding of left-margin.
  (let ((left-margin 2)))
  ;; !! The default value of left-margin has changed to 123.
  (assert (eq (default-value 'left-margin) 123))
  (with-temp-buffer (assert (eq left-margin 123)))
  ;; Emacs used (current-buffer)'s value for left-margin, 123, instead of
  ;; the actual default value, 1, when storing the old value for left-margin.
  ;; So when it unwound the let, it set the default value to 123!
)

This seems unexpected.

I ran into this while working on a patch-set to optimize
DEFVAR_PER_BUFFER.  This current unwinding behavior is pretty clearly a
bug in C, so maybe we don't need to preserve it, which hopefully might
allow for an easier implementation of an optimized DEFVAR_PER_BUFFER, if
behavior will change anyway.  (Although I can't say yet exactly what
might be the best change...)