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

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

[Spencer Baugh]

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

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

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> 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?

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

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
> 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?

I mentioned two unexpected things, but I don't think that parapgrah
describes either of them.

First was the behavior that when you setq within a let_default binding,
the binding does not appear local when queried with local-variable-p.
That's relatively minor.  It doesn't seem to me that that paragraph says
anything about "You can have local bindings which don't appear local
when queried with local-variable-p".

Second, and the more important bug, which my code example was about, is
"the default value is set to whatever your current binding is, if you
enter and then exit a nested let-binding".  That seems definitely
uncovered by that paragraph, or any documentation.

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

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

[Spencer Baugh]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> (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.

That's one perspective, but it seems less consistent with the
documentation and with expected behavior.  The documentation for these
variables says:

  Automatically becomes buffer-local when set.

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

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

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

[Spencer Baugh]

Stefan Monnier <monnier@iro.umontreal.ca> 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.

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

[Stefan Monnier]

I just pushed a fix for that, along with the corresponding test case,


        Stefan

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

[Stefan Kangas]

close 44733 28.1
thanks

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

> I just pushed a fix for that, along with the corresponding test case,

It seems like this was fixed but never closed, so I'm closing it now.

commit 8fac2444641567b10f4c38b599636aeae0478e68
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Thu Nov 19 17:13:04 2020 -0500

    * src/data.c (set_internal): Fix bug#44733

    Set the default value when `set` encounters a PER_BUFFER variable
    which has been let-bound globally, to match the behavior seen with
    `make-variable-buffer-local`.

    * test/src/data-tests.el (binding-test--let-buffer-local):
    Add corresponding test.
    (data-tests--set-default-per-buffer): Add tentative test for the
    performance problem encountered in bug#41029.