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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

sbaugh

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



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Spencer Baugh <[hidden email]>
> Date: Wed, 18 Nov 2020 22:11:00 -0500
>
> 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.

Why did you think this is a bug?  The ELisp manual seems to document
what you see:

     A variable can have more than one local binding at a time (e.g., if
  there are nested ‘let’ forms that bind the variable).  The “current
  binding” is the local binding that is actually in effect.  It determines
  the value returned by evaluating the variable symbol, and it is the
  binding acted on by ‘setq’.

Or did I misunderstand what you found unexpected?



Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
>> (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))

This is a bug, indeed.  It should be 123 at this point.

>>   (with-temp-buffer (assert (eq left-margin 1)))

Same here, it should be 123.


        Stefan




Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
>> This is a bug, indeed.  It should be 123 at this point.
> That's one perspective, but it seems less consistent with the
> documentation and with expected behavior.

That's the way all other variables behave:

    (defvar-local sm-foo 1)
    (let ((sm-foo 23))
      (setq sm-foo 45)
      (list sm-foo
            (with-temp-buffer sm-foo)))

and I think it's asking for trouble if

    (let ((sm-foo 23))
      ...)

behaves differently from

    (let (sm-foo)
      (setq sm-foo 23)
      ...)

> and here, we are setting it, with setq.  It would seem that it should
> become buffer-local, then.  Indeed, that's the current behavior, that it
> becomes "pseudo-buffer-local", in that the value is different in this
> buffer from every other buffer.  (But local-variable-p returns nil,
> which is the only indication that something weird is going on.)

Indeed the current behavior is clearly buggy.  Historically, the
behavior of PER_BUFFER variables has been even more unlike that of
`make-variable-buffer-local` but over the years, I've made efforts to
make them behave the same.  Clearly, I missed this spot.


        Stefan




Reply | Threaded
Open this post in threaded view
|

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

sbaugh
Stefan Monnier <[hidden email]> writes:

>>> This is a bug, indeed.  It should be 123 at this point.
>> That's one perspective, but it seems less consistent with the
>> documentation and with expected behavior.
>
> That's the way all other variables behave:
>
>     (defvar-local sm-foo 1)
>     (let ((sm-foo 23))
>       (setq sm-foo 45)
>       (list sm-foo
>             (with-temp-buffer sm-foo)))

Aha, okay, I certainly can't argue with that.  DEFVAR_PER_BUFFER
variables should match that behavior.  I'll update my patch series to
match.

> and I think it's asking for trouble if
>
>     (let ((sm-foo 23))
>       ...)
>
> behaves differently from
>
>     (let (sm-foo)
>       (setq sm-foo 23)
>       ...)

Ah, this example is persuasive to me.

I see also that there is at least some documentation of this behavior,
in the make-variable-buffer-local documentation in variables.texi, which
I missed:

  A peculiar wrinkle of this feature is that binding the variable (with
  @code{let} or other binding constructs) does not create a buffer-local
  binding for it.  Only setting the variable (with @code{set} or
  @code{setq}), while the variable does not have a @code{let}-style
  binding that was made in the current buffer, does so.